| Message ID | 1452856013-29728-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/01/16 11:06, Chris Wilson wrote: > Tvrtko was looking through the execbuffer-ioctl and noticed that the > uABI was tightly coupled to our internal engine identifiers. Close > inspection also revealed that we leak those internal engine identifiers > through the busy-ioctl, and those internal identifiers already do not > match the user identifiers. Fortuitiously, there is only one user of the > set of busy rings from the busy-ioctl, and they only wish to choose > between the RENDER and the BLT engines. > > Let's fix the userspace ABI while we still can. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++---- > drivers/gpu/drm/i915/intel_lrc.c | 5 +++++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > 4 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index bb44bad15403..85797813a3de 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4328,10 +4328,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > if (ret) > goto unref; > > - BUILD_BUG_ON(I915_NUM_RINGS > 16); > - args->busy = obj->active << 16; > - if (obj->last_write_req) > - args->busy |= obj->last_write_req->ring->id; > + args->busy = 0; > + if (obj->active) { > + int i; > + > + for (i = 0; i < I915_NUM_RINGS; i++) { > + struct drm_i915_gem_request *req; > + > + req = obj->last_read_req[i]; > + if (req) > + args->busy |= 1 << (16 + req->ring->exec_id); If I got it right bit 16 was RCS, now will always be clear. And blitter was bit 17 and now is 19. Regards, Tvrtko > + } > + if (obj->last_write_req) > + args->busy |= obj->last_write_req->ring->exec_id; > + } > > unref: > drm_gem_object_unreference(&obj->base); > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index f5d89c845ede..4aa209483237 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2024,6 +2024,7 @@ static int logical_render_ring_init(struct drm_device *dev) > > ring->name = "render ring"; > ring->id = RCS; > + ring->exec_id = I915_EXEC_RENDER; > ring->mmio_base = RENDER_RING_BASE; > > logical_ring_default_irqs(ring, GEN8_RCS_IRQ_SHIFT); > @@ -2073,6 +2074,7 @@ static int logical_bsd_ring_init(struct drm_device *dev) > > ring->name = "bsd ring"; > ring->id = VCS; > + ring->exec_id = I915_EXEC_BSD; > ring->mmio_base = GEN6_BSD_RING_BASE; > > logical_ring_default_irqs(ring, GEN8_VCS1_IRQ_SHIFT); > @@ -2088,6 +2090,7 @@ static int logical_bsd2_ring_init(struct drm_device *dev) > > ring->name = "bsd2 ring"; > ring->id = VCS2; > + ring->exec_id = I915_EXEC_BSD; > ring->mmio_base = GEN8_BSD2_RING_BASE; > > logical_ring_default_irqs(ring, GEN8_VCS2_IRQ_SHIFT); > @@ -2103,6 +2106,7 @@ static int logical_blt_ring_init(struct drm_device *dev) > > ring->name = "blitter ring"; > ring->id = BCS; > + ring->exec_id = I915_EXEC_BLT; > ring->mmio_base = BLT_RING_BASE; > > logical_ring_default_irqs(ring, GEN8_BCS_IRQ_SHIFT); > @@ -2118,6 +2122,7 @@ static int logical_vebox_ring_init(struct drm_device *dev) > > ring->name = "video enhancement ring"; > ring->id = VECS; > + ring->exec_id = I915_EXEC_VEBOX; > ring->mmio_base = VEBOX_RING_BASE; > > logical_ring_default_irqs(ring, GEN8_VECS_IRQ_SHIFT); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 8cd8aabcc3ff..310d151c0db2 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2680,6 +2680,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev) > > ring->name = "render ring"; > ring->id = RCS; > + ring->exec_id = I915_EXEC_RENDER; > ring->mmio_base = RENDER_RING_BASE; > > if (INTEL_INFO(dev)->gen >= 8) { > @@ -2828,6 +2829,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev) > > ring->name = "bsd ring"; > ring->id = VCS; > + ring->exec_id = I915_EXEC_BSD; > > ring->write_tail = ring_write_tail; > if (INTEL_INFO(dev)->gen >= 6) { > @@ -2904,6 +2906,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev) > > ring->name = "bsd2 ring"; > ring->id = VCS2; > + ring->exec_id = I915_EXEC_BSD; > > ring->write_tail = ring_write_tail; > ring->mmio_base = GEN8_BSD2_RING_BASE; > @@ -2934,6 +2937,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev) > > ring->name = "blitter ring"; > ring->id = BCS; > + ring->exec_id = I915_EXEC_BLT; > > ring->mmio_base = BLT_RING_BASE; > ring->write_tail = ring_write_tail; > @@ -2991,6 +2995,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev) > > ring->name = "video enhancement ring"; > ring->id = VECS; > + ring->exec_id = I915_EXEC_VEBOX; > > ring->mmio_base = VEBOX_RING_BASE; > ring->write_tail = ring_write_tail; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 7349d9258191..2067f4700caa 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -156,6 +156,7 @@ struct intel_engine_cs { > } id; > #define I915_NUM_RINGS 5 > #define LAST_USER_RING (VECS + 1) > + unsigned int exec_id; > u32 mmio_base; > struct drm_device *dev; > struct intel_ringbuffer *buffer; >
On Fri, Jan 15, 2016 at 11:58:32AM +0000, Tvrtko Ursulin wrote: > > On 15/01/16 11:06, Chris Wilson wrote: > >Tvrtko was looking through the execbuffer-ioctl and noticed that the > >uABI was tightly coupled to our internal engine identifiers. Close > >inspection also revealed that we leak those internal engine identifiers > >through the busy-ioctl, and those internal identifiers already do not > >match the user identifiers. Fortuitiously, there is only one user of the > >set of busy rings from the busy-ioctl, and they only wish to choose > >between the RENDER and the BLT engines. > > > >Let's fix the userspace ABI while we still can. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >--- > > drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++---- > > drivers/gpu/drm/i915/intel_lrc.c | 5 +++++ > > drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++ > > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > > 4 files changed, 25 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >index bb44bad15403..85797813a3de 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -4328,10 +4328,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > > if (ret) > > goto unref; > > > >- BUILD_BUG_ON(I915_NUM_RINGS > 16); > >- args->busy = obj->active << 16; > >- if (obj->last_write_req) > >- args->busy |= obj->last_write_req->ring->id; > >+ args->busy = 0; > >+ if (obj->active) { > >+ int i; > >+ > >+ for (i = 0; i < I915_NUM_RINGS; i++) { > >+ struct drm_i915_gem_request *req; > >+ > >+ req = obj->last_read_req[i]; > >+ if (req) > >+ args->busy |= 1 << (16 + req->ring->exec_id); > > If I got it right bit 16 was RCS, now will always be clear. And > blitter was bit 17 and now is 19. Sssh. You weren't meant to point that out. I am willing to take the hit in order to decouple the uABI from internals. I am also willing to codify this busy-ioctl ABI into igt! -Chris
On 15/01/16 12:27, Chris Wilson wrote: > On Fri, Jan 15, 2016 at 11:58:32AM +0000, Tvrtko Ursulin wrote: >> >> On 15/01/16 11:06, Chris Wilson wrote: >>> Tvrtko was looking through the execbuffer-ioctl and noticed that the >>> uABI was tightly coupled to our internal engine identifiers. Close >>> inspection also revealed that we leak those internal engine identifiers >>> through the busy-ioctl, and those internal identifiers already do not >>> match the user identifiers. Fortuitiously, there is only one user of the >>> set of busy rings from the busy-ioctl, and they only wish to choose >>> between the RENDER and the BLT engines. >>> >>> Let's fix the userspace ABI while we still can. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> --- >>> drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++---- >>> drivers/gpu/drm/i915/intel_lrc.c | 5 +++++ >>> drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++ >>> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + >>> 4 files changed, 25 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>> index bb44bad15403..85797813a3de 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -4328,10 +4328,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, >>> if (ret) >>> goto unref; >>> >>> - BUILD_BUG_ON(I915_NUM_RINGS > 16); >>> - args->busy = obj->active << 16; >>> - if (obj->last_write_req) >>> - args->busy |= obj->last_write_req->ring->id; >>> + args->busy = 0; >>> + if (obj->active) { >>> + int i; >>> + >>> + for (i = 0; i < I915_NUM_RINGS; i++) { >>> + struct drm_i915_gem_request *req; >>> + >>> + req = obj->last_read_req[i]; >>> + if (req) >>> + args->busy |= 1 << (16 + req->ring->exec_id); >> >> If I got it right bit 16 was RCS, now will always be clear. And >> blitter was bit 17 and now is 19. > > Sssh. You weren't meant to point that out. > > I am willing to take the hit in order to decouple the uABI from > internals. > > I am also willing to codify this busy-ioctl ABI into igt! Looks like your DDX is the only user not using it in the boolean mode? And libdrm is a bit confused in its return statements: ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy); if (ret == 0) { bo_gem->idle = !busy.busy; return busy.busy; } else { return false; } return (ret == 0 && busy.busy); Looks like it was a boolean as well until commit 02f93c21e6e1c3dad9d99349989daa84a8c0b5fb quite possibly by accident started exposing the bits. Regards, Tvrtko
On Fri, Jan 15, 2016 at 01:22:39PM +0000, Tvrtko Ursulin wrote: > Looks like your DDX is the only user not using it in the boolean mode? As far as I am aware, that is the only user that worries about which engine the object is currently active on. > And libdrm is a bit confused in its return statements: > > ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy); > if (ret == 0) { > bo_gem->idle = !busy.busy; > return busy.busy; > } else { > return false; > } > return (ret == 0 && busy.busy); > > Looks like it was a boolean as well until commit > 02f93c21e6e1c3dad9d99349989daa84a8c0b5fb quite possibly by accident > started exposing the bits. Hmm, libdrm bo_is_busy() was always meant to be boolean and that patch postdates when we started storing read/write bits in the return value. So definitely an unintentional leakage. -Chris
On 15/01/16 13:57, Chris Wilson wrote: > On Fri, Jan 15, 2016 at 01:22:39PM +0000, Tvrtko Ursulin wrote: >> Looks like your DDX is the only user not using it in the boolean mode? > > As far as I am aware, that is the only user that worries about which > engine the object is currently active on. > >> And libdrm is a bit confused in its return statements: >> >> ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy); >> if (ret == 0) { >> bo_gem->idle = !busy.busy; >> return busy.busy; >> } else { >> return false; >> } >> return (ret == 0 && busy.busy); >> >> Looks like it was a boolean as well until commit >> 02f93c21e6e1c3dad9d99349989daa84a8c0b5fb quite possibly by accident >> started exposing the bits. > > Hmm, libdrm bo_is_busy() was always meant to be boolean and that patch > postdates when we started storing read/write bits in the return value. > So definitely an unintentional leakage. In that case I think just respin with comment corrections in uapi header for drm_i915_gem_busy? Regards, Tvrtko
On Fri, Jan 15, 2016 at 04:02:52PM +0000, Tvrtko Ursulin wrote: > > On 15/01/16 13:57, Chris Wilson wrote: > >On Fri, Jan 15, 2016 at 01:22:39PM +0000, Tvrtko Ursulin wrote: > >>Looks like your DDX is the only user not using it in the boolean mode? > > > >As far as I am aware, that is the only user that worries about which > >engine the object is currently active on. > > > >>And libdrm is a bit confused in its return statements: > >> > >> ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy); > >> if (ret == 0) { > >> bo_gem->idle = !busy.busy; > >> return busy.busy; > >> } else { > >> return false; > >> } > >> return (ret == 0 && busy.busy); > >> > >>Looks like it was a boolean as well until commit > >>02f93c21e6e1c3dad9d99349989daa84a8c0b5fb quite possibly by accident > >>started exposing the bits. > > > >Hmm, libdrm bo_is_busy() was always meant to be boolean and that patch > >postdates when we started storing read/write bits in the return value. > >So definitely an unintentional leakage. > > In that case I think just respin with comment corrections in uapi > header for drm_i915_gem_busy? /** Return busy status * * A return of 0 implies that the object is idle (after * having flushed any pending activity), and a non-zero return that * the object is still in-flight on the GPU. (The GPU has not yet * signaled completion for all pending requests that reference the * object.) * * The returned dword is split into two fields to indicate both * the engines on which the object is being read, and the * engine on which is currently being writtern to (if any). * * The low word (bits 0:15) indicate if the object is being written * to by any engine (there can only be one, as the GEM implicit * synchronisation rules force writes to be serialised.) Only the * engine for last write is reported. * * The high word (bits 16:31) are a bitmask of which engines are * currently reading from the object. * * The value of each engine is the same as specified in the * EXECBUFFER2 ioctl, i.e. I915_EXEC_RENDER, I915_EXEC_BSD etc. * (Note I915_EXEC_DEFAULT is a symbolic value and is mapped to * the I915_EXEC_RENDER engine for execution, and so never reported * as active itself.) */
On 15/01/16 16:19, Chris Wilson wrote: > On Fri, Jan 15, 2016 at 04:02:52PM +0000, Tvrtko Ursulin wrote: >> >> On 15/01/16 13:57, Chris Wilson wrote: >>> On Fri, Jan 15, 2016 at 01:22:39PM +0000, Tvrtko Ursulin wrote: >>>> Looks like your DDX is the only user not using it in the boolean mode? >>> >>> As far as I am aware, that is the only user that worries about which >>> engine the object is currently active on. >>> >>>> And libdrm is a bit confused in its return statements: >>>> >>>> ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy); >>>> if (ret == 0) { >>>> bo_gem->idle = !busy.busy; >>>> return busy.busy; >>>> } else { >>>> return false; >>>> } >>>> return (ret == 0 && busy.busy); >>>> >>>> Looks like it was a boolean as well until commit >>>> 02f93c21e6e1c3dad9d99349989daa84a8c0b5fb quite possibly by accident >>>> started exposing the bits. >>> >>> Hmm, libdrm bo_is_busy() was always meant to be boolean and that patch >>> postdates when we started storing read/write bits in the return value. >>> So definitely an unintentional leakage. >> >> In that case I think just respin with comment corrections in uapi >> header for drm_i915_gem_busy? > > /** Return busy status > * > * A return of 0 implies that the object is idle (after > * having flushed any pending activity), and a non-zero return that > * the object is still in-flight on the GPU. (The GPU has not yet > * signaled completion for all pending requests that reference the > * object.) > * > * The returned dword is split into two fields to indicate both > * the engines on which the object is being read, and the > * engine on which is currently being writtern to (if any). > * > * The low word (bits 0:15) indicate if the object is being written > * to by any engine (there can only be one, as the GEM implicit > * synchronisation rules force writes to be serialised.) Only the > * engine for last write is reported. > * > * The high word (bits 16:31) are a bitmask of which engines are > * currently reading from the object. > * > * The value of each engine is the same as specified in the > * EXECBUFFER2 ioctl, i.e. I915_EXEC_RENDER, I915_EXEC_BSD etc. > * (Note I915_EXEC_DEFAULT is a symbolic value and is mapped to > * the I915_EXEC_RENDER engine for execution, and so never reported > * as active itself.) > */ > Very good, r-b on the resulting patch. Regards, Tvrtko
On 15/01/16 11:24, Patchwork wrote: > == Summary == > > Built on 615fbad7f4cea73b1a8eccdcc942c8ca1a708dab drm-intel-nightly: 2016y-01m-15d-09h-46m-32s UTC integration manifest > > Test gem_storedw_loop: > Subgroup basic-render: > dmesg-warn -> PASS (bdw-nuci7) > pass -> DMESG-WARN (skl-i7k-2) UNSTABLE Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=93693 > Test kms_flip: > Subgroup basic-flip-vs-modeset: > dmesg-warn -> PASS (skl-i5k-2) > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-a: > pass -> DMESG-WARN (byt-nuc) Known unrelated: https://bugs.freedesktop.org/show_bug.cgi?id=93121 > > bdw-nuci7 total:138 pass:129 dwarn:0 dfail:0 fail:0 skip:9 > bdw-ultra total:138 pass:132 dwarn:0 dfail:0 fail:0 skip:6 > bsw-nuc-2 total:141 pass:115 dwarn:2 dfail:0 fail:0 skip:24 > byt-nuc total:141 pass:122 dwarn:4 dfail:0 fail:0 skip:15 > hsw-brixbox total:141 pass:134 dwarn:0 dfail:0 fail:0 skip:7 > hsw-gt2 total:141 pass:137 dwarn:0 dfail:0 fail:0 skip:4 > ilk-hp8440p total:141 pass:101 dwarn:3 dfail:0 fail:0 skip:37 > skl-i5k-2 total:141 pass:132 dwarn:1 dfail:0 fail:0 skip:8 > skl-i7k-2 total:141 pass:131 dwarn:2 dfail:0 fail:0 skip:8 > snb-dellxps total:141 pass:122 dwarn:5 dfail:0 fail:0 skip:14 > snb-x220t total:141 pass:122 dwarn:5 dfail:0 fail:1 skip:13 > > Results at /archive/results/CI_IGT_test/Patchwork_1195/ > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > Patch merged. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index bb44bad15403..85797813a3de 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4328,10 +4328,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, if (ret) goto unref; - BUILD_BUG_ON(I915_NUM_RINGS > 16); - args->busy = obj->active << 16; - if (obj->last_write_req) - args->busy |= obj->last_write_req->ring->id; + args->busy = 0; + if (obj->active) { + int i; + + for (i = 0; i < I915_NUM_RINGS; i++) { + struct drm_i915_gem_request *req; + + req = obj->last_read_req[i]; + if (req) + args->busy |= 1 << (16 + req->ring->exec_id); + } + if (obj->last_write_req) + args->busy |= obj->last_write_req->ring->exec_id; + } unref: drm_gem_object_unreference(&obj->base); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index f5d89c845ede..4aa209483237 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2024,6 +2024,7 @@ static int logical_render_ring_init(struct drm_device *dev) ring->name = "render ring"; ring->id = RCS; + ring->exec_id = I915_EXEC_RENDER; ring->mmio_base = RENDER_RING_BASE; logical_ring_default_irqs(ring, GEN8_RCS_IRQ_SHIFT); @@ -2073,6 +2074,7 @@ static int logical_bsd_ring_init(struct drm_device *dev) ring->name = "bsd ring"; ring->id = VCS; + ring->exec_id = I915_EXEC_BSD; ring->mmio_base = GEN6_BSD_RING_BASE; logical_ring_default_irqs(ring, GEN8_VCS1_IRQ_SHIFT); @@ -2088,6 +2090,7 @@ static int logical_bsd2_ring_init(struct drm_device *dev) ring->name = "bsd2 ring"; ring->id = VCS2; + ring->exec_id = I915_EXEC_BSD; ring->mmio_base = GEN8_BSD2_RING_BASE; logical_ring_default_irqs(ring, GEN8_VCS2_IRQ_SHIFT); @@ -2103,6 +2106,7 @@ static int logical_blt_ring_init(struct drm_device *dev) ring->name = "blitter ring"; ring->id = BCS; + ring->exec_id = I915_EXEC_BLT; ring->mmio_base = BLT_RING_BASE; logical_ring_default_irqs(ring, GEN8_BCS_IRQ_SHIFT); @@ -2118,6 +2122,7 @@ static int logical_vebox_ring_init(struct drm_device *dev) ring->name = "video enhancement ring"; ring->id = VECS; + ring->exec_id = I915_EXEC_VEBOX; ring->mmio_base = VEBOX_RING_BASE; logical_ring_default_irqs(ring, GEN8_VECS_IRQ_SHIFT); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 8cd8aabcc3ff..310d151c0db2 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2680,6 +2680,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev) ring->name = "render ring"; ring->id = RCS; + ring->exec_id = I915_EXEC_RENDER; ring->mmio_base = RENDER_RING_BASE; if (INTEL_INFO(dev)->gen >= 8) { @@ -2828,6 +2829,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev) ring->name = "bsd ring"; ring->id = VCS; + ring->exec_id = I915_EXEC_BSD; ring->write_tail = ring_write_tail; if (INTEL_INFO(dev)->gen >= 6) { @@ -2904,6 +2906,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev) ring->name = "bsd2 ring"; ring->id = VCS2; + ring->exec_id = I915_EXEC_BSD; ring->write_tail = ring_write_tail; ring->mmio_base = GEN8_BSD2_RING_BASE; @@ -2934,6 +2937,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev) ring->name = "blitter ring"; ring->id = BCS; + ring->exec_id = I915_EXEC_BLT; ring->mmio_base = BLT_RING_BASE; ring->write_tail = ring_write_tail; @@ -2991,6 +2995,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev) ring->name = "video enhancement ring"; ring->id = VECS; + ring->exec_id = I915_EXEC_VEBOX; ring->mmio_base = VEBOX_RING_BASE; ring->write_tail = ring_write_tail; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 7349d9258191..2067f4700caa 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -156,6 +156,7 @@ struct intel_engine_cs { } id; #define I915_NUM_RINGS 5 #define LAST_USER_RING (VECS + 1) + unsigned int exec_id; u32 mmio_base; struct drm_device *dev; struct intel_ringbuffer *buffer;
Tvrtko was looking through the execbuffer-ioctl and noticed that the uABI was tightly coupled to our internal engine identifiers. Close inspection also revealed that we leak those internal engine identifiers through the busy-ioctl, and those internal identifiers already do not match the user identifiers. Fortuitiously, there is only one user of the set of busy rings from the busy-ioctl, and they only wish to choose between the RENDER and the BLT engines. Let's fix the userspace ABI while we still can. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++---- drivers/gpu/drm/i915/intel_lrc.c | 5 +++++ drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++ drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 4 files changed, 25 insertions(+), 4 deletions(-)