[PATCH 1/6] drm/i915: Warn if a pipe is enabled with a bogus port

[PATCH 1/6] drm/i915: Warn if a pipe is enabled with a bogus port

[Damien Lespiau]

If TRANS_DDI_FUNC_CTL has been wrongly programmed with an incorrect
port, we are currently trying to read PORT_CLK_SEL(port) with an
initialized value.

Handle that case by returning PORT_CLK_SEL_NONE and warning about it.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

to add to first
---
 drivers/gpu/drm/i915/intel_ddi.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 56bb7cb..3b5838d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1157,7 +1157,7 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
 				       enum pipe pipe)
 {
 	uint32_t temp, ret;
-	enum port port;
+	enum port port = I915_MAX_PORTS;
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 	int i;
@@ -1173,7 +1173,10 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
 				port = i;
 	}
 
-	ret = I915_READ(PORT_CLK_SEL(port));
+	if (port == I915_MAX_PORTS)
+		ret = PORT_CLK_SEL_NONE;
+	else
+		ret = I915_READ(PORT_CLK_SEL(port));
 
 	DRM_DEBUG_KMS("Pipe %c connected to port %c using clock 0x%08x\n",
 		      pipe_name(pipe), port_name(port), ret);
@@ -1207,6 +1210,10 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
 		case PORT_CLK_SEL_WRPLL2:
 			dev_priv->ddi_plls.wrpll2_refcount++;
 			break;
+		case PORT_CLK_SEL_NONE:
+			WARN(1, "Pipe %c enabled but no clock selected\n",
+			     pipe_name(pipe));
+			break;
 		}
 	}
 }
-- 
1.7.7.5

[PATCH 2/6] drm/i915: Initialize wait in intel_ddi_prepare_link_retrain()

[Damien Lespiau]

We weren't initializing wait, which could lead to the use of a random
value from the stack in the "if (wait)" condition.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3b5838d..a1505e3 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1398,7 +1398,7 @@ void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder)
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
 	enum port port = intel_dig_port->port;
-	bool wait;
+	bool wait = false;
 	uint32_t val;
 
 	if (I915_READ(DP_TP_CTL(port)) & DP_TP_CTL_ENABLE) {
-- 
1.7.7.5

[PATCH 3/6] drm/i915: Error out if we are trying to use VGA with SPLL already in use

[Damien Lespiau]

Our static analysis tool noticed that 'reg' could be used uninitialized if
we are trying to get a PLL to drive VGA and SPLL is already in use
(plls->spll_refcoung != 0).

In the (error) case above, let's return false to the caller and emit an
error.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a1505e3..0eab200 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
 			plls->spll_refcount++;
 			reg = SPLL_CTL;
 			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
+		} else {
+			DRM_ERROR("SPLL already in use\n");
+			return false;
 		}
 
 		WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
-- 
1.7.7.5

[PATCH 4/6] drm/i915: Cleanup if the EDP transcoder has a bobug input value

[Damien Lespiau]

In the case where the hardware has been wrongly programmed and the EDP
TRANS_DDI_FUNC_CTL register has a bogus value in its EDP Input field, we
were using the pipe variable uninitialized.

In this case, shutdown the transcoder. It will be programmed correctly
the next time we try to enabled eDP.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 502cb28..875baf1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9107,6 +9107,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 			case TRANS_DDI_EDP_INPUT_C_ONOFF:
 				pipe = PIPE_C;
 				break;
+			default:
+				/* A bogus value has been programmed, disable
+				 * the transcoder */
+				WARN(1, "Bogus eDP source %08x\n", tmp);
+				intel_ddi_disable_transcoder_func(dev_priv,
+						TRANSCODER_EDP);
+				goto setup_pipes;
 			}
 
 			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
@@ -9117,6 +9124,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		}
 	}
 
+setup_pipes:
 	for_each_pipe(pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 
-- 
1.7.7.5

[PATCH 5/6] drm/i915: Rename intel_ddi_enable_pipe_func() to transcoder_func()

[Damien Lespiau]

We are really talking about the transcoder function here and the disable
version uses trancoder in its name already, so let's try to be
consistent.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |    2 +-
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_drv.h     |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 0eab200..9c556ff 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -953,7 +953,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
 	}
 }
 
