| Message ID | 20190726171240.6008-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/uc: Don't fail on HuC firmware failure | expand |
Quoting Michal Wajdeczko (2019-07-26 18:12:40) > HuC is usually not a critical component, so we can safely ignore > firmware load or authentication failures unless HuC was explicitly > requested by the user. How soon before on-demand loading? :) -Chris
Quoting Michal Wajdeczko (2019-07-26 18:12:40) > HuC is usually not a critical component, so we can safely ignore > firmware load or authentication failures unless HuC was explicitly > requested by the user. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 ++++---- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 3 ++- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 6 ++++++ > 3 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 5b9b20d1cb6d..99419c5c0ad3 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -472,7 +472,7 @@ int intel_uc_init_hw(struct intel_uc *uc) > > if (intel_uc_is_using_huc(uc)) { > ret = intel_huc_fw_upload(huc); > - if (ret) > + if (ret && intel_uc_fw_is_overridden(&huc->fw)) > goto err_out; > } > > @@ -494,9 +494,9 @@ int intel_uc_init_hw(struct intel_uc *uc) > if (ret) > goto err_log_capture; > > - if (intel_uc_is_using_huc(uc)) { > + if (intel_uc_is_using_huc(uc) && intel_uc_fw_is_loaded(&huc->fw)) { Can we even load the huc fw is !using_huc()? > ret = intel_huc_auth(huc); > - if (ret) > + if (ret && intel_uc_fw_is_overridden(&huc->fw)) > goto err_communication; > } > > @@ -515,7 +515,7 @@ int intel_uc_init_hw(struct intel_uc *uc) > dev_info(i915->drm.dev, "GuC submission %s\n", > enableddisabled(intel_uc_is_using_guc_submission(uc))); > dev_info(i915->drm.dev, "HuC %s\n", > - enableddisabled(intel_uc_is_using_huc(uc))); > + enableddisabled(intel_huc_is_authenticated(huc))); Seems reasonable by itself. > > return 0; > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > index 5fbdd17a864b..1e9ae38e5b10 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > @@ -146,7 +146,8 @@ __uc_fw_override(struct intel_uc_fw *uc_fw) > break; > } > > - return uc_fw->path; > + uc_fw->user_overridden = uc_fw->path; uc_fw->user_overridden = uc_fw->path && *uc_fw->path; That is i915.huc_firmware="" should be a convenient way to disable loading. If we agree on that "creative misuse" of the modparam, or if you can reassure me that it works correctly anyway, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Fri, 26 Jul 2019 20:57:12 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-07-26 18:12:40) >> HuC is usually not a critical component, so we can safely ignore >> firmware load or authentication failures unless HuC was explicitly >> requested by the user. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 ++++---- >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 3 ++- >> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 6 ++++++ >> 3 files changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index 5b9b20d1cb6d..99419c5c0ad3 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -472,7 +472,7 @@ int intel_uc_init_hw(struct intel_uc *uc) >> >> if (intel_uc_is_using_huc(uc)) { >> ret = intel_huc_fw_upload(huc); >> - if (ret) >> + if (ret && intel_uc_fw_is_overridden(&huc->fw)) >> goto err_out; >> } >> >> @@ -494,9 +494,9 @@ int intel_uc_init_hw(struct intel_uc *uc) >> if (ret) >> goto err_log_capture; >> >> - if (intel_uc_is_using_huc(uc)) { >> + if (intel_uc_is_using_huc(uc) && >> intel_uc_fw_is_loaded(&huc->fw)) { > > Can we even load the huc fw is !using_huc()? as of today, meaning of "intel_uc_is_using_huc" is that HuC was requested to run (ie. enabled via modparam) and not that is now being used. > >> ret = intel_huc_auth(huc); >> - if (ret) >> + if (ret && intel_uc_fw_is_overridden(&huc->fw)) >> goto err_communication; >> } >> >> @@ -515,7 +515,7 @@ int intel_uc_init_hw(struct intel_uc *uc) >> dev_info(i915->drm.dev, "GuC submission %s\n", >> enableddisabled(intel_uc_is_using_guc_submission(uc))); >> dev_info(i915->drm.dev, "HuC %s\n", >> - enableddisabled(intel_uc_is_using_huc(uc))); >> + enableddisabled(intel_huc_is_authenticated(huc))); > > Seems reasonable by itself. > >> >> return 0; >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> index 5fbdd17a864b..1e9ae38e5b10 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> @@ -146,7 +146,8 @@ __uc_fw_override(struct intel_uc_fw *uc_fw) >> break; >> } >> >> - return uc_fw->path; >> + uc_fw->user_overridden = uc_fw->path; > > uc_fw->user_overridden = uc_fw->path && *uc_fw->path; > > That is i915.huc_firmware="" should be a convenient way to disable > loading. hmm, to be working as expected this should done init_early as: if (uc_fw->path && *uc_fw->path) uc_fw->status = INTEL_UC_FIRMWARE_SELECTED; else uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED; > > If we agree on that "creative misuse" of the modparam, or if you can > reassure me that it works correctly anyway, > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 5b9b20d1cb6d..99419c5c0ad3 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -472,7 +472,7 @@ int intel_uc_init_hw(struct intel_uc *uc) if (intel_uc_is_using_huc(uc)) { ret = intel_huc_fw_upload(huc); - if (ret) + if (ret && intel_uc_fw_is_overridden(&huc->fw)) goto err_out; } @@ -494,9 +494,9 @@ int intel_uc_init_hw(struct intel_uc *uc) if (ret) goto err_log_capture; - if (intel_uc_is_using_huc(uc)) { + if (intel_uc_is_using_huc(uc) && intel_uc_fw_is_loaded(&huc->fw)) { ret = intel_huc_auth(huc); - if (ret) + if (ret && intel_uc_fw_is_overridden(&huc->fw)) goto err_communication; } @@ -515,7 +515,7 @@ int intel_uc_init_hw(struct intel_uc *uc) dev_info(i915->drm.dev, "GuC submission %s\n", enableddisabled(intel_uc_is_using_guc_submission(uc))); dev_info(i915->drm.dev, "HuC %s\n", - enableddisabled(intel_uc_is_using_huc(uc))); + enableddisabled(intel_huc_is_authenticated(huc))); return 0; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c index 5fbdd17a864b..1e9ae38e5b10 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -146,7 +146,8 @@ __uc_fw_override(struct intel_uc_fw *uc_fw) break; } - return uc_fw->path; + uc_fw->user_overridden = uc_fw->path; + return uc_fw->user_overridden; } /** diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h index eddbb237fabe..898d43eff634 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h @@ -58,6 +58,7 @@ enum intel_uc_fw_type { */ struct intel_uc_fw { const char *path; + bool user_overridden; size_t size; struct drm_i915_gem_object *obj; enum intel_uc_fw_status status; @@ -144,6 +145,11 @@ static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw) return __intel_uc_fw_status(uc_fw) != INTEL_UC_FIRMWARE_NOT_SUPPORTED; } +static inline bool intel_uc_fw_is_overridden(const struct intel_uc_fw *uc_fw) +{ + return uc_fw->user_overridden; +} + static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) { if (intel_uc_fw_is_loaded(uc_fw))
HuC is usually not a critical component, so we can safely ignore firmware load or authentication failures unless HuC was explicitly requested by the user. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 ++++---- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 3 ++- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 6 ++++++ 3 files changed, 12 insertions(+), 5 deletions(-)