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@xxxxxxxxx> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx> > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> <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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
