[PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Chris Wilson]

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 (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;
-- 
2.7.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Patchwork]

== 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
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)

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

Re: [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Tvrtko Ursulin]

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;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH igt] tests: Add gem_busy

[Chris Wilson]

Exercise the busy-ioctl and verify it reports the right active engines
using the execbuffer notation.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/Makefile.sources |   1 +
 tests/gem_busy.c       | 233 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 234 insertions(+)
 create mode 100644 tests/gem_busy.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index d2c7d6f..545aca0 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -11,6 +11,7 @@ TESTS_progs_M = \
 	drv_hangman \
 	gem_bad_reloc \
 	gem_basic \
+	gem_busy \
 	gem_caching \
 	gem_close_race \
 	gem_concurrent_blit \
diff --git a/tests/gem_busy.c b/tests/gem_busy.c
new file mode 100644
index 0000000..c6b8a8b
--- /dev/null
+++ b/tests/gem_busy.c
@@ -0,0 +1,233 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+
+/* Exercise the busy-ioctl, ensuring the ABI is never broken */
+IGT_TEST_DESCRIPTION("Basic check of busy-ioctl ABI.");
+
+enum { TEST = 0, BUSY, BATCH };
+
+static void __gem_busy(int fd,
+		       uint32_t handle,
+		       uint32_t *read,
+		       uint32_t *write)
+{
+	struct drm_i915_gem_busy busy;
+
+	memset(&busy, 0, sizeof(busy));
+	busy.handle = handle;
+
+	do_ioctl(fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
+
+	*write = busy.busy & 0xffff;
+	*read = busy.busy >> 16;
+}
+
+static uint32_t busy_blt(int fd)
+{
+	const int gen = intel_gen(intel_get_drm_devid(fd));
+	const int has_64bit_reloc = gen >= 8;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 object[2];
+	struct drm_i915_gem_relocation_entry reloc[20], *r;
+	uint32_t read, write;
+	uint32_t *map;
+	int factor = 10;
+	int i = 0;
+
+	memset(object, 0, sizeof(object));
+	object[0].handle = gem_create(fd, 1024*1024);
+	object[1].handle = gem_create(fd, 4096);
+
+	r = memset(reloc, 0, sizeof(reloc));
+	map = gem_mmap__cpu(fd, object[1].handle, 0, 4096, PROT_WRITE);
+	gem_set_domain(fd, object[1].handle,
+		       I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
+
+#define COPY_BLT_CMD		(2<<29|0x53<<22|0x6)
+#define BLT_WRITE_ALPHA		(1<<21)
+#define BLT_WRITE_RGB		(1<<20)
+	while (factor--) {
+		/* XY_SRC_COPY */
+		map[i++] = COPY_BLT_CMD | BLT_WRITE_ALPHA | BLT_WRITE_RGB;
+		if (has_64bit_reloc)
+			map[i-1] += 2;
+		map[i++] = 0xcc << 16 | 1 << 25 | 1 << 24 | (4*1024);
+		map[i++] = 0;
+		map[i++] = 256 << 16 | 1024;
+
+		r->offset = i * sizeof(uint32_t);
+		r->target_handle = object[0].handle;
+		r->read_domains = I915_GEM_DOMAIN_RENDER;
+		r->write_domain = I915_GEM_DOMAIN_RENDER;
+		r++;
+		map[i++] = 0;
+		if (has_64bit_reloc)
+			map[i++] = 0;
+
+		map[i++] = 0;
+		map[i++] = 4096;
+
+		r->offset = i * sizeof(uint32_t);
+		r->target_handle = object[0].handle;
+		r->read_domains = I915_GEM_DOMAIN_RENDER;
+		r->write_domain = 0;
+		r++;
+		map[i++] = 0;
+		if (has_64bit_reloc)
+			map[i++] = 0;
+	}
+	map[i++] = MI_BATCH_BUFFER_END;
+	munmap(map, 4096);
+
+	object[1].relocs_ptr = (uintptr_t)reloc;
+	object[1].relocation_count = r - reloc;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (unsigned long)object;
+	execbuf.buffer_count = 2;
+	if (gen >= 6)
+		execbuf.flags = I915_EXEC_BLT;
+	gem_execbuf(fd, &execbuf);
+
+	__gem_busy(fd, object[0].handle, &read, &write);
+	igt_assert_eq(read, 1 << write);
+	igt_assert_eq(write, gen >= 6 ? I915_EXEC_BLT : I915_EXEC_RENDER);
+
+	igt_debug("Created busy handle %d\n", object[0].handle);
+	gem_close(fd, object[1].handle);
+	return object[0].handle;
+}
+
+static int __gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *eb)
+{
+	int err = 0;
+	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, eb))
+		err = -errno;
+	return err;
+}
+
+static bool exec_noop(int fd,
+		      uint32_t *handles,
+		      unsigned ring,
+		      bool write)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 exec[3];
+
+	memset(exec, 0, sizeof(exec));
+	exec[0].handle = handles[BUSY];
+	exec[1].handle = handles[TEST];
+	if (write)
+		exec[1].flags |= EXEC_OBJECT_WRITE;
+	exec[2].handle = handles[BATCH];
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = (uintptr_t)exec;
+	execbuf.buffer_count = 3;
+	execbuf.flags = ring;
+	igt_debug("Queuing handle for %s on ring %d\n",
+		  write ? "writing" : "reading", ring & 0x7);
+	return __gem_execbuf(fd, &execbuf) == 0;
+}
+
+static void test_ring(int fd, unsigned ring, uint32_t flags)
+{
+	uint32_t bbe = MI_BATCH_BUFFER_END;
+	uint32_t handle[3];
+	uint32_t read, write;
+	uint32_t active;
+	unsigned i;
+
+	handle[TEST] = gem_create(fd, 4096);
+	handle[BATCH] = gem_create(fd, 4096);
+	gem_write(fd, handle[BATCH], 0, &bbe, sizeof(bbe));
+
+	/* Create a long running batch which we can use to hog the GPU */
+	handle[BUSY] = busy_blt(fd);
+
+	/* Queue a batch after the busy, it should block and remain "busy" */
+	igt_require(exec_noop(fd, handle, ring | flags, false));
+	__gem_busy(fd, handle[TEST], &read, &write);
+	igt_assert_eq(read, 1 << ring);
+	igt_assert_eq(write, 0);
+
+	/* Now queue it on all available rings, and set it as write on this */
+	active = 0;
+	for (i = I915_EXEC_RENDER; i <= I915_EXEC_VEBOX; i++) {
+		if (exec_noop(fd, handle, i | flags, i == ring))
+			active |= 1 << i;
+	}
+	__gem_busy(fd, handle[TEST], &read, &write);
+	igt_assert_eq(read, active);
+	igt_assert_eq(write, ring);
+
+	/* Check that our long batch was long enough */
+	__gem_busy(fd, handle[BUSY], &read, &write);
+	igt_assert(write);
+
+	for (i = TEST; i <= BATCH; i++)
+		gem_close(fd, handle[i]);
+}
+
+static bool has_semaphores(int fd)
+{
+	struct drm_i915_getparam gp;
+	int val = -1;
+
+	memset(&gp, 0, sizeof(gp));
+	gp.param = I915_PARAM_HAS_SEMAPHORES;
+	gp.value = &val;
+
+	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	errno = 0;
+
+	return val > 0;
+}
+
+igt_main
+{
+	int fd = -1;
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		fd = drm_open_driver(DRIVER_INTEL);
+		igt_require(has_semaphores(fd));
+	}
+
+	igt_subtest("render")
+		test_ring(fd, I915_EXEC_RENDER, 0);
+	igt_subtest("bsd")
+		test_ring(fd, I915_EXEC_BSD, 1<<13 /*I915_EXEC_BSD_RING1*/);
+	igt_subtest("bsd2")
+		test_ring(fd, I915_EXEC_BSD, 2<<13 /*I915_EXEC_BSD_RING2*/);
+	igt_subtest("blt")
+		test_ring(fd, I915_EXEC_BLT, 0);
+	igt_subtest("vebox")
+		test_ring(fd, I915_EXEC_VEBOX, 0);
+
+	igt_fixture
+		close(fd);
+}
-- 
2.7.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt] tests: Add gem_busy

