diff mbox

[RFC,20/21] drm/i915: Convert 'ring_idle()' to use requests not seqnos

Message ID 1412604925-11290-21-git-send-email-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Harrison Oct. 6, 2014, 2:15 p.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

For: VIZ-4377
Signed-off-by: John.C.Harrison@Intel.com
---
 drivers/gpu/drm/i915/i915_irq.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Daniel Vetter Oct. 19, 2014, 2:09 p.m. UTC | #1 
On Mon, Oct 06, 2014 at 03:15:24PM +0100, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@Intel.com

We have places that shovel stuff onto the ring without an explicit
add_request. Or at least we've had, so this needs a full audit, and that
audit needs to be in the commit message.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4847ed5..c2a7127 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3030,18 +3030,18 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> -static u32
> -ring_last_seqno(struct intel_engine_cs *ring)
> +static struct drm_i915_gem_request *
> +ring_last_request(struct intel_engine_cs *ring)
>  {
>  	return list_entry(ring->request_list.prev,
> -			  struct drm_i915_gem_request, list)->seqno;
> +			  struct drm_i915_gem_request, list);
>  }
>  
>  static bool
> -ring_idle(struct intel_engine_cs *ring, u32 seqno)
> +ring_idle(struct intel_engine_cs *ring)
>  {
>  	return (list_empty(&ring->request_list) ||
> -		i915_seqno_passed(seqno, ring_last_seqno(ring)));
> +		i915_gem_request_completed(ring_last_request(ring), false));
>  }
>  
>  static bool
> @@ -3261,7 +3261,7 @@ static void i915_hangcheck_elapsed(unsigned long data)
>  		acthd = intel_ring_get_active_head(ring);
>  
>  		if (ring->hangcheck.seqno == seqno) {
> -			if (ring_idle(ring, seqno)) {
> +			if (ring_idle(ring)) {
>  				ring->hangcheck.action = HANGCHECK_IDLE;
>  
>  				if (waitqueue_active(&ring->irq_queue)) {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
John Harrison Oct. 28, 2014, 2:03 p.m. UTC | #2 
On 19/10/2014 15:09, Daniel Vetter wrote:
> On Mon, Oct 06, 2014 at 03:15:24PM +0100, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> For: VIZ-4377
>> Signed-off-by: John.C.Harrison@Intel.com
> We have places that shovel stuff onto the ring without an explicit
> add_request. Or at least we've had, so this needs a full audit, and that
> audit needs to be in the commit message.
> -Daniel
Not sure what you mean. There is no functional change here. The old 
version pulled the seqno out of the last request entry in the list and 
compared that to the hardware seqno value to check for completion. The 
new version gets the same request entry and does the same completion 
test, just in request parlance. If commands are being dumped on the ring 
without an request being added to the request list then the old version 
would still not have been checking for them. It would still only look at 
the last piece of work that actually did call add_request and see if 
that has completed or not.

>> ---
>>   drivers/gpu/drm/i915/i915_irq.c |   12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 4847ed5..c2a7127 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3030,18 +3030,18 @@ static void gen8_disable_vblank(struct drm_device *dev, int pipe)
>>   	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>>   }
>>   
>> -static u32
>> -ring_last_seqno(struct intel_engine_cs *ring)
>> +static struct drm_i915_gem_request *
>> +ring_last_request(struct intel_engine_cs *ring)
>>   {
>>   	return list_entry(ring->request_list.prev,
>> -			  struct drm_i915_gem_request, list)->seqno;
>> +			  struct drm_i915_gem_request, list);
>>   }
>>   
>>   static bool
>> -ring_idle(struct intel_engine_cs *ring, u32 seqno)
>> +ring_idle(struct intel_engine_cs *ring)
>>   {
>>   	return (list_empty(&ring->request_list) ||
>> -		i915_seqno_passed(seqno, ring_last_seqno(ring)));
>> +		i915_gem_request_completed(ring_last_request(ring), false));
>>   }
>>   
>>   static bool
>> @@ -3261,7 +3261,7 @@ static void i915_hangcheck_elapsed(unsigned long data)
>>   		acthd = intel_ring_get_active_head(ring);
>>   
>>   		if (ring->hangcheck.seqno == seqno) {
>> -			if (ring_idle(ring, seqno)) {
>> +			if (ring_idle(ring)) {
>>   				ring->hangcheck.action = HANGCHECK_IDLE;
>>   
>>   				if (waitqueue_active(&ring->irq_queue)) {
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Nov. 3, 2014, 10:44 a.m. UTC | #3 
On Tue, Oct 28, 2014 at 02:03:14PM +0000, John Harrison wrote:
> On 19/10/2014 15:09, Daniel Vetter wrote:
> >We have places that shovel stuff onto the ring without an explicit
> >add_request. Or at least we've had, so this needs a full audit, and that
> >audit needs to be in the commit message.
> Not sure what you mean. There is no functional change here. The old version
> pulled the seqno out of the last request entry in the list and compared that
> to the hardware seqno value to check for completion. The new version gets
> the same request entry and does the same completion test, just in request
> parlance. If commands are being dumped on the ring without an request being
> added to the request list then the old version would still not have been
> checking for them. It would still only look at the last piece of work that
> actually did call add_request and see if that has completed or not.

Oh, I've pretty badly mixed stuff up ;-)

The issue I'm seeing here though is acessing the request list, from irq
context, without any locks. Pre-existing, but still deserves a FIXME
comment.

Wrt the change itself I'm not sure it's that useful, this is fairly
low-level code.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4847ed5..c2a7127 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3030,18 +3030,18 @@  static void gen8_disable_vblank(struct drm_device *dev, int pipe)
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-static u32
-ring_last_seqno(struct intel_engine_cs *ring)
+static struct drm_i915_gem_request *
+ring_last_request(struct intel_engine_cs *ring)
 {
 	return list_entry(ring->request_list.prev,
-			  struct drm_i915_gem_request, list)->seqno;
+			  struct drm_i915_gem_request, list);
 }
 
 static bool
-ring_idle(struct intel_engine_cs *ring, u32 seqno)
+ring_idle(struct intel_engine_cs *ring)
 {
 	return (list_empty(&ring->request_list) ||
-		i915_seqno_passed(seqno, ring_last_seqno(ring)));
+		i915_gem_request_completed(ring_last_request(ring), false));
 }
 
 static bool
@@ -3261,7 +3261,7 @@  static void i915_hangcheck_elapsed(unsigned long data)
 		acthd = intel_ring_get_active_head(ring);
 
 		if (ring->hangcheck.seqno == seqno) {
-			if (ring_idle(ring, seqno)) {
+			if (ring_idle(ring)) {
 				ring->hangcheck.action = HANGCHECK_IDLE;
 
 				if (waitqueue_active(&ring->irq_queue)) {