drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids
diff mbox

Message ID 1452856013-29728-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson Jan. 15, 2016, 11:06 a.m. UTC
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(-)

Comments

Tvrtko Ursulin Jan. 15, 2016, 11:58 a.m. UTC | #1
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;
>
Chris Wilson Jan. 15, 2016, 12:27 p.m. UTC | #2
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
Tvrtko Ursulin Jan. 15, 2016, 1:22 p.m. UTC | #3
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
Chris Wilson Jan. 15, 2016, 1:57 p.m. UTC | #4
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
Tvrtko Ursulin Jan. 15, 2016, 4:02 p.m. UTC | #5
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
Chris Wilson Jan. 15, 2016, 4:19 p.m. UTC | #6
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.)
         */
Tvrtko Ursulin Jan. 15, 2016, 4:24 p.m. UTC | #7
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
Tvrtko Ursulin Jan. 21, 2016, 11:05 a.m. UTC | #8
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

Patch
diff mbox

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;