[PATCH 0/3] drm/i915: More page flip vs. reset improvements

[PATCH 0/3] drm/i915: More page flip vs. reset improvements

[ville.syrjala]

Here are some more flip vs. reset handling fixes.

I think this series fixes all the usual hangs in the flip vs. reset handling.
It even sends out the page flip events to user space.

I can now run a GL game, stop the rings, wait for the reset to kick in, and
then go on playing as if nothing happened.

i-g-t test will follow ASAP.

[PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Someone may be waiting for a flip that will never complete due to a GPU
reset. Wake up all such waiters after the GPU reset processing has
finished.

v2: Dropped the wake_up_all() from i915_handle_error() since
    we no longer wait for pending flips with struct_mutex held.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2cd97d1..672816a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -915,6 +915,8 @@ static void i915_error_work_func(struct work_struct *work)
 		for_each_ring(ring, dev_priv, i)
 			wake_up_all(&ring->irq_queue);
 
+		wake_up_all(&dev_priv->pending_flip_queue);
+
 		wake_up_all(&dev_priv->gpu_error.reset_queue);
 	}
 }
-- 
1.7.12.4

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

[PATCH v2 2/3] drm/i915: Really wait for pending flips when panning

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since obj->pending_flips was never set, intel_pipe_set_base() never
actually waited for pending page flips to complete.

We really do want to wait for the pending flips, because otherwise the
mmio surface base address update could overtake the flip, and you
could end up with an old frame on the screen once the flip really
completes.

Just call intel_crtc_wait_pending_flips() prior to calling
intel_pipe_set_base() instead of calling just intel_finish_fb()
from intel_pipe_set_base(). Moving the call outside of
intel_pipe_set_base() avoids calling it twice from the full
modeset path.

v2: Wait for pending flips w/o holding struct_mutex

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 11e72de..f0c6416 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2301,9 +2301,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return ret;
 	}
 
-	if (crtc->fb)
-		intel_finish_fb(crtc->fb);
-
 	ret = dev_priv->display.update_plane(crtc, fb, x, y);
 	if (ret) {
 		intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);
@@ -8042,6 +8039,8 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 			goto fail;
 		}
 	} else if (config->fb_changed) {
+		intel_crtc_wait_for_pending_flips(set->crtc);
+
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
 	}
-- 
1.7.12.4

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

[PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

GPU reset will drop all flips that are still in the ring. So after the
reset, call update_plane() for all CRTCs to make sure the primary
planes are scanning out from the correct buffer.

The base address update will also generate a FLIP_DONE interrupt, which
will complete any pending flips. That means user space will get its
page flip events and won't get stuck waiting for them.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 672816a..873552d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -917,6 +917,8 @@ static void i915_error_work_func(struct work_struct *work)
 
 		wake_up_all(&dev_priv->pending_flip_queue);
 
+		intel_display_handle_reset(dev);
+
 		wake_up_all(&dev_priv->gpu_error.reset_queue);
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f0c6416..9838da1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2218,6 +2218,32 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	return dev_priv->display.update_plane(crtc, fb, x, y);
 }
 
+void intel_display_handle_reset(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	/*
+	 * Flips in the rings have been nuked by the reset,
+	 * so just update the base address of all primary
+	 * planes to the the last fb to make sure we're
+	 * showing the correct fb after a reset.
+	 *
+	 * As a nice side effect this also generates a FLIP_DONE
+	 * interrupt which will complete pending page flips, and
+	 * thus user space will get its events and not get stuck.
+	 */
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		mutex_lock(&crtc->mutex);
+
+		if (to_intel_crtc(crtc)->active)
+			dev_priv->display.update_plane(crtc, crtc->fb, crtc->x, crtc->y);
+
+		mutex_unlock(&crtc->mutex);
+	}
+}
+
 static int
 intel_finish_fb(struct drm_framebuffer *old_fb)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d282052..0dc14c2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -694,4 +694,6 @@ extern bool
 intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
 extern void intel_ddi_fdi_disable(struct drm_crtc *crtc);
 
+extern void intel_display_handle_reset(struct drm_device *dev);
+
 #endif /* __INTEL_DRV_H__ */
-- 
1.7.12.4

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

