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

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

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.

	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.

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

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