-void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)
+void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 875baf1..2f4f50f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3443,7 +3443,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 
 	intel_ddi_set_pipe_settings(crtc);
-	intel_ddi_enable_pipe_func(crtc);
+	intel_ddi_enable_transcoder_func(crtc);
 
 	intel_enable_pipe(dev_priv, pipe, is_pch_port);
 	intel_enable_plane(dev_priv, plane, pipe);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 010e998..8b12cbb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -682,7 +682,7 @@ extern bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 				   enum pipe *pipe);
 extern int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv);
 extern void intel_ddi_pll_init(struct drm_device *dev);
-extern void intel_ddi_enable_pipe_func(struct drm_crtc *crtc);
+extern void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
 extern void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
 					      enum transcoder cpu_transcoder);
 extern void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
-- 
1.7.7.5

[PATCH 6/6] drm/i915: Use BUG() in a case of a programming error

[Damien Lespiau]

The port number should always be correctly set. Do the same thing as the
switch above and use BUG() to signal that branch is not supposed to be
taken.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3921d87..a998c3c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2837,8 +2837,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		name = "DPDDC-D";
 		break;
 	default:
-		WARN(1, "Invalid port %c\n", port_name(port));
-		break;
+		BUG();
 	}
 
 	if (is_edp(intel_dp))
-- 
1.7.7.5

Re: [PATCH 2/6] drm/i915: Initialize wait in intel_ddi_prepare_link_retrain()

[Ben Widawsky]

On Thu, Mar 07, 2013 at 03:30:24PM +0000, Damien Lespiau wrote:
> We weren't initializing wait, which could lead to the use of a random
> value from the stack in the "if (wait)" condition.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/6] drm/i915: Error out if we are trying to use VGA with SPLL already in use

[Ben Widawsky]

On Thu, Mar 07, 2013 at 03:30:25PM +0000, Damien Lespiau wrote:
> Our static analysis tool noticed that 'reg' could be used uninitialized if
> we are trying to get a PLL to drive VGA and SPLL is already in use
> (plls->spll_refcoung != 0).
> 
> In the (error) case above, let's return false to the caller and emit an
> error.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index a1505e3..0eab200 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
>  			plls->spll_refcount++;
>  			reg = SPLL_CTL;
>  			intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
> +		} else {
> +			DRM_ERROR("SPLL already in use\n");
> +			return false;
>  		}
>  
>  		WARN(I915_READ(reg) & SPLL_PLL_ENABLE,

I know very little about this code, but the warning still seems valuable
to me.

How about instead of else, move the reg = SPLL_CTL above the 'if'

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/6] drm/i915: Warn if a pipe is enabled with a bogus port

[Paulo Zanoni]

Hi

2013/3/7 Damien Lespiau <damien.lespiau@intel.com>:
> If TRANS_DDI_FUNC_CTL has been wrongly programmed with an incorrect
> port, we are currently trying to read PORT_CLK_SEL(port) with an
> initialized value.

s/initialized/uninitialized/g

>
> Handle that case by returning PORT_CLK_SEL_NONE and warning about it.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>
> to add to first

First!

> ---
>  drivers/gpu/drm/i915/intel_ddi.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 56bb7cb..3b5838d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1157,7 +1157,7 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
>                                        enum pipe pipe)
>  {
>         uint32_t temp, ret;
> -       enum port port;
> +       enum port port = I915_MAX_PORTS;
>         enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>                                                                       pipe);
>         int i;
> @@ -1173,7 +1173,10 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
>                                 port = i;
>         }
>
> -       ret = I915_READ(PORT_CLK_SEL(port));
> +       if (port == I915_MAX_PORTS)
> +               ret = PORT_CLK_SEL_NONE;

I would put the WARN here (see below).

> +       else
> +               ret = I915_READ(PORT_CLK_SEL(port));
>
>         DRM_DEBUG_KMS("Pipe %c connected to port %c using clock 0x%08x\n",
>                       pipe_name(pipe), port_name(port), ret);
> @@ -1207,6 +1210,10 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>                 case PORT_CLK_SEL_WRPLL2:
>                         dev_priv->ddi_plls.wrpll2_refcount++;
>                         break;
> +               case PORT_CLK_SEL_NONE:
> +                       WARN(1, "Pipe %c enabled but no clock selected\n",
> +                            pipe_name(pipe));

