diff mbox

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

Message ID 1506074867-32714-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sagar.a.kamble@intel.com Sept. 22, 2017, 10:07 a.m. UTC 
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>
---
 drivers/gpu/drm/i915/intel_huc.c | 22 +++++++---------------
 drivers/gpu/drm/i915/intel_uc.c  | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uc.h  |  3 ++-
 3 files changed, 28 insertions(+), 17 deletions(-)

Comments

Joonas Lahtinen Sept. 22, 2017, 11:39 a.m. UTC | #1 
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
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 6145fa0..d3da4d3 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -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.
+ * 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;
-	struct intel_huc *huc = &dev_priv->huc;
 	struct i915_vma *vma;
+	u32 offset;
 	int ret;
-	u32 data[2];
 
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return;
@@ -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);
 	if (ret) {
 		DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret);
 		goto out;
@@ -266,7 +260,6 @@  void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 				HUC_FW_VERIFIED,
 				HUC_FW_VERIFIED,
 				50);
-
 	if (ret) {
 		DRM_ERROR("HuC: Authentication failed %d\n", ret);
 		goto out;
@@ -275,4 +268,3 @@  void intel_guc_auth_huc(struct drm_i915_private *dev_priv)
 out:
 	i915_vma_unpin(vma);
 }
-
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0178ba4..88c9471 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -328,6 +328,24 @@  static void guc_disable_communication(struct intel_guc *guc)
 	guc->send = intel_guc_send_nop;
 }
 
+/**
+ * intel_guc_auth_huc() - authenticate ucode
+ * @guc: struct intel_guc*
+ * @offset: rsa offset w.r.t ggtt base of huc vma
+ *
+ * triggers a huc fw authentication request to the guc via intel_guc_send
+ * authenticate_huc interface.
+ */
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
+{
+	u32 action[] = {
+		INTEL_GUC_ACTION_AUTHENTICATE_HUC,
+		rsa_offset
+	};
+
+	return intel_guc_send(guc, action, ARRAY_SIZE(action));
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
@@ -390,7 +408,7 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
-	intel_guc_auth_huc(dev_priv);
+	intel_huc_auth(&dev_priv->huc);
 	if (i915.enable_guc_submission) {
 		if (i915.guc_log_level >= 0)
 			gen9_enable_guc_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7703c9a..6966349 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -211,6 +211,7 @@  struct intel_huc {
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
+int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 
 static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
 {
@@ -254,6 +255,6 @@  static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 /* intel_huc.c */
 void intel_huc_select_fw(struct intel_huc *huc);
 void intel_huc_init_hw(struct intel_huc *huc);
-void intel_guc_auth_huc(struct drm_i915_private *dev_priv);
+void intel_huc_auth(struct intel_huc *huc);
 
 #endif