Re: [PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[Chris Wilson]

On Fri, Feb 15, 2013 at 05:07:46PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> GPU reset will drop all flips that are still in the ring. So after the
> reset, call update_plane() for all CRTCs to make sure the primary
> planes are scanning out from the correct buffer.
> 
> The base address update will also generate a FLIP_DONE interrupt, which
> will complete any pending flips. That means user space will get its
> page flip events and won't get stuck waiting for them.

Not for all generations. There's no harm in explicitly finishing the
pageflip, and then the FLIP_DONE is just a normal spurious interrupt.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[Ville Syrjälä]

On Fri, Feb 15, 2013 at 03:28:33PM +0000, Chris Wilson wrote:
> On Fri, Feb 15, 2013 at 05:07:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > GPU reset will drop all flips that are still in the ring. So after the
> > reset, call update_plane() for all CRTCs to make sure the primary
> > planes are scanning out from the correct buffer.
> > 
> > The base address update will also generate a FLIP_DONE interrupt, which
> > will complete any pending flips. That means user space will get its
> > page flip events and won't get stuck waiting for them.
> 
> Not for all generations.

Hmm OK those seem to be the ones with the pending flip status bit
(Gen4 and older?).

But can someone explain why on those platforms we also check the
vblank interrupt status bit before handling the page flip interrupt?

Also wtf is i965_irq_handler doing? Based on the code it seems to be
expecting the pending flip interrupt to happen when the CS executes the
instruction, and then it'll finish the page flip from the following
vblank irq. BSpec doesn't agree, and still maintains that the page flip
interrupt will be generated when the flip really completes. Even if the
hardware would work the way the code suggests, the code is racy if the
ISR is delayed a bit and ends up handling the previous vblank interrupt
and the following flip pending interrupt at the same time. We'd really
need to check the live flip pending status from the ISR to make it
work right.

> There's no harm in explicitly finishing the
> pageflip, and then the FLIP_DONE is just a normal spurious interrupt.

All right. I suppose I'll need to make that change.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[Chris Wilson]

On Fri, Feb 15, 2013 at 06:56:03PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 15, 2013 at 03:28:33PM +0000, Chris Wilson wrote:
> > On Fri, Feb 15, 2013 at 05:07:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > GPU reset will drop all flips that are still in the ring. So after the
> > > reset, call update_plane() for all CRTCs to make sure the primary
> > > planes are scanning out from the correct buffer.
> > > 
> > > The base address update will also generate a FLIP_DONE interrupt, which
> > > will complete any pending flips. That means user space will get its
> > > page flip events and won't get stuck waiting for them.
> > 
> > Not for all generations.
> 
> Hmm OK those seem to be the ones with the pending flip status bit
> (Gen4 and older?).

Yes. Also notably the ones unlikely to survive the GPU reset :)

> But can someone explain why on those platforms we also check the
> vblank interrupt status bit before handling the page flip interrupt?

For those generations, we are meant to detect the transition of pending
flip status from 1 -> 0. That transition of course doesn't generate an
interrupt (rather it stops generating one), so the nearest I could come
up with was in anticipating the vblank after we saw the pending flip
status was close enough. In the case of a pending vblank raising an
interrupt just as the pending flip status is asserted, shouldn't MSI
prevent the pending flip from being asserted as we process the IIR for
the vblank. Of course not all of those chipsets even have MSI. I'm not
even sure if we can close that race.

> Also wtf is i965_irq_handler doing? Based on the code it seems to be
> expecting the pending flip interrupt to happen when the CS executes the
> instruction, and then it'll finish the page flip from the following
> vblank irq. BSpec doesn't agree, and still maintains that the page flip
> interrupt will be generated when the flip really completes. Even if the
> hardware would work the way the code suggests, the code is racy if the
> ISR is delayed a bit and ends up handling the previous vblank interrupt
> and the following flip pending interrupt at the same time. We'd really
> need to check the live flip pending status from the ISR to make it
> work right.

Just be sure that you are reading the bspec for the original gen4 that
has not been clobbered by later refinements. If it stands that we can
trust the flip done interrupt for gen4, then that simplifies that
routine greatly. And could even close the mystery pageflipping is broken
on gen4 bugs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling

[Chris Wilson]

On Fri, Feb 15, 2013 at 05:07:44PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Someone may be waiting for a flip that will never complete due to a GPU
> reset. Wake up all such waiters after the GPU reset processing has
> finished.
> 
> v2: Dropped the wake_up_all() from i915_handle_error() since
>     we no longer wait for pending flips with struct_mutex held.

Isn't the wake_up(pending_flip_queue) superseded by performing the
explicit do_intel_finish_page_flip() in patch 3?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling

[Ville Syrjälä]

