diff mbox

[v2] drm/i915: Wait for pending flips in intel_pipe_set_base()

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

Commit Message

Ville Syrjala Nov. 5, 2012, 1:20 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

intel_pipe_set_base() never actually waited for any pending page flips
on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
the current front buffer. But the pending flips were actually tracked
in the BO of the previous front buffer, so the call to intel_finish_fb()
never did anything useful.

intel_crtc_wait_for_pending_flips() is the current _working_ way to wait
for pending page flips. So use it in intel_pipe_set_base() too. Some
refactoring was necessary to avoid locking struct_mutex twice.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips()
    just wraps intel_crtc_wait_for_pending_flips_locked().

 drivers/gpu/drm/i915/intel_display.c |   76 ++++++++++++++++++---------------
 1 files changed, 41 insertions(+), 35 deletions(-)

Comments

Chris Wilson Nov. 5, 2012, 3:33 p.m. UTC | #1 
On Mon,  5 Nov 2012 15:20:53 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_pipe_set_base() never actually waited for any pending page flips
> on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> the current front buffer. But the pending flips were actually tracked
> in the BO of the previous front buffer, so the call to intel_finish_fb()
> never did anything useful.
> 
> intel_crtc_wait_for_pending_flips() is the current _working_ way to wait
> for pending page flips. So use it in intel_pipe_set_base() too. Some
> refactoring was necessary to avoid locking struct_mutex twice.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Daniel Vetter Nov. 26, 2012, 9:21 p.m. UTC | #2 
On Mon, Nov 05, 2012 at 03:20:53PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_pipe_set_base() never actually waited for any pending page flips
> on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> the current front buffer. But the pending flips were actually tracked
> in the BO of the previous front buffer, so the call to intel_finish_fb()
> never did anything useful.
> 
> intel_crtc_wait_for_pending_flips() is the current _working_ way to wait
> for pending page flips. So use it in intel_pipe_set_base() too. Some
> refactoring was necessary to avoid locking struct_mutex twice.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips()
>     just wraps intel_crtc_wait_for_pending_flips_locked().

Sorry for the long delay in looking at this. One bikeshed here: I prefer
the patch changelog before the sob lines so that it gets included in the
commit message - most often it's rather interesting read, especially for
patches that take a few revisions to get right. More substantial comment
below.

>  drivers/gpu/drm/i915/intel_display.c |   76 ++++++++++++++++++---------------
>  1 files changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1a38267..a18e6e6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2228,6 +2228,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
>  	}
>  }
>  
> +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned long flags;
> +	bool pending;
> +
> +	if (atomic_read(&dev_priv->mm.wedged))
> +		return false;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	return pending;
> +}
> +
> +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (crtc->fb == NULL)
> +		return;
> +
> +	wait_event(dev_priv->pending_flip_queue,
> +		   !intel_crtc_has_pending_flip(crtc));

I think we also need to add a dev_priv->mm.wedged check here, since the
gpu might die and never execute the pageflip. Otoh we don't complete any
pageflips that never executed due to a gpu hang, so maybe also add a big
FIXME. But with the wedged check we should at least not hang in an
non-interruptible wait.

The other thing is that the wait_even in finish_fb is not superflous,
since we should never see a framebuffer with pending flips for _this_ crtc
(it could have a pending flip on another crtc). So I think that code in
finish_fb should die, leaving just the comment and the finish_gpu call.

Cheers, Daniel

PS: Testcase would be awesome, but I have no ideas beyond what we already
have in flip_test unfortunately ...
Daniel Vetter Nov. 26, 2012, 9:52 p.m. UTC | #3 
On Mon, Nov 26, 2012 at 10:21:28PM +0100, Daniel Vetter wrote:
> On Mon, Nov 05, 2012 at 03:20:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > intel_pipe_set_base() never actually waited for any pending page flips
> > on the CRTC. It looks like it tried to, by calling intel_finish_fb() on
> > the current front buffer. But the pending flips were actually tracked
> > in the BO of the previous front buffer, so the call to intel_finish_fb()
> > never did anything useful.
> > 
> > intel_crtc_wait_for_pending_flips() is the current _working_ way to wait
> > for pending page flips. So use it in intel_pipe_set_base() too. Some
> > refactoring was necessary to avoid locking struct_mutex twice.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > v2: Shuffle the code around so that intel_crtc_wait_for_pending_flips()
> >     just wraps intel_crtc_wait_for_pending_flips_locked().
> 
> Sorry for the long delay in looking at this. One bikeshed here: I prefer
> the patch changelog before the sob lines so that it gets included in the
> commit message - most often it's rather interesting read, especially for
> patches that take a few revisions to get right. More substantial comment
> below.
> 
> >  drivers/gpu/drm/i915/intel_display.c |   76 ++++++++++++++++++---------------
> >  1 files changed, 41 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1a38267..a18e6e6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2228,6 +2228,46 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
> >  	}
> >  }
> >  
> > +static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	unsigned long flags;
> > +	bool pending;
> > +
> > +	if (atomic_read(&dev_priv->mm.wedged))
> > +		return false;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	pending = to_intel_crtc(crtc)->unpin_work != NULL;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	return pending;
> > +}
> > +
> > +static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	if (crtc->fb == NULL)
> > +		return;
> > +
> > +	wait_event(dev_priv->pending_flip_queue,
> > +		   !intel_crtc_has_pending_flip(crtc));
> 
> I think we also need to add a dev_priv->mm.wedged check here, since the
> gpu might die and never execute the pageflip. Otoh we don't complete any
> pageflips that never executed due to a gpu hang, so maybe also add a big
> FIXME. But with the wedged check we should at least not hang in an
> non-interruptible wait.
> 
> The other thing is that the wait_even in finish_fb is not superflous,
> since we should never see a framebuffer with pending flips for _this_ crtc
> (it could have a pending flip on another crtc). So I think that code in
> finish_fb should die, leaving just the comment and the finish_gpu call.
> 
> Cheers, Daniel
> 
> PS: Testcase would be awesome, but I have no ideas beyond what we already
> have in flip_test unfortunately ...

Ok, the patch is actually correct, I just couldn't read C code for a
moment. Still I think a v3 with the superflous wait_event in finish_fb
ditched would look nice.

Now Chris reminded me that you have a few other patches around this in the
"drm/i915: i915_gem_execbuffer_wait_for_flips and other flip stuff"
thread. And I have a few reset state transition improvements in the
pipeline for 3.9 at

http://cgit.freedesktop.org/~danvet/drm/log/?h=robustify-reset-transitions

Can you please rebase that entire patch series on top of those patches?
For patch 3 I think we should go with option b) and outright kill the
wait-for-flips in the execbuf ioctl. Maybe harrass Jesse and Kristian for
a formal ack on that one.

Also, if you can throw an additional patch on top to properly clear any
outstanding flips when the gpu hangs, that would be awesome - since
currently pageflips will get busted if the gpu hangs. And if you could
throw a testcase in for fun ...

Cheers, Daniel

PS: The testcase should be easy to pull of by adapting ZZ_hangman with a
different workload. Like flip_test ...
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1a38267..a18e6e6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2228,6 +2228,46 @@  static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
 	}
 }
 
+static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long flags;
+	bool pending;
+
+	if (atomic_read(&dev_priv->mm.wedged))
+		return false;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	pending = to_intel_crtc(crtc)->unpin_work != NULL;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	return pending;
+}
+
+static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (crtc->fb == NULL)
+		return;
+
+	wait_event(dev_priv->pending_flip_queue,
+		   !intel_crtc_has_pending_flip(crtc));
+
+	intel_finish_fb(crtc->fb);
+}
+
+static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+
+	mutex_lock(&dev->struct_mutex);
+	intel_crtc_wait_for_pending_flips_locked(crtc);
+	mutex_unlock(&dev->struct_mutex);
+}
+
 static int
 intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		    struct drm_framebuffer *fb)
@@ -2261,8 +2301,7 @@  intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return ret;
 	}
 
-	if (crtc->fb)
-		intel_finish_fb(crtc->fb);
+	intel_crtc_wait_for_pending_flips_locked(crtc);
 
 	ret = dev_priv->display.update_plane(crtc, fb, x, y);
 	if (ret) {
@@ -2901,39 +2940,6 @@  static void ironlake_fdi_disable(struct drm_crtc *crtc)
 	udelay(100);
 }
 
-static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long flags;
-	bool pending;
-
-	if (atomic_read(&dev_priv->mm.wedged))
-		return false;
-
-	spin_lock_irqsave(&dev->event_lock, flags);
-	pending = to_intel_crtc(crtc)->unpin_work != NULL;
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
-	return pending;
-}
-
-static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (crtc->fb == NULL)
-		return;
-
-	wait_event(dev_priv->pending_flip_queue,
-		   !intel_crtc_has_pending_flip(crtc));
-
-	mutex_lock(&dev->struct_mutex);
-	intel_finish_fb(crtc->fb);
-	mutex_unlock(&dev->struct_mutex);
-}
-
 static bool ironlake_crtc_driving_pch(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;