| Message ID | 20180418093342.449-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Tvrtko Ursulin (2018-04-18 12:33:42) > From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Cc: Tony Ye <tony.ye@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 > break; > case I915_PARAM_HAS_LLC: > value = HAS_LLC(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index c74f5df3fb5a..80b32460567d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -2052,7 +2052,9 @@ eb_select_engine(struct drm_i915_private *dev_priv, > return NULL; > } > > - if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { > + if (user_ring_id == I915_EXEC_BSD && > + (INTEL_INFO(dev_priv)->ring_mask & > + (ENGINE_MASK(_VCS(1)) | ENGINE_MASK(_VCS(2))))) { > unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; > > if (bsd_idx == I915_EXEC_BSD_DEFAULT) { > @@ -2068,6 +2070,12 @@ eb_select_engine(struct drm_i915_private *dev_priv, > } > > engine = dev_priv->engine[_VCS(bsd_idx)]; > + > + /* > + * FIXME Temporarily map VCS2 to VCS1 on Icelake. > + */ > + if (!engine && bsd_idx) > + engine = dev_priv->engine[_VCS(2)]; > } else { > engine = dev_priv->engine[user_ring_map[user_ring_id]]; > } > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > Joonas Lahtinen > Sent: Wednesday, April 18, 2018 3:43 AM > To: Intel-gfx@lists.freedesktop.org; Tvrtko Ursulin <tursulin@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@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@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > Cc: Tony Ye <tony.ye@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.
> -----Original Message----- > From: Tvrtko Ursulin <tursulin@ursulin.net> > Sent: Wednesday, April 18, 2018 2:34 AM > To: Intel-gfx@lists.freedesktop.org > Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris > Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon > <jon.bloomfield@intel.com>; Ye, Tony <tony.ye@intel.com> > Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second > VCS instance > > From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Cc: Tony Ye <tony.ye@intel.com> I would advocate this patch being merged while the new execbuf API is being developed. Currently there is no way to submit to 2 engine skus with non-sequential engine id's. This doesn't introduce a new ABI, and there is no reason that I can see that the new execbuf solution couldn't be made backward compatible with this. Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
On 20/04/2018 15:19, Bloomfield, Jon wrote: >> -----Original Message----- >> From: Tvrtko Ursulin <tursulin@ursulin.net> >> Sent: Wednesday, April 18, 2018 2:34 AM >> To: Intel-gfx@lists.freedesktop.org >> Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris >> Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon >> <jon.bloomfield@intel.com>; Ye, Tony <tony.ye@intel.com> >> Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second >> VCS instance >> >> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Jon Bloomfield <jon.bloomfield@intel.com> >> Cc: Tony Ye <tony.ye@intel.com> > I would advocate this patch being merged while the new execbuf API is being > developed. Currently there is no way to submit to 2 engine skus with non-sequential > engine id's. This doesn't introduce a new ABI, and there is no reason that I can see > that the new execbuf solution couldn't be made backward compatible with this. It is a bit of a awkward period to commit to this permanently because it only solves a subset of problem space and that makes it a hard sell in that context. If there was legacy userspace which ran on 2 VCS Gen11 then maybe, but otherwise I think best is just wait for the new execbuf API. Or in fact would there be _any_ upstream userspace using this before the new execbuf API happens? Regards, Tvrtko
> -----Original Message----- > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Sent: Friday, April 20, 2018 9:56 AM > To: Bloomfield, Jon <jon.bloomfield@intel.com>; Tvrtko Ursulin > <tursulin@ursulin.net>; Intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean > any second VCS instance > > > On 20/04/2018 15:19, Bloomfield, Jon wrote: > >> -----Original Message----- > >> From: Tvrtko Ursulin <tursulin@ursulin.net> > >> Sent: Wednesday, April 18, 2018 2:34 AM > >> To: Intel-gfx@lists.freedesktop.org > >> Cc: tursulin@ursulin.net; Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; Chris > >> Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon > >> <jon.bloomfield@intel.com>; Ye, Tony <tony.ye@intel.com> > >> Subject: [PATCH] drm/i915/icl: Adjust BSD2 semantics to mean any second > >> VCS instance > >> > >> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Jon Bloomfield <jon.bloomfield@intel.com> > >> Cc: Tony Ye <tony.ye@intel.com> > > I would advocate this patch being merged while the new execbuf API is > being > > developed. Currently there is no way to submit to 2 engine skus with non- > sequential > > engine id's. This doesn't introduce a new ABI, and there is no reason that I > can see > > that the new execbuf solution couldn't be made backward compatible with > this. > > It is a bit of a awkward period to commit to this permanently because it > only solves a subset of problem space and that makes it a hard sell in > that context. > > If there was legacy userspace which ran on 2 VCS Gen11 then maybe, but > otherwise I think best is just wait for the new execbuf API. Or in fact > would there be _any_ upstream userspace using this before the new > execbuf API happens? > Fair point. Will you be physically inhibiting this legacy ABI for gen11? If you intend to allow it it's worth merging, because right now it simply doesn't work. If you will never allow the legacy ABI, and will forcibly prevent it (hardcode HAS_BSD2==0, for gen11+), then I agree we may as well carry the patch as a delta until the new execbuf lands.
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]; break; case I915_PARAM_HAS_LLC: value = HAS_LLC(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c74f5df3fb5a..80b32460567d 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -2052,7 +2052,9 @@ eb_select_engine(struct drm_i915_private *dev_priv, return NULL; } - if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { + if (user_ring_id == I915_EXEC_BSD && + (INTEL_INFO(dev_priv)->ring_mask & + (ENGINE_MASK(_VCS(1)) | ENGINE_MASK(_VCS(2))))) { unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; if (bsd_idx == I915_EXEC_BSD_DEFAULT) { @@ -2068,6 +2070,12 @@ eb_select_engine(struct drm_i915_private *dev_priv, } engine = dev_priv->engine[_VCS(bsd_idx)]; + + /* + * FIXME Temporarily map VCS2 to VCS1 on Icelake. + */ + if (!engine && bsd_idx) + engine = dev_priv->engine[_VCS(2)]; } else { engine = dev_priv->engine[user_ring_map[user_ring_id]]; }