On Fri, Mar 10, 2017 at 01:11:06PM +0100, Michal Wajdeczko wrote:
> On Fri, Mar 10, 2017 at 12:46:06PM +0100, Arkadiusz Hiler wrote:
> > Current version of intel_guc_init_hw() does a lot:
> >  - cares about submission
> >  - loads huc
> >  - implement WA
> > 
> > This change offloads some of the logic to intel_uc_init_hw(), which now
> > cares about the above.
> > 
> > v2: rename guc_hw_reset and fix typo in define name (M. Wajdeczko)
> > v3: rename once again
> > v4: remove spurious comments and add some style (J. Lahtinen)
> > v5: flow changes, got rid of dead checks (M. Wajdeczko)
> > v6: rebase
> > 
> > Cc: Anusha Srivatsa <anusha.sriva...@intel.com>
> > Cc: Michal Winiarski <michal.winiar...@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hi...@intel.com>
> > ---
> 
> snip
> 
> > @@ -397,42 +379,22 @@ int intel_guc_init_hw(struct intel_guc *guc)
> >  {
> >     struct drm_i915_private *dev_priv = guc_to_i915(guc);
> >     const char *fw_path = guc->fw.path;
> > -   int retries, ret, err;
> > +   int ret;
> >  
> >     DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
> >             fw_path,
> >             intel_uc_fw_status_repr(guc->fw.fetch_status),
> >             intel_uc_fw_status_repr(guc->fw.load_status));
> >  
> > -   /* Loading forbidden, or no firmware to load? */
> > -   if (!i915.enable_guc_loading) {
> > -           err = 0;
> > -           goto fail;
> > -   } else if (fw_path == NULL) {
> > -           /* Device is known to have no uCode (e.g. no GuC) */
> > -           err = -ENXIO;
> > -           goto fail;
> > +   if (!fw_path) {
> > +           return -ENXIO;
> >     } else if (*fw_path == '\0') {
> > -           /* Device has a GuC but we don't know what f/w to load? */
> >             WARN(1, "No GuC firmware known for this platform!\n");
> > -           err = -ENODEV;
> > -           goto fail;
> > +           return -ENODEV;
> >     }
> >  
> > -   /* Fetch failed, or already fetched but failed to load? */
> > -   if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS) {
> > -           err = -EIO;
> > -           goto fail;
> > -   } else if (guc->fw.load_status == INTEL_UC_FIRMWARE_FAIL) {
> > -           err = -ENOEXEC;
> > -           goto fail;
> > -   }
> > -
> > -   guc_interrupts_release(dev_priv);
> > -   gen9_reset_guc_interrupts(dev_priv);
> > -
> > -   /* We need to notify the guc whenever we change the GGTT */
> > -   i915_ggtt_enable_guc(dev_priv);
> > +   if (guc->fw.fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> > +           return -EIO;
> 
> Above fw_path checks seem to be redundant as we also look for fetch status 
> here.

Got rid of it in the path simplifying patch. Thanks.

> > @@ -63,6 +84,98 @@ void intel_uc_init_fw(struct drm_i915_private *dev_priv)
> >     intel_guc_init_fw(&dev_priv->guc);
> >  }
> >  
> > +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> > +{
> > +   int ret, attempts;
> > +   const int guc_wa_hash_check_not_set_attempts = 3;
> 
> In all other places, we put const vars definitions as first statements.
> And maybe this const should be accompanied by the /* Wa */ comment
> that is shown below. Also changing prefix to "gen9" maybe helpful

Went with using it directly, as Joonas suggested. The current if-else
statement makes meaning obvious enough without having the need to name
it.

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to