Re: [PATCH v2 2/6] drm/i915/uc: Move uC fw helper code into dedicated files

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

On Mon, 2017-10-02 at 14:01 +0000, Michal Wajdeczko wrote:
> This is a prerequisite to unblock next steps.

This is not a very good commit message when somebody is looking at this
patch individually. Something along lines "To improve code separation,
move uC firmware helper code into dedicated files."

> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

<SNIP>

>  void intel_uc_init_fw(struct drm_i915_private *dev_priv)
>  {
> -	fetch_uc_fw(dev_priv, &dev_priv->huc.fw);
> -	fetch_uc_fw(dev_priv, &dev_priv->guc.fw);
> +	i915_fetch_uc_fw(dev_priv, &dev_priv->huc.fw);
> +	i915_fetch_uc_fw(dev_priv, &dev_priv->guc.fw);
>  }

The microcontroller firmware most definitely is not a driver only
construct, so just call the function intel_uc_fw_init for less
confusion. We also validate and place in GTT the FW in addition to
fetching it.

>  void intel_uc_fini_fw(struct drm_i915_private *dev_priv)
>  {
> -	__intel_uc_fw_fini(&dev_priv->guc.fw);
> -	__intel_uc_fw_fini(&dev_priv->huc.fw);
> +	intel_uc_fw_fini(&dev_priv->guc.fw);
> +	intel_uc_fw_fini(&dev_priv->huc.fw);
>  }

And then this looks more symmetric, too.

When we have 'intel_uc_fw.h' for 'intel_uc_fw' object, I pretty much
assume to only see intel_uc_fw_* functions in there. Also, please
reoder the .h and .c so that init is first, and fini is after.

With those changes, this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

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