I would put this WARN inside intel_ddi_get_crtc_pll, because that's
the function that really detects the "problem". Currently the function
has only 1 caller, but this could change in the future, and other
callers should also get their WARNs.


> +                       break;
>                 }
>         }
>  }
> --
> 1.7.7.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: Error out if we are trying to use VGA with SPLL already in use

[Paulo Zanoni]

Hi

2013/3/8 Ben Widawsky <ben@bwidawsk.net>:
> On Thu, Mar 07, 2013 at 03:30:25PM +0000, Damien Lespiau wrote:
>> Our static analysis tool noticed that 'reg' could be used uninitialized if
>> we are trying to get a PLL to drive VGA and SPLL is already in use
>> (plls->spll_refcoung != 0).

This was also reported by a human a few weeks ago and went to my TODO
list. Thanks for clearing my TODO list :)

>>
>> In the (error) case above, let's return false to the caller and emit an
>> error.
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index a1505e3..0eab200 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
>>                       plls->spll_refcount++;
>>                       reg = SPLL_CTL;
>>                       intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
>> +             } else {
>> +                     DRM_ERROR("SPLL already in use\n");
>> +                     return false;
>>               }
>>
>>               WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
>
> I know very little about this code, but the warning still seems valuable
> to me.

The WARN will still be hit in case no one is using SPLL but it's
enabled. It's a different problem.

The code added by Damien is correct, but it should never happen with
the current code because only the analog encoder tries to use SPLL and
there's only 1 analog encoder. But we can always introduce bugs to
trigger these errors :)

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

>
> How about instead of else, move the reg = SPLL_CTL above the 'if'
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/6] drm/i915: Cleanup if the EDP transcoder has a bobug input value

[Paulo Zanoni]

Hi

2013/3/7 Damien Lespiau <damien.lespiau@intel.com>:
> In the case where the hardware has been wrongly programmed and the EDP
> TRANS_DDI_FUNC_CTL register has a bogus value in its EDP Input field, we
> were using the pipe variable uninitialized.
>
> In this case, shutdown the transcoder. It will be programmed correctly
> the next time we try to enabled eDP.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 502cb28..875baf1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9107,6 +9107,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>                         case TRANS_DDI_EDP_INPUT_C_ONOFF:
>                                 pipe = PIPE_C;
>                                 break;
> +                       default:
> +                               /* A bogus value has been programmed, disable
> +                                * the transcoder */
> +                               WARN(1, "Bogus eDP source %08x\n", tmp);
> +                               intel_ddi_disable_transcoder_func(dev_priv,
> +                                               TRANSCODER_EDP);

I imagine this patch is a result of checking some code analyzer and
not real hardware, so we didn't really test this code on real
hardware, right? The WARN is definitely a must-have, but I kinda fear
this "disable" code since we can easily freeze the machine if we get
the mode set sequence wrong. Anyway, if the hardware is programmed in
an inconsistent way I don't really think there's a "right way" to
disable it, so:

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

But we need to watch out in case users start reporting about machines
that freeze during boot.

> +                               goto setup_pipes;
>                         }
>
>                         crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> @@ -9117,6 +9124,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>                 }
>         }
>
> +setup_pipes:
>         for_each_pipe(pipe) {
>                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>
> --
> 1.7.7.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/6] drm/i915: Rename intel_ddi_enable_pipe_func() to transcoder_func()

[Paulo Zanoni]

Hi

2013/3/7 Damien Lespiau <damien.lespiau@intel.com>:
> We are really talking about the transcoder function here and the disable
> version uses trancoder in its name already, so let's try to be
> consistent.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 0eab200..9c556ff 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -953,7 +953,7 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>         }
>  }
>
> -void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)
> +void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  {
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 875baf1..2f4f50f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3443,7 +3443,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         intel_crtc_load_lut(crtc);
>
>         intel_ddi_set_pipe_settings(crtc);
> -       intel_ddi_enable_pipe_func(crtc);
> +       intel_ddi_enable_transcoder_func(crtc);
>
>         intel_enable_pipe(dev_priv, pipe, is_pch_port);
>         intel_enable_plane(dev_priv, plane, pipe);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 010e998..8b12cbb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -682,7 +682,7 @@ extern bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>                                    enum pipe *pipe);
>  extern int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv);
>  extern void intel_ddi_pll_init(struct drm_device *dev);
> -extern void intel_ddi_enable_pipe_func(struct drm_crtc *crtc);
> +extern void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
>  extern void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
>                                               enum transcoder cpu_transcoder);
>  extern void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
> --
> 1.7.7.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 6/6] drm/i915: Use BUG() in a case of a programming error

