Re: [PATCH 8/8] drm/i915/uc: Simplify firwmare path handling

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

On Fri, Feb 17, 2017 at 03:29:17PM +0100, Michal Wajdeczko wrote:
> On Fri, Feb 17, 2017 at 02:05:57PM +0100, Arkadiusz Hiler wrote:
> 
> Typo in subject s/firwmare/firmware
> 
> 
> > Currently fw->path values can represent one of three possible states:
> > 
> >  1) NULL - device without the uC
> >  2) '\0' - device with the uC but have no firmware
> >  3) else - device with the uC and we have firmware
> > 
> > Second case is used only to WARN at a later stage.
> > 
> > We can WARN right away and merge cases 1 and 2.
> > 
> > Code can be even further simplified and common (HuC/GuC logic) happening
> > right before the fetch can be offloaded to the common function.
> > 
> > v2: fewer temporary variables, more straightforward flow (M. Wajdeczko)
> > 
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 39 +++++++++++----------------------
> >  drivers/gpu/drm/i915/intel_huc.c        | 20 +++++------------
> >  drivers/gpu/drm/i915/intel_uc.c         |  5 +++--
> >  3 files changed, 22 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 549a254..aade185 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -433,12 +433,8 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  		intel_uc_fw_status_repr(guc_fw->load_status));
> >  
> >  	if (fw_path == NULL) {
> > -		/* Device is known to have no uCode (e.g. no GuC) */
> > +		/* We do not have uCode for the device */
> >  		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");
> > -		return -ENODEV;
> >  	}
> >  
> >  	/* Fetch failed, or already fetched but failed to load? */
> > @@ -474,7 +470,6 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  	return 0;
> >  }
> >  
> > -
> >  /**
> >   * intel_guc_fetch_fw() - determine and fetch firmware
> >   * @guc:	intel_guc struct
> > @@ -487,39 +482,31 @@ int intel_guc_init_hw(struct drm_i915_private *dev_priv)
> >  void intel_guc_fetch_fw(struct intel_guc *guc)
> >  {
> >  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > -	const char *fw_path;
> >  
> > -	if (!HAS_GUC_UCODE(dev_priv)) {
> > -		fw_path = NULL;
> > -	} else if (IS_SKYLAKE(dev_priv)) {
> > -		fw_path = I915_SKL_GUC_UCODE;
> > +	guc->fw.path = NULL;
> > +	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
> > +	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
> > +	guc->fw.fw = INTEL_UC_FW_TYPE_GUC;
> 
> Maybe for above code we can add new function:
> 
> 	void intel_uc_fw_init_early(struct intel_uc_fw *fw,
> 	                            enum intel_uc_fw_type type);
> 
> and use it for both guc and huc:
> 	
> 	intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC);
> 	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC);

That's something I want to get to, but this serries is already getting
pretty swollen and this would mean yet another patch in the series, yet
another round of review

Let stick not only to digestible patches but digestible series as well.

> > +
> > +	if (IS_SKYLAKE(dev_priv)) {
> > +		guc->fw.path = I915_SKL_GUC_UCODE;
> >  		guc->fw.major_ver_wanted = SKL_FW_MAJOR;
> >  		guc->fw.minor_ver_wanted = SKL_FW_MINOR;
> >  	} else if (IS_BROXTON(dev_priv)) {
> > -		fw_path = I915_BXT_GUC_UCODE;
> > +		guc->fw.path = I915_BXT_GUC_UCODE;
> >  		guc->fw.major_ver_wanted = BXT_FW_MAJOR;
> >  		guc->fw.minor_ver_wanted = BXT_FW_MINOR;
> >  	} else if (IS_KABYLAKE(dev_priv)) {
> > -		fw_path = I915_KBL_GUC_UCODE;
> > +		guc->fw.path = I915_KBL_GUC_UCODE;
> >  		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
> >  		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
> >  	} else {
> > -		fw_path = "";	/* unknown device */
> > +		WARN(1, "No GuC firmware known for platform with GuC!\n");
> 
> Maybe simpler DRM_ERROR will be sufficient? We don't need callstack.

I just stuck to what was already used, but DRM_ERROR should be
sufficient.

> > +		i915.enable_guc_loading = 0;
> 
> What about making this firmware path guess work part of the early init or
> sanitize options function? Note that actual fetch is already done by in 
> different function, so mostly we just need to pick nice name for the
> new function. Maybe
> 
> 	int intel_guc_init_fw() ?
> 
> Note that changing i915.enable_guc param here has implication on other
> actions (like Huc loading) and thus forcing redundant checks elsewhere

Same point as with intel_uc_fw_init_early. On my TODO for vol. 2.

> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 2bb49b7..6f2d3f6 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -267,8 +267,9 @@ static void uc_fetch_fw(struct drm_i915_private *dev_priv,
> >  	size_t size;
> >  	int err;
> >  
> > -	DRM_DEBUG_DRIVER("before requesting firmware: uC fw fetch status %s\n",
> > -		intel_uc_fw_status_repr(uc_fw->fetch_status));
> > +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> > +
> > +	DRM_DEBUG_DRIVER("uC firmware pending, path %s\n", uc_fw->path);
> 
> If you are not planning to remove "intel_uc_fw_status_repr()" then please it.

What?

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