Re: [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

Re: [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

[Jonathan Nieder]

Hi Timo,

Timo Aaltonen wrote:

> commit d698c826bb93663e4b349a78fbd30443654d37cd upstream.
>
> Please pull this for v3.2.x, should be the last missing (kernel) piece
> for https://bugs.freedesktop.org/show_bug.cgi?id=48838
[...]
>              It should be more reliable to wait for the pending
> operations to complete first, even though we still have a window for
> userspace to submit a broken command buffer during the modeset.

That sounds hazy --- is it tested?

Presumably this is needed for 3.4.y and 3.5.y as well, right?  Do you
know when the bad behavior was introduced?  Does 3.0.y need the fix as
well?

Thanks and hope that helps,
Jonathan

Re: [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

[Timo Aaltonen]

On 12.09.2012 18:09, Jonathan Nieder wrote:
> Hi Timo,
> 
> Timo Aaltonen wrote:
> 
>> commit d698c826bb93663e4b349a78fbd30443654d37cd upstream.

oops, script-fail.. that's the local branch sha, real one is further
down (0f91128d88bbb8b0a8e7bb93df2c40680871d45a).

>> Please pull this for v3.2.x, should be the last missing (kernel) piece
>> for https://bugs.freedesktop.org/show_bug.cgi?id=48838
> [...]
>>              It should be more reliable to wait for the pending
>> operations to complete first, even though we still have a window for
>> userspace to submit a broken command buffer during the modeset.
> 
> That sounds hazy --- is it tested?

Well, these are usually hard to verify fixed. The commit is mentioned on
fdo bugs 45413 and 48838, should fix some GPU hangs.

> Presumably this is needed for 3.4.y and 3.5.y as well, right?  Do you
> know when the bad behavior was introduced?  Does 3.0.y need the fix as
> well?

It's from 3.5-rc1, don't know when it was introduced. Maybe it would
make sense for 3.4 at least.


-- 
t

Re: [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

[Jonathan Nieder]

Timo Aaltonen wrote:

> Well, these are usually hard to verify fixed. The commit is mentioned on
> fdo bugs 45413 and 48838, should fix some GPU hangs.

The stable kernel rules are very clear about this:

 - It must be obviously correct and tested.

Please ensure the backport gets tested on a machine that was
affected by the problem.  That doesn't mean we need 100%
confidence that the fix worked, but if I understand you
correctly it is possible that nobody tested the backport on
real hardware at all.

An ack from someone on the i915 team would be welcome as well.

Thanks and hope that helps,
Jonathan

Re: [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

[Daniel Vetter]

On Wed, Sep 12, 2012 at 09:03:03AM -0700, Jonathan Nieder wrote:
> Timo Aaltonen wrote:
> 
> > Well, these are usually hard to verify fixed. The commit is mentioned on
> > fdo bugs 45413 and 48838, should fix some GPU hangs.
> 
> The stable kernel rules are very clear about this:
> 
>  - It must be obviously correct and tested.
> 
> Please ensure the backport gets tested on a machine that was
> affected by the problem.  That doesn't mean we need 100%
> confidence that the fix worked, but if I understand you
> correctly it is possible that nobody tested the backport on
> real hardware at all.
> 
> An ack from someone on the i915 team would be welcome as well.

It's the right thing. The comment about "hough we still have a window for
userspace to submit a broken command buffer during the modeset" just
re-stresses that client can still kill the gpu. But with this fix here
userspace has at least a chance to get it right, whereas before it just
fell over sometimes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

[Jonathan Nieder]

Daniel Vetter wrote:
> On Wed, Sep 12, 2012 at 09:03:03AM -0700, Jonathan Nieder wrote:

>> An ack from someone on the i915 team would be welcome as well.
>
> It's the right thing. The comment about "hough we still have a window for
> userspace to submit a broken command buffer during the modeset" just
> re-stresses that client can still kill the gpu. But with this fix here
> userspace has at least a chance to get it right, whereas before it just
> fell over sometimes.

Thanks for the quick and clear feedback.  Makes sense.

Jonathan

[PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

[Chris Wilson]

During modeset we have to disable the pipe to reconfigure its timings
and maybe its size. Userspace may have queued up command buffers that
depend upon the pipe running in a certain configuration and so the
commands may become confused across the modeset. At the moment, we use a
less than satisfactory kick-scanline-waits should the GPU hang during
the modeset. It should be more reliable to wait for the pending
operations to complete first, even though we still have a window for
userspace to submit a broken command buffer during the modeset.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8be3091..72980be 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2910,16 +2910,14 @@ static void intel_clear_scanline_wait(struct drm_device *dev)
 
 static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
-	struct drm_i915_gem_object *obj;
-	struct drm_i915_private *dev_priv;
+	struct drm_device *dev = crtc->dev;
 
 	if (crtc->fb == NULL)
 		return;
 
-	obj = to_intel_framebuffer(crtc->fb)->obj;
-	dev_priv = crtc->dev->dev_private;
-	wait_event(dev_priv->pending_flip_queue,
-		   atomic_read(&obj->pending_flip) == 0);
+	mutex_lock(&dev->struct_mutex);
+	intel_finish_fb(crtc->fb);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static bool intel_crtc_driving_pch(struct drm_crtc *crtc)
@@ -3381,23 +3379,6 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 	struct drm_device *dev = crtc->dev;
 
-	/* Flush any pending WAITs before we disable the pipe. Note that
-	 * we need to drop the struct_mutex in order to acquire it again
-	 * during the lowlevel dpms routines around a couple of the
-	 * operations. It does not look trivial nor desirable to move
-	 * that locking higher. So instead we leave a window for the
-	 * submission of further commands on the fb before we can actually
-	 * disable it. This race with userspace exists anyway, and we can
-	 * only rely on the pipe being disabled by userspace after it
-	 * receives the hotplug notification and has flushed any pending
-	 * batches.
-	 */
-	if (crtc->fb) {
-		mutex_lock(&dev->struct_mutex);
-		intel_finish_fb(crtc->fb);
-		mutex_unlock(&dev->struct_mutex);
-	}
-
 	crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
 	assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane);
 	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
-- 
1.7.10

Re: [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe

[Daniel Vetter]

On Tue, Apr 17, 2012 at 10:05:38AM +0100, Chris Wilson wrote:
> During modeset we have to disable the pipe to reconfigure its timings
> and maybe its size. Userspace may have queued up command buffers that
> depend upon the pipe running in a certain configuration and so the
> commands may become confused across the modeset. At the moment, we use a
> less than satisfactory kick-scanline-waits should the GPU hang during
> the modeset. It should be more reliable to wait for the pending
> operations to complete first, even though we still have a window for
> userspace to submit a broken command buffer during the modeset.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48