[PATCH 0/3] drm/i915: HSW modeset hang fix

[PATCH 0/3] drm/i915: HSW modeset hang fix

[ville.syrjala]

As Paulo explained at [1], there's a modeset related hang occuring on HSW.
I bisected it down to removing a workaround meant for pre-production hardware,
and Paulo cooked up a workaround that worked for him. Unfortunately I was still 
able to trigger the hangs by disabling all planes on pipe B but leaving the pipe 
active, and the doing a modeset on pipe A.

The actual problem can occur when we enable any plane on a pipe that is not
fully running yet. When a pipe is considered running depends on a bit on the 
type of encoder used. But in any case we should not really be enabling planes
on disabled pipes anyway, so implementing an actual workaround should not
even be necessary. We just need to be careful when we enable planes.

For me it seems Jani already fixed the problem when he proposed killing the cursor
enable calls from the .mode_set hooks. So it looks like our normal plane enabling
during .crtc_enable happens late enough for the pipe to be fully up.

I actually want to refactor the plane enabling/disabling during our
.crtc_{enable,disable} a bit. But as that doesn't seem necessary to prevent the
hangs, I'm posting these minimal fixes first, and then we can go crazy later.

[1] http://lists.freedesktop.org/archives/intel-gfx/2013-September/033040.html

[PATCH 1/3] drm/i915: do not update cursor in crtc mode set

[ville.syrjala]

From: Jani Nikula <jani.nikula@intel.com>

The cursor is disabled before crtc mode set in crtc disable (and we
assert this is the case), and enabled afterwards in crtc enable. Do not
update it in crtc mode set.

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

v2: Add note about HSW hangs - vsyrjala

Cc: stable@vger.kernel.org
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c0e0cf..18043a2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4888,9 +4888,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		}
 	}
 
-	/* Ensure that the cursor is valid for the new mode before changing... */
-	intel_crtc_update_cursor(crtc, true);
-
 	if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
 		/*
 		 * Ensure we match the reduced clock's P to the target clock.
@@ -5780,9 +5777,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		intel_crtc->config.dpll.p2 = clock.p2;
 	}
 
-	/* Ensure that the cursor is valid for the new mode before changing... */
-	intel_crtc_update_cursor(crtc, true);
-
 	/* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
 	if (intel_crtc->config.has_pch_encoder) {
 		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
@@ -6270,9 +6264,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (!intel_ddi_pll_mode_set(crtc))
 		return -EINVAL;
 
-	/* Ensure that the cursor is valid for the new mode before changing... */
-	intel_crtc_update_cursor(crtc, true);
-
 	if (intel_crtc->config.has_dp_encoder)
 		intel_dp_set_m_n(intel_crtc);
 
-- 
1.8.1.5

[PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe

[ville.syrjala]

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

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 18043a2..d0137b6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	u32 base, pos;
 	bool visible;
 
+	if (!intel_crtc->active)
+		return;
+
 	pos = 0;
 
 	if (on && crtc->enabled && crtc->fb) {
-- 
1.8.1.5

[PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[ville.syrjala]

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

On HSW enabling a plane on a disabled pipe may hang the entire system.
And there's no good reason for doing it ever, so just don't.

Cc: stable@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d9c7a66..4f11eb1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.y2 = crtc_y + crtc_h,
 	};
 	const struct drm_rect clip = {
-		.x2 = crtc->mode.hdisplay,
-		.y2 = crtc->mode.vdisplay,
+		.x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
+		.y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
 	};
 
 	intel_fb = to_intel_framebuffer(fb);
-- 
1.8.1.5

Re: [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe

[Chris Wilson]

On Thu, Sep 12, 2013 at 10:45:42PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On HSW enabling a plane on a disabled pipe may hang the entire system.
> And there's no good reason for doing it ever, so just don't.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 18043a2..d0137b6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	u32 base, pos;
>  	bool visible;
>  
> +	if (!intel_crtc->active)
> +		return;

This is misleading since we do expect to call this function whilst
turning off the crtc. This check makes it appear that such calls might
be wrong. Also the !crtc->enabled following intel_crtc->active makes
ones question their sanity.

So I feel this check detracts from readability of the function.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: do not update cursor in crtc mode set

[Chris Wilson]

On Thu, Sep 12, 2013 at 10:45:41PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Jani Nikula <jani.nikula@intel.com>
> 
> The cursor is disabled before crtc mode set in crtc disable (and we
> assert this is the case), and enabled afterwards in crtc enable. Do not
> update it in crtc mode set.
> 
> On HSW enabling a plane on a disabled pipe may hang the entire system.
> And there's no good reason for doing it ever, so just don't.
> 
> v2: Add note about HSW hangs - vsyrjala
> 
> Cc: stable@vger.kernel.org
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Where did the asserts go? :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[Chris Wilson]

On Thu, Sep 12, 2013 at 10:45:43PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On HSW enabling a plane on a disabled pipe may hang the entire system.
> And there's no good reason for doing it ever, so just don't.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index d9c7a66..4f11eb1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.y2 = crtc_y + crtc_h,
>  	};
>  	const struct drm_rect clip = {
> -		.x2 = crtc->mode.hdisplay,
> -		.y2 = crtc->mode.vdisplay,
> +		.x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
> +		.y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
>  	};

Too much magic that looks like it would have interesting effects later
in the function. This function should only be called on an active CRTC,
so declare it so:

  if (WARN_ON(!intel_crtc->active))
    return -EMONKEY;

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Don't enable the cursor on a disable pipe

[Ville Syrjälä]

On Thu, Sep 12, 2013 at 09:08:17PM +0100, Chris Wilson wrote:
> On Thu, Sep 12, 2013 at 10:45:42PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > And there's no good reason for doing it ever, so just don't.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 18043a2..d0137b6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6793,6 +6793,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >  	u32 base, pos;
> >  	bool visible;
> >  
> > +	if (!intel_crtc->active)
> > +		return;
> 
> This is misleading since we do expect to call this function whilst
> turning off the crtc. This check makes it appear that such calls might
> be wrong.

crtc->active is true until the pipe has been fully turned off.

> Also the !crtc->enabled following intel_crtc->active makes
> ones question their sanity.

I actually forgot we had that there. If you recall I'm actually
removing it in my cursor visibility fixes. But I didn't realize that
we don't actually clear crtc->config to zero for inactive pipes, so
I actually would have introduced a bug there.

> So I feel this check detracts from readability of the function.

I just wanted something minimal for stable. But since we have the
->enabled check there currently I think this patch could just be
dropped as ->enabled should match ->active outside modeset
operations, and during modeset operations we're covered by Jani's
patch to remove the extra cursor calls.

But I need to amend my earlier visibility fixes to add either ->active
or ->enabled check back. Or I need to make it so that crtc->config
gets cleared for disabled pipes.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 1/3] drm/i915: do not update cursor in crtc mode set

[Ville Syrjälä]

On Thu, Sep 12, 2013 at 09:09:05PM +0100, Chris Wilson wrote:
> On Thu, Sep 12, 2013 at 10:45:41PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Jani Nikula <jani.nikula@intel.com>
> > 
> > The cursor is disabled before crtc mode set in crtc disable (and we
> > assert this is the case), and enabled afterwards in crtc enable. Do not
> > update it in crtc mode set.
> > 
> > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > And there's no good reason for doing it ever, so just don't.
> > 
> > v2: Add note about HSW hangs - vsyrjala
> > 
> > Cc: stable@vger.kernel.org
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Where did the asserts go? :)

I asked Jani to move them around a bit, which AFAIK he didn't yet. So
I figured we'd just go with a minimal fix for stable first and then
slap on more asserts later in dinq.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[Ville Syrjälä]

On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote:
> On Thu, Sep 12, 2013 at 10:45:43PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > And there's no good reason for doing it ever, so just don't.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index d9c7a66..4f11eb1 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		.y2 = crtc_y + crtc_h,
> >  	};
> >  	const struct drm_rect clip = {
> > -		.x2 = crtc->mode.hdisplay,
> > -		.y2 = crtc->mode.vdisplay,
> > +		.x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
> > +		.y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
> >  	};
> 
> Too much magic that looks like it would have interesting effects later
> in the function. This function should only be called on an active CRTC,
> so declare it so:
> 
>   if (WARN_ON(!intel_crtc->active))
>     return -EMONKEY;

I'm actually perfectly happy to let users set up planes on a disabled
pipe. We allow it for cursors already, so why not all planes? Especially
as cursors will be just drm_planes in the future.

And actually we do have a PIPECONF_ENABLED check in there that I was
going to remove but forgot. That already prevents setting up a plane when
the pipe is off outside modeset operations. During modeset even if we add
an ->active check we might be left with a race window when the pipe is not
fully up and running yet. I think to catch those we just need to slap
asserts at the appropriate place in modeset.

