| Message ID | 20190522190057.848-4-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: HuC updates | expand |
Quoting Michal Wajdeczko (2019-05-22 20:00:57) > Without breaking existing usage, slightly update HuC status codes > to provide more info to the clients: > 1 if HuC firmware is loaded and verified, > 0 if HuC firmware is not enabled, > -ENOPKG if HuC firmware is not loaded, > -ENODEV if HuC is not present on this platform. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tony Ye <tony.ye@intel.com> > --- > drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c > index aac17916e130..98deb4ee60a7 100644 > --- a/drivers/gpu/drm/i915/intel_huc.c > +++ b/drivers/gpu/drm/i915/intel_huc.c > @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc) > * intel_huc_check_status() - check HuC status > * @huc: intel_huc structure > * > - * 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. > + * Return: > + * * 1 if HuC firmware is loaded and verified, > + * * 0 if HuC firmware is not enabled, > + * * -ENOPKG if HuC firmware is not loaded, > + * * -ENODEV if HuC is not present on this platform. > */ > int intel_huc_check_status(struct intel_huc *huc) > { > @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc) > if (!HAS_HUC(i915)) > return -ENODEV; > > - return huc->verified; > + if (!USES_HUC(i915)) > + return 0; > + > + return huc->verified ? 1 : -ENOPKG; I still think EOPNOTSUPP is a better error though for the user preventing the huc being loaded -- as opposed to the result of verification being the non-error value. error == unable to setup huc 0/1 == result from talking to huc Better ask someone else for a third opinion. -Chris
On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2019-05-22 20:00:57) >> Without breaking existing usage, slightly update HuC status codes >> to provide more info to the clients: >> 1 if HuC firmware is loaded and verified, >> 0 if HuC firmware is not enabled, >> -ENOPKG if HuC firmware is not loaded, >> -ENODEV if HuC is not present on this platform. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Tony Ye <tony.ye@intel.com> >> --- >> drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c >> b/drivers/gpu/drm/i915/intel_huc.c >> index aac17916e130..98deb4ee60a7 100644 >> --- a/drivers/gpu/drm/i915/intel_huc.c >> +++ b/drivers/gpu/drm/i915/intel_huc.c >> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc) >> * intel_huc_check_status() - check HuC status >> * @huc: intel_huc structure >> * >> - * 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. >> + * Return: >> + * * 1 if HuC firmware is loaded and verified, >> + * * 0 if HuC firmware is not enabled, >> + * * -ENOPKG if HuC firmware is not loaded, >> + * * -ENODEV if HuC is not present on this platform. >> */ >> int intel_huc_check_status(struct intel_huc *huc) >> { >> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc) >> if (!HAS_HUC(i915)) >> return -ENODEV; >> >> - return huc->verified; >> + if (!USES_HUC(i915)) >> + return 0; >> + >> + return huc->verified ? 1 : -ENOPKG; > > I still think EOPNOTSUPP is a better error though for the user > preventing the huc being loaded -- as opposed to the result of > verification being the non-error value. > > error == unable to setup huc > 0/1 == result from talking to huc but your 0 here overlaps with unable to setup huc error, so from the ABI perspective, imho, is bad. also, from media team pov, as they want to have HuC always on, the only non-error case is when user explicitly decided otherwise. > > Better ask someone else for a third opinion. Check end of the message [1] but I'm fine waiting for more opinions. [1] https://lists.freedesktop.org/archives/intel-gfx/2019-May/199066.html Michal
Quoting Michal Wajdeczko (2019-05-23 01:25:00) > On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2019-05-22 20:00:57) > >> Without breaking existing usage, slightly update HuC status codes > >> to provide more info to the clients: > >> 1 if HuC firmware is loaded and verified, > >> 0 if HuC firmware is not enabled, > >> -ENOPKG if HuC firmware is not loaded, > >> -ENODEV if HuC is not present on this platform. > >> > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Tony Ye <tony.ye@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++---- > >> 1 file changed, 9 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_huc.c > >> b/drivers/gpu/drm/i915/intel_huc.c > >> index aac17916e130..98deb4ee60a7 100644 > >> --- a/drivers/gpu/drm/i915/intel_huc.c > >> +++ b/drivers/gpu/drm/i915/intel_huc.c > >> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc) > >> * intel_huc_check_status() - check HuC status > >> * @huc: intel_huc structure > >> * > >> - * 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. > >> + * Return: > >> + * * 1 if HuC firmware is loaded and verified, > >> + * * 0 if HuC firmware is not enabled, > >> + * * -ENOPKG if HuC firmware is not loaded, > >> + * * -ENODEV if HuC is not present on this platform. > >> */ > >> int intel_huc_check_status(struct intel_huc *huc) > >> { > >> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc) > >> if (!HAS_HUC(i915)) > >> return -ENODEV; > >> > >> - return huc->verified; > >> + if (!USES_HUC(i915)) > >> + return 0; > >> + > >> + return huc->verified ? 1 : -ENOPKG; > > > > I still think EOPNOTSUPP is a better error though for the user > > preventing the huc being loaded -- as opposed to the result of > > verification being the non-error value. > > > > error == unable to setup huc > > 0/1 == result from talking to huc > > but your 0 here overlaps with unable to setup huc error, > so from the ABI perspective, imho, is bad. > > also, from media team pov, as they want to have HuC always on, > the only non-error case is when user explicitly decided otherwise. Trying to look things from external perspective, if HUC_(AUTHENTICATION_)STATUS is queried 1 => "authenticated", 0 => "not authenticated" makes most sense. Both media drivers also first check for errors, then convert the return value to boolean, so it would be compatible with that. Regards, Joonas
On Fri, 31 May 2019 12:13:20 +0200, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > Quoting Michal Wajdeczko (2019-05-23 01:25:00) >> On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson >> <chris@chris-wilson.co.uk> wrote: >> >> > Quoting Michal Wajdeczko (2019-05-22 20:00:57) >> >> Without breaking existing usage, slightly update HuC status codes >> >> to provide more info to the clients: >> >> 1 if HuC firmware is loaded and verified, >> >> 0 if HuC firmware is not enabled, >> >> -ENOPKG if HuC firmware is not loaded, >> >> -ENODEV if HuC is not present on this platform. >> >> >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> >> Cc: Tony Ye <tony.ye@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++---- >> >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c >> >> b/drivers/gpu/drm/i915/intel_huc.c >> >> index aac17916e130..98deb4ee60a7 100644 >> >> --- a/drivers/gpu/drm/i915/intel_huc.c >> >> +++ b/drivers/gpu/drm/i915/intel_huc.c >> >> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc) >> >> * intel_huc_check_status() - check HuC status >> >> * @huc: intel_huc structure >> >> * >> >> - * 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. >> >> + * Return: >> >> + * * 1 if HuC firmware is loaded and verified, >> >> + * * 0 if HuC firmware is not enabled, >> >> + * * -ENOPKG if HuC firmware is not loaded, >> >> + * * -ENODEV if HuC is not present on this platform. >> >> */ >> >> int intel_huc_check_status(struct intel_huc *huc) >> >> { >> >> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc) >> >> if (!HAS_HUC(i915)) >> >> return -ENODEV; >> >> >> >> - return huc->verified; >> >> + if (!USES_HUC(i915)) >> >> + return 0; >> >> + >> >> + return huc->verified ? 1 : -ENOPKG; >> > >> > I still think EOPNOTSUPP is a better error though for the user >> > preventing the huc being loaded -- as opposed to the result of >> > verification being the non-error value. >> > >> > error == unable to setup huc >> > 0/1 == result from talking to huc >> >> but your 0 here overlaps with unable to setup huc error, >> so from the ABI perspective, imho, is bad. >> >> also, from media team pov, as they want to have HuC always on, >> the only non-error case is when user explicitly decided otherwise. > > Trying to look things from external perspective, if > HUC_(AUTHENTICATION_)STATUS > is queried 1 => "authenticated", 0 => "not authenticated" makes most > sense. If you want to treat that as "authenticated" status then I fully agree. But authentication status is just a one technical detail how we load the firmware on hardware. Users mainly want to check if HuC was correctly enabled on their system, so this is more like HUC_(ENABLED_)STATUS. Then we have 1 => "enabled/active" and 0 => "disabled" > > Both media drivers also first check for errors, then convert the return > value to boolean, so it would be compatible with that. Both solutions are compatible with current media drivers implementations, we don't want to break ABI, just to make it more useful. Now these drivers just checks for success of the ioctl() call alone and ignore any reported error since we don't provide any meaningful codes why HuC is not working. We forced them to check dmesg for any hint. Providing clear notation of 1(active) 0(disabled) and set of possible errno codes why not explicitly disabled HuC is not working well, will, imho, be much more useful than 1(authenticated) and 0(not authenticated, dunno why) Again, proposed here approach is aligned with Tony's opinion. Regards, Michal > > Regards, Joonas
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c index aac17916e130..98deb4ee60a7 100644 --- a/drivers/gpu/drm/i915/intel_huc.c +++ b/drivers/gpu/drm/i915/intel_huc.c @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc) * intel_huc_check_status() - check HuC status * @huc: intel_huc structure * - * 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. + * Return: + * * 1 if HuC firmware is loaded and verified, + * * 0 if HuC firmware is not enabled, + * * -ENOPKG if HuC firmware is not loaded, + * * -ENODEV if HuC is not present on this platform. */ int intel_huc_check_status(struct intel_huc *huc) { @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc) if (!HAS_HUC(i915)) return -ENODEV; - return huc->verified; + if (!USES_HUC(i915)) + return 0; + + return huc->verified ? 1 : -ENOPKG; }
Without breaking existing usage, slightly update HuC status codes to provide more info to the clients: 1 if HuC firmware is loaded and verified, 0 if HuC firmware is not enabled, -ENOPKG if HuC firmware is not loaded, -ENODEV if HuC is not present on this platform. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tony Ye <tony.ye@intel.com> --- drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)