Re: [PATCH 8/8] drm/i915/get_params: Add HuC status to getparams

[Arkadiusz Hiler <arkadiusz.hiler@intel.com>]

On Mon, Dec 12, 2016 at 02:21:41PM +0000, Chris Wilson wrote:
> On Mon, Dec 12, 2016 at 03:13:17PM +0100, Arkadiusz Hiler wrote:
> > On Fri, Dec 09, 2016 at 01:59:45PM +0100, Michal Wajdeczko wrote:
> > > On Thu, Dec 08, 2016 at 03:02:19PM -0800, anushasr wrote:
> > > > From: Peter Antoine <peter.antoine@intel.com>
> > > > 
> > > > This patch will allow for getparams to return the status of the HuC.
> > > > As the HuC has to be validated by the GuC this patch uses the validated
> > > > status to show when the HuC is loaded and ready for use. You cannot use
> > > > the loaded status as with the GuC as the HuC is verified after it is
> > > > loaded and is not usable until it is verified.
> > > > 
> > > > v2: removed the forewakes as the registers are already force-woken.
> > > >      (T.Ursulin)
> > > > v4: rebased.
> > > > v5: rebased on top of drm-tip.
> > > > v6: rebased. Removed any reference to intel_huc.h
> > > > 
> > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c         |  4 ++++
> > > >  drivers/gpu/drm/i915/intel_huc_loader.c | 12 ++++++++++++
> > > >  drivers/gpu/drm/i915/intel_uc.h         |  1 +
> > > >  include/uapi/drm/i915_drm.h             |  1 +
> > > >  4 files changed, 18 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 85a47c2..6be06a27 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -49,6 +49,7 @@
> > > >  #include "i915_trace.h"
> > > >  #include "i915_vgpu.h"
> > > >  #include "intel_drv.h"
> > > > +#include "intel_uc.h"
> > > >  
> > > >  static struct drm_driver driver;
> > > >  
> > > > @@ -349,6 +350,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> > > >  		 */
> > > >  		value = 1;
> > > >  		break;
> > > > +	case I915_PARAM_HAS_HUC:
> > > 
> > > For me this param name does not match returned result which maybe misleading.
> > > Note that other HAS params return static driver/hw capability, not runtime.
> > > 
> > > I guess PARAM_HUC_STATUS would be better (0=no huc, 1=pending, 2=ok, -1=failed)
> > > And you can cache huc status in intel_huc struct and make final modification in
> > > intel_huc_auth() function to avoid registry read (unless we want to detect later
> > > crash of the huc using this reg read)
> > 
> > Why should userspace care for those intermediary states? From what I
> > know (docs and patches from the cover letter) userspace is interested
> > only in being able to use HuC. If something is not working you have
> > DebugFS for that exact purpose.
> > 
> > As of cacheing - seems like a good idea to limit reg reads.
> 
> Is GETPARAM(HAS_HUC) a hot path? Should we even encourage it?

Actually... Good point. HAS_HUC is used once each time you initialise
libva client.

> Are there any other users of intel_huc_is_valid()?

intel_guc_auth_huc() uses 
ret = wait_for((I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED) > 0, 50);

So it can be reused there.

> As for userspace simply asking where huc is enabled, we already have
> that in the ABI via the module parameter, so you need to justify why
> this is preferred (in addition to the available information).

Yeah, we do change the values of module parameters. But a lot of them
are uid 0 only and we have PARAMS for those.

Do anything userspace use those actually? Do we plan to use them instead
of the getparams since now on?


> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx