| Message ID | 20181016113414.31356-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/huc: Normalize HuC status returned by I915_PARAM_HAS_HUC | expand |
On Tue, 16 Oct 2018 14:16:48 +0200, Patchwork <patchwork@emeril.freedesktop.org> wrote: > == Series Details == > > Series: drm/i915/huc: Normalize HuC status returned by I915_PARAM_HAS_HUC > URL : https://patchwork.freedesktop.org/series/51060/ > State : failure > > == Summary == > > = CI Bug Log - changes from CI_DRM_4985 -> Patchwork_10471 = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_10471 absolutely need to > be > verified manually. > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_10471, please notify your bug team to allow > them > to document this new failure mode, which will reduce false positives > in CI. > > External URL: > https://patchwork.freedesktop.org/api/1.0/series/51060/revisions/1/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in > Patchwork_10471: > > === IGT changes === > > ==== Possible regressions ==== > > igt@drv_module_reload@basic-no-display: > fi-bwr-2160: PASS -> INCOMPLETE Nice try, but bwr does not even have GuC/HuC, so unrelated Michal
On Tue, Oct 16, 2018 at 11:34:14AM +0000, Michal Wajdeczko wrote: > In response for I915_PARAM_HAS_HUC we are returning value that > indicates if HuC firmware was loaded and verified. However, our > previously used positive value was based on specific register bit > which is about to change on future platform. Let's normalize our > return values to 0 and 1 before clients will start to use Gen9 value. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Haihao Xiang <haihao.xiang@intel.com> It's "tweaking" the ABI, but hopefully we can avoid breaking anything. intel-vaapi-driver is doing !!(getparam) and media-driver is being optimistic (i.e. assuming that HuC is present without any checks). I don't know of any other components using this, and as you've mentioned in the commit message - HuC is still under unsafe param, so: Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> -Michał > --- > drivers/gpu/drm/i915/intel_huc.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-)
Quoting Michal Wajdeczko (2018-10-16 12:34:14) > In response for I915_PARAM_HAS_HUC we are returning value that > indicates if HuC firmware was loaded and verified. However, our > previously used positive value was based on specific register bit > which is about to change on future platform. Let's normalize our > return values to 0 and 1 before clients will start to use Gen9 value. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Haihao Xiang <haihao.xiang@intel.com> > --- > drivers/gpu/drm/i915/intel_huc.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c > index 37ef540d..46498aa 100644 > --- a/drivers/gpu/drm/i915/intel_huc.c > +++ b/drivers/gpu/drm/i915/intel_huc.c > @@ -108,8 +108,9 @@ int intel_huc_auth(struct intel_huc *huc) > * This function reads status register to verify if HuC > * firmware was successfully loaded. > * > - * Returns positive value if HuC firmware is loaded and verified > - * and -ENODEV if HuC is not present. > + * 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. > */ > int intel_huc_check_status(struct intel_huc *huc) > { > @@ -120,8 +121,8 @@ int intel_huc_check_status(struct intel_huc *huc) > return -ENODEV; > > intel_runtime_pm_get(dev_priv); > - status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; > + status = I915_READ(HUC_STATUS2); > intel_runtime_pm_put(dev_priv); > > - return status; > + return status & HUC_FW_VERIFIED ? 1 : 0; Wouldn't it have been simpler to just change u32 status to bool status Then I wouldn't even have to look at a ternary for a boolean. -Chris
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index 37ef540d..46498aa 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -108,8 +108,9 @@ int intel_huc_auth(struct intel_huc *huc) * This function reads status register to verify if HuC * firmware was successfully loaded. * - * Returns positive value if HuC firmware is loaded and verified - * and -ENODEV if HuC is not present. + * 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. */ int intel_huc_check_status(struct intel_huc *huc) { @@ -120,8 +121,8 @@ int intel_huc_check_status(struct intel_huc *huc) return -ENODEV; intel_runtime_pm_get(dev_priv); - status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED; + status = I915_READ(HUC_STATUS2); intel_runtime_pm_put(dev_priv); - return status; + return status & HUC_FW_VERIFIED ? 1 : 0; }
In response for I915_PARAM_HAS_HUC we are returning value that indicates if HuC firmware was loaded and verified. However, our previously used positive value was based on specific register bit which is about to change on future platform. Let's normalize our return values to 0 and 1 before clients will start to use Gen9 value. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Haihao Xiang <haihao.xiang@intel.com> --- drivers/gpu/drm/i915/intel_huc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)