[Tvrtko Ursulin]

On 15/01/16 13:53, Chris Wilson wrote:
> Exercise the busy-ioctl and verify it reports the right active engines
> using the execbuffer notation.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/Makefile.sources |   1 +
>   tests/gem_busy.c       | 233 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 234 insertions(+)
>   create mode 100644 tests/gem_busy.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index d2c7d6f..545aca0 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -11,6 +11,7 @@ TESTS_progs_M = \
>   	drv_hangman \
>   	gem_bad_reloc \
>   	gem_basic \
> +	gem_busy \
>   	gem_caching \
>   	gem_close_race \
>   	gem_concurrent_blit \
> diff --git a/tests/gem_busy.c b/tests/gem_busy.c
> new file mode 100644
> index 0000000..c6b8a8b
> --- /dev/null
> +++ b/tests/gem_busy.c
> @@ -0,0 +1,233 @@
> +/*
> + * Copyright © 2015 Intel Corporation

2016

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "igt.h"
> +
> +/* Exercise the busy-ioctl, ensuring the ABI is never broken */
> +IGT_TEST_DESCRIPTION("Basic check of busy-ioctl ABI.");
> +
> +enum { TEST = 0, BUSY, BATCH };
> +
> +static void __gem_busy(int fd,
> +		       uint32_t handle,
> +		       uint32_t *read,
> +		       uint32_t *write)
> +{
> +	struct drm_i915_gem_busy busy;
> +
> +	memset(&busy, 0, sizeof(busy));
> +	busy.handle = handle;
> +
> +	do_ioctl(fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
> +
> +	*write = busy.busy & 0xffff;
> +	*read = busy.busy >> 16;
> +}
> +
> +static uint32_t busy_blt(int fd)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	const int has_64bit_reloc = gen >= 8;
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 object[2];
> +	struct drm_i915_gem_relocation_entry reloc[20], *r;
> +	uint32_t read, write;
> +	uint32_t *map;
> +	int factor = 10;
> +	int i = 0;
> +
> +	memset(object, 0, sizeof(object));
> +	object[0].handle = gem_create(fd, 1024*1024);
> +	object[1].handle = gem_create(fd, 4096);
> +
> +	r = memset(reloc, 0, sizeof(reloc));
> +	map = gem_mmap__cpu(fd, object[1].handle, 0, 4096, PROT_WRITE);
> +	gem_set_domain(fd, object[1].handle,
> +		       I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> +
> +#define COPY_BLT_CMD		(2<<29|0x53<<22|0x6)
> +#define BLT_WRITE_ALPHA		(1<<21)
> +#define BLT_WRITE_RGB		(1<<20)
> +	while (factor--) {
> +		/* XY_SRC_COPY */
> +		map[i++] = COPY_BLT_CMD | BLT_WRITE_ALPHA | BLT_WRITE_RGB;
> +		if (has_64bit_reloc)
> +			map[i-1] += 2;
> +		map[i++] = 0xcc << 16 | 1 << 25 | 1 << 24 | (4*1024);
> +		map[i++] = 0;
> +		map[i++] = 256 << 16 | 1024;
> +
> +		r->offset = i * sizeof(uint32_t);
> +		r->target_handle = object[0].handle;
> +		r->read_domains = I915_GEM_DOMAIN_RENDER;
> +		r->write_domain = I915_GEM_DOMAIN_RENDER;
> +		r++;
> +		map[i++] = 0;
> +		if (has_64bit_reloc)
> +			map[i++] = 0;
> +
> +		map[i++] = 0;
> +		map[i++] = 4096;
> +
> +		r->offset = i * sizeof(uint32_t);
> +		r->target_handle = object[0].handle;
> +		r->read_domains = I915_GEM_DOMAIN_RENDER;
> +		r->write_domain = 0;
> +		r++;
> +		map[i++] = 0;
> +		if (has_64bit_reloc)
> +			map[i++] = 0;
> +	}
> +	map[i++] = MI_BATCH_BUFFER_END;
> +	munmap(map, 4096);
> +
> +	object[1].relocs_ptr = (uintptr_t)reloc;
> +	object[1].relocation_count = r - reloc;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = (unsigned long)object;
> +	execbuf.buffer_count = 2;
> +	if (gen >= 6)
> +		execbuf.flags = I915_EXEC_BLT;
> +	gem_execbuf(fd, &execbuf);
> +
> +	__gem_busy(fd, object[0].handle, &read, &write);
> +	igt_assert_eq(read, 1 << write);
> +	igt_assert_eq(write, gen >= 6 ? I915_EXEC_BLT : I915_EXEC_RENDER);

Will the blits be long enough for this to be stable?

> +
> +	igt_debug("Created busy handle %d\n", object[0].handle);
> +	gem_close(fd, object[1].handle);
> +	return object[0].handle;
> +}
> +
> +static int __gem_execbuf(int fd, struct drm_i915_gem_execbuffer2 *eb)
> +{
> +	int err = 0;
> +	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, eb))
> +		err = -errno;
> +	return err;
> +}
> +
> +static bool exec_noop(int fd,
> +		      uint32_t *handles,
> +		      unsigned ring,
> +		      bool write)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 exec[3];
> +
> +	memset(exec, 0, sizeof(exec));
> +	exec[0].handle = handles[BUSY];
> +	exec[1].handle = handles[TEST];
> +	if (write)
> +		exec[1].flags |= EXEC_OBJECT_WRITE;
> +	exec[2].handle = handles[BATCH];
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = (uintptr_t)exec;
> +	execbuf.buffer_count = 3;
> +	execbuf.flags = ring;
> +	igt_debug("Queuing handle for %s on ring %d\n",
> +		  write ? "writing" : "reading", ring & 0x7);
> +	return __gem_execbuf(fd, &execbuf) == 0;
> +}
> +
> +static void test_ring(int fd, unsigned ring, uint32_t flags)
> +{
> +	uint32_t bbe = MI_BATCH_BUFFER_END;
> +	uint32_t handle[3];
> +	uint32_t read, write;
> +	uint32_t active;
> +	unsigned i;
> +
> +	handle[TEST] = gem_create(fd, 4096);
> +	handle[BATCH] = gem_create(fd, 4096);
> +	gem_write(fd, handle[BATCH], 0, &bbe, sizeof(bbe));
> +
> +	/* Create a long running batch which we can use to hog the GPU */
> +	handle[BUSY] = busy_blt(fd);
> +
> +	/* Queue a batch after the busy, it should block and remain "busy" */
> +	igt_require(exec_noop(fd, handle, ring | flags, false));

Too bad we don't get ENODEV but a boring old EINVAL on !HAS_BSD2 - this 
was this can be too skip happy if something else gets broken. :(

> +	__gem_busy(fd, handle[TEST], &read, &write);
> +	igt_assert_eq(read, 1 << ring);
> +	igt_assert_eq(write, 0);
> +
> +	/* Now queue it on all available rings, and set it as write on this */
> +	active = 0;
> +	for (i = I915_EXEC_RENDER; i <= I915_EXEC_VEBOX; i++) {
> +		if (exec_noop(fd, handle, i | flags, i == ring))
> +			active |= 1 << i;
> +	}
> +	__gem_busy(fd, handle[TEST], &read, &write);
> +	igt_assert_eq(read, active);
> +	igt_assert_eq(write, ring);
> +
> +	/* Check that our long batch was long enough */
> +	__gem_busy(fd, handle[BUSY], &read, &write);
> +	igt_assert(write);
> +
> +	for (i = TEST; i <= BATCH; i++)
> +		gem_close(fd, handle[i]);
> +}
> +
> +static bool has_semaphores(int fd)
> +{
> +	struct drm_i915_getparam gp;
> +	int val = -1;
> +
> +	memset(&gp, 0, sizeof(gp));
> +	gp.param = I915_PARAM_HAS_SEMAPHORES;
> +	gp.value = &val;
> +
> +	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	errno = 0;
> +
> +	return val > 0;
> +}
> +
> +igt_main
> +{
> +	int fd = -1;
> +
> +	igt_skip_on_simulation();
> +
> +	igt_fixture {
> +		fd = drm_open_driver(DRIVER_INTEL);
> +		igt_require(has_semaphores(fd));

Copy&paste, or?

> +	}
> +
> +	igt_subtest("render")
> +		test_ring(fd, I915_EXEC_RENDER, 0);
> +	igt_subtest("bsd")
> +		test_ring(fd, I915_EXEC_BSD, 1<<13 /*I915_EXEC_BSD_RING1*/);
> +	igt_subtest("bsd2")
> +		test_ring(fd, I915_EXEC_BSD, 2<<13 /*I915_EXEC_BSD_RING2*/);
> +	igt_subtest("blt")
> +		test_ring(fd, I915_EXEC_BLT, 0);
> +	igt_subtest("vebox")
> +		test_ring(fd, I915_EXEC_VEBOX, 0);
> +
> +	igt_fixture
> +		close(fd);
> +}
>

Looks OK to me,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Maybe mark as basic since it is ABI?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt] tests: Add gem_busy

[Chris Wilson]

On Fri, Jan 15, 2016 at 02:45:49PM +0000, Tvrtko Ursulin wrote:
> 
> On 15/01/16 13:53, Chris Wilson wrote:
> >Exercise the busy-ioctl and verify it reports the right active engines
> >using the execbuffer notation.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >  tests/Makefile.sources |   1 +
> >  tests/gem_busy.c       | 233 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 234 insertions(+)
> >  create mode 100644 tests/gem_busy.c
> >
> >diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> >index d2c7d6f..545aca0 100644
> >--- a/tests/Makefile.sources
> >+++ b/tests/Makefile.sources
> >@@ -11,6 +11,7 @@ TESTS_progs_M = \
> >  	drv_hangman \
> >  	gem_bad_reloc \
> >  	gem_basic \
> >+	gem_busy \
> >  	gem_caching \
> >  	gem_close_race \
> >  	gem_concurrent_blit \
> >diff --git a/tests/gem_busy.c b/tests/gem_busy.c
> >new file mode 100644
> >index 0000000..c6b8a8b
> >--- /dev/null
> >+++ b/tests/gem_busy.c
> >@@ -0,0 +1,233 @@
> >+/*
> >+ * Copyright © 2015 Intel Corporation
> 
> 2016

Already?

> >+	__gem_busy(fd, object[0].handle, &read, &write);
> >+	igt_assert_eq(read, 1 << write);
> >+	igt_assert_eq(write, gen >= 6 ? I915_EXEC_BLT : I915_EXEC_RENDER);
> 
> Will the blits be long enough for this to be stable?

I can bump it by a factor of 5 easily enough, that should put it in the
range of a few milliseconds even on a fast system. If we get rescheduled,
we might miss the window for the test.

I did think of asserting that the blit was still busy eash time.

> >+	/* Queue a batch after the busy, it should block and remain "busy" */
> >+	igt_require(exec_noop(fd, handle, ring | flags, false));
> 
> Too bad we don't get ENODEV but a boring old EINVAL on !HAS_BSD2 -
> this was this can be too skip happy if something else gets broken.
> :(

True, but it would be better to have a test that checked which rings the
kernel said were on the hardware and that execbuffer supported them.

> >+igt_main
> >+{
> >+	int fd = -1;
> >+
> >+	igt_skip_on_simulation();
> >+
> >+	igt_fixture {
> >+		fd = drm_open_driver(DRIVER_INTEL);
> >+		igt_require(has_semaphores(fd));
> 
> Copy&paste, or?

A requirement. At least on the current infrastructure. It uses the sync
with the write on the BLT ring to block the GPU on the other rings, so
that the nops don't complete too early. We would need a busy workload on
each engine - code that we don't have yet.

Without semaphores, and without deferred scheduling, we current wait for
the busy workload to complete before scheduling on the next...

If I lie about the busywork being a write target, we will get parallel
execution rather than blocking and so not be able to set the right
flags. I couldn't see a way around semaphores without gen specific new
code.

> Maybe mark as basic since it is ABI?

Can't until the kernel abides by the abi we just concocted :) I
definitely don't trust my judgement as to what is basic. Surviving weeks
of stress testing is a basic requirement for us :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Chris Wilson]

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.)
         */

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Chris Wilson]

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.

v2: Update the uAPI documentation to explain the identifiers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 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 +
 include/uapi/drm/i915_drm.h             | 33 +++++++++++++++++++++++++++++----
 5 files changed, 54 insertions(+), 8 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 (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;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index acf21026c78a..6a19371391fa 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -812,10 +812,35 @@ struct drm_i915_gem_busy {
 	/** Handle of the buffer to check for busy */
 	__u32 handle;
 
-	/** Return busy status (1 if busy, 0 if idle).
-	 * The high word is used to indicate on which rings the object
-	 * currently resides:
-	 *  16:31 - busy (r or r/w) rings (16 render, 17 bsd, 18 blt, etc)
+	/** 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 it is currently being written (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 the last write is reported.
+	 *
+	 * The high word (bits 16:31) are a bitmask of which engines are
+	 * currently reading from the object. Multiple engines may be
+	 * reading from the object simultaneously.
+	 *
+	 * 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 it is never
+	 * reported as active itself. Some hardware may have parallel
+	 * execution engines, e.g. multiple media engines, which are
+	 * mapped to the same identifier in the EXECBUFFER2 ioctl and
+	 * so are not separately reported for busyness.
 	 */
 	__u32 busy;
 };
-- 
2.7.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Chris Wilson]

On Fri, Jan 15, 2016 at 04:51:46PM +0000, 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.
> 
> v2: Update the uAPI documentation to explain the identifiers.

Testcase: igt/gem_busy
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids (rev2)

[Patchwork]

== Summary ==

Built on 615fbad7f4cea73b1a8eccdcc942c8ca1a708dab drm-intel-nightly: 2016y-01m-15d-09h-46m-32s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (skl-i5k-2) UNSTABLE
                dmesg-warn -> PASS       (bdw-nuci7) UNSTABLE
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (skl-i5k-2)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (snb-x220t)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (byt-nuc)

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 
ivb-t430s        total:135  pass:122  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:141  pass:131  dwarn:2   dfail:0   fail:0   skip:8  
skl-i7k-2        total:141  pass:132  dwarn:1   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:121  dwarn:5   dfail:0   fail:2   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1202/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Daniel Vetter]

On Fri, Jan 15, 2016 at 04:51:46PM +0000, 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.
> 
> v2: Update the uAPI documentation to explain the identifiers.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Acked-by: 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 +
>  include/uapi/drm/i915_drm.h             | 33 +++++++++++++++++++++++++++++----
>  5 files changed, 54 insertions(+), 8 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 (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;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index acf21026c78a..6a19371391fa 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -812,10 +812,35 @@ struct drm_i915_gem_busy {
>  	/** Handle of the buffer to check for busy */
>  	__u32 handle;
>  
> -	/** Return busy status (1 if busy, 0 if idle).
> -	 * The high word is used to indicate on which rings the object
> -	 * currently resides:
> -	 *  16:31 - busy (r or r/w) rings (16 render, 17 bsd, 18 blt, etc)
> +	/** 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 it is currently being written (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 the last write is reported.
> +	 *
> +	 * The high word (bits 16:31) are a bitmask of which engines are
> +	 * currently reading from the object. Multiple engines may be
> +	 * reading from the object simultaneously.
> +	 *
> +	 * 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 it is never
> +	 * reported as active itself. Some hardware may have parallel
> +	 * execution engines, e.g. multiple media engines, which are
> +	 * mapped to the same identifier in the EXECBUFFER2 ioctl and
> +	 * so are not separately reported for busyness.
>  	 */
>  	__u32 busy;
>  };
> -- 
> 2.7.0.rc3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx