Re: [PATCH 4/8] drm/i915/uc: Rename intel_?uc_init() to intel_?uc_fetch_fw()

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

On Wed, Feb 22, 2017 at 03:59:01PM +0200, Joonas Lahtinen wrote:
> On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> > Trying to have subject_verb_object ordering and more descriptive names,
> > the intel_huc_init() and intel_guc_init() functions are renamed:
> > 
> >  * `intel_guc` is the subject, so those functions now take intel_guc
> >    structure, instead of the dev_priv
> >  * fetch is the verb
> >  * fw is the object which better describes the function's role
> > 
> > Same change is done for the huc counterpart.
> > 
> > Also we bulk call both functions from higher-level intel_uc_fetch_fw:
> >  * intel being the subject (taking the dev_priv as param)
> >  * fetch being the verb
> >  * uc_fw being the subject
> > 
> > v2: settle on intel_uc_fetch_fw name (M. Wajdeczko)
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> 
> <SNIP>
> 
> > @@ -609,8 +609,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >  	if (ret)
> >  		goto cleanup_irq;
> >  
> > -	intel_huc_init(dev_priv);
> > -	intel_guc_init(dev_priv);
> > +	intel_uc_fetch_fw(dev_priv);
> 
> intel_uc_init fits this context. (See below)

Answer below.

> 
> >  /**
> > - * intel_guc_init() - define parameters and fetch firmware
> > - * @dev_priv:	i915 device private
> > + * intel_guc_fetch_fw() - define parameters and fetch firmware
> > + * @guc:	intel_guc struct
> >   *
> >   * Called early during driver load, but after GEM is initialised.
> >   *
> >   * The firmware will be transferred to the GuC's memory later,
> >   * when intel_guc_init_hw() is called.
> >   */
> 
> "define parameters" is little vague, maybe "select and fetch firmware"?

I like those verbs. Let start using it through the whole thing!

> 
> > -void intel_guc_init(struct drm_i915_private *dev_priv)
> > +void intel_guc_fetch_fw(struct intel_guc *guc)
> >  {
> > -	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> > +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> >  	const char *fw_path;
> >  
> >  	if (!HAS_GUC(dev_priv)) {
> 
> This parameter dance needs to be moved away from guc_fetch_fw function,
> into intel_sanitize_options (I'm pretty sure I've mentioned this
> earlier).

This is removed in patch 8, as the fetch_fw is called conditionally.

> > @@ -751,23 +751,23 @@ void intel_guc_init(struct drm_i915_private *dev_priv)
> >  		fw_path = NULL;
> >  	} else if (IS_SKYLAKE(dev_priv)) {
> >  		fw_path = I915_SKL_GUC_UCODE;
> > -		guc_fw->major_ver_wanted = SKL_FW_MAJOR;
> > -		guc_fw->minor_ver_wanted = SKL_FW_MINOR;
> > +		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->major_ver_wanted = BXT_FW_MAJOR;
> > -		guc_fw->minor_ver_wanted = BXT_FW_MINOR;
> > +		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->major_ver_wanted = KBL_FW_MAJOR;
> > -		guc_fw->minor_ver_wanted = KBL_FW_MINOR;
> > +		guc->fw.major_ver_wanted = KBL_FW_MAJOR;
> > +		guc->fw.minor_ver_wanted = KBL_FW_MINOR;
> >  	} else {
> >  		fw_path = "";	/* unknown device */
> >  	}
> >  
> > -	guc_fw->path = fw_path;
> > -	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
> > -	guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> > +	guc->fw.path = fw_path;
> 
> Just get rid of fw_path variable and assign directly, also hoist the
> warning to the else branch, which can then do "return;"

This is done done in patch 8.

> > +	guc->fw.fetch_status = INTEL_UC_FIRMWARE_NONE;
> > +	guc->fw.load_status = INTEL_UC_FIRMWARE_NONE;
> 
> Hoist this assignment before the if block, so no need to special for
> the early return from else branch.

This is done done in patch 8.

> <SNIP>
> 
> >  /**
> > - * intel_huc_init() - initiate HuC firmware loading request
> > - * @dev_priv: the drm_i915_private device
> > + * intel_huc_fetch_fw() - initiate HuC firmware loading request
> 
> Correct this commit too to be more descriptive.

Okay.

> > + * @huc:	intel_huc struct
> >   *
> >   * Called early during driver load, but after GEM is initialised. The loading
> >   * will continue only when driver explicitly specify firmware name and version.
> > @@ -152,42 +152,41 @@ static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
> >   *
> >   * The DMA-copying to HW is done later when intel_huc_init_hw() is called.
> >   */
> > -void intel_huc_init(struct drm_i915_private *dev_priv)
> > +void intel_huc_fetch_fw(struct intel_huc *huc)
> >  {
> > -	struct intel_huc *huc = &dev_priv->huc;
> > -	struct intel_uc_fw *huc_fw = &huc->fw;
> > +	struct drm_i915_private *dev_priv = huc_to_i915(huc);
> >  	const char *fw_path = NULL;
> 
> Similarly arrange to get rid of fw_path here.

Patch 8 in the series addresses that issue as well. Maybe I should move
them around?

> <SNIP>
> 
> > @@ -30,6 +30,12 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  	mutex_init(&dev_priv->guc.send_mutex);
> >  }
> >  
> > +void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
> 
> This function might be worth calling intel_uc_init (See above), if the
> need comes to add other stuff. But either way.

This is quite confusing now. I was fine it being named init, someone
suggested to be more descriptive with the name, as it is not general
enough to be "init". Seemed reasonable enough for me, so I incorporated
that in the respin.

This is turning into some heavy bikeshedding now...

I agree that it's more than fetch, it actually selects + fetches +
populates the structures.

I'll gladly ignore previous feedback on being to vague with name and
just go with init, but let give the _fw postfix one last chance:


intel_guc_init_fw {
    intel_guc_select_fw
    if (NULL != guc.fw.path)
	    intel_uc_prepare_fw
}

Where select does what the guc's fetch fw does sans the uc_fetch call.

Also intel_{g,h}uc_select_fw can be made part of the sanitize options,
but I think it better belongs here.

That's is basing on your suggestions for the other patch.


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