* [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS @ 2020-01-22 19:48 Michal Wajdeczko 2020-01-22 23:52 ` Daniele Ceraolo Spurio ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Michal Wajdeczko @ 2020-01-22 19:48 UTC (permalink / raw) To: intel-gfx 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. 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> --- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 32a069841c14..ad4d9e16a24e 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -209,7 +209,7 @@ 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; with_intel_runtime_pm(gt->uncore->rpm, wakeref) -- 2.19.2 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-22 19:48 [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS Michal Wajdeczko @ 2020-01-22 23:52 ` Daniele Ceraolo Spurio 2020-01-23 15:02 ` Chris Wilson 2020-01-23 2:43 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork 2020-01-23 18:12 ` [Intel-gfx] [PATCH] " Ye, Tony 2 siblings, 1 reply; 19+ messages in thread From: Daniele Ceraolo Spurio @ 2020-01-22 23:52 UTC (permalink / raw) To: Michal Wajdeczko, intel-gfx On 1/22/20 11:48 AM, 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. > > 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> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Do we need a fixes tag? Daniele > --- > drivers/gpu/drm/i915/gt/uc/intel_huc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > index 32a069841c14..ad4d9e16a24e 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > @@ -209,7 +209,7 @@ 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; > > with_intel_runtime_pm(gt->uncore->rpm, wakeref) > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-22 23:52 ` Daniele Ceraolo Spurio @ 2020-01-23 15:02 ` Chris Wilson 2020-01-23 15:38 ` Michal Wajdeczko 0 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2020-01-23 15:02 UTC (permalink / raw) To: Daniele Ceraolo Spurio, Michal Wajdeczko, intel-gfx Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) > > > On 1/22/20 11:48 AM, 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. > > > > 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> > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Once upon a time did you (Michal) not argue we should indicate the lack of firmware in the error code? Something like if (!HAS_GT_UC(gt->i915)) return -ENODEV; if (!intel_huc_is_supported(huc)) return -ENOEXEC; _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-23 15:02 ` Chris Wilson @ 2020-01-23 15:38 ` Michal Wajdeczko 2020-01-23 15:51 ` Chris Wilson ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Michal Wajdeczko @ 2020-01-23 15:38 UTC (permalink / raw) To: Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >> >> >> On 1/22/20 11:48 AM, 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. >> > >> > 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> >> >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Once upon a time did you (Michal) not argue we should indicate the lack > of firmware in the error code? Something like > > if (!HAS_GT_UC(gt->i915)) > return -ENODEV; > > if (!intel_huc_is_supported(huc)) > return -ENOEXEC; Yes, we discussed this here [1] together with [2] but we didn't conclude our discussion due to different opinions on how represent some states, in particular "manually disabled" state. In this patch I just wanted to restore old notation. But we can start new discussion, here is summary: ------------------+----------+----------+---------- HuC state | today* | option A | option B ------------------+----------+----------+---------- no HuC hardware | -ENODEV | -ENODEV | -ENODEV GuC fw disabled | 0 | 0 | -EOPNOTSUPP HuC fw disabled | 0 | 0 | -EOPNOTSUPP HuC fw missing | 0 | -ENOPKG | -ENOEXEC HuC fw error | 0 | -ENOEXEC | -ENOEXEC HuC fw fail | 0 | -EACCES | 0 HuC authenticated | 1 | 1 | 1 ------------------+----------+----------+---------- Note that all above should be compatible with media driver, which explicitly looks for no error and value 1 Michal [1] https://patchwork.freedesktop.org/patch/306419/?series=61001&rev=1 [2] https://patchwork.freedesktop.org/series/60800/#rev1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-23 15:38 ` Michal Wajdeczko @ 2020-01-23 15:51 ` Chris Wilson 2020-01-26 17:41 ` Michal Wajdeczko 2020-01-23 18:26 ` Ye, Tony 2020-01-23 19:50 ` Ye, Tony 2 siblings, 1 reply; 19+ messages in thread From: Chris Wilson @ 2020-01-23 15:51 UTC (permalink / raw) To: Daniele Ceraolo Spurio, Michal Wajdeczko, intel-gfx Quoting Michal Wajdeczko (2020-01-23 15:38:52) > On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) > >> > >> > >> On 1/22/20 11:48 AM, 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. > >> > > >> > 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> > >> > >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > > Once upon a time did you (Michal) not argue we should indicate the lack > > of firmware in the error code? Something like > > > > if (!HAS_GT_UC(gt->i915)) > > return -ENODEV; > > > > if (!intel_huc_is_supported(huc)) > > return -ENOEXEC; > > Yes, we discussed this here [1] together with [2] but we didn't > conclude our discussion due to different opinions on how represent > some states, in particular "manually disabled" state. > > In this patch I just wanted to restore old notation. > > But we can start new discussion, here is summary: > > ------------------+----------+----------+---------- > HuC state | today* | option A | option B > ------------------+----------+----------+---------- > no HuC hardware | -ENODEV | -ENODEV | -ENODEV > GuC fw disabled | 0 | 0 | -EOPNOTSUPP > HuC fw disabled | 0 | 0 | -EOPNOTSUPP > HuC fw missing | 0 | -ENOPKG | -ENOEXEC > HuC fw error | 0 | -ENOEXEC | -ENOEXEC > HuC fw fail | 0 | -EACCES | 0 > HuC authenticated | 1 | 1 | 1 > ------------------+----------+----------+---------- By fw fail, you mean we loaded the firmware (to our knowledge) correctly, but HUC_STATUS is not reported as valid? If so, I support option B. I like the idea of saying "no HuC" (machine too old) "no firmware" (user action, or lack thereof) 0 (fw unhappy) 1 (fw reports success) In between states for failures in fw loading? Not so sure. But I can see the nicety in distinguishing between lack of firmware and some random failure in loading the firmware (the former being user action required to rectify, command line parameter whatever and the latter being the firmware file is either invalid or a stray neutrino prevented loading). Imo the error messages should be about why we cannot probe/trust the HUC_STATUS register. If everything is setup correctly then the returned value should be from reading the register. I dislike only returning 1 if supported, and converting a valid read of 0 into another error. So Option B :) > Note that all above should be compatible with media driver, > which explicitly looks for no error and value 1 Cool. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-23 15:51 ` Chris Wilson @ 2020-01-26 17:41 ` Michal Wajdeczko 2020-02-05 0:43 ` Ye, Tony 0 siblings, 1 reply; 19+ messages in thread From: Michal Wajdeczko @ 2020-01-26 17:41 UTC (permalink / raw) To: Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2020-01-23 15:38:52) >> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >> > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >> >> >> >> >> >> On 1/22/20 11:48 AM, 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. >> >> > >> >> > 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> >> >> >> >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> > >> > Once upon a time did you (Michal) not argue we should indicate the >> lack >> > of firmware in the error code? Something like >> > >> > if (!HAS_GT_UC(gt->i915)) >> > return -ENODEV; >> > >> > if (!intel_huc_is_supported(huc)) >> > return -ENOEXEC; >> >> Yes, we discussed this here [1] together with [2] but we didn't >> conclude our discussion due to different opinions on how represent >> some states, in particular "manually disabled" state. >> >> In this patch I just wanted to restore old notation. >> >> But we can start new discussion, here is summary: >> >> ------------------+----------+----------+---------- >> HuC state | today* | option A | option B >> ------------------+----------+----------+---------- >> no HuC hardware | -ENODEV | -ENODEV | -ENODEV >> GuC fw disabled | 0 | 0 | -EOPNOTSUPP >> HuC fw disabled | 0 | 0 | -EOPNOTSUPP >> HuC fw missing | 0 | -ENOPKG | -ENOEXEC >> HuC fw error | 0 | -ENOEXEC | -ENOEXEC >> HuC fw fail | 0 | -EACCES | 0 >> HuC authenticated | 1 | 1 | 1 >> ------------------+----------+----------+---------- > > By fw fail, you mean we loaded the firmware (to our knowledge) > correctly, but HUC_STATUS is not reported as valid? > > If so, I support option B. I like the idea of saying > "no HuC" (machine too old) > "no firmware" (user action, or lack thereof) > 0 (fw unhappy) > 1 (fw reports success) > > In between states for failures in fw loading? Not so sure. But I can see > the nicety in distinguishing between lack of firmware and some random > failure in loading the firmware (the former being user action required > to rectify, command line parameter whatever and the latter being the > firmware file is either invalid or a stray neutrino prevented loading). > > Imo the error messages should be about why we cannot probe/trust the > HUC_STATUS register. If everything is setup correctly then the returned > value should be from reading the register. I dislike only returning 1 if > supported, and converting a valid read of 0 into another error. > > So Option B :) But I'm not sure that option B is consistent in error reporting, as "fw unhappy" is definitely an serious error but is represented as plain non-error "0" status, while "fw disabled" (user action) is treated as error ------------------+---------- HuC state | option B ------------------+---------- no HuC hardware | -ENODEV GuC fw disabled | -EOPNOTSUPP -> user decision, why error? HuC fw disabled | -EOPNOTSUPP -> user decision, why error? HuC fw missing | -ENOEXEC HuC fw error | -ENOEXEC HuC fw fail | 0 -> unlikely, but still fw/hw error HuC authenticated | 1 ------------------+---------- On other hand, option A treats all error conditions as errors, leaving status codes only for normal operations: disabled(0)/authenticated(1): ------------------+---------- HuC state | option A ------------------+---------- no HuC hardware | -ENODEV -> you shouldn't ask GuC fw disabled | 0 -> user decision, not an error HuC fw disabled | 0 -> user decision, not an error HuC fw missing | -ENOPKG -> fw not installed correctly HuC fw error | -ENOEXEC -> bad/wrong fw HuC fw fail | -EACCES -> fw/hw error HuC authenticated | 1 ------------------+---------- But since I'm not an active HuC user, will leave final decision to others. /Michal > >> Note that all above should be compatible with media driver, >> which explicitly looks for no error and value 1 > > Cool. > -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-26 17:41 ` Michal Wajdeczko @ 2020-02-05 0:43 ` Ye, Tony 2020-02-11 17:53 ` Fosha, Robert M 0 siblings, 1 reply; 19+ messages in thread From: Ye, Tony @ 2020-02-05 0:43 UTC (permalink / raw) To: Michal Wajdeczko, Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On 1/27/2020 1:41 AM, Michal Wajdeczko wrote: > On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > >> Quoting Michal Wajdeczko (2020-01-23 15:38:52) >>> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson >>> <chris@chris-wilson.co.uk> wrote: >>> >>> > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>> >> >>> >> >>> >> On 1/22/20 11:48 AM, 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. >>> >> > >>> >> > 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> >>> >> >>> >> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> > >>> > Once upon a time did you (Michal) not argue we should indicate the >>> lack >>> > of firmware in the error code? Something like >>> > >>> > if (!HAS_GT_UC(gt->i915)) >>> > return -ENODEV; >>> > >>> > if (!intel_huc_is_supported(huc)) >>> > return -ENOEXEC; >>> >>> Yes, we discussed this here [1] together with [2] but we didn't >>> conclude our discussion due to different opinions on how represent >>> some states, in particular "manually disabled" state. >>> >>> In this patch I just wanted to restore old notation. >>> >>> But we can start new discussion, here is summary: >>> >>> ------------------+----------+----------+---------- >>> HuC state | today* | option A | option B >>> ------------------+----------+----------+---------- >>> no HuC hardware | -ENODEV | -ENODEV | -ENODEV >>> GuC fw disabled | 0 | 0 | -EOPNOTSUPP >>> HuC fw disabled | 0 | 0 | -EOPNOTSUPP >>> HuC fw missing | 0 | -ENOPKG | -ENOEXEC >>> HuC fw error | 0 | -ENOEXEC | -ENOEXEC >>> HuC fw fail | 0 | -EACCES | 0 >>> HuC authenticated | 1 | 1 | 1 >>> ------------------+----------+----------+---------- >> >> By fw fail, you mean we loaded the firmware (to our knowledge) >> correctly, but HUC_STATUS is not reported as valid? >> >> If so, I support option B. I like the idea of saying >> "no HuC" (machine too old) >> "no firmware" (user action, or lack thereof) >> 0 (fw unhappy) >> 1 (fw reports success) >> >> In between states for failures in fw loading? Not so sure. But I can see >> the nicety in distinguishing between lack of firmware and some random >> failure in loading the firmware (the former being user action required >> to rectify, command line parameter whatever and the latter being the >> firmware file is either invalid or a stray neutrino prevented loading). >> >> Imo the error messages should be about why we cannot probe/trust the >> HUC_STATUS register. If everything is setup correctly then the returned >> value should be from reading the register. I dislike only returning 1 if >> supported, and converting a valid read of 0 into another error. >> >> So Option B :) > > But I'm not sure that option B is consistent in error reporting, as > "fw unhappy" is definitely an serious error but is represented as plain > non-error "0" status, while "fw disabled" (user action) is treated as error > > ------------------+---------- > HuC state | option B > ------------------+---------- > no HuC hardware | -ENODEV > GuC fw disabled | -EOPNOTSUPP -> user decision, why error? > HuC fw disabled | -EOPNOTSUPP -> user decision, why error? > HuC fw missing | -ENOEXEC > HuC fw error | -ENOEXEC > HuC fw fail | 0 -> unlikely, but still fw/hw error > HuC authenticated | 1 > ------------------+---------- > > On other hand, option A treats all error conditions as errors, leaving > status codes only for normal operations: disabled(0)/authenticated(1): > > ------------------+---------- > HuC state | option A > ------------------+---------- > no HuC hardware | -ENODEV -> you shouldn't ask > GuC fw disabled | 0 -> user decision, not an error > HuC fw disabled | 0 -> user decision, not an error > HuC fw missing | -ENOPKG -> fw not installed correctly > HuC fw error | -ENOEXEC -> bad/wrong fw > HuC fw fail | -EACCES -> fw/hw error > HuC authenticated | 1 > ------------------+---------- Vote for Option A. Regards, Tony > > But since I'm not an active HuC user, will leave final decision to others. > > /Michal > > >> >>> Note that all above should be compatible with media driver, >>> which explicitly looks for no error and value 1 >> >> Cool. >> -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-02-05 0:43 ` Ye, Tony @ 2020-02-11 17:53 ` Fosha, Robert M 2020-02-11 19:57 ` Michal Wajdeczko 0 siblings, 1 reply; 19+ messages in thread From: Fosha, Robert M @ 2020-02-11 17:53 UTC (permalink / raw) To: Ye, Tony, Michal Wajdeczko, Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On 2/4/20 4:43 PM, Ye, Tony wrote: > > > On 1/27/2020 1:41 AM, Michal Wajdeczko wrote: >> On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >>> Quoting Michal Wajdeczko (2020-01-23 15:38:52) >>>> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson >>>> <chris@chris-wilson.co.uk> wrote: >>>> >>>> > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>>> >> >>>> >> >>>> >> On 1/22/20 11:48 AM, 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. >>>> >> > >>>> >> > 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> >>>> >> >>>> >> Reviewed-by: Daniele Ceraolo Spurio >>>> <daniele.ceraolospurio@intel.com> >>>> > >>>> > Once upon a time did you (Michal) not argue we should indicate >>>> the lack >>>> > of firmware in the error code? Something like >>>> > >>>> > if (!HAS_GT_UC(gt->i915)) >>>> > return -ENODEV; >>>> > >>>> > if (!intel_huc_is_supported(huc)) >>>> > return -ENOEXEC; >>>> >>>> Yes, we discussed this here [1] together with [2] but we didn't >>>> conclude our discussion due to different opinions on how represent >>>> some states, in particular "manually disabled" state. >>>> >>>> In this patch I just wanted to restore old notation. >>>> >>>> But we can start new discussion, here is summary: >>>> >>>> ------------------+----------+----------+---------- >>>> HuC state | today* | option A | option B >>>> ------------------+----------+----------+---------- >>>> no HuC hardware | -ENODEV | -ENODEV | -ENODEV >>>> GuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>> HuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>> HuC fw missing | 0 | -ENOPKG | -ENOEXEC >>>> HuC fw error | 0 | -ENOEXEC | -ENOEXEC >>>> HuC fw fail | 0 | -EACCES | 0 >>>> HuC authenticated | 1 | 1 | 1 >>>> ------------------+----------+----------+---------- >>> >>> By fw fail, you mean we loaded the firmware (to our knowledge) >>> correctly, but HUC_STATUS is not reported as valid? >>> >>> If so, I support option B. I like the idea of saying >>> "no HuC" (machine too old) >>> "no firmware" (user action, or lack thereof) >>> 0 (fw unhappy) >>> 1 (fw reports success) >>> >>> In between states for failures in fw loading? Not so sure. But I can >>> see >>> the nicety in distinguishing between lack of firmware and some random >>> failure in loading the firmware (the former being user action required >>> to rectify, command line parameter whatever and the latter being the >>> firmware file is either invalid or a stray neutrino prevented loading). >>> >>> Imo the error messages should be about why we cannot probe/trust the >>> HUC_STATUS register. If everything is setup correctly then the returned >>> value should be from reading the register. I dislike only returning >>> 1 if >>> supported, and converting a valid read of 0 into another error. >>> >>> So Option B :) >> >> But I'm not sure that option B is consistent in error reporting, as >> "fw unhappy" is definitely an serious error but is represented as plain >> non-error "0" status, while "fw disabled" (user action) is treated as >> error >> >> ------------------+---------- >> HuC state | option B >> ------------------+---------- >> no HuC hardware | -ENODEV >> GuC fw disabled | -EOPNOTSUPP -> user decision, why error? >> HuC fw disabled | -EOPNOTSUPP -> user decision, why error? >> HuC fw missing | -ENOEXEC >> HuC fw error | -ENOEXEC >> HuC fw fail | 0 -> unlikely, but still fw/hw error >> HuC authenticated | 1 >> ------------------+---------- >> >> On other hand, option A treats all error conditions as errors, leaving >> status codes only for normal operations: disabled(0)/authenticated(1): >> >> ------------------+---------- >> HuC state | option A >> ------------------+---------- >> no HuC hardware | -ENODEV -> you shouldn't ask >> GuC fw disabled | 0 -> user decision, not an error >> HuC fw disabled | 0 -> user decision, not an error >> HuC fw missing | -ENOPKG -> fw not installed correctly >> HuC fw error | -ENOEXEC -> bad/wrong fw >> HuC fw fail | -EACCES -> fw/hw error >> HuC authenticated | 1 >> ------------------+---------- > > Vote for Option A. > > Regards, > Tony > Are we ok to move forward on this? Michal, are you working on updating the patch? -Rob >> >> But since I'm not an active HuC user, will leave final decision to >> others. >> >> /Michal >> >> >>> >>>> Note that all above should be compatible with media driver, >>>> which explicitly looks for no error and value 1 >>> >>> Cool. >>> -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-02-11 17:53 ` Fosha, Robert M @ 2020-02-11 19:57 ` Michal Wajdeczko 2020-02-11 21:54 ` Fosha, Robert M 0 siblings, 1 reply; 19+ messages in thread From: Michal Wajdeczko @ 2020-02-11 19:57 UTC (permalink / raw) To: Ye, Tony, Daniele Ceraolo Spurio, intel-gfx, Chris Wilson, Fosha, Robert M On Tue, 11 Feb 2020 18:53:05 +0100, Fosha, Robert M <robert.m.fosha@intel.com> wrote: > > > On 2/4/20 4:43 PM, Ye, Tony wrote: >> >> >> On 1/27/2020 1:41 AM, Michal Wajdeczko wrote: >>> On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson >>> <chris@chris-wilson.co.uk> wrote: >>> >>>> Quoting Michal Wajdeczko (2020-01-23 15:38:52) >>>>> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson >>>>> <chris@chris-wilson.co.uk> wrote: >>>>> >>>>> > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>>>> >> >>>>> >> >>>>> >> On 1/22/20 11:48 AM, 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. >>>>> >> > >>>>> >> > 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> >>>>> >> >>>>> >> Reviewed-by: Daniele Ceraolo Spurio >>>>> <daniele.ceraolospurio@intel.com> >>>>> > >>>>> > Once upon a time did you (Michal) not argue we should indicate the >>>>> lack >>>>> > of firmware in the error code? Something like >>>>> > >>>>> > if (!HAS_GT_UC(gt->i915)) >>>>> > return -ENODEV; >>>>> > >>>>> > if (!intel_huc_is_supported(huc)) >>>>> > return -ENOEXEC; >>>>> >>>>> Yes, we discussed this here [1] together with [2] but we didn't >>>>> conclude our discussion due to different opinions on how represent >>>>> some states, in particular "manually disabled" state. >>>>> >>>>> In this patch I just wanted to restore old notation. >>>>> >>>>> But we can start new discussion, here is summary: >>>>> >>>>> ------------------+----------+----------+---------- >>>>> HuC state | today* | option A | option B >>>>> ------------------+----------+----------+---------- >>>>> no HuC hardware | -ENODEV | -ENODEV | -ENODEV >>>>> GuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>>> HuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>>> HuC fw missing | 0 | -ENOPKG | -ENOEXEC >>>>> HuC fw error | 0 | -ENOEXEC | -ENOEXEC >>>>> HuC fw fail | 0 | -EACCES | 0 >>>>> HuC authenticated | 1 | 1 | 1 >>>>> ------------------+----------+----------+---------- >>>> >>>> By fw fail, you mean we loaded the firmware (to our knowledge) >>>> correctly, but HUC_STATUS is not reported as valid? >>>> >>>> If so, I support option B. I like the idea of saying >>>> "no HuC" (machine too old) >>>> "no firmware" (user action, or lack thereof) >>>> 0 (fw unhappy) >>>> 1 (fw reports success) >>>> >>>> In between states for failures in fw loading? Not so sure. But I can >>>> see >>>> the nicety in distinguishing between lack of firmware and some random >>>> failure in loading the firmware (the former being user action required >>>> to rectify, command line parameter whatever and the latter being the >>>> firmware file is either invalid or a stray neutrino prevented >>>> loading). >>>> >>>> Imo the error messages should be about why we cannot probe/trust the >>>> HUC_STATUS register. If everything is setup correctly then the >>>> returned >>>> value should be from reading the register. I dislike only returning 1 >>>> if >>>> supported, and converting a valid read of 0 into another error. >>>> >>>> So Option B :) >>> >>> But I'm not sure that option B is consistent in error reporting, as >>> "fw unhappy" is definitely an serious error but is represented as plain >>> non-error "0" status, while "fw disabled" (user action) is treated as >>> error >>> >>> ------------------+---------- >>> HuC state | option B >>> ------------------+---------- >>> no HuC hardware | -ENODEV >>> GuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>> HuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>> HuC fw missing | -ENOEXEC >>> HuC fw error | -ENOEXEC >>> HuC fw fail | 0 -> unlikely, but still fw/hw error >>> HuC authenticated | 1 >>> ------------------+---------- >>> >>> On other hand, option A treats all error conditions as errors, leaving >>> status codes only for normal operations: disabled(0)/authenticated(1): >>> >>> ------------------+---------- >>> HuC state | option A >>> ------------------+---------- >>> no HuC hardware | -ENODEV -> you shouldn't ask >>> GuC fw disabled | 0 -> user decision, not an error >>> HuC fw disabled | 0 -> user decision, not an error >>> HuC fw missing | -ENOPKG -> fw not installed correctly >>> HuC fw error | -ENOEXEC -> bad/wrong fw >>> HuC fw fail | -EACCES -> fw/hw error >>> HuC authenticated | 1 >>> ------------------+---------- >> >> Vote for Option A. >> >> Regards, >> Tony >> > > Are we ok to move forward on this? Michal, are you working on updating > the patch? I can update the patch, but I don't know which option to implement: Tony votes for Option A, Chris for Option B, what's your choice? Michal > > -Rob > >>> >>> But since I'm not an active HuC user, will leave final decision to >>> others. >>> >>> /Michal >>> >>> >>>> >>>>> Note that all above should be compatible with media driver, >>>>> which explicitly looks for no error and value 1 >>>> >>>> Cool. >>>> -Chris >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-02-11 19:57 ` Michal Wajdeczko @ 2020-02-11 21:54 ` Fosha, Robert M 2020-02-21 3:56 ` Ye, Tony 0 siblings, 1 reply; 19+ messages in thread From: Fosha, Robert M @ 2020-02-11 21:54 UTC (permalink / raw) To: Michal Wajdeczko, Ye, Tony, Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On 2/11/20 11:57 AM, Michal Wajdeczko wrote: > On Tue, 11 Feb 2020 18:53:05 +0100, Fosha, Robert M > <robert.m.fosha@intel.com> wrote: > >> >> >> On 2/4/20 4:43 PM, Ye, Tony wrote: >>> >>> >>> On 1/27/2020 1:41 AM, Michal Wajdeczko wrote: >>>> On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson >>>> <chris@chris-wilson.co.uk> wrote: >>>> >>>>> Quoting Michal Wajdeczko (2020-01-23 15:38:52) >>>>>> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson >>>>>> <chris@chris-wilson.co.uk> wrote: >>>>>> >>>>>> > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>>>>> >> >>>>>> >> >>>>>> >> On 1/22/20 11:48 AM, 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. >>>>>> >> > >>>>>> >> > 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> >>>>>> >> >>>>>> >> Reviewed-by: Daniele Ceraolo Spurio >>>>>> <daniele.ceraolospurio@intel.com> >>>>>> > >>>>>> > Once upon a time did you (Michal) not argue we should indicate >>>>>> the lack >>>>>> > of firmware in the error code? Something like >>>>>> > >>>>>> > if (!HAS_GT_UC(gt->i915)) >>>>>> > return -ENODEV; >>>>>> > >>>>>> > if (!intel_huc_is_supported(huc)) >>>>>> > return -ENOEXEC; >>>>>> >>>>>> Yes, we discussed this here [1] together with [2] but we didn't >>>>>> conclude our discussion due to different opinions on how represent >>>>>> some states, in particular "manually disabled" state. >>>>>> >>>>>> In this patch I just wanted to restore old notation. >>>>>> >>>>>> But we can start new discussion, here is summary: >>>>>> >>>>>> ------------------+----------+----------+---------- >>>>>> HuC state | today* | option A | option B >>>>>> ------------------+----------+----------+---------- >>>>>> no HuC hardware | -ENODEV | -ENODEV | -ENODEV >>>>>> GuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>>>> HuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>>>> HuC fw missing | 0 | -ENOPKG | -ENOEXEC >>>>>> HuC fw error | 0 | -ENOEXEC | -ENOEXEC >>>>>> HuC fw fail | 0 | -EACCES | 0 >>>>>> HuC authenticated | 1 | 1 | 1 >>>>>> ------------------+----------+----------+---------- >>>>> >>>>> By fw fail, you mean we loaded the firmware (to our knowledge) >>>>> correctly, but HUC_STATUS is not reported as valid? >>>>> >>>>> If so, I support option B. I like the idea of saying >>>>> "no HuC" (machine too old) >>>>> "no firmware" (user action, or lack thereof) >>>>> 0 (fw unhappy) >>>>> 1 (fw reports success) >>>>> >>>>> In between states for failures in fw loading? Not so sure. But I >>>>> can see >>>>> the nicety in distinguishing between lack of firmware and some random >>>>> failure in loading the firmware (the former being user action >>>>> required >>>>> to rectify, command line parameter whatever and the latter being the >>>>> firmware file is either invalid or a stray neutrino prevented >>>>> loading). >>>>> >>>>> Imo the error messages should be about why we cannot probe/trust the >>>>> HUC_STATUS register. If everything is setup correctly then the >>>>> returned >>>>> value should be from reading the register. I dislike only >>>>> returning 1 if >>>>> supported, and converting a valid read of 0 into another error. >>>>> >>>>> So Option B :) >>>> >>>> But I'm not sure that option B is consistent in error reporting, as >>>> "fw unhappy" is definitely an serious error but is represented as >>>> plain >>>> non-error "0" status, while "fw disabled" (user action) is treated >>>> as error >>>> >>>> ------------------+---------- >>>> HuC state | option B >>>> ------------------+---------- >>>> no HuC hardware | -ENODEV >>>> GuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>>> HuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>>> HuC fw missing | -ENOEXEC >>>> HuC fw error | -ENOEXEC >>>> HuC fw fail | 0 -> unlikely, but still fw/hw error >>>> HuC authenticated | 1 >>>> ------------------+---------- >>>> >>>> On other hand, option A treats all error conditions as errors, leaving >>>> status codes only for normal operations: disabled(0)/authenticated(1): >>>> >>>> ------------------+---------- >>>> HuC state | option A >>>> ------------------+---------- >>>> no HuC hardware | -ENODEV -> you shouldn't ask >>>> GuC fw disabled | 0 -> user decision, not an error >>>> HuC fw disabled | 0 -> user decision, not an error >>>> HuC fw missing | -ENOPKG -> fw not installed correctly >>>> HuC fw error | -ENOEXEC -> bad/wrong fw >>>> HuC fw fail | -EACCES -> fw/hw error >>>> HuC authenticated | 1 >>>> ------------------+---------- >>> >>> Vote for Option A. >>> >>> Regards, >>> Tony >>> >> >> Are we ok to move forward on this? Michal, are you working on >> updating the patch? > > I can update the patch, but I don't know which option to implement: > Tony votes for Option A, Chris for Option B, what's your choice? > > Michal > I don't have a preference and would trust both Tony and Chris over myself. However, if Tony is using HuC more I would vote for his prefered option. -Rob >> >> -Rob >> >>>> >>>> But since I'm not an active HuC user, will leave final decision to >>>> others. >>>> >>>> /Michal >>>> >>>> >>>>> >>>>>> Note that all above should be compatible with media driver, >>>>>> which explicitly looks for no error and value 1 >>>>> >>>>> Cool. >>>>> -Chris >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-02-11 21:54 ` Fosha, Robert M @ 2020-02-21 3:56 ` Ye, Tony 2020-02-28 11:33 ` Joonas Lahtinen 0 siblings, 1 reply; 19+ messages in thread From: Ye, Tony @ 2020-02-21 3:56 UTC (permalink / raw) To: Fosha, Robert M, Michal Wajdeczko, Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On 2/12/2020 5:54 AM, Fosha, Robert M wrote: > > > On 2/11/20 11:57 AM, Michal Wajdeczko wrote: >> On Tue, 11 Feb 2020 18:53:05 +0100, Fosha, Robert M >> <robert.m.fosha@intel.com> wrote: >> >>> >>> >>> On 2/4/20 4:43 PM, Ye, Tony wrote: >>>> >>>> >>>> On 1/27/2020 1:41 AM, Michal Wajdeczko wrote: >>>>> On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson >>>>> <chris@chris-wilson.co.uk> wrote: >>>>> >>>>>> Quoting Michal Wajdeczko (2020-01-23 15:38:52) >>>>>>> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson >>>>>>> <chris@chris-wilson.co.uk> wrote: >>>>>>> >>>>>>> > Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>>>>>> >> >>>>>>> >> >>>>>>> >> On 1/22/20 11:48 AM, 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. >>>>>>> >> > >>>>>>> >> > 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> >>>>>>> >> >>>>>>> >> Reviewed-by: Daniele Ceraolo Spurio >>>>>>> <daniele.ceraolospurio@intel.com> >>>>>>> > >>>>>>> > Once upon a time did you (Michal) not argue we should indicate >>>>>>> the lack >>>>>>> > of firmware in the error code? Something like >>>>>>> > >>>>>>> > if (!HAS_GT_UC(gt->i915)) >>>>>>> > return -ENODEV; >>>>>>> > >>>>>>> > if (!intel_huc_is_supported(huc)) >>>>>>> > return -ENOEXEC; >>>>>>> >>>>>>> Yes, we discussed this here [1] together with [2] but we didn't >>>>>>> conclude our discussion due to different opinions on how represent >>>>>>> some states, in particular "manually disabled" state. >>>>>>> >>>>>>> In this patch I just wanted to restore old notation. >>>>>>> >>>>>>> But we can start new discussion, here is summary: >>>>>>> >>>>>>> ------------------+----------+----------+---------- >>>>>>> HuC state | today* | option A | option B >>>>>>> ------------------+----------+----------+---------- >>>>>>> no HuC hardware | -ENODEV | -ENODEV | -ENODEV >>>>>>> GuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>>>>> HuC fw disabled | 0 | 0 | -EOPNOTSUPP >>>>>>> HuC fw missing | 0 | -ENOPKG | -ENOEXEC >>>>>>> HuC fw error | 0 | -ENOEXEC | -ENOEXEC >>>>>>> HuC fw fail | 0 | -EACCES | 0 >>>>>>> HuC authenticated | 1 | 1 | 1 >>>>>>> ------------------+----------+----------+---------- >>>>>> >>>>>> By fw fail, you mean we loaded the firmware (to our knowledge) >>>>>> correctly, but HUC_STATUS is not reported as valid? >>>>>> >>>>>> If so, I support option B. I like the idea of saying >>>>>> "no HuC" (machine too old) >>>>>> "no firmware" (user action, or lack thereof) >>>>>> 0 (fw unhappy) >>>>>> 1 (fw reports success) >>>>>> >>>>>> In between states for failures in fw loading? Not so sure. But I >>>>>> can see >>>>>> the nicety in distinguishing between lack of firmware and some random >>>>>> failure in loading the firmware (the former being user action >>>>>> required >>>>>> to rectify, command line parameter whatever and the latter being the >>>>>> firmware file is either invalid or a stray neutrino prevented >>>>>> loading). >>>>>> >>>>>> Imo the error messages should be about why we cannot probe/trust the >>>>>> HUC_STATUS register. If everything is setup correctly then the >>>>>> returned >>>>>> value should be from reading the register. I dislike only >>>>>> returning 1 if >>>>>> supported, and converting a valid read of 0 into another error. >>>>>> >>>>>> So Option B :) >>>>> >>>>> But I'm not sure that option B is consistent in error reporting, as >>>>> "fw unhappy" is definitely an serious error but is represented as >>>>> plain >>>>> non-error "0" status, while "fw disabled" (user action) is treated >>>>> as error >>>>> >>>>> ------------------+---------- >>>>> HuC state | option B >>>>> ------------------+---------- >>>>> no HuC hardware | -ENODEV >>>>> GuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>>>> HuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>>>> HuC fw missing | -ENOEXEC >>>>> HuC fw error | -ENOEXEC >>>>> HuC fw fail | 0 -> unlikely, but still fw/hw error >>>>> HuC authenticated | 1 >>>>> ------------------+---------- >>>>> >>>>> On other hand, option A treats all error conditions as errors, leaving >>>>> status codes only for normal operations: disabled(0)/authenticated(1): >>>>> >>>>> ------------------+---------- >>>>> HuC state | option A >>>>> ------------------+---------- >>>>> no HuC hardware | -ENODEV -> you shouldn't ask >>>>> GuC fw disabled | 0 -> user decision, not an error >>>>> HuC fw disabled | 0 -> user decision, not an error >>>>> HuC fw missing | -ENOPKG -> fw not installed correctly >>>>> HuC fw error | -ENOEXEC -> bad/wrong fw >>>>> HuC fw fail | -EACCES -> fw/hw error >>>>> HuC authenticated | 1 >>>>> ------------------+---------- >>>> >>>> Vote for Option A. >>>> >>>> Regards, >>>> Tony >>>> >>> >>> Are we ok to move forward on this? Michal, are you working on >>> updating the patch? >> >> I can update the patch, but I don't know which option to implement: >> Tony votes for Option A, Chris for Option B, what's your choice? >> >> Michal >> > > I don't have a preference and would trust both Tony and Chris over > myself. However, if Tony is using HuC more I would vote for his prefered > option. > > -Rob Option B takes enable_guc=0|1 (user doesn't want to load HuC FW) as error, and takes FW fail (failed to xfer or init/auth the fw) as 0. It would confuse the user. Regards, Tony > >>> >>> -Rob >>> >>>>> >>>>> But since I'm not an active HuC user, will leave final decision to >>>>> others. >>>>> >>>>> /Michal >>>>> >>>>> >>>>>> >>>>>>> Note that all above should be compatible with media driver, >>>>>>> which explicitly looks for no error and value 1 >>>>>> >>>>>> Cool. >>>>>> -Chris >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-02-21 3:56 ` Ye, Tony @ 2020-02-28 11:33 ` Joonas Lahtinen 2020-02-28 13:44 ` Michal Wajdeczko 0 siblings, 1 reply; 19+ messages in thread From: Joonas Lahtinen @ 2020-02-28 11:33 UTC (permalink / raw) To: Fosha, Robert M, Ye, Tony, Chris Wilson, Daniele Ceraolo Spurio, Michal Wajdeczko, intel-gfx > >>>>> ------------------+---------- > >>>>> HuC state | option B > >>>>> ------------------+---------- > >>>>> no HuC hardware | -ENODEV > >>>>> GuC fw disabled | -EOPNOTSUPP -> user decision, why error? > >>>>> HuC fw disabled | -EOPNOTSUPP -> user decision, why error? > >>>>> HuC fw missing | -ENOEXEC > >>>>> HuC fw error | -ENOEXEC > >>>>> HuC fw fail | 0 -> unlikely, but still fw/hw error > >>>>> HuC authenticated | 1 > >>>>> ------------------+---------- > >>>>> > >>>>> On other hand, option A treats all error conditions as errors, leaving > >>>>> status codes only for normal operations: disabled(0)/authenticated(1): > >>>>> > >>>>> ------------------+---------- > >>>>> HuC state | option A > >>>>> ------------------+---------- > >>>>> no HuC hardware | -ENODEV -> you shouldn't ask > >>>>> GuC fw disabled | 0 -> user decision, not an error > >>>>> HuC fw disabled | 0 -> user decision, not an error > >>>>> HuC fw missing | -ENOPKG -> fw not installed correctly > >>>>> HuC fw error | -ENOEXEC -> bad/wrong fw > >>>>> HuC fw fail | -EACCES -> fw/hw error > >>>>> HuC authenticated | 1 > >>>>> ------------------+---------- Let's go with Option B here. It correctly reports anything as error if it precedes the actual action of authentication and prevents it from happening. So the result one gets is 0/1 is for the authentication status which is the original intent here. Or alternatively an error if the authentication was not attempted. Regards, Joonas _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-02-28 11:33 ` Joonas Lahtinen @ 2020-02-28 13:44 ` Michal Wajdeczko 0 siblings, 0 replies; 19+ messages in thread From: Michal Wajdeczko @ 2020-02-28 13:44 UTC (permalink / raw) To: Joonas Lahtinen, Fosha, Robert M, Ye, Tony, Chris Wilson, Daniele Ceraolo Spurio, intel-gfx On 28.02.2020 12:33, Joonas Lahtinen wrote: >>>>>>> ------------------+---------- >>>>>>> HuC state | option B >>>>>>> ------------------+---------- >>>>>>> no HuC hardware | -ENODEV >>>>>>> GuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>>>>>> HuC fw disabled | -EOPNOTSUPP -> user decision, why error? >>>>>>> HuC fw missing | -ENOEXEC >>>>>>> HuC fw error | -ENOEXEC >>>>>>> HuC fw fail | 0 -> unlikely, but still fw/hw error >>>>>>> HuC authenticated | 1 >>>>>>> ------------------+---------- >>>>>>> >>>>>>> On other hand, option A treats all error conditions as errors, leaving >>>>>>> status codes only for normal operations: disabled(0)/authenticated(1): >>>>>>> >>>>>>> ------------------+---------- >>>>>>> HuC state | option A >>>>>>> ------------------+---------- >>>>>>> no HuC hardware | -ENODEV -> you shouldn't ask >>>>>>> GuC fw disabled | 0 -> user decision, not an error >>>>>>> HuC fw disabled | 0 -> user decision, not an error >>>>>>> HuC fw missing | -ENOPKG -> fw not installed correctly >>>>>>> HuC fw error | -ENOEXEC -> bad/wrong fw >>>>>>> HuC fw fail | -EACCES -> fw/hw error >>>>>>> HuC authenticated | 1 >>>>>>> ------------------+---------- > > Let's go with Option B here. Tony was ok with option A... > > It correctly reports anything as error if it precedes > the actual action of authentication and prevents it from > happening. Option A do the same, including correctly reporting corrupted/wrong firmware as error (as this is the most likely reason that prevents successful HuC authentication on given platform). The main difference here is user decision to disable HuC, that is now treated as the only one non-error reason that HuC is not available for use. > > So the result one gets is 0/1 is for the authentication > status which is the original intent here. I'm not sure that from user POV original intent was to explicitly check HuC authentication status but rather if HuC is available for use. Our redundant [1] check of fw authentication status during this ioctl is just internal detail, not necessary something user was interested from this action. Michal [1] as we are already checking this register during HuC fw load _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-23 15:38 ` Michal Wajdeczko 2020-01-23 15:51 ` Chris Wilson @ 2020-01-23 18:26 ` Ye, Tony 2020-01-23 18:43 ` Michal Wajdeczko 2020-01-23 19:50 ` Ye, Tony 2 siblings, 1 reply; 19+ messages in thread From: Ye, Tony @ 2020-01-23 18:26 UTC (permalink / raw) To: Michal Wajdeczko, Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On 1/23/2020 7:38 AM, Michal Wajdeczko wrote: > On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > >> Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>> >>> >>> On 1/22/20 11:48 AM, 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. >>> > >>> > 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> >>> >>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> Once upon a time did you (Michal) not argue we should indicate the lack >> of firmware in the error code? Something like >> >> if (!HAS_GT_UC(gt->i915)) >> return -ENODEV; >> >> if (!intel_huc_is_supported(huc)) >> return -ENOEXEC; > > Yes, we discussed this here [1] together with [2] but we didn't > conclude our discussion due to different opinions on how represent > some states, in particular "manually disabled" state. > > In this patch I just wanted to restore old notation. > > But we can start new discussion, here is summary: > > ------------------+----------+----------+---------- > HuC state | today* | option A | option B > ------------------+----------+----------+---------- > no HuC hardware | -ENODEV | -ENODEV | -ENODEV > GuC fw disabled | 0 | 0 | -EOPNOTSUPP > HuC fw disabled | 0 | 0 | -EOPNOTSUPP > HuC fw missing | 0 | -ENOPKG | -ENOEXEC > HuC fw error | 0 | -ENOEXEC | -ENOEXEC > HuC fw fail | 0 | -EACCES | 0 What is the difference of HuC fw error and HuC fw fail here? Regards, Tony > HuC authenticated | 1 | 1 | 1 > ------------------+----------+----------+---------- > > Note that all above should be compatible with media driver, > which explicitly looks for no error and value 1 > > Michal > > [1] https://patchwork.freedesktop.org/patch/306419/?series=61001&rev=1 > [2] https://patchwork.freedesktop.org/series/60800/#rev1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-23 18:26 ` Ye, Tony @ 2020-01-23 18:43 ` Michal Wajdeczko 0 siblings, 0 replies; 19+ messages in thread From: Michal Wajdeczko @ 2020-01-23 18:43 UTC (permalink / raw) To: Daniele Ceraolo Spurio, intel-gfx, Chris Wilson, Ye, Tony On Thu, 23 Jan 2020 19:26:58 +0100, Ye, Tony <tony.ye@intel.com> wrote: > > > On 1/23/2020 7:38 AM, Michal Wajdeczko wrote: >> On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >>> Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>>> >>>> >>>> On 1/22/20 11:48 AM, 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. >>>> > >>>> > 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> >>>> >>>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> >>> Once upon a time did you (Michal) not argue we should indicate the lack >>> of firmware in the error code? Something like >>> >>> if (!HAS_GT_UC(gt->i915)) >>> return -ENODEV; >>> >>> if (!intel_huc_is_supported(huc)) >>> return -ENOEXEC; >> Yes, we discussed this here [1] together with [2] but we didn't >> conclude our discussion due to different opinions on how represent >> some states, in particular "manually disabled" state. >> In this patch I just wanted to restore old notation. >> But we can start new discussion, here is summary: >> ------------------+----------+----------+---------- >> HuC state | today* | option A | option B >> ------------------+----------+----------+---------- >> no HuC hardware | -ENODEV | -ENODEV | -ENODEV >> GuC fw disabled | 0 | 0 | -EOPNOTSUPP >> HuC fw disabled | 0 | 0 | -EOPNOTSUPP >> HuC fw missing | 0 | -ENOPKG | -ENOEXEC >> HuC fw error | 0 | -ENOEXEC | -ENOEXEC >> HuC fw fail | 0 | -EACCES | 0 > > What is the difference of HuC fw error and HuC fw fail here? see corresponding internal fw status codes: INTEL_UC_FIRMWARE_ERROR, /* invalid format or version */ INTEL_UC_FIRMWARE_FAIL, /* failed to xfer or init/auth the fw */ > > Regards, > Tony > >> HuC authenticated | 1 | 1 | 1 >> ------------------+----------+----------+---------- >> Note that all above should be compatible with media driver, >> which explicitly looks for no error and value 1 >> Michal >> [1] https://patchwork.freedesktop.org/patch/306419/?series=61001&rev=1 >> [2] https://patchwork.freedesktop.org/series/60800/#rev1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-23 15:38 ` Michal Wajdeczko 2020-01-23 15:51 ` Chris Wilson 2020-01-23 18:26 ` Ye, Tony @ 2020-01-23 19:50 ` Ye, Tony 2 siblings, 0 replies; 19+ messages in thread From: Ye, Tony @ 2020-01-23 19:50 UTC (permalink / raw) To: Michal Wajdeczko, Daniele Ceraolo Spurio, intel-gfx, Chris Wilson On 1/23/2020 7:38 AM, Michal Wajdeczko wrote: > On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > >> Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33) >>> >>> >>> On 1/22/20 11:48 AM, 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. >>> > >>> > 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> >>> >>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> >> Once upon a time did you (Michal) not argue we should indicate the lack >> of firmware in the error code? Something like >> >> if (!HAS_GT_UC(gt->i915)) >> return -ENODEV; >> >> if (!intel_huc_is_supported(huc)) >> return -ENOEXEC; > > Yes, we discussed this here [1] together with [2] but we didn't > conclude our discussion due to different opinions on how represent > some states, in particular "manually disabled" state. > > In this patch I just wanted to restore old notation. > > But we can start new discussion, here is summary: > > ------------------+----------+----------+---------- > HuC state | today* | option A | option B > ------------------+----------+----------+---------- > no HuC hardware | -ENODEV | -ENODEV | -ENODEV > GuC fw disabled | 0 | 0 | -EOPNOTSUPP > HuC fw disabled | 0 | 0 | -EOPNOTSUPP > HuC fw missing | 0 | -ENOPKG | -ENOEXEC > HuC fw error | 0 | -ENOEXEC | -ENOEXEC > HuC fw fail | 0 | -EACCES | 0 > HuC authenticated | 1 | 1 | 1 > ------------------+----------+----------+---------- > > Note that all above should be compatible with media driver, > which explicitly looks for no error and value 1 From user space perspective, e.g. the new igt huc_copy, option B looks like this: pg.param = I915_PARAM_HUC_STATUS; pg.value = &val; ret = ioctl(fd, DRM_IOCTL_I915_GETPARAM, &pg); ------------------+----------+----------+------------+---------- HuC state | ret | pg.value | errno | huc_copy ------------------+----------+----------+------------+---------- no HuC hardware | -1 | 0 | -ENODEV | SKIP GuC fw disabled | -1 | 0 | -EOPNOTSUPP| SKIP HuC fw disabled | -1 | 0 | -EOPNOTSUPP| SKIP HuC fw missing | -1 | 0 | -ENOEXEC | FAIL HuC fw error | -1 | 0 | -ENOEXEC | FAIL HuC fw fail | 0 | 0 | 0 | FAIL HuC authenticated | 0 | 1 | 0 | continue ------------------+----------+----------+------------+---------- It can distinguish the SKIP and FAIL conditions. But looks not elegant enough. The pg.value is wasted as it is not pushed back to user space when intel_huc_check_status() < 0. case I915_PARAM_HUC_STATUS: value = intel_huc_check_status(&i915->gt.uc.huc); if (value < 0) return value; break; Regards, Tony > > Michal > > [1] https://patchwork.freedesktop.org/patch/306419/?series=61001&rev=1 > [2] https://patchwork.freedesktop.org/series/60800/#rev1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-22 19:48 [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS Michal Wajdeczko 2020-01-22 23:52 ` Daniele Ceraolo Spurio @ 2020-01-23 2:43 ` Patchwork 2020-01-23 14:57 ` Michal Wajdeczko 2020-01-23 18:12 ` [Intel-gfx] [PATCH] " Ye, Tony 2 siblings, 1 reply; 19+ messages in thread From: Patchwork @ 2020-01-23 2:43 UTC (permalink / raw) To: Michal Wajdeczko; +Cc: intel-gfx == Series Details == Series: drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS URL : https://patchwork.freedesktop.org/series/72419/ State : failure == Summary == CI Bug Log - changes from CI_DRM_7799 -> Patchwork_16219 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_16219 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_16219, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/index.html Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_16219: ### IGT changes ### #### Possible regressions #### * igt@gem_exec_parallel@contexts: - fi-byt-j1900: NOTRUN -> [FAIL][1] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/fi-byt-j1900/igt@gem_exec_parallel@contexts.html Known issues ------------ Here are the changes found in Patchwork_16219 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@i915_selftest@live_blt: - fi-ivb-3770: [PASS][2] -> [DMESG-FAIL][3] ([i915#725]) [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-ivb-3770/igt@i915_selftest@live_blt.html [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/fi-ivb-3770/igt@i915_selftest@live_blt.html * igt@kms_chamelium@dp-crc-fast: - fi-cml-u2: [PASS][4] -> [FAIL][5] ([i915#262]) [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html * igt@kms_chamelium@hdmi-hpd-fast: - fi-kbl-7500u: [PASS][6] -> [FAIL][7] ([fdo#111096] / [i915#323]) [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html #### Possible fixes #### * igt@gem_close_race@basic-threads: - fi-byt-j1900: [INCOMPLETE][8] ([i915#45]) -> [PASS][9] [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-byt-j1900/igt@gem_close_race@basic-threads.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/fi-byt-j1900/igt@gem_close_race@basic-threads.html * igt@i915_selftest@live_blt: - fi-hsw-4770r: [DMESG-FAIL][10] ([i915#553] / [i915#725]) -> [PASS][11] [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-hsw-4770r/igt@i915_selftest@live_blt.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/fi-hsw-4770r/igt@i915_selftest@live_blt.html [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096 [i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262 [i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323 [i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45 [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553 [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725 Participating hosts (50 -> 45) ------------------------------ Additional (2): fi-glk-dsi fi-ilk-650 Missing (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus Build changes ------------- * CI: CI-20190529 -> None * Linux: CI_DRM_7799 -> Patchwork_16219 CI-20190529: 20190529 CI_DRM_7799: 0f8a46a25a7781ef6ede604c9cb50f82cfb5e960 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_5377: 1e6cb3e75925cf623df04f78430ae9299632ec3f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_16219: c6f09c2f7c9d2d2b128c8673453f58d168bf06fb @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == c6f09c2f7c9d drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/index.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-23 2:43 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork @ 2020-01-23 14:57 ` Michal Wajdeczko 0 siblings, 0 replies; 19+ messages in thread From: Michal Wajdeczko @ 2020-01-23 14:57 UTC (permalink / raw) To: Patchwork, intel-gfx On Thu, 23 Jan 2020 03:43:04 +0100, Patchwork <patchwork@emeril.freedesktop.org> wrote: > == Series Details == > > Series: drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS > URL : https://patchwork.freedesktop.org/series/72419/ > State : failure > > == Summary == > > CI Bug Log - changes from CI_DRM_7799 -> Patchwork_16219 > ==================================================== > > Summary > ------- > > **FAILURE** > > Serious unknown changes coming with Patchwork_16219 absolutely need to > be > verified manually. > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_16219, please notify your bug team to allow > them > to document this new failure mode, which will reduce false positives > in CI. > > External URL: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/index.html > > Possible new issues > ------------------- > > Here are the unknown changes that may have been introduced in > Patchwork_16219: > > ### IGT changes ### > > #### Possible regressions #### > > * igt@gem_exec_parallel@contexts: > - fi-byt-j1900: NOTRUN -> [FAIL][1] > [1]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16219/fi-byt-j1900/igt@gem_exec_parallel@contexts.html no HuC here, unrelated _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS 2020-01-22 19:48 [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS Michal Wajdeczko 2020-01-22 23:52 ` Daniele Ceraolo Spurio 2020-01-23 2:43 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork @ 2020-01-23 18:12 ` Ye, Tony 2 siblings, 0 replies; 19+ messages in thread From: Ye, Tony @ 2020-01-23 18:12 UTC (permalink / raw) To: Michal Wajdeczko, intel-gfx Acked-by: Tony Ye <tony.ye@intel.com> On 1/22/2020 11:48 AM, 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. > > 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> > --- > drivers/gpu/drm/i915/gt/uc/intel_huc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > index 32a069841c14..ad4d9e16a24e 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > @@ -209,7 +209,7 @@ 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; > > with_intel_runtime_pm(gt->uncore->rpm, wakeref) > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-02-28 13:44 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-22 19:48 [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS Michal Wajdeczko 2020-01-22 23:52 ` Daniele Ceraolo Spurio 2020-01-23 15:02 ` Chris Wilson 2020-01-23 15:38 ` Michal Wajdeczko 2020-01-23 15:51 ` Chris Wilson 2020-01-26 17:41 ` Michal Wajdeczko 2020-02-05 0:43 ` Ye, Tony 2020-02-11 17:53 ` Fosha, Robert M 2020-02-11 19:57 ` Michal Wajdeczko 2020-02-11 21:54 ` Fosha, Robert M 2020-02-21 3:56 ` Ye, Tony 2020-02-28 11:33 ` Joonas Lahtinen 2020-02-28 13:44 ` Michal Wajdeczko 2020-01-23 18:26 ` Ye, Tony 2020-01-23 18:43 ` Michal Wajdeczko 2020-01-23 19:50 ` Ye, Tony 2020-01-23 2:43 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork 2020-01-23 14:57 ` Michal Wajdeczko 2020-01-23 18:12 ` [Intel-gfx] [PATCH] " Ye, Tony
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.