[PATCH 1/5] drm/i915/vlv: reduce GT FIFO error info to a debug message

[PATCH 1/5] drm/i915/vlv: reduce GT FIFO error info to a debug message

[Jesse Barnes]

It indicates a probable BIOS bug, but it appears to be harmless, and
there's nothing the user can do about it anyway, so reduce to a debug
msg.  I've filed a bug with the BIOS folks about it anyway, so hopefully
they'll fix whatever GT SB read they were doing when the GT was off.

References: https://bugs.freedesktop.org/show_bug.cgi?id=69396
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 102fc49..10054b5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3804,7 +3804,8 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
 	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
-		DRM_ERROR("GT fifo had a previous error %x\n", gtfifodbg);
+		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
+				 gtfifodbg);
 		I915_WRITE(GTFIFODBG, gtfifodbg);
 	}
 
-- 
1.8.3.1

[PATCH 2/5] drm/i915: use wait_event_timeout when waiting for flip completions

[Jesse Barnes]

We're shutting the crtc off and don't want to hang forever.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bc25481..8da1c96 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2912,8 +2912,9 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 
 	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
 
-	wait_event(dev_priv->pending_flip_queue,
-		   !intel_crtc_has_pending_flip(crtc));
+	wait_event_timeout(dev_priv->pending_flip_queue,
+			   !intel_crtc_has_pending_flip(crtc),
+			   msecs_to_jiffies(50));
 
 	mutex_lock(&dev->struct_mutex);
 	intel_finish_fb(crtc->fb);
-- 
1.8.3.1

[PATCH 3/5] drm/i915/vlv: warn on bad VLV PLL divider values

[Jesse Barnes]

This avoids a divide by zero and warns appropriately on this serious bug.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8da1c96..9a83236 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5109,6 +5109,11 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
 	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
 	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
 
+	if (!clock.n || !(clock.p1 * clock.p2)) {
+		WARN(1, "bad divider values on pipe %d\n", crtc->pipe);
+		return;
+	}
+
 	clock.vco = refclk * clock.m1 * clock.m2 / clock.n;
 	clock.dot = 2 * clock.vco / (clock.p1 * clock.p2);
 
-- 
1.8.3.1

[PATCH 4/5] drm/i915/vlv: untangle integrated clock source handling v2

[Jesse Barnes]

The global integrated clock source bit resides in DPLL B on VLV, but we
were treating it as a per-pipe resource.  It needs to be set whenever
any PLL is active, so pull setting the bit out of vlv_update_pll and
into vlv_enable_pll.  Also add a vlv_disable_pll to prevent disabling it
when pipe B shuts down.

I'm guessing on the references here, I expect this to bite any config
where multiple displays are active or displays are moved from pipe to
pipe.

v2: re-add bits in vlv_update_pll to keep from confusing the state checker

References: https://bugs.freedesktop.org/show_bug.cgi?id=67245
References: https://bugs.freedesktop.org/show_bug.cgi?id=69693
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9a83236..2c040cd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1387,6 +1387,13 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
 	if (IS_MOBILE(dev_priv->dev) && !IS_I830(dev_priv->dev))
 		assert_panel_unlocked(dev_priv, crtc->pipe);
 
+	/* Make sure to use the integrated clock source */
+	if (!crtc->pipe)
+		I915_WRITE(DPLL(1), I915_READ(DPLL(1)) |
+			   DPLL_INTEGRATED_CRI_CLK_VLV);
+	else
+		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
+
 	I915_WRITE(reg, dpll);
 	POSTING_READ(reg);
 	udelay(150);
@@ -1477,6 +1484,20 @@ static void i9xx_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 	POSTING_READ(DPLL(pipe));
 }
 
