[PATCH 0/4] some follow-up fixlets from the modeset rework

[PATCH 0/4] some follow-up fixlets from the modeset rework

[Daniel Vetter]

Hi all,

Just a few small follow-ups
- 2 patches to make pch/fdi handling more strict and add more asserts to check
  that.
- 2 patches to make dpms handling around modesets more strict.

All four have been included in the modeset-rework branch since a long time and
hence seen tons of testing already. But I've figured they're not part of the
core rework, hence I've split them out.

Flames, comments & review highly welcome.

Cheers, Daniel

Daniel Vetter (4):
  drm/i915: rip out intel_disable_pch_ports
  drm/i915: don't disable fdi links harder in ilk_crtc_enable
  drm/i915: don't call dpms funcs after set_mode
  drm/i915: update dpms property in set_mode

 drivers/gpu/drm/i915/intel_display.c | 84 +++++-------------------------------
 1 file changed, 11 insertions(+), 73 deletions(-)

-- 
1.7.11.2

[PATCH 1/4] drm/i915: rip out intel_disable_pch_ports

[Daniel Vetter]

Even with the old crtc helper code we should have disabled all
encoders on that pipe by now, and with the new code this would
definitely paper over a bug. We already have the necessary checks
in place in intel_disable_transcoder, so if we accidentally leave
a pch port on, this will be caught.

Hence just rip this all out.

Note that up to the patch in this giant modeset series that removes
the LVDS special case to avoid disabling LVDS in the encoder->prepare
callback ("drm/i915/lvds: ditch ->prepare special case"), this was not
the case for all outputs.

Also note that in

commit 1b3c7a47f993bf9ab6c4c7cc3bbf5588052b58f4
Author: Zhenyu Wang <zhenyuw@linux.intel.com>
Date:   Wed Nov 25 13:09:38 2009 +0800

    drm/i915: Fix LVDS stability issue on Ironlake

this was already discovered independently and worked around. How I
bloody hate this entire mess of cludges piled on top of other cludges.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 60 ------------------------------------
 1 file changed, 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e061acd..6c06109 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1860,59 +1860,6 @@ static void intel_disable_plane(struct drm_i915_private *dev_priv,
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
-static void disable_pch_dp(struct drm_i915_private *dev_priv,
-			   enum pipe pipe, int reg, u32 port_sel)
-{
-	u32 val = I915_READ(reg);
-	if (dp_pipe_enabled(dev_priv, pipe, port_sel, val)) {
-		DRM_DEBUG_KMS("Disabling pch dp %x on pipe %d\n", reg, pipe);
-		I915_WRITE(reg, val & ~DP_PORT_EN);
-	}
-}
-
-static void disable_pch_hdmi(struct drm_i915_private *dev_priv,
-			     enum pipe pipe, int reg)
-{
-	u32 val = I915_READ(reg);
-	if (hdmi_pipe_enabled(dev_priv, pipe, val)) {
-		DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n",
-			      reg, pipe);
-		I915_WRITE(reg, val & ~PORT_ENABLE);
-	}
-}
-
-/* Disable any ports connected to this transcoder */
-static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
-				    enum pipe pipe)
-{
-	u32 reg, val;
-
-	val = I915_READ(PCH_PP_CONTROL);
-	I915_WRITE(PCH_PP_CONTROL, val | PANEL_UNLOCK_REGS);
-
-	disable_pch_dp(dev_priv, pipe, PCH_DP_B, TRANS_DP_PORT_SEL_B);
-	disable_pch_dp(dev_priv, pipe, PCH_DP_C, TRANS_DP_PORT_SEL_C);
-	disable_pch_dp(dev_priv, pipe, PCH_DP_D, TRANS_DP_PORT_SEL_D);
-
-	reg = PCH_ADPA;
-	val = I915_READ(reg);
-	if (adpa_pipe_enabled(dev_priv, pipe, val))
-		I915_WRITE(reg, val & ~ADPA_DAC_ENABLE);
-
-	reg = PCH_LVDS;
-	val = I915_READ(reg);
-	if (lvds_pipe_enabled(dev_priv, pipe, val)) {
-		DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe, val);
-		I915_WRITE(reg, val & ~LVDS_PORT_EN);
-		POSTING_READ(reg);
-		udelay(100);
-	}
-
-	disable_pch_hdmi(dev_priv, pipe, HDMIB);
-	disable_pch_hdmi(dev_priv, pipe, HDMIC);
-	disable_pch_hdmi(dev_priv, pipe, HDMID);
-}
-
 int
 intel_pin_and_fence_fb_obj(struct drm_device *dev,
 			   struct drm_i915_gem_object *obj,
@@ -3311,13 +3258,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	ironlake_fdi_disable(crtc);
 
-	/* This is a horrible layering violation; we should be doing this in
-	 * the connector/encoder ->prepare instead, but we don't always have
-	 * enough information there about the config to know whether it will
-	 * actually be necessary or just cause undesired flicker.
-	 */
-	intel_disable_pch_ports(dev_priv, pipe);
-
 	intel_disable_transcoder(dev_priv, pipe);
 
 	if (HAS_PCH_CPT(dev)) {
-- 
1.7.11.2

Re: [PATCH 1/4] drm/i915: rip out intel_disable_pch_ports

[Jesse Barnes]

On Thu,  6 Sep 2012 22:08:32 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Even with the old crtc helper code we should have disabled all
> encoders on that pipe by now, and with the new code this would
> definitely paper over a bug. We already have the necessary checks
> in place in intel_disable_transcoder, so if we accidentally leave
> a pch port on, this will be caught.
> 
> Hence just rip this all out.
> 
> Note that up to the patch in this giant modeset series that removes
> the LVDS special case to avoid disabling LVDS in the encoder->prepare
> callback ("drm/i915/lvds: ditch ->prepare special case"), this was not
> the case for all outputs.
> 
> Also note that in
> 
> commit 1b3c7a47f993bf9ab6c4c7cc3bbf5588052b58f4
> Author: Zhenyu Wang <zhenyuw@linux.intel.com>
> Date:   Wed Nov 25 13:09:38 2009 +0800
> 
>     drm/i915: Fix LVDS stability issue on Ironlake
> 
> this was already discovered independently and worked around. How I
> bloody hate this entire mess of cludges piled on top of other cludges.

Yes this was always an ugly layering violation too.  I think I was
worried about a mode set on one crtc getting stale config bits from
another that hadn't been touched...

Anyway now we've fixed it properly.

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

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 2/4] drm/i915: don't disable fdi links harder in ilk_crtc_enable

[Daniel Vetter]

Because they should have been disabled when shutting down the display
pipe previously. To ensure that this is the case, add a few assserts
instead of unconditionally disabling the fdi link.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6c06109..0973797 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3182,10 +3182,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	is_pch_port = intel_crtc_driving_pch(crtc);
 
-	if (is_pch_port)
+	if (is_pch_port) {
 		ironlake_fdi_pll_enable(intel_crtc);
-	else
-		ironlake_fdi_disable(crtc);
+	} else {
+		assert_fdi_tx_disabled(dev_priv, pipe);
+		assert_fdi_rx_disabled(dev_priv, pipe);
+	}
 
 	/* Enable panel fitting for LVDS */
 	if (dev_priv->pch_pf_size &&
-- 
1.7.11.2

Re: [PATCH 2/4] drm/i915: don't disable fdi links harder in ilk_crtc_enable

[Jesse Barnes]

On Thu,  6 Sep 2012 22:08:33 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Because they should have been disabled when shutting down the display
> pipe previously. To ensure that this is the case, add a few assserts
> instead of unconditionally disabling the fdi link.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6c06109..0973797 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3182,10 +3182,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	is_pch_port = intel_crtc_driving_pch(crtc);
>  
> -	if (is_pch_port)
> +	if (is_pch_port) {
>  		ironlake_fdi_pll_enable(intel_crtc);
> -	else
> -		ironlake_fdi_disable(crtc);
> +	} else {
> +		assert_fdi_tx_disabled(dev_priv, pipe);
> +		assert_fdi_rx_disabled(dev_priv, pipe);
> +	}
>  
>  	/* Enable panel fitting for LVDS */
>  	if (dev_priv->pch_pf_size &&

Yep.

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

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/4] drm/i915: don't call dpms funcs after set_mode

[Daniel Vetter]

... because our current set_mode implementation doesn't bother to adjust
for the dpms state, we just forcefully update it. So stop pretending that
we're better than we are and rip out this extranous call.

Note that this totally confuses userspace, because the exposed connector
property isn't actually updated ...

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0973797..805324d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7236,7 +7236,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 	struct drm_mode_set save_set;
 	struct intel_set_config *config;
 	int ret;
-	int i;
 
 	BUG_ON(!set);
 	BUG_ON(!set->crtc);
@@ -7300,15 +7299,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 			ret = -EINVAL;
 			goto fail;
 		}
-
-		if (set->crtc->enabled) {
-			DRM_DEBUG_KMS("Setting connector DPMS state to on\n");
-			for (i = 0; i < set->num_connectors; i++) {
-				DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id,
-					      drm_get_connector_name(set->connectors[i]));
-				set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON);
-			}
-		}
 	} else if (config->fb_changed) {
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
-- 
1.7.11.2

Re: [PATCH 3/4] drm/i915: don't call dpms funcs after set_mode

[Jesse Barnes]

On Thu,  6 Sep 2012 22:08:34 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> ... because our current set_mode implementation doesn't bother to adjust
> for the dpms state, we just forcefully update it. So stop pretending that
> we're better than we are and rip out this extranous call.
> 
> Note that this totally confuses userspace, because the exposed connector
> property isn't actually updated ...
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0973797..805324d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7236,7 +7236,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	struct drm_mode_set save_set;
>  	struct intel_set_config *config;
>  	int ret;
> -	int i;
>  
>  	BUG_ON(!set);
>  	BUG_ON(!set->crtc);
> @@ -7300,15 +7299,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			ret = -EINVAL;
>  			goto fail;
>  		}
> -
> -		if (set->crtc->enabled) {
> -			DRM_DEBUG_KMS("Setting connector DPMS state to on\n");
> -			for (i = 0; i < set->num_connectors; i++) {
> -				DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id,
> -					      drm_get_connector_name(set->connectors[i]));
> -				set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON);
> -			}
> -		}
>  	} else if (config->fb_changed) {
>  		ret = intel_pipe_set_base(set->crtc,
>  					  set->x, set->y, set->fb);

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

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 4/4] drm/i915: update dpms property in set_mode

