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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ke, 2017-02-22 at 16:29 +0100, Arkadiusz Hiler wrote:
> On Wed, Feb 22, 2017 at 03:59:01PM +0200, Joonas Lahtinen wrote:
> > 
> > > + * @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?

Nah, it's fine, the intermediary steps need to be working (for
bisecting), but not necessarily 100% pretty. If it's addressed later,
it's good.

> > > @@ -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...

That's why actual code in the mailing list is the only right way,
discussion in IRC can be misleading :)

> 
> 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)

if (guc.fw.patch) to stick to coding style.

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

Sounds good to me.

> 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.

Thats, correct, select_fw should be here.


        if (!HAS_GUC(dev_priv)) {
                i915.enable_guc_loading = 0;
                i915.enable_guc_submission = 0;
        } else {
                /* A negative value means "use platform default" */
                if (i915.enable_guc_loading < 0)
                        i915.enable_guc_loading = HAS_GUC_UCODE(dev_priv);
                if (i915.enable_guc_submission < 0)
                        i915.enable_guc_submission = HAS_GUC_SCHED(dev_priv);
        }

This part is a perfect fit to the sanitize_options function, because
that's what it does, makes sure we don't try to enable something we
don't have.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx