[PATCH 0/3] Some more modeset fallout

[PATCH 0/3] Some more modeset fallout

[Daniel Vetter]

Hi all,

A few fixes. Insights for patch 3 especially highly welcome, patches 1-2 are
just normal fixes begging for a bit of review.

Cheers, Daniel

Daniel Vetter (3):
  drm/i915: fix up the IBX transcoder B check
  drm/i915: correctly update crtc->x/y in set_base
  [HACK] fix ilk/gm45 dp dpms off/on cycles

 drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] drm/i915: fix up the IBX transcoder B check

[Daniel Vetter]

This has been added in

commit de9a35abb3b343a25065449234e47a76c4f3454a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Jun 5 11:03:40 2012 +0200

    drm/i915: assert that the IBX port transcoder select w/a is implemented

Unfortunately I've failed to notice that these checks are not just
called for the port that is about to be disabled, but for all (which
makes sense for an assert ...), and the WARN missfired when disabling
another pipe than the one with the dp port.

Hence also check whether the port is actually disabled.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f26fb3f..b8e5a51 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
 	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
 	     reg, pipe_name(pipe));
 
-	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
+	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0
+	     && (val & DP_PIPEB_SELECT),
 	     "IBX PCH dp port still using transcoder B\n");
 }
 
@@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
 	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
 	     reg, pipe_name(pipe));
 
-	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
+	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0
+	     && (val & SDVO_PIPE_B_SELECT),
 	     "IBX PCH hdmi port still using transcoder B\n");
 }
 
-- 
1.7.10.4

Re: [PATCH 1/3] drm/i915: fix up the IBX transcoder B check

[Paulo Zanoni]

2012/9/10 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This has been added in
>
> commit de9a35abb3b343a25065449234e47a76c4f3454a
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Jun 5 11:03:40 2012 +0200
>
>     drm/i915: assert that the IBX port transcoder select w/a is implemented
>
> Unfortunately I've failed to notice that these checks are not just
> called for the port that is about to be disabled, but for all (which
> makes sense for an assert ...), and the WARN missfired when disabling
> another pipe than the one with the dp port.
>
> Hence also check whether the port is actually disabled.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Looks fine:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f26fb3f..b8e5a51 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
>              "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
>              reg, pipe_name(pipe));
>
> -       WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> +       WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0
> +            && (val & DP_PIPEB_SELECT),
>              "IBX PCH dp port still using transcoder B\n");
>  }
>
> @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
>              "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
>              reg, pipe_name(pipe));
>
> -       WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> +       WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0
> +            && (val & SDVO_PIPE_B_SELECT),
>              "IBX PCH hdmi port still using transcoder B\n");
>  }
>
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/3] drm/i915: fix up the IBX transcoder B check

[Daniel Vetter]

On Tue, Sep 11, 2012 at 11:57:56AM -0300, Paulo Zanoni wrote:
> 2012/9/10 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > This has been added in
> >
> > commit de9a35abb3b343a25065449234e47a76c4f3454a
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Tue Jun 5 11:03:40 2012 +0200
> >
> >     drm/i915: assert that the IBX port transcoder select w/a is implemented
> >
> > Unfortunately I've failed to notice that these checks are not just
> > called for the port that is about to be disabled, but for all (which
> > makes sense for an assert ...), and the WARN missfired when disabling
> > another pipe than the one with the dp port.
> >
> > Hence also check whether the port is actually disabled.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Looks fine:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Picked up for -fixes, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: fix up the IBX transcoder B check

[Jesse Barnes]

On Mon, 10 Sep 2012 21:58:29 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> This has been added in
> 
> commit de9a35abb3b343a25065449234e47a76c4f3454a
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Jun 5 11:03:40 2012 +0200
> 
>     drm/i915: assert that the IBX port transcoder select w/a is implemented
> 
> Unfortunately I've failed to notice that these checks are not just
> called for the port that is about to be disabled, but for all (which
> makes sense for an assert ...), and the WARN missfired when disabling
> another pipe than the one with the dp port.
> 
> Hence also check whether the port is actually disabled.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f26fb3f..b8e5a51 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
>  	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
>  	     reg, pipe_name(pipe));
>  
> -	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0
> +	     && (val & DP_PIPEB_SELECT),
>  	     "IBX PCH dp port still using transcoder B\n");
>  }
>  
> @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
>  	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
>  	     reg, pipe_name(pipe));
>  
> -	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0
> +	     && (val & SDVO_PIPE_B_SELECT),
>  	     "IBX PCH hdmi port still using transcoder B\n");
>  }
>  