[Paulo Zanoni]

Hi

2013/3/7 Damien Lespiau <damien.lespiau@intel.com>:
> The port number should always be correctly set. Do the same thing as the
> switch above and use BUG() to signal that branch is not supposed to be
> taken.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3921d87..a998c3c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2837,8 +2837,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>                 name = "DPDDC-D";
>                 break;
>         default:
> -               WARN(1, "Invalid port %c\n", port_name(port));
> -               break;
> +               BUG();
>         }
>
>         if (is_edp(intel_dp))
> --
> 1.7.7.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: Error out if we are trying to use VGA with SPLL already in use

[Daniel Vetter]

On Fri, Mar 22, 2013 at 06:14:25PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/8 Ben Widawsky <ben@bwidawsk.net>:
> > On Thu, Mar 07, 2013 at 03:30:25PM +0000, Damien Lespiau wrote:
> >> Our static analysis tool noticed that 'reg' could be used uninitialized if
> >> we are trying to get a PLL to drive VGA and SPLL is already in use
> >> (plls->spll_refcoung != 0).
> 
> This was also reported by a human a few weeks ago and went to my TODO
> list. Thanks for clearing my TODO list :)
> 
> >>
> >> In the (error) case above, let's return false to the caller and emit an
> >> error.
> >>
> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ddi.c |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index a1505e3..0eab200 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -898,6 +898,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> >>                       plls->spll_refcount++;
> >>                       reg = SPLL_CTL;
> >>                       intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL;
> >> +             } else {
> >> +                     DRM_ERROR("SPLL already in use\n");
> >> +                     return false;
> >>               }
> >>
> >>               WARN(I915_READ(reg) & SPLL_PLL_ENABLE,
> >
> > I know very little about this code, but the warning still seems valuable
> > to me.
> 
> The WARN will still be hit in case no one is using SPLL but it's
> enabled. It's a different problem.
> 
> The code added by Damien is correct, but it should never happen with
> the current code because only the analog encoder tries to use SPLL and
> there's only 1 analog encoder. But we can always introduce bugs to
> trigger these errors :)
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@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 4/6] drm/i915: Cleanup if the EDP transcoder has a bobug input value

[Daniel Vetter]

On Fri, Mar 22, 2013 at 06:34:43PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/7 Damien Lespiau <damien.lespiau@intel.com>:
> > In the case where the hardware has been wrongly programmed and the EDP
> > TRANS_DDI_FUNC_CTL register has a bogus value in its EDP Input field, we
> > were using the pipe variable uninitialized.
> >
> > In this case, shutdown the transcoder. It will be programmed correctly
> > the next time we try to enabled eDP.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 502cb28..875baf1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9107,6 +9107,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >                         case TRANS_DDI_EDP_INPUT_C_ONOFF:
> >                                 pipe = PIPE_C;
> >                                 break;
> > +                       default:
> > +                               /* A bogus value has been programmed, disable
> > +                                * the transcoder */
> > +                               WARN(1, "Bogus eDP source %08x\n", tmp);
> > +                               intel_ddi_disable_transcoder_func(dev_priv,
> > +                                               TRANSCODER_EDP);
> 
> I imagine this patch is a result of checking some code analyzer and
> not real hardware, so we didn't really test this code on real
> hardware, right? The WARN is definitely a must-have, but I kinda fear
> this "disable" code since we can easily freeze the machine if we get
> the mode set sequence wrong. Anyway, if the hardware is programmed in
> an inconsistent way I don't really think there's a "right way" to
> disable it, so:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch.

> But we need to watch out in case users start reporting about machines
> that freeze during boot.

And added a little note to the patch to this effect.
-Daniel
> 
> > +                               goto setup_pipes;
> >                         }
> >
> >                         crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > @@ -9117,6 +9124,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >                 }
> >         }
> >
> > +setup_pipes:
> >         for_each_pipe(pipe) {
> >                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >
> > --
> > 1.7.7.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> 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 6/6] drm/i915: Use BUG() in a case of a programming error

