[PATCH 1/2] drm/i915: time out of load detect busy-waits

[PATCH 1/2] drm/i915: time out of load detect busy-waits

[Daniel Vetter]

If we try to do that and the scanlines just wouldn't advance, we
busy-hang the machine holding the modeset mutex. Not great for
debugging.

References: https://bugzilla.kernel.org/show_bug.cgi?id=43020
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_crt.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 0976137..d30779e 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -383,6 +383,7 @@ intel_crt_load_detect(struct intel_crt *crt)
 	} else {
 		bool restore_vblank = false;
 		int count, detect;
+		unsigned long timeout;
 
 		/*
 		* If there isn't any border, add some.
@@ -407,22 +408,32 @@ intel_crt_load_detect(struct intel_crt *crt)
 		/*
 		 * Wait for the border to be displayed
 		 */
-		while (I915_READ(pipe_dsl_reg) >= vactive)
-			;
-		while ((dsl = I915_READ(pipe_dsl_reg)) <= vsample)
-			;
+		if (wait_for(I915_READ(pipe_dsl_reg) >= vactive, 1000))
+			DRM_ERROR("timed out waiting for vactive in "
+				  "load_detect, scanline: %u\n",
+				  I915_READ(pipe_dsl_reg));
+		if (wait_for((dsl = I915_READ(pipe_dsl_reg)) <= vsample, 1000))
+			DRM_ERROR("timed out waiting for vsample in "
+				  "load_detect, scanline: %u\n",
+				  I915_READ(pipe_dsl_reg));
 		/*
 		 * Watch ST00 for an entire scanline
 		 */
 		detect = 0;
 		count = 0;
+		timeout = jiffies + msecs_to_jiffies(1000);
 		do {
 			count++;
 			/* Read the ST00 VGA status register */
 			st00 = I915_READ8(VGA_MSR_WRITE);
 			if (st00 & (1 << 4))
 				detect++;
-		} while ((I915_READ(pipe_dsl_reg) == dsl));
+		} while ((I915_READ(pipe_dsl_reg) == dsl) &&
+			 !time_after(jiffies, timeout));
+
+		if (time_after(jiffies, timeout))
+			DRM_ERROR("timed out while load-detecting, scanline: %u\n",
+				  I915_READ(pipe_dsl_reg));
 
 		/* restore vblank if necessary */
 		if (restore_vblank)
-- 
1.7.10

[PATCH 2/2] drm/i915: fixup load-detect on enabled, but not active pipe

[Daniel Vetter]

Somehow we have a fast-path that tries to avoid going through
the load-detect code when the encode already has a crtc associated.
But this fails horribly when the crtc is off. The load detect pipe
itself manages this case well (and also does not forget to restore the
dpms state), so just rip out this special case.

The issue seems to go back all the way to the commit that originally
introduced load-detection on the vga output:

commit e4a5d54f924ea5ce2913d9d0687d034004816465
Author: Ma Ling <ling.ma@intel.com>
Date:   Tue May 26 11:31:00 2009 +0800

    drm/i915: Add support for VGA load detection (pre-945).

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43020
Reported-by: Jean Delvare <khali@linux-fr.org>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_crt.c |   29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index d30779e..a46889b 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -460,8 +460,8 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 {
 	struct drm_device *dev = connector->dev;
 	struct intel_crt *crt = intel_attached_crt(connector);
-	struct drm_crtc *crtc;
 	enum drm_connector_status status;
+	struct intel_load_detect_pipe tmp;
 
 	if (I915_HAS_HOTPLUG(dev)) {
 		if (intel_crt_detect_hotplug(connector)) {
@@ -480,23 +480,16 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 		return connector->status;
 
 	/* for pre-945g platforms use load detect */
-	crtc = crt->base.base.crtc;
-	if (crtc && crtc->enabled) {
-		status = intel_crt_load_detect(crt);
-	} else {
-		struct intel_load_detect_pipe tmp;
-
-		if (intel_get_load_detect_pipe(&crt->base, connector, NULL,
-					       &tmp)) {
-			if (intel_crt_detect_ddc(connector))
-				status = connector_status_connected;
-			else
-				status = intel_crt_load_detect(crt);
-			intel_release_load_detect_pipe(&crt->base, connector,
-						       &tmp);
-		} else
-			status = connector_status_unknown;
-	}
+	if (intel_get_load_detect_pipe(&crt->base, connector, NULL,
+				       &tmp)) {
+		if (intel_crt_detect_ddc(connector))
+			status = connector_status_connected;
+		else
+			status = intel_crt_load_detect(crt);
+		intel_release_load_detect_pipe(&crt->base, connector,
+					       &tmp);
+	} else
+		status = connector_status_unknown;
 
 	return status;
 }
-- 
1.7.10

Re: [PATCH 2/2] drm/i915: fixup load-detect on enabled, but not active pipe

[Chris Wilson]

On Fri, 20 Apr 2012 21:03:36 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Somehow we have a fast-path that tries to avoid going through
> the load-detect code when the encode already has a crtc associated.
> But this fails horribly when the crtc is off. The load detect pipe
> itself manages this case well (and also does not forget to restore the
> dpms state), so just rip out this special case.
> 
> The issue seems to go back all the way to the commit that originally
> introduced load-detection on the vga output:
> 
> commit e4a5d54f924ea5ce2913d9d0687d034004816465
> Author: Ma Ling <ling.ma@intel.com>
> Date:   Tue May 26 11:31:00 2009 +0800
> 
>     drm/i915: Add support for VGA load detection (pre-945).
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43020
> Reported-by: Jean Delvare <khali@linux-fr.org>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'm happy that intel_get_load_detect_pipe() does the right thing if the
connector is already active (so that we don't blank an active display
when checking for disconnections). I was worried that this meant we had
a deeper bug with our connection state, but as Daniel made me realise it
is that the "fast path" is lacking the enable.

For this and subsuquent TV fix, begone foul code,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: fixup load-detect on enabled, but not active pipe

[Daniel Vetter]

On Fri, Apr 20, 2012 at 08:44:59PM +0100, Chris Wilson wrote:
> On Fri, 20 Apr 2012 21:03:36 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Somehow we have a fast-path that tries to avoid going through
> > the load-detect code when the encode already has a crtc associated.
> > But this fails horribly when the crtc is off. The load detect pipe
> > itself manages this case well (and also does not forget to restore the
> > dpms state), so just rip out this special case.
> > 
> > The issue seems to go back all the way to the commit that originally
> > introduced load-detection on the vga output:
> > 
> > commit e4a5d54f924ea5ce2913d9d0687d034004816465
> > Author: Ma Ling <ling.ma@intel.com>
> > Date:   Tue May 26 11:31:00 2009 +0800
> > 
> >     drm/i915: Add support for VGA load detection (pre-945).
> > 
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43020
> > Reported-by: Jean Delvare <khali@linux-fr.org>
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I'm happy that intel_get_load_detect_pipe() does the right thing if the
> connector is already active (so that we don't blank an active display
> when checking for disconnections). I was worried that this meant we had
> a deeper bug with our connection state, but as Daniel made me realise it
> is that the "fast path" is lacking the enable.
> 
> For this and subsuquent TV fix, begone foul code,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I've picked up this one for -fixes ant the other two for -next, thanks for
the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/fixup: fixup tv load-detect on enabled but not active crtc

[Daniel Vetter]

When fixing up the crt load detect code I've failed to notice the same
problem in the tv load detect code. Again, unconditionally use the
load detect pipe infrastructure, it gets things right.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_tv.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 67f444d..2e626b8 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1251,9 +1251,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 	mode = reported_modes[0];
 	drm_mode_set_crtcinfo(&mode, 0);
 
-	if (intel_tv->base.base.crtc && intel_tv->base.base.crtc->enabled) {
-		type = intel_tv_detect_type(intel_tv, connector);
-	} else if (force) {
+	if (force) {
 		struct intel_load_detect_pipe tmp;
 
 		if (intel_get_load_detect_pipe(&intel_tv->base, connector,
-- 
1.7.10

Re: [PATCH 1/2] drm/i915: time out of load detect busy-waits

[Chris Wilson]

On Fri, 20 Apr 2012 21:03:35 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> If we try to do that and the scanlines just wouldn't advance, we
> busy-hang the machine holding the modeset mutex. Not great for
> debugging.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=43020
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

A second is a bit much, 5 vblanks should do, say 100ms, but this is just
paranoid code. Unfortunately if it hits it means the machine is likely
to keep hitting it. Are little, but noticeable periodic stutters
actually better than a freeze?
Otherwise,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: time out of load detect busy-waits

[Chris Wilson]

On Fri, 20 Apr 2012 21:03:35 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> If we try to do that and the scanlines just wouldn't advance, we
> busy-hang the machine holding the modeset mutex. Not great for
> debugging.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=43020
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewer hangs head in shame:

> +		if (wait_for(I915_READ(pipe_dsl_reg) >= vactive, 1000))
> +			DRM_ERROR("timed out waiting for vactive in "
> +				  "load_detect, scanline: %u\n",
> +				  I915_READ(pipe_dsl_reg));
> +		if (wait_for((dsl = I915_READ(pipe_dsl_reg)) <= vsample, 1000))
> +			DRM_ERROR("timed out waiting for vsample in "
> +				  "load_detect, scanline: %u\n",
> +				  I915_READ(pipe_dsl_reg));

wait_for() catches us out everytime we convert and existing while(),
because the predicate is when it stops. Perhaps if we had a wait_until,
but anyway the fix here is:

if (wait_for(I915_READ(pipe_dsl_reg) < vactive, 1000))
  ...
if (wait_for((dsl = I915_READ(pipe_dsl_reg)) > vsample, 1000))
  ...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: time out of load detect busy-waits

[Daniel Vetter]

On Wed, Apr 25, 2012 at 06:14:37PM +0100, Chris Wilson wrote:
> On Fri, 20 Apr 2012 21:03:35 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > If we try to do that and the scanlines just wouldn't advance, we
> > busy-hang the machine holding the modeset mutex. Not great for
> > debugging.
> > 
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=43020
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewer hangs head in shame:
> 
> > +		if (wait_for(I915_READ(pipe_dsl_reg) >= vactive, 1000))
> > +			DRM_ERROR("timed out waiting for vactive in "
> > +				  "load_detect, scanline: %u\n",
> > +				  I915_READ(pipe_dsl_reg));
> > +		if (wait_for((dsl = I915_READ(pipe_dsl_reg)) <= vsample, 1000))
> > +			DRM_ERROR("timed out waiting for vsample in "
> > +				  "load_detect, scanline: %u\n",
> > +				  I915_READ(pipe_dsl_reg));
> 
> wait_for() catches us out everytime we convert and existing while(),
> because the predicate is when it stops. Perhaps if we had a wait_until,
> but anyway the fix here is:
> 
> if (wait_for(I915_READ(pipe_dsl_reg) < vactive, 1000))
>   ...
> if (wait_for((dsl = I915_READ(pipe_dsl_reg)) > vsample, 1000))
>   ...
dinq rectified, it never happened. Thanks for catching this.

/me hides in the shame corner
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/2] drm/i915: time out of load detect busy-waits

[Chris Wilson]

On Wed, 25 Apr 2012 21:26:50 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Apr 25, 2012 at 06:14:37PM +0100, Chris Wilson wrote:
> > On Fri, 20 Apr 2012 21:03:35 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > If we try to do that and the scanlines just wouldn't advance, we
> > > busy-hang the machine holding the modeset mutex. Not great for
> > > debugging.
> > > 
> > > References: https://bugzilla.kernel.org/show_bug.cgi?id=43020
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Reviewer hangs head in shame:
> > 
> > > +		if (wait_for(I915_READ(pipe_dsl_reg) >= vactive, 1000))
> > > +			DRM_ERROR("timed out waiting for vactive in "
> > > +				  "load_detect, scanline: %u\n",
> > > +				  I915_READ(pipe_dsl_reg));
> > > +		if (wait_for((dsl = I915_READ(pipe_dsl_reg)) <= vsample, 1000))
> > > +			DRM_ERROR("timed out waiting for vsample in "
> > > +				  "load_detect, scanline: %u\n",
> > > +				  I915_READ(pipe_dsl_reg));
> > 
> > wait_for() catches us out everytime we convert and existing while(),
> > because the predicate is when it stops. Perhaps if we had a wait_until,
> > but anyway the fix here is:
> > 
> > if (wait_for(I915_READ(pipe_dsl_reg) < vactive, 1000))
> >   ...
> > if (wait_for((dsl = I915_READ(pipe_dsl_reg)) > vsample, 1000))
> >   ...
> dinq rectified, it never happened. Thanks for catching this.

wait_for() has even more subtleties in store for us, the unwary coder.
By default, it uses a 1ms sleep between polling the register, chosen to
be kind whilst waiting for panel bits to power up which do take a fair
amount of time. Here, that extra delay causes us to sample the vsync
rather than the border. The quirk of the [vh]sync is that the monitor bit
of ST00 is always true. And since we always seem to pick that row to read
we always think there is a CRT present.

The choice is either to use the busy-polling variant, wait_for_atomic,
or restructure the entire block to use a single timeout with direct
reads. And whilst you are modifying the code, convert the polling reads
to I915_READ_NOTRACE().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: time out of load detect busy-waits

[Daniel Vetter]

On Sat, Apr 28, 2012 at 10:00:29PM +0100, Chris Wilson wrote:
> On Wed, 25 Apr 2012 21:26:50 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Apr 25, 2012 at 06:14:37PM +0100, Chris Wilson wrote:
> > > On Fri, 20 Apr 2012 21:03:35 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > If we try to do that and the scanlines just wouldn't advance, we
> > > > busy-hang the machine holding the modeset mutex. Not great for
> > > > debugging.
> > > > 
> > > > References: https://bugzilla.kernel.org/show_bug.cgi?id=43020
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > Reviewer hangs head in shame:
> > > 
> > > > +		if (wait_for(I915_READ(pipe_dsl_reg) >= vactive, 1000))
> > > > +			DRM_ERROR("timed out waiting for vactive in "
> > > > +				  "load_detect, scanline: %u\n",
> > > > +				  I915_READ(pipe_dsl_reg));
> > > > +		if (wait_for((dsl = I915_READ(pipe_dsl_reg)) <= vsample, 1000))
> > > > +			DRM_ERROR("timed out waiting for vsample in "
> > > > +				  "load_detect, scanline: %u\n",
> > > > +				  I915_READ(pipe_dsl_reg));
> > > 
> > > wait_for() catches us out everytime we convert and existing while(),
> > > because the predicate is when it stops. Perhaps if we had a wait_until,
> > > but anyway the fix here is:
> > > 
> > > if (wait_for(I915_READ(pipe_dsl_reg) < vactive, 1000))
> > >   ...
> > > if (wait_for((dsl = I915_READ(pipe_dsl_reg)) > vsample, 1000))
> > >   ...
> > dinq rectified, it never happened. Thanks for catching this.
> 
> wait_for() has even more subtleties in store for us, the unwary coder.
> By default, it uses a 1ms sleep between polling the register, chosen to
> be kind whilst waiting for panel bits to power up which do take a fair
> amount of time. Here, that extra delay causes us to sample the vsync
> rather than the border. The quirk of the [vh]sync is that the monitor bit
> of ST00 is always true. And since we always seem to pick that row to read
> we always think there is a CRT present.
> 
> The choice is either to use the busy-polling variant, wait_for_atomic,
> or restructure the entire block to use a single timeout with direct
> reads. And whilst you are modifying the code, convert the polling reads
> to I915_READ_NOTRACE().

Thanks a lot for digging into this, I've only managed to do the bisect
before I've haeded off into the w/e. I've dropped this patch for now, too
much fail in it. The underlying issue of not properly doing load-detect on
a active but disabled crtc is fixed, so this isn't required any more.

I'll look a this again later, hopefully clue strikes me by then.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48