| Message ID | 20171201103317.48416-6-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2017-12-01 10:33:16) > Our new "enable_guc" modparam allows to control whenever HuC > should be loaded. However existing code will try load and > authenticate HuC always when we use the GuC. This patch is > trying to enforce modparam selection. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > -void intel_huc_auth(struct intel_huc *huc) > +int intel_huc_auth(struct intel_huc *huc) > { > struct drm_i915_private *i915 = huc_to_i915(huc); > struct intel_guc *guc = &i915->guc; > @@ -213,14 +213,14 @@ void intel_huc_auth(struct intel_huc *huc) > int ret; > > if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) > - return; > + return -ENOEXEC; > > vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0, > PIN_OFFSET_BIAS | GUC_WOPCM_TOP); > if (IS_ERR(vma)) { > - DRM_ERROR("failed to pin huc fw object %d\n", > - (int)PTR_ERR(vma)); > - return; > + ret = (int)PTR_ERR(vma); The advantage here is that (int) is now implicit. Looks ok. I'd recommend we add the various expected combinations to drv_module_reload. "enable_guc=0" "enable_guc=0x1" "enable_guc=0x2" "enable_guc=0x3" etc? -Chris
On 12/1/2017 4:03 PM, Michal Wajdeczko wrote: > Our new "enable_guc" modparam allows to control whenever HuC > should be loaded. However existing code will try load and > authenticate HuC always when we use the GuC. This patch is > trying to enforce modparam selection. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> <snip> > int intel_uc_init_hw(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > + struct intel_huc *huc = &dev_priv->huc; > int ret, attempts; > > if (!USES_GUC(dev_priv)) > @@ -220,7 +221,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > if (ret) > goto err_submission; > > - intel_huc_init_hw(&dev_priv->huc); > + if (USES_HUC(dev_priv)) { > + ret = intel_huc_init_hw(huc); > + if (ret) > + break; this break should be "goto err_submission" as GuC is still not ready. looks like user has to be very careful with param now that HuC failure can block GuC tasks too. > + } > + > intel_guc_init_params(guc); > ret = intel_guc_fw_upload(guc); > if (ret == 0 || ret != -EAGAIN) > @@ -238,7 +244,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > if (ret) > goto err_log_capture; > > - intel_huc_auth(&dev_priv->huc); > + if (USES_HUC(dev_priv)) { > + ret = intel_huc_auth(huc); > + if (ret) > + goto err_interrupts; > + } > + > if (USES_GUC_SUBMISSION(dev_priv)) { > if (i915_modparams.guc_log_level >= 0) > gen9_enable_guc_interrupts(dev_priv); > @@ -252,6 +263,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > guc->fw.major_ver_found, guc->fw.minor_ver_found); > dev_info(dev_priv->drm.dev, "GuC submission %s\n", > enableddisabled(USES_GUC_SUBMISSION(dev_priv))); > + dev_info(dev_priv->drm.dev, "HuC %s\n", > + enableddisabled(USES_HUC(dev_priv))); > > return 0; >
On Fri, 01 Dec 2017 17:39:38 +0100, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > > > On 12/1/2017 4:03 PM, Michal Wajdeczko wrote: >> Our new "enable_guc" modparam allows to control whenever HuC >> should be loaded. However existing code will try load and >> authenticate HuC always when we use the GuC. This patch is >> trying to enforce modparam selection. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > <snip> >> int intel_uc_init_hw(struct drm_i915_private *dev_priv) >> { >> struct intel_guc *guc = &dev_priv->guc; >> + struct intel_huc *huc = &dev_priv->huc; >> int ret, attempts; >> if (!USES_GUC(dev_priv)) >> @@ -220,7 +221,12 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> if (ret) >> goto err_submission; >> - intel_huc_init_hw(&dev_priv->huc); >> + if (USES_HUC(dev_priv)) { >> + ret = intel_huc_init_hw(huc); >> + if (ret) >> + break; > this break should be "goto err_submission" as GuC is still not ready. but GuC may also be not ready due to guc fw upload failure ... > looks like user has to be very careful with param now that HuC failure > can block GuC tasks too. yep, this is lowlight of this patch, as all selected options are now 'required' >> + } >> + >> intel_guc_init_params(guc); >> ret = intel_guc_fw_upload(guc); >> if (ret == 0 || ret != -EAGAIN) >> @@ -238,7 +244,12 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> if (ret) >> goto err_log_capture; >> - intel_huc_auth(&dev_priv->huc); >> + if (USES_HUC(dev_priv)) { >> + ret = intel_huc_auth(huc); >> + if (ret) >> + goto err_interrupts; >> + } >> + >> if (USES_GUC_SUBMISSION(dev_priv)) { >> if (i915_modparams.guc_log_level >= 0) >> gen9_enable_guc_interrupts(dev_priv); >> @@ -252,6 +263,8 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> guc->fw.major_ver_found, guc->fw.minor_ver_found); >> dev_info(dev_priv->drm.dev, "GuC submission %s\n", >> enableddisabled(USES_GUC_SUBMISSION(dev_priv))); >> + dev_info(dev_priv->drm.dev, "HuC %s\n", >> + enableddisabled(USES_HUC(dev_priv))); >> return 0; >>
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index 6d0e050..abe301f 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -181,17 +181,17 @@ static int huc_ucode_xfer(struct intel_uc_fw *huc_fw, struct i915_vma *vma) * intel_huc_init_hw() - load HuC uCode to device * @huc: intel_huc structure * - * Called from guc_setup() during driver loading and also after a GPU reset. - * Be note that HuC loading must be done before GuC loading. + * Called from intel_uc_init_hw() during driver loading and also after a GPU + * reset. Be note that HuC loading must be done before GuC loading. * * The firmware image should have already been fetched into memory by the - * earlier call to intel_huc_init(), so here we need only check that + * earlier call to intel_uc_init_fw(), so here we need only check that * is succeeded, and then transfer the image to the h/w. * */ -void intel_huc_init_hw(struct intel_huc *huc) +int intel_huc_init_hw(struct intel_huc *huc) { - intel_uc_fw_upload(&huc->fw, huc_ucode_xfer); + return intel_uc_fw_upload(&huc->fw, huc_ucode_xfer); } /** @@ -205,7 +205,7 @@ void intel_huc_init_hw(struct intel_huc *huc) * signature through intel_guc_auth_huc(). It then waits for 50ms for * firmware verification ACK and unpins the object. */ -void intel_huc_auth(struct intel_huc *huc) +int intel_huc_auth(struct intel_huc *huc) { struct drm_i915_private *i915 = huc_to_i915(huc); struct intel_guc *guc = &i915->guc; @@ -213,14 +213,14 @@ void intel_huc_auth(struct intel_huc *huc) int ret; if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) - return; + return -ENOEXEC; vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0, PIN_OFFSET_BIAS | GUC_WOPCM_TOP); if (IS_ERR(vma)) { - DRM_ERROR("failed to pin huc fw object %d\n", - (int)PTR_ERR(vma)); - return; + ret = (int)PTR_ERR(vma); + DRM_ERROR("HuC: Failed to pin huc fw object %d\n", ret); + return ret; } ret = intel_guc_auth_huc(guc, @@ -243,4 +243,5 @@ void intel_huc_auth(struct intel_huc *huc) out: i915_vma_unpin(vma); + return ret; } diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h index 3d757bc..40039db 100644 --- a/drivers/gpu/drm/i915/intel_huc.h +++ b/drivers/gpu/drm/i915/intel_huc.h @@ -35,7 +35,7 @@ struct intel_huc { }; void intel_huc_init_early(struct intel_huc *huc); -void intel_huc_init_hw(struct intel_huc *huc); -void intel_huc_auth(struct intel_huc *huc); +int intel_huc_init_hw(struct intel_huc *huc); +int intel_huc_auth(struct intel_huc *huc); #endif diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 8a761af..6c30a4d 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -174,6 +174,7 @@ static void guc_disable_communication(struct intel_guc *guc) int intel_uc_init_hw(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; + struct intel_huc *huc = &dev_priv->huc; int ret, attempts; if (!USES_GUC(dev_priv)) @@ -220,7 +221,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) if (ret) goto err_submission; - intel_huc_init_hw(&dev_priv->huc); + if (USES_HUC(dev_priv)) { + ret = intel_huc_init_hw(huc); + if (ret) + break; + } + intel_guc_init_params(guc); ret = intel_guc_fw_upload(guc); if (ret == 0 || ret != -EAGAIN) @@ -238,7 +244,12 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) if (ret) goto err_log_capture; - intel_huc_auth(&dev_priv->huc); + if (USES_HUC(dev_priv)) { + ret = intel_huc_auth(huc); + if (ret) + goto err_interrupts; + } + if (USES_GUC_SUBMISSION(dev_priv)) { if (i915_modparams.guc_log_level >= 0) gen9_enable_guc_interrupts(dev_priv); @@ -252,6 +263,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) guc->fw.major_ver_found, guc->fw.minor_ver_found); dev_info(dev_priv->drm.dev, "GuC submission %s\n", enableddisabled(USES_GUC_SUBMISSION(dev_priv))); + dev_info(dev_priv->drm.dev, "HuC %s\n", + enableddisabled(USES_HUC(dev_priv))); return 0;
Our new "enable_guc" modparam allows to control whenever HuC should be loaded. However existing code will try load and authenticate HuC always when we use the GuC. This patch is trying to enforce modparam selection. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/intel_huc.c | 21 +++++++++++---------- drivers/gpu/drm/i915/intel_huc.h | 4 ++-- drivers/gpu/drm/i915/intel_uc.c | 17 +++++++++++++++-- 3 files changed, 28 insertions(+), 14 deletions(-)