+static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	u32 val = 0;
+
+	/* Make sure the pipe isn't still relying on us */
+	assert_pipe_disabled(dev_priv, pipe);
+
+	/* Leave integrated clock source enabled for the other pipe */
+	if (pipe)
+		val = I915_READ(DPLL(pipe)) & DPLL_INTEGRATED_CRI_CLK_VLV;
+	I915_WRITE(DPLL(pipe), val);
+	POSTING_READ(DPLL(pipe));
+}
+
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv, int port)
 {
 	u32 port_mask;
@@ -3887,7 +3908,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
 
-	if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+	if (IS_VALLEYVIEW(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+		vlv_disable_pll(dev_priv, pipe);
+	else if (!IS_VALLEYVIEW(dev))
 		i9xx_disable_pll(dev_priv, pipe);
 
 	intel_crtc->active = false;
@@ -4629,7 +4652,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 		DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_CLOCK_VLV;
 	if (pipe)
 		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
-
 	dpll |= DPLL_VCO_ENABLE;
 	crtc->config.dpll_hw_state.dpll = dpll;
 
-- 
1.8.3.1

[PATCH 5/5] drm/i915/vlv: add valleyview_crtc_disable function

[Jesse Barnes]

To handle disabling DP after the CPU pipe is disabled, per the
workaround.

References: https://bugs.freedesktop.org/show_bug.cgi?id=58152
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2c040cd..9a7136c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3873,6 +3873,57 @@ static void i9xx_pfit_disable(struct intel_crtc *crtc)
 	I915_WRITE(PFIT_CONTROL, 0);
 }
 
+static void valleyview_crtc_disable(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_encoder *encoder;
+	int pipe = intel_crtc->pipe;
+	int plane = intel_crtc->plane;
+
+	if (!intel_crtc->active)
+		return;
+
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (!(encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+		      encoder->type == INTEL_OUTPUT_EDP))
+			encoder->disable(encoder);
+
+	/* Give the overlay scaler a chance to disable if it's on this pipe */
+	intel_crtc_wait_for_pending_flips(crtc);
+	drm_vblank_off(dev, pipe);
+
+	if (dev_priv->fbc.plane == plane)
+		intel_disable_fbc(dev);
+
+	intel_crtc_dpms_overlay(intel_crtc, false);
+	intel_crtc_update_cursor(crtc, false);
+	intel_disable_planes(crtc);
+	intel_disable_plane(dev_priv, plane, pipe);
+
+	intel_disable_pipe(dev_priv, pipe);
+
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+		    encoder->type == INTEL_OUTPUT_EDP)
+			encoder->disable(encoder);
+
+	i9xx_pfit_disable(intel_crtc);
+
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->post_disable)
+			encoder->post_disable(encoder);
+
+	if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
+		vlv_disable_pll(dev_priv, pipe);
+
+	intel_crtc->active = false;
+	intel_update_watermarks(crtc);
+
+	intel_update_fbc(dev);
+}
+
 static void i9xx_crtc_disable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3908,10 +3959,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
 
