[PATCH 1/3] drm/i915: make DP configuration vars less confusing in ironlake_crtc_mode_se

[PATCH 1/3] drm/i915: make DP configuration vars less confusing in ironlake_crtc_mode_se

[Jesse Barnes]

Both PCH and CPU eDP are DP, so set the is_dp flag to true.  Add
is_cpu_edp and is_pch_edp bools to make checking for each less verbose
(rather than has_edp_encoder && !intel_encoder_is_pch_edp() sprinkled
everywhere).  And rename the "has_edp_encoder" variable to just
"edp_encoder".

With the above variables cleaned up, the rest of the code becomes a bit
more readable and clear.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d13e8a4..f1e13b7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5955,9 +5955,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	u32 dpll, fp = 0, fp2 = 0, dspcntr, pipeconf;
 	bool ok, has_reduced_clock = false, is_sdvo = false;
 	bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
-	struct intel_encoder *has_edp_encoder = NULL;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct intel_encoder *encoder;
+	struct intel_encoder *encoder, *edp_encoder = NULL;
 	const intel_limit_t *limit;
 	int ret;
 	struct fdi_m_n m_n = {0};
@@ -5966,6 +5965,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	int target_clock, pixel_multiplier, lane, link_bw, factor;
 	unsigned int pipe_bpp;
 	bool dither;
+	bool is_cpu_edp = false, is_pch_edp = false;
 
 	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
 		if (encoder->base.crtc != crtc)
@@ -5991,7 +5991,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 			is_dp = true;
 			break;
 		case INTEL_OUTPUT_EDP:
-			has_edp_encoder = encoder;
+			is_dp = true;
+			if (intel_encoder_is_pch_edp(&encoder->base))
+				is_pch_edp = true;
+			else
+				is_cpu_edp = true;
+			edp_encoder = encoder;
 			break;
 		}
 
@@ -6054,15 +6059,13 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	lane = 0;
 	/* CPU eDP doesn't require FDI link, so just set DP M/N
 	   according to current link config */
-	if (has_edp_encoder &&
-	    !intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
+	if (is_cpu_edp) {
 		target_clock = mode->clock;
-		intel_edp_link_config(has_edp_encoder,
-				      &lane, &link_bw);
+		intel_edp_link_config(edp_encoder, &lane, &link_bw);
 	} else {
 		/* [e]DP over FDI requires target mode clock
 		   instead of link clock */
-		if (is_dp || intel_encoder_is_pch_edp(&has_edp_encoder->base))
+		if (is_dp)
 			target_clock = mode->clock;
 		else
 			target_clock = adjusted_mode->clock;
@@ -6153,7 +6156,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		}
 		dpll |= DPLL_DVO_HIGH_SPEED;
 	}
-	if (is_dp || intel_encoder_is_pch_edp(&has_edp_encoder->base))
+	if (is_dp && !is_cpu_edp)
 		dpll |= DPLL_DVO_HIGH_SPEED;
 
 	/* compute bitmask from p1 value */
