Re: [PATCH v6] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler <arkadiusz.hiler@intel.com>]

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.srivatsa@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@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