| Message ID | 20220427002617.1767295-2-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Prepare for GSC-loaded HuC | expand |
On Tue, Apr 26, 2022 at 05:26:14PM -0700, Daniele Ceraolo Spurio wrote: > The huc_is_authenticated function return is based on our SW tracking of > the HuC auth status. However, around suspend/resume and reset this can > go out of sync with the actual HW state, which is why we use > huc_check_state() to look at the actual HW state. Instead of having this > duality, just make huc_is_authenticated() return the HW state and use it > everywhere we need to know if HuC is running. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_huc.c | 23 ++++++++++++++--------- > drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ----- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > index 556829de9c172..773020e69589a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > @@ -80,6 +80,18 @@ void intel_huc_fini(struct intel_huc *huc) > intel_uc_fw_fini(&huc->fw); > } > > +static bool huc_is_authenticated(struct intel_huc *huc) > +{ > + struct intel_gt *gt = huc_to_gt(huc); > + intel_wakeref_t wakeref; > + u32 status = 0; > + > + 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; > +} > + > /** > * intel_huc_auth() - Authenticate HuC uCode > * @huc: intel_huc structure > @@ -96,7 +108,7 @@ int intel_huc_auth(struct intel_huc *huc) > struct intel_guc *guc = >->uc.guc; > int ret; > > - GEM_BUG_ON(intel_huc_is_authenticated(huc)); > + GEM_BUG_ON(huc_is_authenticated(huc)); > > if (!intel_uc_fw_is_loaded(&huc->fw)) > return -ENOEXEC; > @@ -150,10 +162,6 @@ int intel_huc_auth(struct intel_huc *huc) > */ > int intel_huc_check_status(struct intel_huc *huc) > { > - struct intel_gt *gt = huc_to_gt(huc); > - intel_wakeref_t wakeref; > - u32 status = 0; > - > switch (__intel_uc_fw_status(&huc->fw)) { > case INTEL_UC_FIRMWARE_NOT_SUPPORTED: > return -ENODEV; > @@ -167,10 +175,7 @@ int intel_huc_check_status(struct intel_huc *huc) > 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; oh, these variable names look so generic, while it looks like the only usage for them is for mask = loaded and value = loaded... But anyway it is better this indirection with some generic name than duplicating the definition depending on platform in here... so: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > + return huc_is_authenticated(huc); > } > > /** > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > index 73ec670800f2b..77d813840d76c 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > @@ -50,11 +50,6 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) > return intel_uc_fw_is_available(&huc->fw); > } > > -static inline bool intel_huc_is_authenticated(struct intel_huc *huc) > -{ > - return intel_uc_fw_is_running(&huc->fw); > -} > - > void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p); > > #endif > -- > 2.25.1 >
On 4/26/2022 5:26 PM, Daniele Ceraolo Spurio wrote: > The huc_is_authenticated function return is based on our SW tracking of > the HuC auth status. However, around suspend/resume and reset this can > go out of sync with the actual HW state, which is why we use > huc_check_state() to look at the actual HW state. Instead of having this > duality, just make huc_is_authenticated() return the HW state and use it > everywhere we need to know if HuC is running. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_huc.c | 23 ++++++++++++++--------- > drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ----- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > index 556829de9c172..773020e69589a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > @@ -80,6 +80,18 @@ void intel_huc_fini(struct intel_huc *huc) > intel_uc_fw_fini(&huc->fw); > } > > +static bool huc_is_authenticated(struct intel_huc *huc) > +{ > + struct intel_gt *gt = huc_to_gt(huc); > + intel_wakeref_t wakeref; > + u32 status = 0; > + > + 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; > +} > + > /** > * intel_huc_auth() - Authenticate HuC uCode > * @huc: intel_huc structure > @@ -96,7 +108,7 @@ int intel_huc_auth(struct intel_huc *huc) > struct intel_guc *guc = >->uc.guc; > int ret; > > - GEM_BUG_ON(intel_huc_is_authenticated(huc)); > + GEM_BUG_ON(huc_is_authenticated(huc)); It looks like even on gen9 HuC is surviving the reset, so this BUG_ON is now being triggered. I'm going to just change this to a BUG_ON on intel_uc_fw_is_running() for now, which would be equivalent to what we have right now, and in the meantime I'll follow up with the HuC team to see if we can just skip this (and the huc_fw_upload) if HuC shows up as already authenticated. Daniele > > if (!intel_uc_fw_is_loaded(&huc->fw)) > return -ENOEXEC; > @@ -150,10 +162,6 @@ int intel_huc_auth(struct intel_huc *huc) > */ > int intel_huc_check_status(struct intel_huc *huc) > { > - struct intel_gt *gt = huc_to_gt(huc); > - intel_wakeref_t wakeref; > - u32 status = 0; > - > switch (__intel_uc_fw_status(&huc->fw)) { > case INTEL_UC_FIRMWARE_NOT_SUPPORTED: > return -ENODEV; > @@ -167,10 +175,7 @@ int intel_huc_check_status(struct intel_huc *huc) > 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; > + return huc_is_authenticated(huc); > } > > /** > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > index 73ec670800f2b..77d813840d76c 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > @@ -50,11 +50,6 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) > return intel_uc_fw_is_available(&huc->fw); > } > > -static inline bool intel_huc_is_authenticated(struct intel_huc *huc) > -{ > - return intel_uc_fw_is_running(&huc->fw); > -} > - > void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p); > > #endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 556829de9c172..773020e69589a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -80,6 +80,18 @@ void intel_huc_fini(struct intel_huc *huc) intel_uc_fw_fini(&huc->fw); } +static bool huc_is_authenticated(struct intel_huc *huc) +{ + struct intel_gt *gt = huc_to_gt(huc); + intel_wakeref_t wakeref; + u32 status = 0; + + 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; +} + /** * intel_huc_auth() - Authenticate HuC uCode * @huc: intel_huc structure @@ -96,7 +108,7 @@ int intel_huc_auth(struct intel_huc *huc) struct intel_guc *guc = >->uc.guc; int ret; - GEM_BUG_ON(intel_huc_is_authenticated(huc)); + GEM_BUG_ON(huc_is_authenticated(huc)); if (!intel_uc_fw_is_loaded(&huc->fw)) return -ENOEXEC; @@ -150,10 +162,6 @@ int intel_huc_auth(struct intel_huc *huc) */ int intel_huc_check_status(struct intel_huc *huc) { - struct intel_gt *gt = huc_to_gt(huc); - intel_wakeref_t wakeref; - u32 status = 0; - switch (__intel_uc_fw_status(&huc->fw)) { case INTEL_UC_FIRMWARE_NOT_SUPPORTED: return -ENODEV; @@ -167,10 +175,7 @@ int intel_huc_check_status(struct intel_huc *huc) 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; + return huc_is_authenticated(huc); } /** diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index 73ec670800f2b..77d813840d76c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -50,11 +50,6 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) return intel_uc_fw_is_available(&huc->fw); } -static inline bool intel_huc_is_authenticated(struct intel_huc *huc) -{ - return intel_uc_fw_is_running(&huc->fw); -} - void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p); #endif
The huc_is_authenticated function return is based on our SW tracking of the HuC auth status. However, around suspend/resume and reset this can go out of sync with the actual HW state, which is why we use huc_check_state() to look at the actual HW state. Instead of having this duality, just make huc_is_authenticated() return the HW state and use it everywhere we need to know if HuC is running. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 23 ++++++++++++++--------- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 ----- 2 files changed, 14 insertions(+), 14 deletions(-)