@@ -6198,8 +6201,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	/* PCH eDP needs FDI, but CPU eDP does not */
 	if (!intel_crtc->no_pll) {
-		if (!has_edp_encoder ||
-		    intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
+		if (!is_cpu_edp) {
 			I915_WRITE(PCH_FP0(pipe), fp);
 			I915_WRITE(PCH_DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
 
@@ -6277,7 +6279,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		pipeconf |= PIPECONF_DITHER_EN;
 		pipeconf |= PIPECONF_DITHER_TYPE_SP;
 	}
-	if (is_dp || intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
+	if (is_dp && !is_cpu_edp) {
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 	} else {
 		/* For non-DP output, clear any trans DP clock recovery setting.*/
@@ -6287,9 +6289,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
 	}
 
-	if (!intel_crtc->no_pll &&
-	    (!has_edp_encoder ||
-	     intel_encoder_is_pch_edp(&has_edp_encoder->base))) {
+	if (!intel_crtc->no_pll && (!edp_encoder || is_pch_edp)) {
 		I915_WRITE(PCH_DPLL(pipe), dpll);
 
 		/* Wait for the clocks to stabilize. */
@@ -6367,10 +6367,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
 	I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
 
-	if (has_edp_encoder &&
-	    !intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
+	if (is_cpu_edp)
 		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
-	}
 
 	I915_WRITE(PIPECONF(pipe), pipeconf);
 	POSTING_READ(PIPECONF(pipe));
-- 
1.7.4.1

[PATCH 2/3] drm/i915: check PPS regs for sanity when using eDP

[Jesse Barnes]

If these regs don't have valid values, the panel won't come up, and may
even cause a system hang.  So do a basic sanity check when an eDP panel
is detected.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 110552f..6346b29 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2462,6 +2462,13 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
 		pp_div = I915_READ(PCH_PP_DIVISOR);
 
+		if (!pp_on || !pp_off || !pp_div) {
+			DRM_INFO("bad panel power sequencing delays, disabling panel\n");
+			intel_dp_encoder_destroy(&intel_dp->base.base);
+			intel_dp_destroy(&intel_connector->base);
+			return;
+		}
+
 		/* Pull timing values out of registers */
 		cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
 			PANEL_POWER_UP_DELAY_SHIFT;
-- 
1.7.4.1

[PATCH 3/3] drm/i915: allow PCH PWM override on IVB

[Jesse Barnes]

On IVB, there are two sets of panel backlight regs: one in the CPU and
one in the PCH.  The CPU ones aren't generally used, so on IVB make sure
we allow the PCH regs to actually control the backlight.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_panel.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cad45ff..b007c4e 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -306,6 +306,18 @@ static void intel_panel_init_backlight(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/*
+	 * IVB has CPU eDP backlight regs too, set things up to let the
+	 * PCH regs control the backlight
+	 */
+	if (IS_IVYBRIDGE(dev)) {
+		u32 pwm;
+
+		I915_WRITE(BLC_PWM_CPU_CTL2, PWM_ENABLE);
+		I915_WRITE(BLC_PWM_CPU_CTL, 0);
+		I915_WRITE(BLC_PWM_PCH_CTL1, PWM_ENABLE | (1<<30));
+	}
+
 	dev_priv->backlight_level = intel_panel_get_backlight(dev);
 	dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
 }
-- 
1.7.4.1

Re: [PATCH 2/3] drm/i915: check PPS regs for sanity when using eDP

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1403 bytes --]

On Tue, 10 Apr 2012 11:58:04 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> If these regs don't have valid values, the panel won't come up, and may
> even cause a system hang.  So do a basic sanity check when an eDP panel
> is detected.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 110552f..6346b29 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2462,6 +2462,13 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
>  		pp_div = I915_READ(PCH_PP_DIVISOR);
>  
> +		if (!pp_on || !pp_off || !pp_div) {
> +			DRM_INFO("bad panel power sequencing delays, disabling panel\n");
> +			intel_dp_encoder_destroy(&intel_dp->base.base);
> +			intel_dp_destroy(&intel_connector->base);
> +			return;
> +		}
> +
>  		/* Pull timing values out of registers */
>  		cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
>  			PANEL_POWER_UP_DELAY_SHIFT;

Forgot to add, this one references
https://bugs.freedesktop.org/show_bug.cgi?id=44305

so if it looks ok, please add that to the commit message.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/3] drm/i915: make DP configuration vars less confusing in ironlake_crtc_mode_se

[Chris Wilson]

On Tue, 10 Apr 2012 11:58:03 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Both PCH and CPU eDP are DP, so set the is_dp flag to true.  Add
> is_cpu_edp and is_pch_edp bools to make checking for each less verbose
> (rather than has_edp_encoder && !intel_encoder_is_pch_edp() sprinkled
> everywhere).  And rename the "has_edp_encoder" variable to just
> "edp_encoder".
> 
> With the above variables cleaned up, the rest of the code becomes a bit
> more readable and clear.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Ok, that took quite a bit of reading to be sure that the changes were
consistent. Unfortunately it also looks to be in its simplest form.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm/i915: check PPS regs for sanity when using eDP

[Chris Wilson]

On Tue, 10 Apr 2012 11:58:04 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> If these regs don't have valid values, the panel won't come up, and may
> even cause a system hang.  So do a basic sanity check when an eDP panel
> is detected.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Other than adding a cleanup: error path (definitely before the third
person cut'n'pastes the intel_dp*destroy()), and that checking for zero
seems a little crude,

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: allow PCH PWM override on IVB

[Daniel Vetter]

On Tue, Apr 10, 2012 at 11:58:05AM -0700, Jesse Barnes wrote:
> On IVB, there are two sets of panel backlight regs: one in the CPU and
> one in the PCH.  The CPU ones aren't generally used, so on IVB make sure
> we allow the PCH regs to actually control the backlight.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cad45ff..b007c4e 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -306,6 +306,18 @@ static void intel_panel_init_backlight(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	/*
> +	 * IVB has CPU eDP backlight regs too, set things up to let the
> +	 * PCH regs control the backlight
> +	 */
> +	if (IS_IVYBRIDGE(dev)) {
> +		u32 pwm;

That pwm thing seems unused ...
-Daniel
> +
> +		I915_WRITE(BLC_PWM_CPU_CTL2, PWM_ENABLE);
> +		I915_WRITE(BLC_PWM_CPU_CTL, 0);
> +		I915_WRITE(BLC_PWM_PCH_CTL1, PWM_ENABLE | (1<<30));
> +	}
> +
>  	dev_priv->backlight_level = intel_panel_get_backlight(dev);
>  	dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
>  }
> -- 
> 1.7.4.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/3] drm/i915: allow PCH PWM override on IVB

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1262 bytes --]

On Tue, 10 Apr 2012 23:50:45 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Apr 10, 2012 at 11:58:05AM -0700, Jesse Barnes wrote:
> > On IVB, there are two sets of panel backlight regs: one in the CPU and
> > one in the PCH.  The CPU ones aren't generally used, so on IVB make sure
> > we allow the PCH regs to actually control the backlight.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index cad45ff..b007c4e 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -306,6 +306,18 @@ static void intel_panel_init_backlight(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	/*
> > +	 * IVB has CPU eDP backlight regs too, set things up to let the
> > +	 * PCH regs control the backlight
> > +	 */
> > +	if (IS_IVYBRIDGE(dev)) {
> > +		u32 pwm;
> 
> That pwm thing seems unused ...
> -Daniel

Oh yeah we can drop that now.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/3] drm/i915: make DP configuration vars less confusing in ironlake_crtc_mode_se

[Daniel Vetter]

On Tue, Apr 10, 2012 at 10:22:09PM +0100, Chris Wilson wrote:
> On Tue, 10 Apr 2012 11:58:03 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Both PCH and CPU eDP are DP, so set the is_dp flag to true.  Add
> > is_cpu_edp and is_pch_edp bools to make checking for each less verbose
> > (rather than has_edp_encoder && !intel_encoder_is_pch_edp() sprinkled
> > everywhere).  And rename the "has_edp_encoder" variable to just
> > "edp_encoder".
> > 
> > With the above variables cleaned up, the rest of the code becomes a bit
> > more readable and clear.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Ok, that took quite a bit of reading to be sure that the changes were
> consistent. Unfortunately it also looks to be in its simplest form.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch. I don't know what to do with the
other two patches, are they supposed to be for -fixes?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/3] drm/i915: make DP configuration vars less confusing in ironlake_crtc_mode_se

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1251 bytes --]

On Wed, 11 Apr 2012 11:53:55 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Apr 10, 2012 at 10:22:09PM +0100, Chris Wilson wrote:
> > On Tue, 10 Apr 2012 11:58:03 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > Both PCH and CPU eDP are DP, so set the is_dp flag to true.  Add
> > > is_cpu_edp and is_pch_edp bools to make checking for each less verbose
> > > (rather than has_edp_encoder && !intel_encoder_is_pch_edp() sprinkled
> > > everywhere).  And rename the "has_edp_encoder" variable to just
> > > "edp_encoder".
> > > 
> > > With the above variables cleaned up, the rest of the code becomes a bit
> > > more readable and clear.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > Ok, that took quite a bit of reading to be sure that the changes were
> > consistent. Unfortunately it also looks to be in its simplest form.
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Queued for -next, thanks for the patch. I don't know what to do with the
> other two patches, are they supposed to be for -fixes?

I don't think they're urgent enough for -fixes.  They're still just
fixing issues on my IVB SDV.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/3] drm/i915: check PPS regs for sanity when using eDP

[Daniel Vetter]

On Tue, Apr 10, 2012 at 10:26:56PM +0100, Chris Wilson wrote:
> On Tue, 10 Apr 2012 11:58:04 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > If these regs don't have valid values, the panel won't come up, and may
> > even cause a system hang.  So do a basic sanity check when an eDP panel
> > is detected.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Other than adding a cleanup: error path (definitely before the third
> person cut'n'pastes the intel_dp*destroy()), and that checking for zero
> seems a little crude,
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48