On Fri, Feb 15, 2013 at 11:53:14PM +0000, Chris Wilson wrote:
> On Fri, Feb 15, 2013 at 05:07:44PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Someone may be waiting for a flip that will never complete due to a GPU
> > reset. Wake up all such waiters after the GPU reset processing has
> > finished.
> > 
> > v2: Dropped the wake_up_all() from i915_handle_error() since
> >     we no longer wait for pending flips with struct_mutex held.
> 
> Isn't the wake_up(pending_flip_queue) superseded by performing the
> explicit do_intel_finish_page_flip() in patch 3?

Yes that's correct. But I actually forgot to remove the wake_up patch
from my tree when I tested this. I'll run a few more tests just to make
sure it still works.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[Ville Syrjälä]

On Fri, Feb 15, 2013 at 11:47:52PM +0000, Chris Wilson wrote:
> On Fri, Feb 15, 2013 at 06:56:03PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 15, 2013 at 03:28:33PM +0000, Chris Wilson wrote:
> > > On Fri, Feb 15, 2013 at 05:07:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > GPU reset will drop all flips that are still in the ring. So after the
> > > > reset, call update_plane() for all CRTCs to make sure the primary
> > > > planes are scanning out from the correct buffer.
> > > > 
> > > > The base address update will also generate a FLIP_DONE interrupt, which
> > > > will complete any pending flips. That means user space will get its
> > > > page flip events and won't get stuck waiting for them.
> > > 
> > > Not for all generations.
> > 
> > Hmm OK those seem to be the ones with the pending flip status bit
> > (Gen4 and older?).
> 
> Yes. Also notably the ones unlikely to survive the GPU reset :)
> 
> > But can someone explain why on those platforms we also check the
> > vblank interrupt status bit before handling the page flip interrupt?
> 
> For those generations, we are meant to detect the transition of pending
> flip status from 1 -> 0. That transition of course doesn't generate an
> interrupt (rather it stops generating one), so the nearest I could come
> up with was in anticipating the vblank after we saw the pending flip
> status was close enough. In the case of a pending vblank raising an
> interrupt just as the pending flip status is asserted, shouldn't MSI
> prevent the pending flip from being asserted as we process the IIR for
> the vblank. Of course not all of those chipsets even have MSI. I'm not
> even sure if we can close that race.

This is what I *think* gen2-gen4 Bspec are telling me:

1. CS executes MI_DISPLAY_FLIP
   -> ISR:flip bit 0 -> 1
2. Flip completes
   -> ISR:flip bit 1 -> 0
   -> IIR:flip bit 0 -> 1
   -> interrupt generated

But unfortuntely I have no hardware to test that.


But if I'm reading the BSpec incorrectly (or if it's inaccurate), then
I think we can close the race with this code:

i965_irq_handler()
{
...
	if (iir & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT)
		intel_prepare_page_flip();

	if (iir & PIPE_START_VBLANK_INTERRUPT_STATUS &&
	    (I915_READ(ISR) & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT) == 0)
		intel_finish_page_flip();
...
}

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[Chris Wilson]

On Mon, Feb 18, 2013 at 12:15:34PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 15, 2013 at 11:47:52PM +0000, Chris Wilson wrote:
> > On Fri, Feb 15, 2013 at 06:56:03PM +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 15, 2013 at 03:28:33PM +0000, Chris Wilson wrote:
> > > > On Fri, Feb 15, 2013 at 05:07:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > GPU reset will drop all flips that are still in the ring. So after the
> > > > > reset, call update_plane() for all CRTCs to make sure the primary
> > > > > planes are scanning out from the correct buffer.
> > > > > 
> > > > > The base address update will also generate a FLIP_DONE interrupt, which
> > > > > will complete any pending flips. That means user space will get its
> > > > > page flip events and won't get stuck waiting for them.
> > > > 
> > > > Not for all generations.
> > > 
> > > Hmm OK those seem to be the ones with the pending flip status bit
> > > (Gen4 and older?).
> > 
> > Yes. Also notably the ones unlikely to survive the GPU reset :)
> > 
> > > But can someone explain why on those platforms we also check the
> > > vblank interrupt status bit before handling the page flip interrupt?
> > 
> > For those generations, we are meant to detect the transition of pending
> > flip status from 1 -> 0. That transition of course doesn't generate an
> > interrupt (rather it stops generating one), so the nearest I could come
> > up with was in anticipating the vblank after we saw the pending flip
> > status was close enough. In the case of a pending vblank raising an
> > interrupt just as the pending flip status is asserted, shouldn't MSI
> > prevent the pending flip from being asserted as we process the IIR for
> > the vblank. Of course not all of those chipsets even have MSI. I'm not
> > even sure if we can close that race.
> 
> This is what I *think* gen2-gen4 Bspec are telling me:
> 
> 1. CS executes MI_DISPLAY_FLIP
>    -> ISR:flip bit 0 -> 1
> 2. Flip completes
>    -> ISR:flip bit 1 -> 0
>    -> IIR:flip bit 0 -> 1
>    -> interrupt generated

Ah, no. The IIR mirrors the ISR and it really does continually generate
the PendingFlip interrupt on gen2/3.

It wasn't until midway through the gen3 cycle, that they introduced the
behaviour you describe (which is what the "pending flip is actually flip
done" bit in ECOSKPD is about). But since we already have to try
and handle gen2/3, I didn't think it was worth a second gen3 code path
with the revised behaviour.

Can you send that as a patch for gen4? As I have a few people hitting
pageflip system hangs, but only on gen4 afaict.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[Ville Syrjälä]

On Mon, Feb 18, 2013 at 10:47:37AM +0000, Chris Wilson wrote:
> On Mon, Feb 18, 2013 at 12:15:34PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 15, 2013 at 11:47:52PM +0000, Chris Wilson wrote:
> > > On Fri, Feb 15, 2013 at 06:56:03PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Feb 15, 2013 at 03:28:33PM +0000, Chris Wilson wrote:
> > > > > On Fri, Feb 15, 2013 at 05:07:46PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > GPU reset will drop all flips that are still in the ring. So after the
> > > > > > reset, call update_plane() for all CRTCs to make sure the primary
> > > > > > planes are scanning out from the correct buffer.
> > > > > > 
> > > > > > The base address update will also generate a FLIP_DONE interrupt, which
> > > > > > will complete any pending flips. That means user space will get its
> > > > > > page flip events and won't get stuck waiting for them.
> > > > > 
> > > > > Not for all generations.
> > > > 
> > > > Hmm OK those seem to be the ones with the pending flip status bit
> > > > (Gen4 and older?).
> > > 
> > > Yes. Also notably the ones unlikely to survive the GPU reset :)
> > > 
> > > > But can someone explain why on those platforms we also check the
> > > > vblank interrupt status bit before handling the page flip interrupt?
> > > 
> > > For those generations, we are meant to detect the transition of pending
> > > flip status from 1 -> 0. That transition of course doesn't generate an
> > > interrupt (rather it stops generating one), so the nearest I could come
> > > up with was in anticipating the vblank after we saw the pending flip
> > > status was close enough. In the case of a pending vblank raising an
> > > interrupt just as the pending flip status is asserted, shouldn't MSI
> > > prevent the pending flip from being asserted as we process the IIR for
> > > the vblank. Of course not all of those chipsets even have MSI. I'm not
> > > even sure if we can close that race.
> > 
> > This is what I *think* gen2-gen4 Bspec are telling me:
> > 
> > 1. CS executes MI_DISPLAY_FLIP
> >    -> ISR:flip bit 0 -> 1
> > 2. Flip completes
> >    -> ISR:flip bit 1 -> 0
> >    -> IIR:flip bit 0 -> 1
> >    -> interrupt generated
> 
> Ah, no. The IIR mirrors the ISR and it really does continually generate
> the PendingFlip interrupt on gen2/3.

Continually? Oh that's nasty, but makes sense if it's just a level
interrupt.

> It wasn't until midway through the gen3 cycle, that they introduced the
> behaviour you describe (which is what the "pending flip is actually flip
> done" bit in ECOSKPD is about). But since we already have to try
> and handle gen2/3, I didn't think it was worth a second gen3 code path
> with the revised behaviour.

Ok. I see we never actually enable the pending flip interrupt on gen2/3
due to this. The same race exists there as well then. I can try to make
a patch for that too.

And we turn off the bit in ECOSKPD on gen3 to make sure we always behave
the same way. Makes sense.

> Can you send that as a patch for gen4? As I have a few people hitting
> pageflip system hangs, but only on gen4 afaict.

I must admit that I'm confused now. Which model does Gen4 actually
follow, pending flip or flip done?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Update primary planes after a GPU reset

[Chris Wilson]

On Mon, Feb 18, 2013 at 01:50:26PM +0200, Ville Syrjälä wrote:
> > Can you send that as a patch for gen4? As I have a few people hitting
> > pageflip system hangs, but only on gen4 afaict.
> 
> I must admit that I'm confused now. Which model does Gen4 actually
> follow, pending flip or flip done?

The original bspec for gen4 doesn't mention FlipDone and the description
of PendingFlip reads very similar to the earlier gen2/gen3
implementation.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling

[Ville Syrjälä]

On Mon, Feb 18, 2013 at 11:58:23AM +0200, Ville Syrjälä wrote:
> On Fri, Feb 15, 2013 at 11:53:14PM +0000, Chris Wilson wrote:
> > On Fri, Feb 15, 2013 at 05:07:44PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Someone may be waiting for a flip that will never complete due to a GPU
> > > reset. Wake up all such waiters after the GPU reset processing has
> > > finished.
> > > 
> > > v2: Dropped the wake_up_all() from i915_handle_error() since
> > >     we no longer wait for pending flips with struct_mutex held.
> > 
> > Isn't the wake_up(pending_flip_queue) superseded by performing the
> > explicit do_intel_finish_page_flip() in patch 3?
> 
> Yes that's correct. But I actually forgot to remove the wake_up patch
> from my tree when I tested this. I'll run a few more tests just to make
> sure it still works.

I just tried it w/o the wake_up_all() and unfortunately it hung :(

Need to think about it a bit more I suppose.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling

[Ville Syrjälä]

On Mon, Feb 18, 2013 at 02:41:00PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 18, 2013 at 11:58:23AM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 15, 2013 at 11:53:14PM +0000, Chris Wilson wrote:
> > > On Fri, Feb 15, 2013 at 05:07:44PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Someone may be waiting for a flip that will never complete due to a GPU
> > > > reset. Wake up all such waiters after the GPU reset processing has
> > > > finished.
> > > > 
> > > > v2: Dropped the wake_up_all() from i915_handle_error() since
> > > >     we no longer wait for pending flips with struct_mutex held.
> > > 
> > > Isn't the wake_up(pending_flip_queue) superseded by performing the
> > > explicit do_intel_finish_page_flip() in patch 3?
> > 
> > Yes that's correct. But I actually forgot to remove the wake_up patch
> > from my tree when I tested this. I'll run a few more tests just to make
> > sure it still works.
> 
> I just tried it w/o the wake_up_all() and unfortunately it hung :(
> 
> Need to think about it a bit more I suppose.

Well after a bit more though I think I know what happened:

1. page flip submitted on crtc which is not the first crtc in the list
2. mode set operation on any crtc (will grab all crtc locks)
3. reset handling loops over crtcs
 3.1 first crtc doesn't have a pending flip, so pending_flip_queue is not woken up
 3.2 code tries to grab first crtc mutex to update the base address
  -> deadlock

So either I need to keep the wake_up_all() call in the reset handling,
or I need to do two loops over the crtcs, first loop would complete all
page flips, and second loop would update the base address registers.
I don't really care which way we go. Anyone else have a preference?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling

[Daniel Vetter]

On Mon, Feb 18, 2013 at 05:31:14PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 18, 2013 at 02:41:00PM +0200, Ville Syrjälä wrote:
> > On Mon, Feb 18, 2013 at 11:58:23AM +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 15, 2013 at 11:53:14PM +0000, Chris Wilson wrote:
> > > > On Fri, Feb 15, 2013 at 05:07:44PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Someone may be waiting for a flip that will never complete due to a GPU
> > > > > reset. Wake up all such waiters after the GPU reset processing has
> > > > > finished.
> > > > > 
> > > > > v2: Dropped the wake_up_all() from i915_handle_error() since
> > > > >     we no longer wait for pending flips with struct_mutex held.
> > > > 
> > > > Isn't the wake_up(pending_flip_queue) superseded by performing the
> > > > explicit do_intel_finish_page_flip() in patch 3?
> > > 
> > > Yes that's correct. But I actually forgot to remove the wake_up patch
> > > from my tree when I tested this. I'll run a few more tests just to make
> > > sure it still works.
> > 
> > I just tried it w/o the wake_up_all() and unfortunately it hung :(
> > 
> > Need to think about it a bit more I suppose.
> 
> Well after a bit more though I think I know what happened:
> 
> 1. page flip submitted on crtc which is not the first crtc in the list
> 2. mode set operation on any crtc (will grab all crtc locks)
> 3. reset handling loops over crtcs
>  3.1 first crtc doesn't have a pending flip, so pending_flip_queue is not woken up
>  3.2 code tries to grab first crtc mutex to update the base address
>   -> deadlock
> 
> So either I need to keep the wake_up_all() call in the reset handling,
> or I need to do two loops over the crtcs, first loop would complete all
> page flips, and second loop would update the base address registers.
> I don't really care which way we go. Anyone else have a preference?

I'd vote for the second approach of first waking everyone up, and then
doing the modeset updates. I guess you could even use
drm_modeset_lock_all, not really worth optimizing things here imo. Also
please add a comment to explain this ordering constraint in the reset
code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch