[Intel-gfx] [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second VCS instance

[Bloomfield, Jon]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Joonas Lahtinen
> Sent: Wednesday, April 18, 2018 3:43 AM
> To: Intel-gfx at lists.freedesktop.org; Tvrtko Ursulin <tursulin at ursulin.net>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean
> any second VCS instance
> 
> Quoting Tvrtko Ursulin (2018-04-18 12:33:42)
> > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >
> > Currently our driver assumes BSD2 means hardware engine instance
> number
> > two. This does not work for Icelake parts with two VCS engines, but which
> > are hardware instances 0 and 2, and not 0 and 1 as with previous parts.
> >
> > This makes the second engine not discoverable via HAS_BSD2 get param,
> nor
> > it can be targetted by execbuf.
> >
> > While we are working on the next generation execbuf put in a hack which
> > allows discovery and access to this second VCS engine using legacy ABI.
> >
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Jon Bloomfield <jon.bloomfield at intel.com>
> > Cc: Tony Ye <tony.ye at intel.com>
> > ---
> > Compile tested only.
> >
> > Also, one could argue if this is just a temporary hack or could actually
> > make sense to have this permanently in. It feels like the ABI semantics of
> > BSD2 are blurry, or at least could be re-blurred for Gen11.
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c            |  8 +++++++-
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++++++-
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index b7dbeba72dec..a185366d9beb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -331,7 +331,13 @@ static int i915_getparam_ioctl(struct drm_device
> *dev, void *data,
> >                 value = !!dev_priv->engine[VECS];
> >                 break;
> >         case I915_PARAM_HAS_BSD2:
> > -               value = !!dev_priv->engine[VCS2];
> > +               /*
> > +                * FIXME: Temporary hack for Icelake.
> > +                *
> > +                * Make semantics of HAS_BSD2 "has second", or "has two"
> VDBOXes
> > +                * instead of "has VDBOX 2nd hardware instance".
> > +                */
> > +               value = dev_priv->engine[VCS2] || dev_priv->engine[VCS3];
> 
> There can be no temporary hacks for the uAPI... You either sign yourself
> up to keep it working indefinitely or don't :)
> 
> Regards, Joonas
This doesn't really change the API does it? In fact I'd argue this is simply fixing
a breakage in the API wrt to previous devices. It makes no sense to expose holes
in the engine map to userspace. If a device has two useable VCS engines, HAS_BSD2
should reflect that, and the second engine (wherever it sits physically), should be
addressable through the legacy BSD2 execbuf interface.