| Message ID | 20200221153209.268712-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS | expand |
On 2/21/2020 11:32 PM, Michal Wajdeczko wrote: > From commit 84b1ca2f0e68 ("drm/i915/uc: prefer intel_gt over i915 > in GuC/HuC paths") we stopped using HUC_STATUS error -ENODEV only > to indicate lack of HuC hardware and we started to use this error > also for all other cases when HuC was not in use or supported. > > Fix that by relying again on HAS_GT_UC macro, since currently > used function intel_huc_is_supported() is based on HuC firmware > support which could be unsupported also due to force disabled > GuC firmware. > > v2: use 0 only for disabled, add more error codes for other failures > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Tony Ye <tony.ye@intel.com> > Cc: Robert M. Fosha <robert.m.fosha@intel.com> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> #v1 > --- > drivers/gpu/drm/i915/gt/uc/intel_huc.c | 29 +++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > index a74b65694512..301bb5d5e59a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > @@ -200,9 +200,13 @@ int intel_huc_auth(struct intel_huc *huc) > * This function reads status register to verify if HuC > * firmware was successfully loaded. > * > - * Returns: 1 if HuC firmware is loaded and verified, > - * 0 if HuC firmware is not loaded and -ENODEV if HuC > - * is not present on this platform. > + * Returns: > + * * 1 if HuC firmware is loaded and verified, > + * * 0 if HuC firmware was disabled, > + * * -ENODEV if HuC is not present on this platform, > + * * -ENOPKG if HuC firmware was not installed, > + * * -ENOEXEC if HuC firmware is invalid, > + * * -EACCES if HuC firmware was not authenticated. > */ > int intel_huc_check_status(struct intel_huc *huc) > { > @@ -210,11 +214,26 @@ int intel_huc_check_status(struct intel_huc *huc) > intel_wakeref_t wakeref; > u32 status = 0; > > - if (!intel_huc_is_supported(huc)) > + if (!HAS_GT_UC(gt->i915)) > return -ENODEV; > > + switch (__intel_uc_fw_status(&huc->fw)) { > + case INTEL_UC_FIRMWARE_NOT_SUPPORTED: > + case INTEL_UC_FIRMWARE_DISABLED: > + return 0; > + case INTEL_UC_FIRMWARE_MISSING: > + return -ENOPKG; > + case INTEL_UC_FIRMWARE_ERROR: > + return -ENOEXEC; What about INTEL_UC_FIRMWARE_FAIL? Regards, Tony > + default: > + break; > + } > + > with_intel_runtime_pm(gt->uncore->rpm, wakeref) > status = intel_uncore_read(gt->uncore, huc->status.reg); > > - return (status & huc->status.mask) == huc->status.value; > + if ((status & huc->status.mask) != huc->status.value) > + return -EACCES; > + > + return 1; > } >
On 25.02.2020 08:49, Ye, Tony wrote: > > > On 2/21/2020 11:32 PM, Michal Wajdeczko wrote: >> From commit 84b1ca2f0e68 ("drm/i915/uc: prefer intel_gt over i915 >> in GuC/HuC paths") we stopped using HUC_STATUS error -ENODEV only >> to indicate lack of HuC hardware and we started to use this error >> also for all other cases when HuC was not in use or supported. >> >> Fix that by relying again on HAS_GT_UC macro, since currently >> used function intel_huc_is_supported() is based on HuC firmware >> support which could be unsupported also due to force disabled >> GuC firmware. >> >> v2: use 0 only for disabled, add more error codes for other failures >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Tony Ye <tony.ye@intel.com> >> Cc: Robert M. Fosha <robert.m.fosha@intel.com> >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> #v1 >> --- >> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 29 +++++++++++++++++++++----- >> 1 file changed, 24 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> index a74b65694512..301bb5d5e59a 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >> @@ -200,9 +200,13 @@ int intel_huc_auth(struct intel_huc *huc) >> * This function reads status register to verify if HuC >> * firmware was successfully loaded. >> * >> - * Returns: 1 if HuC firmware is loaded and verified, >> - * 0 if HuC firmware is not loaded and -ENODEV if HuC >> - * is not present on this platform. >> + * Returns: >> + * * 1 if HuC firmware is loaded and verified, >> + * * 0 if HuC firmware was disabled, >> + * * -ENODEV if HuC is not present on this platform, >> + * * -ENOPKG if HuC firmware was not installed, >> + * * -ENOEXEC if HuC firmware is invalid, >> + * * -EACCES if HuC firmware was not authenticated. >> */ >> int intel_huc_check_status(struct intel_huc *huc) >> { >> @@ -210,11 +214,26 @@ int intel_huc_check_status(struct intel_huc *huc) >> intel_wakeref_t wakeref; >> u32 status = 0; >> - if (!intel_huc_is_supported(huc)) >> + if (!HAS_GT_UC(gt->i915)) >> return -ENODEV; >> + switch (__intel_uc_fw_status(&huc->fw)) { >> + case INTEL_UC_FIRMWARE_NOT_SUPPORTED: >> + case INTEL_UC_FIRMWARE_DISABLED: >> + return 0; >> + case INTEL_UC_FIRMWARE_MISSING: >> + return -ENOPKG; >> + case INTEL_UC_FIRMWARE_ERROR: >> + return -ENOEXEC; > > What about INTEL_UC_FIRMWARE_FAIL? I assumed that we don't need to handle that case here, since we are still checking HuC status register below. But if you want we can improve: 1) return early if FAIL, then check register anyway 2) return early if FAIL, trust fw state and return 1 without checking register (as same register was already checked when we mark fw state as RUNNING) > > Regards, > Tony > >> + default: >> + break; >> + } >> + >> with_intel_runtime_pm(gt->uncore->rpm, wakeref) >> status = intel_uncore_read(gt->uncore, huc->status.reg); >> - return (status & huc->status.mask) == huc->status.value; >> + if ((status & huc->status.mask) != huc->status.value) >> + return -EACCES; >> + >> + return 1; >> } >>
On 2/26/2020 6:02 AM, Michal Wajdeczko wrote: > > > On 25.02.2020 08:49, Ye, Tony wrote: >> >> >> On 2/21/2020 11:32 PM, Michal Wajdeczko wrote: >>> From commit 84b1ca2f0e68 ("drm/i915/uc: prefer intel_gt over i915 >>> in GuC/HuC paths") we stopped using HUC_STATUS error -ENODEV only >>> to indicate lack of HuC hardware and we started to use this error >>> also for all other cases when HuC was not in use or supported. >>> >>> Fix that by relying again on HAS_GT_UC macro, since currently >>> used function intel_huc_is_supported() is based on HuC firmware >>> support which could be unsupported also due to force disabled >>> GuC firmware. >>> >>> v2: use 0 only for disabled, add more error codes for other failures >>> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Tony Ye <tony.ye@intel.com> >>> Cc: Robert M. Fosha <robert.m.fosha@intel.com> >>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> #v1 >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 29 +++++++++++++++++++++----- >>> 1 file changed, 24 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>> index a74b65694512..301bb5d5e59a 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c >>> @@ -200,9 +200,13 @@ int intel_huc_auth(struct intel_huc *huc) >>> * This function reads status register to verify if HuC >>> * firmware was successfully loaded. >>> * >>> - * Returns: 1 if HuC firmware is loaded and verified, >>> - * 0 if HuC firmware is not loaded and -ENODEV if HuC >>> - * is not present on this platform. >>> + * Returns: >>> + * * 1 if HuC firmware is loaded and verified, >>> + * * 0 if HuC firmware was disabled, >>> + * * -ENODEV if HuC is not present on this platform, >>> + * * -ENOPKG if HuC firmware was not installed, >>> + * * -ENOEXEC if HuC firmware is invalid, >>> + * * -EACCES if HuC firmware was not authenticated. >>> */ >>> int intel_huc_check_status(struct intel_huc *huc) >>> { >>> @@ -210,11 +214,26 @@ int intel_huc_check_status(struct intel_huc *huc) >>> intel_wakeref_t wakeref; >>> u32 status = 0; >>> - if (!intel_huc_is_supported(huc)) >>> + if (!HAS_GT_UC(gt->i915)) >>> return -ENODEV; >>> + switch (__intel_uc_fw_status(&huc->fw)) { >>> + case INTEL_UC_FIRMWARE_NOT_SUPPORTED: >>> + case INTEL_UC_FIRMWARE_DISABLED: >>> + return 0; >>> + case INTEL_UC_FIRMWARE_MISSING: >>> + return -ENOPKG; >>> + case INTEL_UC_FIRMWARE_ERROR: >>> + return -ENOEXEC; >> >> What about INTEL_UC_FIRMWARE_FAIL? > > I assumed that we don't need to handle that case here, since we are > still checking HuC status register below. > > But if you want we can improve: > 1) return early if FAIL, then check register anyway > 2) return early if FAIL, trust fw state and return 1 without checking > register (as same register was already checked when we mark fw state as > RUNNING) The current version looks good to me. Thanks. Reviewed-by: Tony Ye <tony.ye@intel.com> > >> >> Regards, >> Tony >> >>> + default: >>> + break; >>> + } >>> + >>> with_intel_runtime_pm(gt->uncore->rpm, wakeref) >>> status = intel_uncore_read(gt->uncore, huc->status.reg); >>> - return (status & huc->status.mask) == huc->status.value; >>> + if ((status & huc->status.mask) != huc->status.value) >>> + return -EACCES; >>> + >>> + return 1; >>> } >>>
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index a74b65694512..301bb5d5e59a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -200,9 +200,13 @@ int intel_huc_auth(struct intel_huc *huc) * This function reads status register to verify if HuC * firmware was successfully loaded. * - * Returns: 1 if HuC firmware is loaded and verified, - * 0 if HuC firmware is not loaded and -ENODEV if HuC - * is not present on this platform. + * Returns: + * * 1 if HuC firmware is loaded and verified, + * * 0 if HuC firmware was disabled, + * * -ENODEV if HuC is not present on this platform, + * * -ENOPKG if HuC firmware was not installed, + * * -ENOEXEC if HuC firmware is invalid, + * * -EACCES if HuC firmware was not authenticated. */ int intel_huc_check_status(struct intel_huc *huc) { @@ -210,11 +214,26 @@ int intel_huc_check_status(struct intel_huc *huc) intel_wakeref_t wakeref; u32 status = 0; - if (!intel_huc_is_supported(huc)) + if (!HAS_GT_UC(gt->i915)) return -ENODEV; + switch (__intel_uc_fw_status(&huc->fw)) { + case INTEL_UC_FIRMWARE_NOT_SUPPORTED: + case INTEL_UC_FIRMWARE_DISABLED: + return 0; + case INTEL_UC_FIRMWARE_MISSING: + return -ENOPKG; + case INTEL_UC_FIRMWARE_ERROR: + return -ENOEXEC; + default: + break; + } + with_intel_runtime_pm(gt->uncore->rpm, wakeref) status = intel_uncore_read(gt->uncore, huc->status.reg); - return (status & huc->status.mask) == huc->status.value; + if ((status & huc->status.mask) != huc->status.value) + return -EACCES; + + return 1; }
From commit 84b1ca2f0e68 ("drm/i915/uc: prefer intel_gt over i915 in GuC/HuC paths") we stopped using HUC_STATUS error -ENODEV only to indicate lack of HuC hardware and we started to use this error also for all other cases when HuC was not in use or supported. Fix that by relying again on HAS_GT_UC macro, since currently used function intel_huc_is_supported() is based on HuC firmware support which could be unsupported also due to force disabled GuC firmware. v2: use 0 only for disabled, add more error codes for other failures Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Tony Ye <tony.ye@intel.com> Cc: Robert M. Fosha <robert.m.fosha@intel.com> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> #v1 --- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 29 +++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)