[Daniel Vetter]

Hopefully this makes userspace slightly less confused about us
frobbing the dpms state behind its back. Yeah, it would be better
to be more careful with not changing the dpms state, but that is
quite more invasive.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 805324d..bff0936 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6806,7 +6806,13 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
 		intel_crtc = to_intel_crtc(connector->encoder->crtc);
 
 		if (prepare_pipes & (1 << intel_crtc->pipe)) {
+			struct drm_property *dpms_property =
+				dev->mode_config.dpms_property;
+
 			connector->dpms = DRM_MODE_DPMS_ON;
+			drm_connector_property_set_value(connector,
+							 dpms_property,
+							 DRM_MODE_DPMS_ON);
 
 			intel_encoder = to_intel_encoder(connector->encoder);
 			intel_encoder->connectors_active = true;
-- 
1.7.11.2

Re: [PATCH 4/4] drm/i915: update dpms property in set_mode

[Jesse Barnes]

On Thu,  6 Sep 2012 22:08:35 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Hopefully this makes userspace slightly less confused about us
> frobbing the dpms state behind its back. Yeah, it would be better
> to be more careful with not changing the dpms state, but that is
> quite more invasive.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 805324d..bff0936 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6806,7 +6806,13 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  		intel_crtc = to_intel_crtc(connector->encoder->crtc);
>  
>  		if (prepare_pipes & (1 << intel_crtc->pipe)) {
> +			struct drm_property *dpms_property =
> +				dev->mode_config.dpms_property;
> +
>  			connector->dpms = DRM_MODE_DPMS_ON;
> +			drm_connector_property_set_value(connector,
> +							 dpms_property,
> +							 DRM_MODE_DPMS_ON);
>  
>  			intel_encoder = to_intel_encoder(connector->encoder);
>  			intel_encoder->connectors_active = true;

Both this and the last one need lots of testing coming from different
DPMS states between fbcon and X, especially across different heads and
connectors.

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

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: update dpms property in set_mode

[Daniel Vetter]

On Thu, Sep 13, 2012 at 01:42:01PM -0700, Jesse Barnes wrote:
> On Thu,  6 Sep 2012 22:08:35 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Hopefully this makes userspace slightly less confused about us
> > frobbing the dpms state behind its back. Yeah, it would be better
> > to be more careful with not changing the dpms state, but that is
> > quite more invasive.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 805324d..bff0936 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6806,7 +6806,13 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
> >  		intel_crtc = to_intel_crtc(connector->encoder->crtc);
> >  
> >  		if (prepare_pipes & (1 << intel_crtc->pipe)) {
> > +			struct drm_property *dpms_property =
> > +				dev->mode_config.dpms_property;
> > +
> >  			connector->dpms = DRM_MODE_DPMS_ON;
> > +			drm_connector_property_set_value(connector,
> > +							 dpms_property,
> > +							 DRM_MODE_DPMS_ON);
> >  
> >  			intel_encoder = to_intel_encoder(connector->encoder);
> >  			intel_encoder->connectors_active = true;
> 
> Both this and the last one need lots of testing coming from different
> DPMS states between fbcon and X, especially across different heads and
> connectors.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I've merged all four patches for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch