diff mbox

drm/i915: Don't skip request retirement if the active list is empty

Message ID 1432827156-9605-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala May 28, 2015, 3:32 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently we can have requests even if though the active list is empty,
so do the request retirement regardless of whether there's anything
on the active list.

The way it happened here is that during suspend intel_ring_idle()
notices the olr hanging around and then proceeds to get rid of it by
adding a request. However since there was nothing on the active lists
i915_gem_retire_requests() didn't clean those up, and so the idle work
never runs, and we leave the GPU "busy" during suspend resulting in a
WARN later.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Chris Wilson May 28, 2015, 3:47 p.m. UTC | #1 
On Thu, May 28, 2015 at 06:32:36PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Apparently we can have requests even if though the active list is empty,
> so do the request retirement regardless of whether there's anything
> on the active list.
> 
> The way it happened here is that during suspend intel_ring_idle()
> notices the olr hanging around and then proceeds to get rid of it by
> adding a request. However since there was nothing on the active lists
> i915_gem_retire_requests() didn't clean those up, and so the idle work
> never runs, and we leave the GPU "busy" during suspend resulting in a
> WARN later.

Whlist I agree (I use list_empty(&ring->request_list);) I strongly
suspect something (i.e. execlists) isn't managing the active_list
correctly. Pretty much the only thing that can generate a request
without an object (and so avoid touching the active_list) is a CS flip,
and I doubt you are using those...

Anyway,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.linux.org
-Chris
Ville Syrjala May 28, 2015, 4:03 p.m. UTC | #2 
On Thu, May 28, 2015 at 04:47:49PM +0100, Chris Wilson wrote:
> On Thu, May 28, 2015 at 06:32:36PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Apparently we can have requests even if though the active list is empty,
> > so do the request retirement regardless of whether there's anything
> > on the active list.
> > 
> > The way it happened here is that during suspend intel_ring_idle()
> > notices the olr hanging around and then proceeds to get rid of it by
> > adding a request. However since there was nothing on the active lists
> > i915_gem_retire_requests() didn't clean those up, and so the idle work
> > never runs, and we leave the GPU "busy" during suspend resulting in a
> > WARN later.
> 
> Whlist I agree (I use list_empty(&ring->request_list);) I strongly
> suspect something (i.e. execlists) isn't managing the active_list
> correctly. Pretty much the only thing that can generate a request
> without an object (and so avoid touching the active_list) is a CS flip,
> and I doubt you are using those...

Oh, I forgot to mention that this was in ringbuffer mode. I guess I
should try execlist too.

> 
> Anyway,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.linux.org
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson May 28, 2015, 4:12 p.m. UTC | #3 
On Thu, May 28, 2015 at 07:03:23PM +0300, Ville Syrjälä wrote:
> On Thu, May 28, 2015 at 04:47:49PM +0100, Chris Wilson wrote:
> > On Thu, May 28, 2015 at 06:32:36PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Apparently we can have requests even if though the active list is empty,
> > > so do the request retirement regardless of whether there's anything
> > > on the active list.
> > > 
> > > The way it happened here is that during suspend intel_ring_idle()
> > > notices the olr hanging around and then proceeds to get rid of it by
> > > adding a request. However since there was nothing on the active lists
> > > i915_gem_retire_requests() didn't clean those up, and so the idle work
> > > never runs, and we leave the GPU "busy" during suspend resulting in a
> > > WARN later.
> > 
> > Whlist I agree (I use list_empty(&ring->request_list);) I strongly
> > suspect something (i.e. execlists) isn't managing the active_list
> > correctly. Pretty much the only thing that can generate a request
> > without an object (and so avoid touching the active_list) is a CS flip,
> > and I doubt you are using those...
> 
> Oh, I forgot to mention that this was in ringbuffer mode. I guess I
> should try execlist too.

Now I am even more curious where the partial request came from. Anyhow,
it is immaterial since olr is just a great big wart overdue for
replacement.
-Chris
Jani Nikula May 29, 2015, 12:22 p.m. UTC | #4 
On Thu, 28 May 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, May 28, 2015 at 06:32:36PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> Apparently we can have requests even if though the active list is empty,
>> so do the request retirement regardless of whether there's anything
>> on the active list.
>> 
>> The way it happened here is that during suspend intel_ring_idle()
>> notices the olr hanging around and then proceeds to get rid of it by
>> adding a request. However since there was nothing on the active lists
>> i915_gem_retire_requests() didn't clean those up, and so the idle work
>> never runs, and we leave the GPU "busy" during suspend resulting in a
>> WARN later.
>
> Whlist I agree (I use list_empty(&ring->request_list);) I strongly
> suspect something (i.e. execlists) isn't managing the active_list
> correctly. Pretty much the only thing that can generate a request
> without an object (and so avoid touching the active_list) is a CS flip,
> and I doubt you are using those...
>
> Anyway,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.linux.org

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.


> -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
Shuang He May 31, 2015, 10:14 a.m. UTC | #5 
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6497
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              315/315              314/315
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                                  320/320              320/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
Jani Nikula June 15, 2015, 11:38 a.m. UTC | #6 
On Fri, 29 May 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 28 May 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Thu, May 28, 2015 at 06:32:36PM +0300, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> 
>>> Apparently we can have requests even if though the active list is empty,
>>> so do the request retirement regardless of whether there's anything
>>> on the active list.
>>> 
>>> The way it happened here is that during suspend intel_ring_idle()
>>> notices the olr hanging around and then proceeds to get rid of it by
>>> adding a request. However since there was nothing on the active lists
>>> i915_gem_retire_requests() didn't clean those up, and so the idle work
>>> never runs, and we leave the GPU "busy" during suspend resulting in a
>>> WARN later.
>>
>> Whlist I agree (I use list_empty(&ring->request_list);) I strongly
>> suspect something (i.e. execlists) isn't managing the active_list
>> correctly. Pretty much the only thing that can generate a request
>> without an object (and so avoid touching the active_list) is a CS flip,
>> and I doubt you are using those...
>>
>> Anyway,
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: stable@vger.linux.org
>
> Pushed to drm-intel-fixes, thanks for the patch and review.

Reverted from drm-intel-fixes because I misapplied it, and applied to
drm-intel-next-fixes instead. Sorry for the noise.

BR,
Jani.



>
> BR,
> Jani.
>
>
>> -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
>
> -- 
> Jani Nikula, Intel Open Source Technology Center
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index be35f04..c35e035 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2813,9 +2813,6 @@  i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 {
 	WARN_ON(i915_verify_lists(ring->dev));
 
-	if (list_empty(&ring->active_list))
-		return;
-
 	/* Retire requests first as we use it above for the early return.
 	 * If we retire requests last, we may use a later seqno and so clear
 	 * the requests lists without clearing the active list, leading to