Won't these warn if the port is disabled rather than enabled?
Shouldn't we be checking for (val & PORT_ENABLE) != 0 *and* pipe B is
selected?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: fix up the IBX transcoder B check

[Daniel Vetter]

On Wed, Sep 12, 2012 at 08:20:12AM -0700, Jesse Barnes wrote:
> On Mon, 10 Sep 2012 21:58:29 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > This has been added in
> > 
> > commit de9a35abb3b343a25065449234e47a76c4f3454a
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Tue Jun 5 11:03:40 2012 +0200
> > 
> >     drm/i915: assert that the IBX port transcoder select w/a is implemented
> > 
> > Unfortunately I've failed to notice that these checks are not just
> > called for the port that is about to be disabled, but for all (which
> > makes sense for an assert ...), and the WARN missfired when disabling
> > another pipe than the one with the dp port.
> > 
> > Hence also check whether the port is actually disabled.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f26fb3f..b8e5a51 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
> >  	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
> >  	     reg, pipe_name(pipe));
> >  
> > -	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> > +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0
> > +	     && (val & DP_PIPEB_SELECT),
> >  	     "IBX PCH dp port still using transcoder B\n");
> >  }
> >  
> > @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
> >  	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
> >  	     reg, pipe_name(pipe));
> >  
> > -	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> > +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0
> > +	     && (val & SDVO_PIPE_B_SELECT),
> >  	     "IBX PCH hdmi port still using transcoder B\n");
> >  }
> >  
> 
> Won't these warn if the port is disabled rather than enabled?
> Shouldn't we be checking for (val & PORT_ENABLE) != 0 *and* pipe B is
> selected?

Recap from our irc discussion: This is for an ibx workaround, where we
can't let a disabled port stay on transcoder B (for it prevents the other
encoder on the same port from reliably getting enabled). The "is this port
properly disabled" check is above the diff context.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 2/3] drm/i915: correctly update crtc->x/y in set_base

[Daniel Vetter]

While reworking the modeset sequence, this got lost in

commit 25c5b2665fe4cc5a93edd29b62e7c05c15dddd26
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jul 8 22:08:04 2012 +0200

    drm/i915: implement new set_mode code flow

I've noticed this because some Xorg versions seem to set up a new mode
with every crtc at (0,0) and then pan to the right multi-monitor
setup. And since some hacks of mine added more calls to mode_set using
the stored crtc->x/y my multi-screen setup blew up.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b8e5a51..fcc7300 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2195,6 +2195,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	old_fb = crtc->fb;
 	crtc->fb = fb;
+	crtc->x = x;
+	crtc->y = y;
 
 	if (old_fb) {
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
@@ -6993,11 +6995,11 @@ bool intel_set_mode(struct drm_crtc *crtc,
 			dev_priv->display.crtc_disable(&intel_crtc->base);
 	}
 
-	if (modeset_pipes) {
+	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
+	 * to set it here already despite that we pass it down the callchain.
+	 */
+	if (modeset_pipes)
 		crtc->mode = *mode;
-		crtc->x = x;
-		crtc->y = y;
-	}
 
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
-- 
1.7.10.4

Re: [PATCH 2/3] drm/i915: correctly update crtc->x/y in set_base

[Jesse Barnes]

On Mon, 10 Sep 2012 21:58:30 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> While reworking the modeset sequence, this got lost in
> 
> commit 25c5b2665fe4cc5a93edd29b62e7c05c15dddd26
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sun Jul 8 22:08:04 2012 +0200
> 
>     drm/i915: implement new set_mode code flow
> 
> I've noticed this because some Xorg versions seem to set up a new mode
> with every crtc at (0,0) and then pan to the right multi-monitor
> setup. And since some hacks of mine added more calls to mode_set using
> the stored crtc->x/y my multi-screen setup blew up.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b8e5a51..fcc7300 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2195,6 +2195,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  
>  	old_fb = crtc->fb;
>  	crtc->fb = fb;
> +	crtc->x = x;
> +	crtc->y = y;
>  
>  	if (old_fb) {
>  		intel_wait_for_vblank(dev, intel_crtc->pipe);
> @@ -6993,11 +6995,11 @@ bool intel_set_mode(struct drm_crtc *crtc,
>  			dev_priv->display.crtc_disable(&intel_crtc->base);
>  	}
>  
> -	if (modeset_pipes) {
> +	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
> +	 * to set it here already despite that we pass it down the callchain.
> +	 */
> +	if (modeset_pipes)
>  		crtc->mode = *mode;
> -		crtc->x = x;
> -		crtc->y = y;
> -	}
>  
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */

I had to check and make sure the full mode set path went down there
too...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: correctly update crtc->x/y in set_base

[Daniel Vetter]

On Wed, Sep 12, 2012 at 08:29:21AM -0700, Jesse Barnes wrote:
> On Mon, 10 Sep 2012 21:58:30 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > While reworking the modeset sequence, this got lost in
> > 
> > commit 25c5b2665fe4cc5a93edd29b62e7c05c15dddd26
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Sun Jul 8 22:08:04 2012 +0200
> > 
> >     drm/i915: implement new set_mode code flow
> > 
> > I've noticed this because some Xorg versions seem to set up a new mode
> > with every crtc at (0,0) and then pan to the right multi-monitor
> > setup. And since some hacks of mine added more calls to mode_set using
> > the stored crtc->x/y my multi-screen setup blew up.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b8e5a51..fcc7300 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2195,6 +2195,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >  
> >  	old_fb = crtc->fb;
> >  	crtc->fb = fb;
> > +	crtc->x = x;
> > +	crtc->y = y;
> >  
> >  	if (old_fb) {
> >  		intel_wait_for_vblank(dev, intel_crtc->pipe);
> > @@ -6993,11 +6995,11 @@ bool intel_set_mode(struct drm_crtc *crtc,
> >  			dev_priv->display.crtc_disable(&intel_crtc->base);
> >  	}
> >  
> > -	if (modeset_pipes) {
> > +	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
> > +	 * to set it here already despite that we pass it down the callchain.
> > +	 */
> > +	if (modeset_pipes)
> >  		crtc->mode = *mode;
> > -		crtc->x = x;
> > -		crtc->y = y;
> > -	}
> >  
> >  	/* Only after disabling all output pipelines that will be changed can we
> >  	 * update the the output configuration. */
> 
> I had to check and make sure the full mode set path went down there
> too...
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

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

[PATCH 3/3] [HACK] fix ilk/gm45 dp dpms off/on cycles

[Daniel Vetter]

Without this we have link train fail. Still need to figure out what
exactly we seem to lose, but without this dp on gm45 and pch dp on ilk
utterly fails in link retrainig at dpms on. The only way to fix this
is to do a full modeset (i.e. eseentially what this patch here does).

Not-Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fcc7300..1aba63c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3438,6 +3438,12 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc,
 	}
 }
 
+static int intel_crtc_mode_set(struct drm_crtc *crtc,
+			       struct drm_display_mode *mode,
+			       struct drm_display_mode *adjusted_mode,
+			       int x, int y,
+			       struct drm_framebuffer *fb);
+
 /**
  * Sets the power management mode of the pipe and plane.
  */
@@ -3451,9 +3457,11 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
 		enable |= intel_encoder->connectors_active;
 
-	if (enable)
+	if (enable) {
+		intel_crtc_mode_set(crtc, &crtc->mode, &crtc->hwmode,
+				    crtc->x, crtc->y, crtc->fb);
 		dev_priv->display.crtc_enable(crtc);
-	else
+	} else
 		dev_priv->display.crtc_disable(crtc);
 
 	intel_crtc_update_sarea(crtc, enable);
-- 
1.7.10.4