Re: [PATCH v7 1/2] drm/i915: Reorganize HuC authentication

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

On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote:
> Prepared intel_auth_huc to separate HuC specific functionality
> from GuC send action. Created new header intel_huc.h to group
> HuC specific declarations.
> 
> v2: Changed argument preparation for AUTHENTICATE_HUC.
> s/intel_auth_huc/intel_huc_auth. Deferred creation of intel_huc.h
> to later patch.
> 
> v3: Rebase as intel_guc.h is removed. Added param description to
> intel_huc_auth. (Michal)
> 
> v4: Rebase as intel_guc.h is added again. :)
> 
> v5: Rebase w.r.t removal of GuC code restructuring.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

<SNIP>

> @@ -225,19 +225,16 @@ void intel_huc_init_hw(struct intel_huc *huc)
>  }
>  
>  /**
> - * intel_guc_auth_huc() - authenticate ucode
> - * @dev_priv: the drm_i915_device
> - *
> - * Triggers a HuC fw authentication request to the GuC via intel_guc_action_
> - * authenticate_huc interface.

This is still an exported function from this file, so should have
kerneldoc comment. Especially now that we have two very similar
exported functions, it would be beneficial to crosslink them in the
resulting documentation that'll look like:

https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#c.intel_guc_init_hw

So proper sentences and capitalisation is much preferred.

I'm also noticing that we're not properly linking all portions of the
code to be included in docs, I'll send a fix for that.

> + * intel_huc_auth() - authenticate ucode
> + * @huc: intel_huc structure
>   */
> -void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
> +void intel_huc_auth(struct intel_huc *huc)
>  {
> +	struct drm_i915_private *dev_priv = huc_to_i915(huc);
>  	struct intel_guc *guc = &dev_priv->guc;

As there's only one more of instance of 'dev_priv' in this func so just
rename it to 'i915' while touching the func. GuC is really close to the
GEM so similar style is good.

> @@ -250,11 +247,8 @@ void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
>  		return;
>  	}
>  
> -	/* Specify auth action and where public signature is. */
> -	data[0] = INTEL_GUC_ACTION_AUTHENTICATE_HUC;
> -	data[1] = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
> -
> -	ret = intel_guc_send(guc, data, ARRAY_SIZE(data));
> +	offset = guc_ggtt_offset(vma) + huc->fw.rsa_offset;
> +	ret = intel_guc_auth_huc(guc, offset);

With the code cleanup, the offset variable seems unnecessary to me,
just do:

	ret = intel_guc_auth_huc(guc,
				 guc_ggtt_offset(vma) + huc->fw.rsa_offset);

Other than that, LGTM.

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