So I gues we don't really need this patch in stable either. Hmm. fixing
bugs is easier than I thought ;)

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 10:40:16AM +0300, Ville Syrjälä wrote:
> On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote:
> > On Thu, Sep 12, 2013 at 10:45:43PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > > And there's no good reason for doing it ever, so just don't.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index d9c7a66..4f11eb1 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > >  		.y2 = crtc_y + crtc_h,
> > >  	};
> > >  	const struct drm_rect clip = {
> > > -		.x2 = crtc->mode.hdisplay,
> > > -		.y2 = crtc->mode.vdisplay,
> > > +		.x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
> > > +		.y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
> > >  	};
> > 
> > Too much magic that looks like it would have interesting effects later
> > in the function. This function should only be called on an active CRTC,
> > so declare it so:
> > 
> >   if (WARN_ON(!intel_crtc->active))
> >     return -EMONKEY;
> 
> I'm actually perfectly happy to let users set up planes on a disabled
> pipe. We allow it for cursors already, so why not all planes? Especially
> as cursors will be just drm_planes in the future.

Oh and BTW the clipping/scaling stuff works perfectly well when stuff
gets fully clipped. dst region will come out as zero, and visible will
be false. That already happens when you simply move the plane outside
the pipe src dimensions.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 10:50:02AM +0300, Ville Syrjälä wrote:
> On Fri, Sep 13, 2013 at 10:40:16AM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote:
> > > On Thu, Sep 12, 2013 at 10:45:43PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > > > And there's no good reason for doing it ever, so just don't.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index d9c7a66..4f11eb1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > >  		.y2 = crtc_y + crtc_h,
> > > >  	};
> > > >  	const struct drm_rect clip = {
> > > > -		.x2 = crtc->mode.hdisplay,
> > > > -		.y2 = crtc->mode.vdisplay,
> > > > +		.x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
> > > > +		.y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
> > > >  	};
> > > 
> > > Too much magic that looks like it would have interesting effects later
> > > in the function. This function should only be called on an active CRTC,
> > > so declare it so:
> > > 
> > >   if (WARN_ON(!intel_crtc->active))
> > >     return -EMONKEY;
> > 
> > I'm actually perfectly happy to let users set up planes on a disabled
> > pipe. We allow it for cursors already, so why not all planes? Especially
> > as cursors will be just drm_planes in the future.
> 
> Oh and BTW the clipping/scaling stuff works perfectly well when stuff
> gets fully clipped. dst region will come out as zero, and visible will
> be false. That already happens when you simply move the plane outside
> the pipe src dimensions.

Hmm. Except the primary disable logic is fscked up in that case. Oh
well, I already knew I have to fix that stuff up.

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/2] drm/i915: add asserts for cursor disabled

[Jani Nikula]

The cursor is supposed to be disabled during crtc mode set (disabled by
ctrc disable). Assert this is the case.

v2: move cursor disabled assert next to plane asserts (Ville)

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d0137b6..0446dc7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1069,6 +1069,26 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
 	     pipe_name(pipe));
 }
 
+static void assert_cursor(struct drm_i915_private *dev_priv,
+			  enum pipe pipe, bool state)
+{
+	struct drm_device *dev = dev_priv->dev;
+	bool cur_state;
+
+	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
+		cur_state = I915_READ(CURCNTR_IVB(pipe)) & CURSOR_MODE;
+	else if (IS_845G(dev) || IS_I865G(dev))
+		cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE;
+	else
+		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
+
+	WARN(cur_state != state,
+	     "cursor on pipe %c assertion failure (expected %s, current %s)\n",
+	     pipe_name(pipe), state_string(state), state_string(cur_state));
+}
+#define assert_cursor_enabled(d, p) assert_cursor(d, p, true)
+#define assert_cursor_disabled(d, p) assert_cursor(d, p, false)
+
 void assert_pipe(struct drm_i915_private *dev_priv,
 		 enum pipe pipe, bool state)
 {
@@ -1670,6 +1690,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
 	u32 val;
 
 	assert_planes_disabled(dev_priv, pipe);
+	assert_cursor_disabled(dev_priv, pipe);
 	assert_sprites_disabled(dev_priv, pipe);
 
 	if (HAS_PCH_LPT(dev_priv->dev))
@@ -1731,6 +1752,7 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	 * or we might hang the display.
 	 */
 	assert_planes_disabled(dev_priv, pipe);
+	assert_cursor_disabled(dev_priv, pipe);
 	assert_sprites_disabled(dev_priv, pipe);
 
 	/* Don't disable pipe A or pipe A PLLs if needed */
@@ -3867,6 +3889,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	dev_priv->display.off(crtc);
 
 	assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane);
+	assert_cursor_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
 	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
 
 	if (crtc->fb) {
-- 
1.7.9.5

[PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling

[Jani Nikula]

Flat out skip anything to do with PLL if we have a DSI encoder (and thus
DSI PLL). Also skip PLL computation if the encoder has already set
clocks. This allows for some tidying up of the code, including a
superfluous call to intel_limit() for LVDS downclock path.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   44 +++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0446dc7..6a51cc2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	refclk = i9xx_get_refclk(crtc, num_connectors);
+	if (is_dsi)
+		goto skip_dpll;
+
+	if (!intel_crtc->config.clock_set) {
+		refclk = i9xx_get_refclk(crtc, num_connectors);
 
-	if (!is_dsi && !intel_crtc->config.clock_set) {
 		/*
 		 * Returns a set of divisors for the desired target clock with
 		 * the given refclk, or FALSE.  The returned values represent
@@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		ok = dev_priv->display.find_dpll(limit, crtc,
 						 intel_crtc->config.port_clock,
 						 refclk, NULL, &clock);
-		if (!ok && !intel_crtc->config.clock_set) {
+		if (!ok) {
 			DRM_ERROR("Couldn't find PLL settings for mode!\n");
 			return -EINVAL;
 		}
-	}
 
-	if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
-		/*
-		 * Ensure we match the reduced clock's P to the target clock.
-		 * If the clocks don't match, we can't switch the display clock
-		 * by using the FP0/FP1. In such case we will disable the LVDS
-		 * downclock feature.
-		*/
-		limit = intel_limit(crtc, refclk);
-		has_reduced_clock =
-			dev_priv->display.find_dpll(limit, crtc,
-						    dev_priv->lvds_downclock,
-						    refclk, &clock,
-						    &reduced_clock);
-	}
-	/* Compat-code for transition, will disappear. */
-	if (!intel_crtc->config.clock_set) {
+		if (is_lvds && dev_priv->lvds_downclock_avail) {
+			/*
+			 * Ensure we match the reduced clock's P to the target
+			 * clock.  If the clocks don't match, we can't switch
+			 * the display clock by using the FP0/FP1. In such case
+			 * we will disable the LVDS downclock feature.
+			 */
+			has_reduced_clock =
+				dev_priv->display.find_dpll(limit, crtc,
+							    dev_priv->lvds_downclock,
+							    refclk, &clock,
+							    &reduced_clock);
+		}
+		/* Compat-code for transition, will disappear. */
 		intel_crtc->config.dpll.n = clock.n;
 		intel_crtc->config.dpll.m1 = clock.m1;
 		intel_crtc->config.dpll.m2 = clock.m2;
@@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
 				num_connectors);
 	} else if (IS_VALLEYVIEW(dev)) {
-		if (!is_dsi)
-			vlv_update_pll(intel_crtc);
+		vlv_update_pll(intel_crtc);
 	} else {
 		i9xx_update_pll(intel_crtc,
 				has_reduced_clock ? &reduced_clock : NULL,
                                 num_connectors);
 	}
 
+skip_dpll:
 	/* Set up the display plane register */
 	dspcntr = DISPPLANE_GAMMA_ENABLE;
 
-- 
1.7.9.5

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Don't enable sprites on a disabled pipe

[Chris Wilson]

On Fri, Sep 13, 2013 at 10:54:45AM +0300, Ville Syrjälä wrote:
> On Fri, Sep 13, 2013 at 10:50:02AM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 13, 2013 at 10:40:16AM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 12, 2013 at 09:13:56PM +0100, Chris Wilson wrote:
> > > > On Thu, Sep 12, 2013 at 10:45:43PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > On HSW enabling a plane on a disabled pipe may hang the entire system.
> > > > > And there's no good reason for doing it ever, so just don't.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index d9c7a66..4f11eb1 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -652,8 +652,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > > > >  		.y2 = crtc_y + crtc_h,
> > > > >  	};
> > > > >  	const struct drm_rect clip = {
> > > > > -		.x2 = crtc->mode.hdisplay,
> > > > > -		.y2 = crtc->mode.vdisplay,
> > > > > +		.x2 = intel_crtc->active ? crtc->mode.hdisplay : 0,
> > > > > +		.y2 = intel_crtc->active ? crtc->mode.vdisplay : 0,
> > > > >  	};
> > > > 
> > > > Too much magic that looks like it would have interesting effects later
> > > > in the function. This function should only be called on an active CRTC,
> > > > so declare it so:
> > > > 
> > > >   if (WARN_ON(!intel_crtc->active))
> > > >     return -EMONKEY;
> > > 
> > > I'm actually perfectly happy to let users set up planes on a disabled
> > > pipe. We allow it for cursors already, so why not all planes? Especially
> > > as cursors will be just drm_planes in the future.
> > 
> > Oh and BTW the clipping/scaling stuff works perfectly well when stuff
> > gets fully clipped. dst region will come out as zero, and visible will
> > be false. That already happens when you simply move the plane outside
> > the pipe src dimensions.
> 
> Hmm. Except the primary disable logic is fscked up in that case. Oh
> well, I already knew I have to fix that stuff up.

That's what caught my eye. I wasn't convinced it would always do the
right thing and erroring out earlier was easier than thinking. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: add asserts for cursor disabled

[Chris Wilson]

On Fri, Sep 13, 2013 at 11:03:08AM +0300, Jani Nikula wrote:
> The cursor is supposed to be disabled during crtc mode set (disabled by
> ctrc disable). Assert this is the case.
> 
> v2: move cursor disabled assert next to plane asserts (Ville)
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Lgtm. There's one extra place for paranoia that I spotted, in
*_update_cursor() we can assert that the cursor in the expected
visibility when we enter the function. That might be overkill for such a
frequently called operation, so just limiting it to the change in
visibility should be paranoid enough.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: add asserts for cursor disabled

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 11:03:08AM +0300, Jani Nikula wrote:
> The cursor is supposed to be disabled during crtc mode set (disabled by
> ctrc disable). Assert this is the case.
> 
> v2: move cursor disabled assert next to plane asserts (Ville)
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |   23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d0137b6..0446dc7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1069,6 +1069,26 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
>  	     pipe_name(pipe));
>  }
>  
> +static void assert_cursor(struct drm_i915_private *dev_priv,
> +			  enum pipe pipe, bool state)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	bool cur_state;
> +
> +	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> +		cur_state = I915_READ(CURCNTR_IVB(pipe)) & CURSOR_MODE;
> +	else if (IS_845G(dev) || IS_I865G(dev))
> +		cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE;
> +	else
> +		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
> +
> +	WARN(cur_state != state,
> +	     "cursor on pipe %c assertion failure (expected %s, current %s)\n",
> +	     pipe_name(pipe), state_string(state), state_string(cur_state));
> +}
> +#define assert_cursor_enabled(d, p) assert_cursor(d, p, true)
> +#define assert_cursor_disabled(d, p) assert_cursor(d, p, false)
> +
>  void assert_pipe(struct drm_i915_private *dev_priv,
>  		 enum pipe pipe, bool state)
>  {
> @@ -1670,6 +1690,7 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
>  	u32 val;
>  
>  	assert_planes_disabled(dev_priv, pipe);
> +	assert_cursor_disabled(dev_priv, pipe);
>  	assert_sprites_disabled(dev_priv, pipe);
>  
>  	if (HAS_PCH_LPT(dev_priv->dev))
> @@ -1731,6 +1752,7 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
>  	 * or we might hang the display.
>  	 */
>  	assert_planes_disabled(dev_priv, pipe);
> +	assert_cursor_disabled(dev_priv, pipe);
>  	assert_sprites_disabled(dev_priv, pipe);
>  
>  	/* Don't disable pipe A or pipe A PLLs if needed */
> @@ -3867,6 +3889,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	dev_priv->display.off(crtc);
>  
>  	assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane);
> +	assert_cursor_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
>  	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
>  
>  	if (crtc->fb) {
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: add asserts for cursor disabled

[Daniel Vetter]

On Fri, Sep 13, 2013 at 02:57:37PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 13, 2013 at 11:03:08AM +0300, Jani Nikula wrote:
> > The cursor is supposed to be disabled during crtc mode set (disabled by
> > ctrc disable). Assert this is the case.
> > 
> > v2: move cursor disabled assert next to plane asserts (Ville)
> > 
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

Re: [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling

[Daniel Vetter]

On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote:
> Flat out skip anything to do with PLL if we have a DSI encoder (and thus
> DSI PLL). Also skip PLL computation if the encoder has already set
> clocks. This allows for some tidying up of the code, including a
> superfluous call to intel_limit() for LVDS downclock path.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Tried to merge this but the baseline seems to be off, at least wrt dinq.
Do I miss some patches that I should apply first?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   44 +++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0446dc7..6a51cc2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  		num_connectors++;
>  	}
>  
> -	refclk = i9xx_get_refclk(crtc, num_connectors);
> +	if (is_dsi)
> +		goto skip_dpll;
> +
> +	if (!intel_crtc->config.clock_set) {
> +		refclk = i9xx_get_refclk(crtc, num_connectors);
>  
> -	if (!is_dsi && !intel_crtc->config.clock_set) {
>  		/*
>  		 * Returns a set of divisors for the desired target clock with
>  		 * the given refclk, or FALSE.  The returned values represent
> @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  		ok = dev_priv->display.find_dpll(limit, crtc,
>  						 intel_crtc->config.port_clock,
>  						 refclk, NULL, &clock);
> -		if (!ok && !intel_crtc->config.clock_set) {
> +		if (!ok) {
>  			DRM_ERROR("Couldn't find PLL settings for mode!\n");
>  			return -EINVAL;
>  		}
> -	}
>  
> -	if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
> -		/*
> -		 * Ensure we match the reduced clock's P to the target clock.
> -		 * If the clocks don't match, we can't switch the display clock
> -		 * by using the FP0/FP1. In such case we will disable the LVDS
> -		 * downclock feature.
> -		*/
> -		limit = intel_limit(crtc, refclk);
> -		has_reduced_clock =
> -			dev_priv->display.find_dpll(limit, crtc,
> -						    dev_priv->lvds_downclock,
> -						    refclk, &clock,
> -						    &reduced_clock);
> -	}
> -	/* Compat-code for transition, will disappear. */
> -	if (!intel_crtc->config.clock_set) {
> +		if (is_lvds && dev_priv->lvds_downclock_avail) {
> +			/*
> +			 * Ensure we match the reduced clock's P to the target
> +			 * clock.  If the clocks don't match, we can't switch
> +			 * the display clock by using the FP0/FP1. In such case
> +			 * we will disable the LVDS downclock feature.
> +			 */
> +			has_reduced_clock =
> +				dev_priv->display.find_dpll(limit, crtc,
> +							    dev_priv->lvds_downclock,
> +							    refclk, &clock,
> +							    &reduced_clock);
> +		}
> +		/* Compat-code for transition, will disappear. */
>  		intel_crtc->config.dpll.n = clock.n;
>  		intel_crtc->config.dpll.m1 = clock.m1;
>  		intel_crtc->config.dpll.m2 = clock.m2;
> @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  				has_reduced_clock ? &reduced_clock : NULL,
>  				num_connectors);
>  	} else if (IS_VALLEYVIEW(dev)) {
> -		if (!is_dsi)
> -			vlv_update_pll(intel_crtc);
> +		vlv_update_pll(intel_crtc);
>  	} else {
>  		i9xx_update_pll(intel_crtc,
>  				has_reduced_clock ? &reduced_clock : NULL,
>                                  num_connectors);
>  	}
>  
> +skip_dpll:
>  	/* Set up the display plane register */
>  	dspcntr = DISPPLANE_GAMMA_ENABLE;
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling

[Jani Nikula]

On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote:
>> Flat out skip anything to do with PLL if we have a DSI encoder (and thus
>> DSI PLL). Also skip PLL computation if the encoder has already set
>> clocks. This allows for some tidying up of the code, including a
>> superfluous call to intel_limit() for LVDS downclock path.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Tried to merge this but the baseline seems to be off, at least wrt dinq.
> Do I miss some patches that I should apply first?

This one depends on "drm/i915: do not update cursor in crtc mode set" in
this thread (my patch but with Ville's commit message amendmend). Did
you push the assert patch before that? It's a good order.

Jani.



> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |   44 +++++++++++++++++-----------------
>>  1 file changed, 22 insertions(+), 22 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 0446dc7..6a51cc2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>>  		num_connectors++;
>>  	}
>>  
>> -	refclk = i9xx_get_refclk(crtc, num_connectors);
>> +	if (is_dsi)
>> +		goto skip_dpll;
>> +
>> +	if (!intel_crtc->config.clock_set) {
>> +		refclk = i9xx_get_refclk(crtc, num_connectors);
>>  
>> -	if (!is_dsi && !intel_crtc->config.clock_set) {
>>  		/*
>>  		 * Returns a set of divisors for the desired target clock with
>>  		 * the given refclk, or FALSE.  The returned values represent
>> @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>>  		ok = dev_priv->display.find_dpll(limit, crtc,
>>  						 intel_crtc->config.port_clock,
>>  						 refclk, NULL, &clock);
>> -		if (!ok && !intel_crtc->config.clock_set) {
>> +		if (!ok) {
>>  			DRM_ERROR("Couldn't find PLL settings for mode!\n");
>>  			return -EINVAL;
>>  		}
>> -	}
>>  
>> -	if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
>> -		/*
>> -		 * Ensure we match the reduced clock's P to the target clock.
>> -		 * If the clocks don't match, we can't switch the display clock
>> -		 * by using the FP0/FP1. In such case we will disable the LVDS
>> -		 * downclock feature.
>> -		*/
>> -		limit = intel_limit(crtc, refclk);
>> -		has_reduced_clock =
>> -			dev_priv->display.find_dpll(limit, crtc,
>> -						    dev_priv->lvds_downclock,
>> -						    refclk, &clock,
>> -						    &reduced_clock);
>> -	}
>> -	/* Compat-code for transition, will disappear. */
>> -	if (!intel_crtc->config.clock_set) {
>> +		if (is_lvds && dev_priv->lvds_downclock_avail) {
>> +			/*
>> +			 * Ensure we match the reduced clock's P to the target
>> +			 * clock.  If the clocks don't match, we can't switch
>> +			 * the display clock by using the FP0/FP1. In such case
>> +			 * we will disable the LVDS downclock feature.
>> +			 */
>> +			has_reduced_clock =
>> +				dev_priv->display.find_dpll(limit, crtc,
>> +							    dev_priv->lvds_downclock,
>> +							    refclk, &clock,
>> +							    &reduced_clock);
>> +		}
>> +		/* Compat-code for transition, will disappear. */
>>  		intel_crtc->config.dpll.n = clock.n;
>>  		intel_crtc->config.dpll.m1 = clock.m1;
>>  		intel_crtc->config.dpll.m2 = clock.m2;
>> @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>>  				has_reduced_clock ? &reduced_clock : NULL,
>>  				num_connectors);
>>  	} else if (IS_VALLEYVIEW(dev)) {
>> -		if (!is_dsi)
>> -			vlv_update_pll(intel_crtc);
>> +		vlv_update_pll(intel_crtc);
>>  	} else {
>>  		i9xx_update_pll(intel_crtc,
>>  				has_reduced_clock ? &reduced_clock : NULL,
>>                                  num_connectors);
>>  	}
>>  
>> +skip_dpll:
>>  	/* Set up the display plane register */
>>  	dspcntr = DISPPLANE_GAMMA_ENABLE;
>>  
>> -- 
>> 1.7.9.5
>> 
>> _______________________________________________
>> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling

[Daniel Vetter]

On Fri, Sep 13, 2013 at 04:38:28PM +0300, Jani Nikula wrote:
> On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote:
> >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus
> >> DSI PLL). Also skip PLL computation if the encoder has already set
> >> clocks. This allows for some tidying up of the code, including a
> >> superfluous call to intel_limit() for LVDS downclock path.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Tried to merge this but the baseline seems to be off, at least wrt dinq.
> > Do I miss some patches that I should apply first?
> 
> This one depends on "drm/i915: do not update cursor in crtc mode set" in
> this thread (my patch but with Ville's commit message amendmend). Did
> you push the assert patch before that? It's a good order.

I've pushed the assert patch to dinq, the real fixes need to go to -fixes.
Tbh I'm a bit confused about what to put where, so please scream ;-)

For Ville's patches I'm waiting a bit for Paulo to take a look and ack
them.
-Daniel

> 
> Jani.
> 
> 
> 
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c |   44 +++++++++++++++++-----------------
> >>  1 file changed, 22 insertions(+), 22 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 0446dc7..6a51cc2 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> >>  		num_connectors++;
> >>  	}
> >>  
> >> -	refclk = i9xx_get_refclk(crtc, num_connectors);
> >> +	if (is_dsi)
> >> +		goto skip_dpll;
> >> +
> >> +	if (!intel_crtc->config.clock_set) {
> >> +		refclk = i9xx_get_refclk(crtc, num_connectors);
> >>  
> >> -	if (!is_dsi && !intel_crtc->config.clock_set) {
> >>  		/*
> >>  		 * Returns a set of divisors for the desired target clock with
> >>  		 * the given refclk, or FALSE.  The returned values represent
> >> @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> >>  		ok = dev_priv->display.find_dpll(limit, crtc,
> >>  						 intel_crtc->config.port_clock,
> >>  						 refclk, NULL, &clock);
> >> -		if (!ok && !intel_crtc->config.clock_set) {
> >> +		if (!ok) {
> >>  			DRM_ERROR("Couldn't find PLL settings for mode!\n");
> >>  			return -EINVAL;
> >>  		}
> >> -	}
> >>  
> >> -	if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
> >> -		/*
> >> -		 * Ensure we match the reduced clock's P to the target clock.
> >> -		 * If the clocks don't match, we can't switch the display clock
> >> -		 * by using the FP0/FP1. In such case we will disable the LVDS
> >> -		 * downclock feature.
> >> -		*/
> >> -		limit = intel_limit(crtc, refclk);
> >> -		has_reduced_clock =
> >> -			dev_priv->display.find_dpll(limit, crtc,
> >> -						    dev_priv->lvds_downclock,
> >> -						    refclk, &clock,
> >> -						    &reduced_clock);
> >> -	}
> >> -	/* Compat-code for transition, will disappear. */
> >> -	if (!intel_crtc->config.clock_set) {
> >> +		if (is_lvds && dev_priv->lvds_downclock_avail) {
> >> +			/*
> >> +			 * Ensure we match the reduced clock's P to the target
> >> +			 * clock.  If the clocks don't match, we can't switch
> >> +			 * the display clock by using the FP0/FP1. In such case
> >> +			 * we will disable the LVDS downclock feature.
> >> +			 */
> >> +			has_reduced_clock =
> >> +				dev_priv->display.find_dpll(limit, crtc,
> >> +							    dev_priv->lvds_downclock,
> >> +							    refclk, &clock,
> >> +							    &reduced_clock);
> >> +		}
> >> +		/* Compat-code for transition, will disappear. */
> >>  		intel_crtc->config.dpll.n = clock.n;
> >>  		intel_crtc->config.dpll.m1 = clock.m1;
> >>  		intel_crtc->config.dpll.m2 = clock.m2;
> >> @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> >>  				has_reduced_clock ? &reduced_clock : NULL,
> >>  				num_connectors);
> >>  	} else if (IS_VALLEYVIEW(dev)) {
> >> -		if (!is_dsi)
> >> -			vlv_update_pll(intel_crtc);
> >> +		vlv_update_pll(intel_crtc);
> >>  	} else {
> >>  		i9xx_update_pll(intel_crtc,
> >>  				has_reduced_clock ? &reduced_clock : NULL,
> >>                                  num_connectors);
> >>  	}
> >>  
> >> +skip_dpll:
> >>  	/* Set up the display plane register */
> >>  	dspcntr = DISPPLANE_GAMMA_ENABLE;
> >>  
> >> -- 
> >> 1.7.9.5
> >> 
> >> _______________________________________________
> >> 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
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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

Re: [PATCH 2/2] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 04:07:19PM +0200, Daniel Vetter wrote:
> On Fri, Sep 13, 2013 at 04:38:28PM +0300, Jani Nikula wrote:
> > On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote:
> > >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus
> > >> DSI PLL). Also skip PLL computation if the encoder has already set
> > >> clocks. This allows for some tidying up of the code, including a
> > >> superfluous call to intel_limit() for LVDS downclock path.
> > >> 
> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Tried to merge this but the baseline seems to be off, at least wrt dinq.
> > > Do I miss some patches that I should apply first?
> > 
> > This one depends on "drm/i915: do not update cursor in crtc mode set" in
> > this thread (my patch but with Ville's commit message amendmend). Did
> > you push the assert patch before that? It's a good order.
> 
> I've pushed the assert patch to dinq, the real fixes need to go to -fixes.
> Tbh I'm a bit confused about what to put where, so please scream ;-)
> 
> For Ville's patches I'm waiting a bit for Paulo to take a look and ack
> them.

For stable we need just (assuming it also fixes Paulo's hangs):
 drm/i915: do not update cursor in crtc mode set

On top of that we can add the cursor assert patch (not sure if we want
that to go to stable):
 drm/i915: add asserts for cursor disabled

I get the impression you applied the assert patch alone. That's probably
not a very good idea since it will (or at least should) scream every time
you do a modeset, unless the other patch is applied as well.

-- 
Ville Syrjälä
Intel OTC