[Intel-gfx] [PATCH 8/8] drm/i915/get_params: Add HuC status to getparams

[Chris Wilson]

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 at 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 at intel.com>
> > > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler at 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?

Are there any other users of intel_huc_is_valid()?

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).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre