drm/i915: Fix VCS ring selection after uapi decoupling
diff mbox

Message ID 1453898323-30925-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Jan. 27, 2016, 12:38 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This got broken in:

   commit de1add360522c876c25ef2bbbbab1c94bdb509ab
   Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
   Date:   Fri Jan 15 15:12:50 2016 +0000

       drm/i915: Decouple execbuf uAPI from internal implementation

BSD ring flags need to be shifted before they can be considered
indices into the ring array.

Reported by Zhipeng Gong.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhipeng Gong <zhipeng.gong@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++----
 include/uapi/drm/i915_drm.h                | 10 ++++++----
 2 files changed, 13 insertions(+), 8 deletions(-)

Comments

Chris Wilson Jan. 27, 2016, 1 p.m. UTC | #1
On Wed, Jan 27, 2016 at 12:38:43PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> This got broken in:
> 
>    commit de1add360522c876c25ef2bbbbab1c94bdb509ab
>    Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>    Date:   Fri Jan 15 15:12:50 2016 +0000
> 
>        drm/i915: Decouple execbuf uAPI from internal implementation
> 
> BSD ring flags need to be shifted before they can be considered
> indices into the ring array.
> 
> Reported by Zhipeng Gong.
> 

This should be hit by gem_busy on appropriate hardware. I need to add
the extra ring to gem_exec_nop and make those all basic test.

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Zhipeng Gong <zhipeng.gong@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++----
>  include/uapi/drm/i915_drm.h                | 10 ++++++----
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 2dc08ce1079a..6fc620af14fa 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1395,12 +1395,15 @@ eb_select_ring(struct drm_i915_private *dev_priv,
>  	}
>  
>  	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
> -		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
> +		unsigned int bsd_idx = (args->flags & I915_EXEC_BSD_MASK) >>
> +					I915_EXEC_BSD_SHIFT;
>  
> -		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
> +		if (bsd_idx == (I915_EXEC_BSD_DEFAULT >> I915_EXEC_BSD_SHIFT)) {
>  			bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
> -		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
> -			   bsd_idx <= I915_EXEC_BSD_RING2) {
> +		} else if (bsd_idx >=
> +			  (I915_EXEC_BSD_RING1 >> I915_EXEC_BSD_SHIFT) &&
> +			  bsd_idx <=
> +			  (I915_EXEC_BSD_RING2 >> I915_EXEC_BSD_SHIFT)) {

Wouldn't inserting bsd_idx >>= I915_EXEC_BSD_SHIFT; here be simpler ?
>  			bsd_idx--;
Tvrtko Ursulin Jan. 27, 2016, 1:42 p.m. UTC | #2
On 27/01/16 13:00, Chris Wilson wrote:
> On Wed, Jan 27, 2016 at 12:38:43PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> This got broken in:
>>
>>     commit de1add360522c876c25ef2bbbbab1c94bdb509ab
>>     Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>     Date:   Fri Jan 15 15:12:50 2016 +0000
>>
>>         drm/i915: Decouple execbuf uAPI from internal implementation
>>
>> BSD ring flags need to be shifted before they can be considered
>> indices into the ring array.
>>
>> Reported by Zhipeng Gong.
>>
>
> This should be hit by gem_busy on appropriate hardware. I need to add
> the extra ring to gem_exec_nop and make those all basic test.

Right hardware is the key.

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Zhipeng Gong <zhipeng.gong@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++----
>>   include/uapi/drm/i915_drm.h                | 10 ++++++----
>>   2 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 2dc08ce1079a..6fc620af14fa 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1395,12 +1395,15 @@ eb_select_ring(struct drm_i915_private *dev_priv,
>>   	}
>>
>>   	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
>> -		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
>> +		unsigned int bsd_idx = (args->flags & I915_EXEC_BSD_MASK) >>
>> +					I915_EXEC_BSD_SHIFT;
>>
>> -		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
>> +		if (bsd_idx == (I915_EXEC_BSD_DEFAULT >> I915_EXEC_BSD_SHIFT)) {
>>   			bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
>> -		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
>> -			   bsd_idx <= I915_EXEC_BSD_RING2) {
>> +		} else if (bsd_idx >=
>> +			  (I915_EXEC_BSD_RING1 >> I915_EXEC_BSD_SHIFT) &&
>> +			  bsd_idx <=
>> +			  (I915_EXEC_BSD_RING2 >> I915_EXEC_BSD_SHIFT)) {
>
> Wouldn't inserting bsd_idx >>= I915_EXEC_BSD_SHIFT; here be simpler ?

Indeed it would, v2 is on the way.

Regards,

Tvrtko

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2dc08ce1079a..6fc620af14fa 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1395,12 +1395,15 @@  eb_select_ring(struct drm_i915_private *dev_priv,
 	}
 
 	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
-		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
+		unsigned int bsd_idx = (args->flags & I915_EXEC_BSD_MASK) >>
+					I915_EXEC_BSD_SHIFT;
 
-		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
+		if (bsd_idx == (I915_EXEC_BSD_DEFAULT >> I915_EXEC_BSD_SHIFT)) {
 			bsd_idx = gen8_dispatch_bsd_ring(dev_priv, file);
-		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
-			   bsd_idx <= I915_EXEC_BSD_RING2) {
+		} else if (bsd_idx >=
+			  (I915_EXEC_BSD_RING1 >> I915_EXEC_BSD_SHIFT) &&
+			  bsd_idx <=
+			  (I915_EXEC_BSD_RING2 >> I915_EXEC_BSD_SHIFT)) {
 			bsd_idx--;
 		} else {
 			DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 6a19371391fa..a5524cc95ff8 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -772,10 +772,12 @@  struct drm_i915_gem_execbuffer2 {
 #define I915_EXEC_HANDLE_LUT		(1<<12)
 
 /** Used for switching BSD rings on the platforms with two BSD rings */
-#define I915_EXEC_BSD_MASK		(3<<13)
-#define I915_EXEC_BSD_DEFAULT		(0<<13) /* default ping-pong mode */
-#define I915_EXEC_BSD_RING1		(1<<13)
-#define I915_EXEC_BSD_RING2		(2<<13)
+#define I915_EXEC_BSD_SHIFT	 (13)
+#define I915_EXEC_BSD_MASK	 (3 << I915_EXEC_BSD_SHIFT)
+/* default ping-pong mode */
+#define I915_EXEC_BSD_DEFAULT	 (0 << I915_EXEC_BSD_SHIFT)
+#define I915_EXEC_BSD_RING1	 (1 << I915_EXEC_BSD_SHIFT)
+#define I915_EXEC_BSD_RING2	 (2 << I915_EXEC_BSD_SHIFT)
 
 /** Tell the kernel that the batchbuffer is processed by
  *  the resource streamer.