-	if (IS_VALLEYVIEW(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
-		vlv_disable_pll(dev_priv, pipe);
-	else if (!IS_VALLEYVIEW(dev))
-		i9xx_disable_pll(dev_priv, pipe);
+	i9xx_disable_pll(dev_priv, pipe);
 
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
@@ -10048,7 +10096,7 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
-		dev_priv->display.crtc_disable = i9xx_crtc_disable;
+		dev_priv->display.crtc_disable = valleyview_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
 		dev_priv->display.update_plane = i9xx_update_plane;
 	} else {
-- 
1.8.3.1

Re: [PATCH 3/5] drm/i915/vlv: warn on bad VLV PLL divider values

[Chris Wilson]

On Fri, Sep 27, 2013 at 12:57:24PM -0700, Jesse Barnes wrote:
> This avoids a divide by zero and warns appropriately on this serious bug.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8da1c96..9a83236 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5109,6 +5109,11 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
>  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
>  
> +	if (!clock.n || !(clock.p1 * clock.p2)) {

if ((clock.n & clock.p1 & clock.p2) == 0) {

or just

if (!clock.n || !clock.p1 || !clock.p2)

> +		WARN(1, "bad divider values on pipe %d\n", crtc->pipe);
> +		return;
> +	}
> +

So I think you want

if (WARN_ONCE(clock.n == 0 || clock.p1 == 0 || clock.p2 == 0,
              "bad divider values on pipe %d (m1=%d, m2=%d, p1=%d, p2=%d, n=%d)\n",
	      crtc->pipe, clock.m1, clock.m2, clock.p1, clock.p2, clock.n))
   return;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915/vlv: add valleyview_crtc_disable function

[Daniel Vetter]

On Fri, Sep 27, 2013 at 12:57:26PM -0700, Jesse Barnes wrote:
> To handle disabling DP after the CPU pipe is disabled, per the
> workaround.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=58152
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

This also applies to g4x apparently and really encoder type checks in the
crtc code is just evil. Furthermore it seems to be already implemented, at
least that's my impression from reading intel_dp.c.
-Daniel


> ---
>  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2c040cd..9a7136c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3873,6 +3873,57 @@ static void i9xx_pfit_disable(struct intel_crtc *crtc)
>  	I915_WRITE(PFIT_CONTROL, 0);
>  }
>  
> +static void valleyview_crtc_disable(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_encoder *encoder;
> +	int pipe = intel_crtc->pipe;
> +	int plane = intel_crtc->plane;
> +
> +	if (!intel_crtc->active)
> +		return;
> +
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		if (!(encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> +		      encoder->type == INTEL_OUTPUT_EDP))
> +			encoder->disable(encoder);
> +
> +	/* Give the overlay scaler a chance to disable if it's on this pipe */
> +	intel_crtc_wait_for_pending_flips(crtc);
> +	drm_vblank_off(dev, pipe);
> +
> +	if (dev_priv->fbc.plane == plane)
> +		intel_disable_fbc(dev);
> +
> +	intel_crtc_dpms_overlay(intel_crtc, false);
> +	intel_crtc_update_cursor(crtc, false);
> +	intel_disable_planes(crtc);
> +	intel_disable_plane(dev_priv, plane, pipe);
> +
> +	intel_disable_pipe(dev_priv, pipe);
> +
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		if (encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> +		    encoder->type == INTEL_OUTPUT_EDP)
> +			encoder->disable(encoder);
> +
> +	i9xx_pfit_disable(intel_crtc);
> +
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		if (encoder->post_disable)
> +			encoder->post_disable(encoder);
> +
> +	if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
> +		vlv_disable_pll(dev_priv, pipe);
> +
> +	intel_crtc->active = false;
> +	intel_update_watermarks(crtc);
> +
> +	intel_update_fbc(dev);
> +}
> +
>  static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -3908,10 +3959,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
>  
> -	if (IS_VALLEYVIEW(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI))
> -		vlv_disable_pll(dev_priv, pipe);
> -	else if (!IS_VALLEYVIEW(dev))
> -		i9xx_disable_pll(dev_priv, pipe);
> +	i9xx_disable_pll(dev_priv, pipe);
>  
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
> @@ -10048,7 +10096,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = valleyview_crtc_enable;
> -		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> +		dev_priv->display.crtc_disable = valleyview_crtc_disable;
>  		dev_priv->display.off = i9xx_crtc_off;
>  		dev_priv->display.update_plane = i9xx_update_plane;
>  	} else {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm/i915/vlv: warn on bad VLV PLL divider values

[Daniel Vetter]

On Fri, Sep 27, 2013 at 09:05:24PM +0100, Chris Wilson wrote:
> On Fri, Sep 27, 2013 at 12:57:24PM -0700, Jesse Barnes wrote:
> > This avoids a divide by zero and warns appropriately on this serious bug.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8da1c96..9a83236 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5109,6 +5109,11 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
> >  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
> >  
> > +	if (!clock.n || !(clock.p1 * clock.p2)) {
> 
> if ((clock.n & clock.p1 & clock.p2) == 0) {
> 
> or just
> 
> if (!clock.n || !clock.p1 || !clock.p2)
> 
> > +		WARN(1, "bad divider values on pipe %d\n", crtc->pipe);
> > +		return;
> > +	}
> > +
> 
> So I think you want
> 
> if (WARN_ONCE(clock.n == 0 || clock.p1 == 0 || clock.p2 == 0,
>               "bad divider values on pipe %d (m1=%d, m2=%d, p1=%d, p2=%d, n=%d)\n",
> 	      crtc->pipe, clock.m1, clock.m2, clock.p1, clock.p2, clock.n))
>    return;

So the bios leaves a broken setup behind or is our hw state readout not
quite careful enough?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm/i915/vlv: warn on bad VLV PLL divider values

[Jesse Barnes]

On Fri, 27 Sep 2013 22:11:33 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Sep 27, 2013 at 09:05:24PM +0100, Chris Wilson wrote:
> > On Fri, Sep 27, 2013 at 12:57:24PM -0700, Jesse Barnes wrote:
> > > This avoids a divide by zero and warns appropriately on this serious bug.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 8da1c96..9a83236 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5109,6 +5109,11 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> > >  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
> > >  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
> > >  
> > > +	if (!clock.n || !(clock.p1 * clock.p2)) {
> > 
> > if ((clock.n & clock.p1 & clock.p2) == 0) {
> > 
> > or just
> > 
> > if (!clock.n || !clock.p1 || !clock.p2)
> > 
> > > +		WARN(1, "bad divider values on pipe %d\n", crtc->pipe);
> > > +		return;
> > > +	}
> > > +
> > 
> > So I think you want
> > 
> > if (WARN_ONCE(clock.n == 0 || clock.p1 == 0 || clock.p2 == 0,
> >               "bad divider values on pipe %d (m1=%d, m2=%d, p1=%d, p2=%d, n=%d)\n",
> > 	      crtc->pipe, clock.m1, clock.m2, clock.p1, clock.p2, clock.n))
> >    return;
> 
> So the bios leaves a broken setup behind or is our hw state readout not
> quite careful enough?

No this just happens when we're really broken.  E.g. we don't set the
integrated clock bit and then try to enable a PLL and fail all over.

Shouldn't happen in practice.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: use wait_event_timeout when waiting for flip completions

[Chris Wilson]

On Fri, Sep 27, 2013 at 12:57:23PM -0700, Jesse Barnes wrote:
> We're shutting the crtc off and don't want to hang forever.

That scares me ever so slightly, especially ignoring the failure. Do you
want to reset the display controller if the interrupts die, or just
report a WARN, or propagate the failure back and make userspace try
again?

But not taking any action at all is a recipe for a silent disaster. It
has taken years to get to the point where we can turn off the pipes
reliably...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915: use wait_event_timeout when waiting for flip completions

[Jesse Barnes]

On Fri, 27 Sep 2013 21:45:39 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, Sep 27, 2013 at 12:57:23PM -0700, Jesse Barnes wrote:
> > We're shutting the crtc off and don't want to hang forever.
> 
> That scares me ever so slightly, especially ignoring the failure. Do you
> want to reset the display controller if the interrupts die, or just
> report a WARN, or propagate the failure back and make userspace try
> again?
> 
> But not taking any action at all is a recipe for a silent disaster. It
> has taken years to get to the point where we can turn off the pipes
> reliably...

Yeah this one is definitely a WIP.  I'd like to warn when this happens
and clean things up rather than just timing out and letting disaster
befall us later.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: use wait_event_timeout when waiting for flip completions

[Daniel Vetter]

On Fri, Sep 27, 2013 at 9:57 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> We're shutting the crtc off and don't want to hang forever.

Reading the source and the test-suite is advisable ;-)
- We actually don't hang here if the MI_DISPLAY_FLIP doesn't happen.
- We already recover the display state (not so relevant here where we
shut it off, but in the set_base path where we have the same problem
with waiting for flips).
- We have extensive test coverage for gpu hangs vs. flips in all kinds
of contrived situations in igt.

That leaves us with the flip not completing in the hw after the
MI_DISPLAY_FLIP has executed. Usually that just means we miss a
workaround or have a bug in our code, and again we have extensive
testcases for this.

Furthermore the hang recover code is ridiculously tricky - just for
3.12-fixes I've hunted down 3 deadlocks in there. By bailing out too
early you have a good chance to confuse the code and actually make
matters worse ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: use wait_event_timeout when waiting for flip completions

[Jesse Barnes]

On Sat, 28 Sep 2013 11:35:11 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Sep 27, 2013 at 9:57 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > We're shutting the crtc off and don't want to hang forever.
> 
> Reading the source and the test-suite is advisable ;-)
> - We actually don't hang here if the MI_DISPLAY_FLIP doesn't happen.
> - We already recover the display state (not so relevant here where we
> shut it off, but in the set_base path where we have the same problem
> with waiting for flips).
> - We have extensive test coverage for gpu hangs vs. flips in all kinds
> of contrived situations in igt.
> 
> That leaves us with the flip not completing in the hw after the
> MI_DISPLAY_FLIP has executed. Usually that just means we miss a
> workaround or have a bug in our code, and again we have extensive
> testcases for this.
> 
> Furthermore the hang recover code is ridiculously tricky - just for
> 3.12-fixes I've hunted down 3 deadlocks in there. By bailing out too
> early you have a good chance to confuse the code and actually make
> matters worse ;-)

Sorry replied from my phone earlier.

Maybe you should read my earlier reply to Chris.

It's all very nice to say "it doesn't hang here".  However it does hang
waiting in this function on this machine.

It could be that the DPLL CRI clock source fix fixes that, I haven't
re-tested yet.

But this was also not meant for submission, and the earlier reply made
that clear (plus the obvious stuff Chris pointed out), so I don't need
the extra snark in this message.

On top of that, if this code has become so complex and fragile, maybe
it's time to re-think it.  Maybe we should drop ring based flips
altogether and just go with MMIO, and forget about all the races and
even pipe off cases, and just emit completions at vblank time or
immediately after the MMIO is written.

-- 
Jesse Barnes, Intel Open Source Technology Center