Re: [PATCH 5/8] drm/i915/uc: Make intel_uc_fw_fetch() static

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

On Wed, Feb 22, 2017 at 04:17:20PM +0200, Joonas Lahtinen wrote:
> On ke, 2017-02-22 at 13:41 +0100, Arkadiusz Hiler wrote:
> > intel_uc_fw_fetch() is confusingly named in the light of recent changes.
> > 
> > It's also in the wrong place - 'guc_loader.h' - it's used for both guc
> > and huc, which was reflected in name, but not it's location, so let's
> > move it to 'intel_uc.c'.
> > 
> > We can make a intel_uc_fw callback out of it, to avoid leaking it
> > outside `intel_uc.c`
> > 
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> 
> <SNIP>
> 
> > @@ -32,7 +36,10 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv)
> >  
> >  void intel_uc_fetch_fw(struct drm_i915_private *dev_priv)
> >  {
> > +	dev_priv->huc.fw.fetch = uc_fetch_fw;
> >  	intel_huc_fetch_fw(&dev_priv->huc);
> > +
> > +	dev_priv->guc.fw.fetch = uc_fetch_fw;
> 
> I'm bit confused, I was under impression these functions were going to
> be different for each uC? If they're not, then it most definitely
> should not be a function pointer.

The issue I presented on the IRC (maybe not clearly enough) was just
placement and naming of that function. The proposition with callback
seemd odd, but since it was backed by couple of people I commited to it.

Glad you spoted that here.

> 	int ret;
> 
> 	ret = intel_huc_select_fw(dev_priv->huc);
> 	if (ret)
> 		goto err;
> 	ret = intel_uc_fw_prepare(&dev_priv->huc->fw);
> 	if (ret)
> 		goto err;
> 
> 	ret = intel_guc_select_fw(dev_priv->guc);
> 	if (ret)
> 		goto err_huc;
> 	ret = intel_uc_fw_prepare(&dev_priv->guc->fw);
> 	if (ret)
> 		
> goto err_huc;
> 
> 	return 0;
> 
> err_huc:
> 	intel_uc_fw_unprepare(&dev_priv->huc->fw);
> err:
> 	return ret;
> 
> By always involving proper onion teardown, no dangling objects are left
> around.

That's exactly what I was missing in my approach. Thanks!

> >  	intel_guc_fetch_fw(&dev_priv->guc);
> >  }
> >  
> > @@ -120,3 +127,137 @@ int intel_guc_sample_forcewake(struct intel_guc *guc)
> > >  	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> >  }
> >  
> > +static void uc_fetch_fw(struct drm_i915_private *dev_priv,
> > +			  struct intel_uc_fw *uc_fw)
> > +{
> > +	struct pci_dev *pdev = dev_priv->drm.pdev;
> > +	struct drm_i915_gem_object *obj;
> > +	const struct firmware *fw = NULL;
> > +	struct uc_css_header *css;
> > +	size_t size;
> > +	int err;
> 
> This function should be named differently, because it doesn't simply
> fetch it, but also validates and makes an object of it (which may be
> left dangling).

intel_uc_fw_prepare you've used in the example above seems like a good
name.

Onto fixing this mess. Thanks for the input :-)

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