[Intel-gfx] [PATCH 07/10] drm/i915/guc: Simplify intel_guc_init_hw()

[Arkadiusz Hiler]

On Fri, Feb 24, 2017 at 06:26:10PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 24, 2017 at 04:40:01PM +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)
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > Cc: Michal Winiarski <michal.winiarski at intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         |   2 +-
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 144 ++++----------------------------
> >  drivers/gpu/drm/i915/intel_uc.c         | 110 ++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_uc.h         |   3 +
> >  4 files changed, 128 insertions(+), 131 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 5b36524..8943c4e 100644
> > @@ -443,42 +425,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  {
> <SNIP>
> > -	/* 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') {
> 
> Hmm, is this case still possible?

In this revision - yes, but unlikely.

HAS_GUC() is true but firmware for the device is not known (see
init_fw()). I think that the initial flow was designed in this case in
mind.

> > -		/* 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;
> > +	else if (guc_fw->load_status == INTEL_UC_FIRMWARE_FAIL)
> > +		return -ENOEXEC;
> 
> Hmm, it looks like you're checking for load failure here, but actual
> load is about to start below ? Did I missed something ?

The status FIRMWARE_FAIL is not used at all in the GuC path (HuC) uses
it. Noted down to give it a closer look.

The check is gone, as it does not make sense, thanks.

> >  	guc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> 
> I guess this status can be set in guc_ucode_xfer() as it uses guc_fw object.

> >  
> > @@ -486,104 +450,24 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  		intel_uc_fw_status_repr(guc_fw->fetch_status),
> >  		intel_uc_fw_status_repr(guc_fw->load_status));
> >  
> > -	err = i915_guc_submission_init(dev_priv);
> > -	if (err)
> > -		goto fail;
> > -
> >  	/*
> >  	 * WaEnableuKernelHeaderValidFix:skl,bxt
> >  	 * For BXT, this is only upto B0 but below WA is required for later
> >  	 * steppings also so this is extended as well.
> >  	 */
> 
> Rebase issue?

Yeah. I've streamlined the whole thing to be:

----------------------------------------------------------
int attempts;
const int guc_wa_hash_check_not_set_attempts = 3;

<SNIP>

/* WaEnableuKernelHeaderValidFix:skl */
/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
if (IS_GEN9(dev_priv))
        attempts = guc_wa_hash_check_not_set_attempts;
else
        attempts = 1;

while (attempts--) ...
----------------------------------------------------------


and dropped the WA comments in the guc_init_hw()



> > +int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> > +{
> <SNIP>
> > +fail:
> > +	/*
> > +	 * We've failed to load the firmware :(
> > +	 *
> > +	 * Decide whether to disable GuC submission and fall back to
> > +	 * execlist mode, and whether to hide the error by returning
> > +	 * zero or to return -EIO, which the caller will treat as a
> > +	 * nonfatal error (i.e. it doesn't prevent driver load, but
> > +	 * marks the GPU as wedged until reset).
> > +	 */
> > +	DRM_ERROR("GuC init failed\n");
> > +	if (i915.enable_guc_loading > 1 || i915.enable_guc_submission > 1)
> 
> Nonzero i915.enable_guc_loading is guaranteed (see your first "if" in this function)

That's why this not a check for "non-zeroness".

It's cheeking for "greater than one", adhering to existing behavior:
 0   = disabled
 1   = enabled with possible fallback (i.e. ignore the error and use execlists)
 > 1 = wedge if this fail

> > +/* WaEnableGuCBootHashCheckNotSet:skl,bxt */
> > +#define GUC_WA_HASH_CHECK_NOT_SET_ATTEMPTS 3
> > +
> 
> Hmm, is it worth to define this macro here as it is used only once and
> it is very specific ? Do you plan to define more similar macros?

Changed to const, but i915 seems to be generally define-biased for
similar things.

-- 
Cheers,
Arek