[Daniel Vetter]

On Fri, Mar 22, 2013 at 06:43:40PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/7 Damien Lespiau <damien.lespiau@intel.com>:
> > The port number should always be correctly set. Do the same thing as the
> > switch above and use BUG() to signal that branch is not supposed to be
> > taken.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Last two patches of this series are merged to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2] drm/i915: Warn if a pipe is enabled with a bogus port

[Damien Lespiau]

If TRANS_DDI_FUNC_CTL has been wrongly programmed with an incorrect
port, we are currently trying to read PORT_CLK_SEL(port) with an
uninitialized value.

Handle that case by returning PORT_CLK_SEL_NONE and warning about it.

v2: Move the warning inside intel_ddi_get_crtc_pll (Paulo Zanoni)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index d26147c..258e38e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1160,7 +1160,7 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
 				       enum pipe pipe)
 {
 	uint32_t temp, ret;
-	enum port port;
+	enum port port = I915_MAX_PORTS;
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 	int i;
@@ -1176,10 +1176,16 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
 				port = i;
 	}
 
-	ret = I915_READ(PORT_CLK_SEL(port));
-
-	DRM_DEBUG_KMS("Pipe %c connected to port %c using clock 0x%08x\n",
-		      pipe_name(pipe), port_name(port), ret);
+	if (port == I915_MAX_PORTS) {
+		WARN(1, "Pipe %c enabled on an unknown port\n",
+		     pipe_name(pipe));
+		ret = PORT_CLK_SEL_NONE;
+	} else {
+		ret = I915_READ(PORT_CLK_SEL(port));
+		DRM_DEBUG_KMS("Pipe %c connected to port %c using clock "
+			      "0x%08x\n", pipe_name(pipe), port_name(port),
+			      ret);
+	}
 
 	return ret;
 }
-- 
1.7.11.7

Re: [PATCH v2] drm/i915: Warn if a pipe is enabled with a bogus port

[Paulo Zanoni]

Hi

2013/3/25 Damien Lespiau <damien.lespiau@intel.com>:
> If TRANS_DDI_FUNC_CTL has been wrongly programmed with an incorrect
> port, we are currently trying to read PORT_CLK_SEL(port) with an
> uninitialized value.
>
> Handle that case by returning PORT_CLK_SEL_NONE and warning about it.
>
> v2: Move the warning inside intel_ddi_get_crtc_pll (Paulo Zanoni)
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index d26147c..258e38e 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1160,7 +1160,7 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
>                                        enum pipe pipe)
>  {
>         uint32_t temp, ret;
> -       enum port port;
> +       enum port port = I915_MAX_PORTS;
>         enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>                                                                       pipe);
>         int i;
> @@ -1176,10 +1176,16 @@ static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
>                                 port = i;
>         }
>
> -       ret = I915_READ(PORT_CLK_SEL(port));
> -
> -       DRM_DEBUG_KMS("Pipe %c connected to port %c using clock 0x%08x\n",
> -                     pipe_name(pipe), port_name(port), ret);
> +       if (port == I915_MAX_PORTS) {
> +               WARN(1, "Pipe %c enabled on an unknown port\n",
> +                    pipe_name(pipe));
> +               ret = PORT_CLK_SEL_NONE;
> +       } else {
> +               ret = I915_READ(PORT_CLK_SEL(port));
> +               DRM_DEBUG_KMS("Pipe %c connected to port %c using clock "
> +                             "0x%08x\n", pipe_name(pipe), port_name(port),
> +                             ret);
> +       }
>
>         return ret;
>  }
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2] drm/i915: Warn if a pipe is enabled with a bogus port

[Daniel Vetter]

On Mon, Mar 25, 2013 at 02:00:16PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/25 Damien Lespiau <damien.lespiau@intel.com>:
> > If TRANS_DDI_FUNC_CTL has been wrongly programmed with an incorrect
> > port, we are currently trying to read PORT_CLK_SEL(port) with an
> > uninitialized value.
> >
> > Handle that case by returning PORT_CLK_SEL_NONE and warning about it.
> >
> > v2: Move the warning inside intel_ddi_get_crtc_pll (Paulo Zanoni)
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@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