[PATCH 00/14] drm/i915: FIFO underrun elimination for PCH platforms

[PATCH 00/14] drm/i915: FIFO underrun elimination for PCH platforms

[ville.syrjala]

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

This series eliminates all spurious PCH FIFO underrun reports on my
machines during a BAT run ('-t basic -x reload -x suspend' actually).
It also eliminates the non-spurious but expected underrun reports
on ILK.

I also embarked on a small scale cleanup of the CPU eDP PLL code while
I was trying to get to the bottom of the underruns on ILK. So I figured
I'd include that work here as well.

I've tested this on ILK, SNB, two IVBs, and one HSW, with as many
displays plugged in as possible. What I could actually test is HSW/BDW
CRT output since I have no machine for that.

The series is available here:
git://github.com/vsyrjala/linux.git pch_fifo_underrun_fix_4

Ville Syrjälä (14):
  drm/i915: Don't use intel_pipe_to_cpu_transcoder() when there's a pipe
    config around
  drm/i915: Set sync polarity from adjusted mode for TRANS_DP_CTL
  drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB
  drm/i915: Enable PCH FIFO underruns later on HSW+
  drm/i915: Re-enable PCH FIO underrun reporting after pipe has been
    disabled
  drm/i915: Check for FIFO underruns after modeset on IVB/HSW and
    CPT/PPT
  drm/i915: Check for CPT and not !IBX in
    ironlake_disable_pch_transcoder()
  drm/i915: Disable FIFO underrun reporting around IBX transcoder B
    workaround
  drm/i915: Hide underruns from eDP PLL and port enable on ILK
  drm/i915: s/DP_PLL_FREQ_160MHZ/DP_PLL_FREQ_162MHZ/
  drm/i915: Remove ILK-A eDP PLL workaround notes
  drm/i915: Clean up eDP PLL state asserts
  drm/i915: Use intel_dp->DP in eDP PLL setup
  drm/i915: Configure eDP PLL freq from ironlake_edp_pll_on()

 drivers/gpu/drm/i915/i915_reg.h            |   2 +-
 drivers/gpu/drm/i915/intel_display.c       |  64 +++++++----
 drivers/gpu/drm/i915/intel_dp.c            | 179 ++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h           |  12 +-
 drivers/gpu/drm/i915/intel_fifo_underrun.c | 121 +++++++++++++++----
 drivers/gpu/drm/i915/intel_hdmi.c          |  11 ++
 drivers/gpu/drm/i915/intel_sdvo.c          |  11 ++
 7 files changed, 285 insertions(+), 115 deletions(-)

-- 
2.4.10

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

[PATCH 01/14] drm/i915: Don't use intel_pipe_to_cpu_transcoder() when there's a pipe config around

[ville.syrjala]

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

No point in doing the crtc->pipe->crtc->config->cpu_transcoder dance
when we can just do crtc->config->cpu_transcoder.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bc1907e..d3cd177 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2106,8 +2106,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe = crtc->pipe;
-	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
-								      pipe);
+	enum transcoder cpu_transcoder = crtc->config->cpu_transcoder;
 	enum pipe pch_transcoder;
 	int reg;
 	u32 val;
@@ -5208,13 +5207,11 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned long mask;
-	enum transcoder transcoder;
+	enum transcoder transcoder = intel_crtc->config->cpu_transcoder;
 
 	if (!crtc->state->active)
 		return 0;
 
-	transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
-
 	mask = BIT(POWER_DOMAIN_PIPE(pipe));
 	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
 	if (intel_crtc->config->pch_pfit.enabled ||
-- 
2.4.10

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

Re: [PATCH 01/14] drm/i915: Don't use intel_pipe_to_cpu_transcoder() when there's a pipe config around

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> No point in doing the crtc->pipe->crtc->config->cpu_transcoder dance
> when we can just do crtc->config->cpu_transcoder.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bc1907e..d3cd177 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2106,8 +2106,7 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum pipe pipe = crtc->pipe;
> -	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> -								      pipe);
> +	enum transcoder cpu_transcoder = crtc->config->cpu_transcoder;
>  	enum pipe pch_transcoder;
>  	int reg;
>  	u32 val;
> @@ -5208,13 +5207,11 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
>  	unsigned long mask;
> -	enum transcoder transcoder;
> +	enum transcoder transcoder = intel_crtc->config->cpu_transcoder;
>  
>  	if (!crtc->state->active)
>  		return 0;
>  
> -	transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
> -
>  	mask = BIT(POWER_DOMAIN_PIPE(pipe));
>  	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
>  	if (intel_crtc->config->pch_pfit.enabled ||
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/14] drm/i915: Set sync polarity from adjusted mode for TRANS_DP_CTL

[ville.syrjala]

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

Rather than looking at crtc->mode (which is the user mode) dig up the
sync polarity settings from the adjusted_mode when programming
TRANS_DP_CTL on CPT/PPT.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.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 d3cd177..99fb33f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4170,6 +4170,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 
 	/* For PCH DP, enable TRANS_DP_CTL */
 	if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) {
+		const struct drm_display_mode *adjusted_mode =
+			&intel_crtc->config->base.adjusted_mode;
 		u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPECONF_BPC_MASK) >> 5;
 		reg = TRANS_DP_CTL(pipe);
 		temp = I915_READ(reg);
@@ -4179,9 +4181,9 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 		temp |= TRANS_DP_OUTPUT_ENABLE;
 		temp |= bpc << 9; /* same format but at 11:9 */
 
-		if (crtc->mode.flags & DRM_MODE_FLAG_PHSYNC)
+		if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
 			temp |= TRANS_DP_HSYNC_ACTIVE_HIGH;
-		if (crtc->mode.flags & DRM_MODE_FLAG_PVSYNC)
+		if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
 			temp |= TRANS_DP_VSYNC_ACTIVE_HIGH;
 
 		switch (intel_trans_dp_port_sel(crtc)) {
-- 
2.4.10

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

Re: [PATCH 02/14] drm/i915: Set sync polarity from adjusted mode for TRANS_DP_CTL

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Rather than looking at crtc->mode (which is the user mode) dig up the
> sync polarity settings from the adjusted_mode when programming
> TRANS_DP_CTL on CPT/PPT.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.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 d3cd177..99fb33f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4170,6 +4170,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  
>  	/* For PCH DP, enable TRANS_DP_CTL */
>  	if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) {
> +		const struct drm_display_mode *adjusted_mode =
> +			&intel_crtc->config->base.adjusted_mode;
>  		u32 bpc = (I915_READ(PIPECONF(pipe)) & PIPECONF_BPC_MASK) >> 5;
>  		reg = TRANS_DP_CTL(pipe);
>  		temp = I915_READ(reg);
> @@ -4179,9 +4181,9 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  		temp |= TRANS_DP_OUTPUT_ENABLE;
>  		temp |= bpc << 9; /* same format but at 11:9 */
>  
> -		if (crtc->mode.flags & DRM_MODE_FLAG_PHSYNC)
> +		if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
>  			temp |= TRANS_DP_HSYNC_ACTIVE_HIGH;
> -		if (crtc->mode.flags & DRM_MODE_FLAG_PVSYNC)
> +		if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
>  			temp |= TRANS_DP_VSYNC_ACTIVE_HIGH;
>  
>  		switch (intel_trans_dp_port_sel(crtc)) {
> 

God I wish we'd rename these structs a bit... "adjusted" and
"crtc->mode" don't really communicate much.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[ville.syrjala]

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

We get spurious PCH FIFO underruns if we enable the reporting too soon
after enabling the crtc. Move it to be the last step, after the encoder
enable. Additionally we need an extra vblank wait, otherwise we still
get the underruns. Presumably the pipe/fdi isn't yet fully up and running
otherwise.

For symmetry, disable the PCH underrun reporting as the first thing,
just before encoder disable, when shutting down the crtc.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 99fb33f..d5cb899 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc->active = true;
 
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	if (HAS_PCH_CPT(dev))
 		cpt_verify_modeset(dev, intel_crtc->pipe);
+
+	if (intel_crtc->config->has_pch_encoder) {
+		/* Must wait for vblank to avoid spurious PCH FIFO underruns */
+		intel_wait_for_vblank(dev, pipe);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
+	}
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
@@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
 	drm_crtc_vblank_off(crtc);
 	assert_vblank_disabled(crtc);
 
-	if (intel_crtc->config->has_pch_encoder)
-		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
-
 	intel_disable_pipe(intel_crtc);
 
 	ironlake_pfit_disable(intel_crtc, false);
-- 
2.4.10

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

Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We get spurious PCH FIFO underruns if we enable the reporting too soon
> after enabling the crtc. Move it to be the last step, after the encoder
> enable. Additionally we need an extra vblank wait, otherwise we still
> get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> otherwise.
> 
> For symmetry, disable the PCH underrun reporting as the first thing,
> just before encoder disable, when shutting down the crtc.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 99fb33f..d5cb899 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->pre_enable)
> @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	if (HAS_PCH_CPT(dev))
>  		cpt_verify_modeset(dev, intel_crtc->pipe);
> +
> +	if (intel_crtc->config->has_pch_encoder) {
> +		/* Must wait for vblank to avoid spurious PCH FIFO underruns */
> +		intel_wait_for_vblank(dev, pipe);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> +	}
>  }
>  
>  /* IPS only exists on ULT machines and is tied to pipe A. */
> @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	u32 reg, temp;
>  
> +	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
>  	drm_crtc_vblank_off(crtc);
>  	assert_vblank_disabled(crtc);
>  
> -	if (intel_crtc->config->has_pch_encoder)
> -		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> -
>  	intel_disable_pipe(intel_crtc);
>  
>  	ironlake_pfit_disable(intel_crtc, false);
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Paulo Zanoni]

2015-10-29 17:25 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We get spurious PCH FIFO underruns if we enable the reporting too soon
> after enabling the crtc. Move it to be the last step, after the encoder
> enable. Additionally we need an extra vblank wait, otherwise we still
> get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> otherwise.
>
> For symmetry, disable the PCH underrun reporting as the first thing,
> just before encoder disable, when shutting down the crtc.

Is there any place that describes where/when a FIFO underrun is
expected and where/when one is an actual problem that needs to be
solved? How do we know the underruns avoided by these patch are not a
signal of real bugs?

The fact that we don't get these 100% of the time suggests that maybe
they're avoidable somehow.

Perhaps you could update the commit message with the info.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 99fb33f..d5cb899 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>         intel_crtc->active = true;
>
>         intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -       intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
> @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>
>         if (HAS_PCH_CPT(dev))
>                 cpt_verify_modeset(dev, intel_crtc->pipe);
> +
> +       if (intel_crtc->config->has_pch_encoder) {
> +               /* Must wait for vblank to avoid spurious PCH FIFO underruns */
> +               intel_wait_for_vblank(dev, pipe);
> +               intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> +       }
>  }
>
>  /* IPS only exists on ULT machines and is tied to pipe A. */
> @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>         int pipe = intel_crtc->pipe;
>         u32 reg, temp;
>
> +       if (intel_crtc->config->has_pch_encoder)
> +               intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> +
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 encoder->disable(encoder);
>
>         drm_crtc_vblank_off(crtc);
>         assert_vblank_disabled(crtc);
>
> -       if (intel_crtc->config->has_pch_encoder)
> -               intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> -
>         intel_disable_pipe(intel_crtc);
>
>         ironlake_pfit_disable(intel_crtc, false);
> --
> 2.4.10
>
> _______________________________________________
> 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

Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Ville Syrjälä]

On Thu, Oct 29, 2015 at 05:57:57PM -0200, Paulo Zanoni wrote:
> 2015-10-29 17:25 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We get spurious PCH FIFO underruns if we enable the reporting too soon
> > after enabling the crtc. Move it to be the last step, after the encoder
> > enable. Additionally we need an extra vblank wait, otherwise we still
> > get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> > otherwise.
> >
> > For symmetry, disable the PCH underrun reporting as the first thing,
> > just before encoder disable, when shutting down the crtc.
> 
> Is there any place that describes where/when a FIFO underrun is
> expected and where/when one is an actual problem that needs to be
> solved? How do we know the underruns avoided by these patch are not a
> signal of real bugs?

Can't be 100% sure since its not documented anywhere. But we've been
getting these since forever now and stuff still works (more or less at
least), so I'm inclined to say we don't have to care about them. Also
in these case we only get PCH FIFO underruns and no CPU pipe
underruns, so I'm tempted to say it's not that serious.

IIRC I once tracked some of these down to having the FDI PLL enabled
with FDI RX/TX disabled. Or something like that, but don't quote me
on that since my memory might be failing here. Obviously that can't
explain it all since I still need the vblank wait to eliminate them.
Anyway, this time I didn't try to narrow it down too much. Instead
my aim was more to reliably eliminate them without permamently disabling
the underrun detection.

In any case, we can't get the bat stuff really working until we get
the results to be stable, and these underruns are one big obstacle
to that.

> 
> The fact that we don't get these 100% of the time suggests that maybe
> they're avoidable somehow.

It might be just about timing and/or the port type. Feel free to dig
into it more if you want, I won't. I think I already wasted enough
time poking at my ILK in an effort to figure out the CPU eDP underruns.

> 
> Perhaps you could update the commit message with the info.
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 99fb33f..d5cb899 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >         intel_crtc->active = true;
> >
> >         intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> > -       intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> >
> >         for_each_encoder_on_crtc(dev, crtc, encoder)
> >                 if (encoder->pre_enable)
> > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >
> >         if (HAS_PCH_CPT(dev))
> >                 cpt_verify_modeset(dev, intel_crtc->pipe);
> > +
> > +       if (intel_crtc->config->has_pch_encoder) {
> > +               /* Must wait for vblank to avoid spurious PCH FIFO underruns */
> > +               intel_wait_for_vblank(dev, pipe);
> > +               intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> > +       }
> >  }
> >
> >  /* IPS only exists on ULT machines and is tied to pipe A. */
> > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >         int pipe = intel_crtc->pipe;
> >         u32 reg, temp;
> >
> > +       if (intel_crtc->config->has_pch_encoder)
> > +               intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> >         for_each_encoder_on_crtc(dev, crtc, encoder)
> >                 encoder->disable(encoder);
> >
> >         drm_crtc_vblank_off(crtc);
> >         assert_vblank_disabled(crtc);
> >
> > -       if (intel_crtc->config->has_pch_encoder)
> > -               intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > -
> >         intel_disable_pipe(intel_crtc);
> >
> >         ironlake_pfit_disable(intel_crtc, false);
> > --
> > 2.4.10
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Daniel Vetter]

On Thu, Oct 29, 2015 at 11:21:28PM +0200, Ville Syrjälä wrote:
> On Thu, Oct 29, 2015 at 05:57:57PM -0200, Paulo Zanoni wrote:
> > 2015-10-29 17:25 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > We get spurious PCH FIFO underruns if we enable the reporting too soon
> > > after enabling the crtc. Move it to be the last step, after the encoder
> > > enable. Additionally we need an extra vblank wait, otherwise we still
> > > get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> > > otherwise.
> > >
> > > For symmetry, disable the PCH underrun reporting as the first thing,
> > > just before encoder disable, when shutting down the crtc.
> > 
> > Is there any place that describes where/when a FIFO underrun is
> > expected and where/when one is an actual problem that needs to be
> > solved? How do we know the underruns avoided by these patch are not a
> > signal of real bugs?
> 
> Can't be 100% sure since its not documented anywhere. But we've been
> getting these since forever now and stuff still works (more or less at
> least), so I'm inclined to say we don't have to care about them. Also
> in these case we only get PCH FIFO underruns and no CPU pipe
> underruns, so I'm tempted to say it's not that serious.
> 
> IIRC I once tracked some of these down to having the FDI PLL enabled
> with FDI RX/TX disabled. Or something like that, but don't quote me
> on that since my memory might be failing here. Obviously that can't
> explain it all since I still need the vblank wait to eliminate them.
> Anyway, this time I didn't try to narrow it down too much. Instead
> my aim was more to reliably eliminate them without permamently disabling
> the underrun detection.
> 
> In any case, we can't get the bat stuff really working until we get
> the results to be stable, and these underruns are one big obstacle
> to that.

Agreed with Ville here, these are old platforms and we want stable BAT
results first. On gen9+ we should try a bit harder, or at least make a
note in JIRA that we need to look into this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Jani Nikula]

On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We get spurious PCH FIFO underruns if we enable the reporting too soon
> after enabling the crtc. Move it to be the last step, after the encoder
> enable. Additionally we need an extra vblank wait, otherwise we still
> get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> otherwise.
>
> For symmetry, disable the PCH underrun reporting as the first thing,
> just before encoder disable, when shutting down the crtc.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 99fb33f..d5cb899 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->pre_enable)
> @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	if (HAS_PCH_CPT(dev))
>  		cpt_verify_modeset(dev, intel_crtc->pipe);
> +
> +	if (intel_crtc->config->has_pch_encoder) {
> +		/* Must wait for vblank to avoid spurious PCH FIFO underruns */
> +		intel_wait_for_vblank(dev, pipe);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);

Nitpick, moving this within the if (has_pch_encoder) isn't documented in
the commit message. Does that change have an impact?

BR,
Jani.

> +	}
>  }
>  
>  /* IPS only exists on ULT machines and is tied to pipe A. */
> @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	u32 reg, temp;
>  
> +	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
>  	drm_crtc_vblank_off(crtc);
>  	assert_vblank_disabled(crtc);
>  
> -	if (intel_crtc->config->has_pch_encoder)
> -		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> -
>  	intel_disable_pipe(intel_crtc);
>  
>  	ironlake_pfit_disable(intel_crtc, false);

-- 
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 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Ville Syrjälä]

On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote:
> On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We get spurious PCH FIFO underruns if we enable the reporting too soon
> > after enabling the crtc. Move it to be the last step, after the encoder
> > enable. Additionally we need an extra vblank wait, otherwise we still
> > get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> > otherwise.
> >
> > For symmetry, disable the PCH underrun reporting as the first thing,
> > just before encoder disable, when shutting down the crtc.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 99fb33f..d5cb899 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >  	intel_crtc->active = true;
> >  
> >  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> > -	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> >  
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		if (encoder->pre_enable)
> > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	if (HAS_PCH_CPT(dev))
> >  		cpt_verify_modeset(dev, intel_crtc->pipe);
> > +
> > +	if (intel_crtc->config->has_pch_encoder) {
> > +		/* Must wait for vblank to avoid spurious PCH FIFO underruns */
> > +		intel_wait_for_vblank(dev, pipe);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> 
> Nitpick, moving this within the if (has_pch_encoder) isn't documented in
> the commit message. Does that change have an impact?

I don't much of a real concern here. I think the following might
happen (all on the same pipe):

1. enable PCH port
2. disable PCH port
3. PCH FIFO underrun just after we've re-enabled the PCH
   underrun reporting
4. enable port A
5. PCH FIFO underrun reporting isn't enabled anymore for this pipe

But since it's driving a non-PCH port anyway, that doesn't seem like
a huge worry. But I suppose I could change it to always enable PCH
FIFO underrun reporting even for port A. It should do no harm at least.

> 
> BR,
> Jani.
> 
> > +	}
> >  }
> >  
> >  /* IPS only exists on ULT machines and is tied to pipe A. */
> > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >  	int pipe = intel_crtc->pipe;
> >  	u32 reg, temp;
> >  
> > +	if (intel_crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		encoder->disable(encoder);
> >  
> >  	drm_crtc_vblank_off(crtc);
> >  	assert_vblank_disabled(crtc);
> >  
> > -	if (intel_crtc->config->has_pch_encoder)
> > -		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > -
> >  	intel_disable_pipe(intel_crtc);
> >  
> >  	ironlake_pfit_disable(intel_crtc, false);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Jani Nikula]

On Fri, 30 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote:
>> On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > We get spurious PCH FIFO underruns if we enable the reporting too soon
>> > after enabling the crtc. Move it to be the last step, after the encoder
>> > enable. Additionally we need an extra vblank wait, otherwise we still
>> > get the underruns. Presumably the pipe/fdi isn't yet fully up and running
>> > otherwise.
>> >
>> > For symmetry, disable the PCH underrun reporting as the first thing,
>> > just before encoder disable, when shutting down the crtc.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
>> >  1 file changed, 9 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 99fb33f..d5cb899 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>> >  	intel_crtc->active = true;
>> >  
>> >  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>> > -	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>> >  
>> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
>> >  		if (encoder->pre_enable)
>> > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>> >  
>> >  	if (HAS_PCH_CPT(dev))
>> >  		cpt_verify_modeset(dev, intel_crtc->pipe);
>> > +
>> > +	if (intel_crtc->config->has_pch_encoder) {
>> > +		/* Must wait for vblank to avoid spurious PCH FIFO underruns */
>> > +		intel_wait_for_vblank(dev, pipe);
>> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>> 
>> Nitpick, moving this within the if (has_pch_encoder) isn't documented in
>> the commit message. Does that change have an impact?
>
> I don't much of a real concern here. I think the following might
> happen (all on the same pipe):
>
> 1. enable PCH port
> 2. disable PCH port
> 3. PCH FIFO underrun just after we've re-enabled the PCH
>    underrun reporting
> 4. enable port A
> 5. PCH FIFO underrun reporting isn't enabled anymore for this pipe
>
> But since it's driving a non-PCH port anyway, that doesn't seem like
> a huge worry. But I suppose I could change it to always enable PCH
> FIFO underrun reporting even for port A. It should do no harm at least.

Mostly I just wanted this change documented in the commit message.

Jani.


>
>> 
>> BR,
>> Jani.
>> 
>> > +	}
>> >  }
>> >  
>> >  /* IPS only exists on ULT machines and is tied to pipe A. */
>> > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>> >  	int pipe = intel_crtc->pipe;
>> >  	u32 reg, temp;
>> >  
>> > +	if (intel_crtc->config->has_pch_encoder)
>> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
>> > +
>> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
>> >  		encoder->disable(encoder);
>> >  
>> >  	drm_crtc_vblank_off(crtc);
>> >  	assert_vblank_disabled(crtc);
>> >  
>> > -	if (intel_crtc->config->has_pch_encoder)
>> > -		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
>> > -
>> >  	intel_disable_pipe(intel_crtc);
>> >  
>> >  	ironlake_pfit_disable(intel_crtc, false);
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
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 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Daniel Vetter]

On Fri, Oct 30, 2015 at 02:08:51PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote:
> > On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > We get spurious PCH FIFO underruns if we enable the reporting too soon
> > > after enabling the crtc. Move it to be the last step, after the encoder
> > > enable. Additionally we need an extra vblank wait, otherwise we still
> > > get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> > > otherwise.
> > >
> > > For symmetry, disable the PCH underrun reporting as the first thing,
> > > just before encoder disable, when shutting down the crtc.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
> > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 99fb33f..d5cb899 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> > >  	intel_crtc->active = true;
> > >  
> > >  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> > > -	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> > >  
> > >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> > >  		if (encoder->pre_enable)
> > > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> > >  
> > >  	if (HAS_PCH_CPT(dev))
> > >  		cpt_verify_modeset(dev, intel_crtc->pipe);
> > > +
> > > +	if (intel_crtc->config->has_pch_encoder) {
> > > +		/* Must wait for vblank to avoid spurious PCH FIFO underruns */
> > > +		intel_wait_for_vblank(dev, pipe);
> > > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> > 
> > Nitpick, moving this within the if (has_pch_encoder) isn't documented in
> > the commit message. Does that change have an impact?
> 
> I don't much of a real concern here. I think the following might
> happen (all on the same pipe):
> 
> 1. enable PCH port
> 2. disable PCH port
> 3. PCH FIFO underrun just after we've re-enabled the PCH
>    underrun reporting
> 4. enable port A
> 5. PCH FIFO underrun reporting isn't enabled anymore for this pipe
> 
> But since it's driving a non-PCH port anyway, that doesn't seem like
> a huge worry. But I suppose I could change it to always enable PCH
> FIFO underrun reporting even for port A. It should do no harm at least.

Iirc we still fail to enable fifo underrun reporting with fastboot (should
fix this now since we update watermarks on takeover). That was the reason
to unconditionally enable fifo underruns even on the pch, to make it work
on platforms where the pch interrupt source is shared. See the pile of
hurt at the end of intel_sanitize_crtc.

I'd just keep it out of the if for now.
-Daniel

> 
> > 
> > BR,
> > Jani.
> > 
> > > +	}
> > >  }
> > >  
> > >  /* IPS only exists on ULT machines and is tied to pipe A. */
> > > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> > >  	int pipe = intel_crtc->pipe;
> > >  	u32 reg, temp;
> > >  
> > > +	if (intel_crtc->config->has_pch_encoder)
> > > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > > +
> > >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> > >  		encoder->disable(encoder);
> > >  
> > >  	drm_crtc_vblank_off(crtc);
> > >  	assert_vblank_disabled(crtc);
> > >  
> > > -	if (intel_crtc->config->has_pch_encoder)
> > > -		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > > -
> > >  	intel_disable_pipe(intel_crtc);
> > >  
> > >  	ironlake_pfit_disable(intel_crtc, false);
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[ville.syrjala]

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

We get spurious PCH FIFO underruns if we enable the reporting too soon
after enabling the crtc. Move it to be the last step, after the encoder
enable. Additionally we need an extra vblank wait, otherwise we still
get the underruns. Presumably the pipe/fdi isn't yet fully up and running
otherwise.

For symmetry, disable the PCH underrun reporting as the first thing,
just before encoder disable, when shutting down the crtc.

v2: Do the PCH underrun enable unconditionally (Jani, Daniel)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e4f5b1f..5ab39af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc->active = true;
 
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -4912,6 +4911,11 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	if (HAS_PCH_CPT(dev))
 		cpt_verify_modeset(dev, intel_crtc->pipe);
+
+	/* Must wait for vblank to avoid spurious PCH FIFO underruns */
+	if (intel_crtc->config->has_pch_encoder)
+		intel_wait_for_vblank(dev, pipe);
+	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
@@ -5040,15 +5044,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
 	drm_crtc_vblank_off(crtc);
 	assert_vblank_disabled(crtc);
 
-	if (intel_crtc->config->has_pch_encoder)
-		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
-
 	intel_disable_pipe(intel_crtc);
 
 	ironlake_pfit_disable(intel_crtc, false);
-- 
2.4.10

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

[PATCH 04/14] drm/i915: Enable PCH FIFO underruns later on HSW+

[ville.syrjala]

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

As we did for ILK/SNB/IVB, move the PCH FIFO underrun enable to happen
after the encoder enable on HSW+. And again, for symmetry, move the
the disable to happen before encoder disable.

I've left out the vblank wait before the enable here because I don't
know if it's needed or not. Actually I don't know if this entire
change is needed as I don't have a HSW/BDW with VGA output.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d5cb899..4fc3d24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4971,11 +4971,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 			encoder->pre_enable(encoder);
 	}
 
-	if (intel_crtc->config->has_pch_encoder) {
-		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
-						      true);
+	if (intel_crtc->config->has_pch_encoder)
 		dev_priv->display.fdi_link_train(crtc);
-	}
 
 	if (!is_dsi)
 		intel_ddi_enable_pipe_clock(intel_crtc);
@@ -5012,6 +5009,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_opregion_notify_encoder(encoder, true);
 	}
 
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+						      true);
+
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
 	hsw_workaround_pipe = pipe_config->hsw_workaround_pipe;
@@ -5096,6 +5097,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 	bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
 
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+						      false);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
@@ -5104,9 +5109,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	drm_crtc_vblank_off(crtc);
 	assert_vblank_disabled(crtc);
 
-	if (intel_crtc->config->has_pch_encoder)
-		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
-						      false);
 	intel_disable_pipe(intel_crtc);
 
 	if (intel_crtc->config->dp_encoder_is_mst)
-- 
2.4.10

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

Re: [PATCH 04/14] drm/i915: Enable PCH FIFO underruns later on HSW+

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> As we did for ILK/SNB/IVB, move the PCH FIFO underrun enable to happen
> after the encoder enable on HSW+. And again, for symmetry, move the
> the disable to happen before encoder disable.
> 
> I've left out the vblank wait before the enable here because I don't
> know if it's needed or not. Actually I don't know if this entire
> change is needed as I don't have a HSW/BDW with VGA output.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d5cb899..4fc3d24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4971,11 +4971,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  			encoder->pre_enable(encoder);
>  	}
>  
> -	if (intel_crtc->config->has_pch_encoder) {
> -		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> -						      true);
> +	if (intel_crtc->config->has_pch_encoder)
>  		dev_priv->display.fdi_link_train(crtc);
> -	}
>  
>  	if (!is_dsi)
>  		intel_ddi_enable_pipe_clock(intel_crtc);
> @@ -5012,6 +5009,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		intel_opregion_notify_encoder(encoder, true);
>  	}
>  
> +	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> +						      true);
> +
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
>  	hsw_workaround_pipe = pipe_config->hsw_workaround_pipe;
> @@ -5096,6 +5097,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  	bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
>  
> +	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> +						      false);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		intel_opregion_notify_encoder(encoder, false);
>  		encoder->disable(encoder);
> @@ -5104,9 +5109,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	drm_crtc_vblank_off(crtc);
>  	assert_vblank_disabled(crtc);
>  
> -	if (intel_crtc->config->has_pch_encoder)
> -		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> -						      false);
>  	intel_disable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->dp_encoder_is_mst)
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/14] drm/i915: Re-enable PCH FIO underrun reporting after pipe has been disabled

[ville.syrjala]

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

Some hardware (IVB/HSW and CPT/PPT) have a shared error interrupt for
all the relevant underrun bits, so in order to keep the error interrupt
enabled, we need to have underrun reporting enabled on all PCH
transocders. Currently we leave the underrun reporting disabled when
the pipe is off, which means we won't get any underrun interrupts
when only a subset of the pipes are active.

Fix the problem by re-enabling the underrun reporting after the pipe has
been disabled. And to avoid the spurious underruns during pipe enable,
disable the underrun reporting before embarking on the pipe enable
sequence. So this way we have the error reporting disabled while
running through the modeset sequence.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4fc3d24..c7cd9f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4857,6 +4857,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
+
+	if (intel_crtc->config->has_pch_encoder)
 		intel_prepare_shared_dpll(intel_crtc);
 
 	if (intel_crtc->config->has_dp_encoder)
@@ -4939,6 +4942,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (WARN_ON(intel_crtc->active))
 		return;
 
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+						      false);
+
 	if (intel_crtc_to_shared_dpll(intel_crtc))
 		intel_enable_shared_dpll(intel_crtc);
 
@@ -5086,6 +5093,9 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 		ironlake_fdi_pll_disable(intel_crtc);
 	}
+
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5133,6 +5143,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
+
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+						      true);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
-- 
2.4.10

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

Re: [PATCH 05/14] drm/i915: Re-enable PCH FIO underrun reporting after pipe has been disabled

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Some hardware (IVB/HSW and CPT/PPT) have a shared error interrupt for
> all the relevant underrun bits, so in order to keep the error interrupt
> enabled, we need to have underrun reporting enabled on all PCH
> transocders. Currently we leave the underrun reporting disabled when
> the pipe is off, which means we won't get any underrun interrupts
> when only a subset of the pipes are active.
> 
> Fix the problem by re-enabling the underrun reporting after the pipe has
> been disabled. And to avoid the spurious underruns during pipe enable,
> disable the underrun reporting before embarking on the pipe enable
> sequence. So this way we have the error reporting disabled while
> running through the modeset sequence.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4fc3d24..c7cd9f7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4857,6 +4857,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  		return;
>  
>  	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> +
> +	if (intel_crtc->config->has_pch_encoder)
>  		intel_prepare_shared_dpll(intel_crtc);

I guess these could be combined under the conditional, but no biggie.

>  
>  	if (intel_crtc->config->has_dp_encoder)
> @@ -4939,6 +4942,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	if (WARN_ON(intel_crtc->active))
>  		return;
>  
> +	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> +						      false);
> +
>  	if (intel_crtc_to_shared_dpll(intel_crtc))
>  		intel_enable_shared_dpll(intel_crtc);
>  
> @@ -5086,6 +5093,9 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  
>  		ironlake_fdi_pll_disable(intel_crtc);
>  	}
> +
> +	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
>  }
>  
>  static void haswell_crtc_disable(struct drm_crtc *crtc)
> @@ -5133,6 +5143,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
> +
> +	if (intel_crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> +						      true);
>  }
>  
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/14] drm/i915: Re-enable PCH FIO underrun reporting after pipe has been disabled

[Ville Syrjälä]

On Thu, Oct 29, 2015 at 12:36:34PM -0700, Jesse Barnes wrote:
> On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Some hardware (IVB/HSW and CPT/PPT) have a shared error interrupt for
> > all the relevant underrun bits, so in order to keep the error interrupt
> > enabled, we need to have underrun reporting enabled on all PCH
> > transocders. Currently we leave the underrun reporting disabled when
> > the pipe is off, which means we won't get any underrun interrupts
> > when only a subset of the pipes are active.
> > 
> > Fix the problem by re-enabling the underrun reporting after the pipe has
> > been disabled. And to avoid the spurious underruns during pipe enable,
> > disable the underrun reporting before embarking on the pipe enable
> > sequence. So this way we have the error reporting disabled while
> > running through the modeset sequence.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4fc3d24..c7cd9f7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4857,6 +4857,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >  		return;
> >  
> >  	if (intel_crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> > +	if (intel_crtc->config->has_pch_encoder)
> >  		intel_prepare_shared_dpll(intel_crtc);
> 
> I guess these could be combined under the conditional, but no biggie.

I did notice the same thing just before sending the patch, but then I
convinced myself that having all the pch underrun enable/disable calls
as clearly separate steps is perhaps nicer, and so didn't bother
changing it.

> 
> >  
> >  	if (intel_crtc->config->has_dp_encoder)
> > @@ -4939,6 +4942,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >  	if (WARN_ON(intel_crtc->active))
> >  		return;
> >  
> > +	if (intel_crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> > +						      false);
> > +
> >  	if (intel_crtc_to_shared_dpll(intel_crtc))
> >  		intel_enable_shared_dpll(intel_crtc);
> >  
> > @@ -5086,6 +5093,9 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >  
> >  		ironlake_fdi_pll_disable(intel_crtc);
> >  	}
> > +
> > +	if (intel_crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> >  }
> >  
> >  static void haswell_crtc_disable(struct drm_crtc *crtc)
> > @@ -5133,6 +5143,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		if (encoder->post_disable)
> >  			encoder->post_disable(encoder);
> > +
> > +	if (intel_crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> > +						      true);
> >  }
> >  
> >  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> > 
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 05/14] drm/i915: Re-enable PCH FIO underrun reporting after pipe has been disabled

[ville.syrjala]

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

Some hardware (IVB/HSW and CPT/PPT) have a shared error interrupt for
all the relevant underrun bits, so in order to keep the error interrupt
enabled, we need to have underrun reporting enabled on all PCH
transocders. Currently we leave the underrun reporting disabled when
the pipe is off, which means we won't get any underrun interrupts
when only a subset of the pipes are active.

Fix the problem by re-enabling the underrun reporting after the pipe has
been disabled. And to avoid the spurious underruns during pipe enable,
disable the underrun reporting before embarking on the pipe enable
sequence. So this way we have the error reporting disabled while
running through the modeset sequence.

v2: Re-enable PCH FIFO underrun reporting unconditionally on pre-HSW

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a2b5327..92f8079 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4857,6 +4857,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
+
+	if (intel_crtc->config->has_pch_encoder)
 		intel_prepare_shared_dpll(intel_crtc);
 
 	if (intel_crtc->config->has_dp_encoder)
@@ -4938,6 +4941,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (WARN_ON(intel_crtc->active))
 		return;
 
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+						      false);
+
 	if (intel_crtc_to_shared_dpll(intel_crtc))
 		intel_enable_shared_dpll(intel_crtc);
 
@@ -5085,6 +5092,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 		ironlake_fdi_pll_disable(intel_crtc);
 	}
+
+	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5132,6 +5141,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
+
+	if (intel_crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
+						      true);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
-- 
2.4.10

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

[PATCH 06/14] drm/i915: Check for FIFO underruns after modeset on IVB/HSW and CPT/PPT

[ville.syrjala]

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

Due to the shared error interrupt on IVB/HSW and CPT/PPT we may not
always get an interrupt on a FIFO underrun. But we can always do an
explicit check (like we do on GMCH platforms that have no underrun
interrupt).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c       |   6 +-
 drivers/gpu/drm/i915/intel_drv.h           |   3 +-
 drivers/gpu/drm/i915/intel_fifo_underrun.c | 121 ++++++++++++++++++++++++-----
 3 files changed, 105 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c7cd9f7..e820147 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4719,9 +4719,9 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	if (IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 
-	/* Underruns don't raise interrupts, so check manually. */
-	if (HAS_GMCH_DISPLAY(dev))
-		i9xx_check_fifo_underruns(dev_priv);
+	/* Underruns don't always raise interrupts, so check manually. */
+	intel_check_cpu_fifo_underruns(dev_priv);
+	intel_check_pch_fifo_underruns(dev_priv);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1a3bbdc..72cc272 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -959,7 +959,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 					 enum pipe pipe);
 void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 					 enum transcoder pch_transcoder);
-void i9xx_check_fifo_underruns(struct drm_i915_private *dev_priv);
+void intel_check_cpu_fifo_underruns(struct drm_i915_private *dev_priv);
+void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv);
 
 /* i915_irq.c */
 void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 54daa66..a546fc3 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -85,37 +85,28 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
 }
 
 /**
- * i9xx_check_fifo_underruns - check for fifo underruns
- * @dev_priv: i915 device instance
+ * intel_check_cpu_fifo_underruns - check for fifo underruns
+ * @crtc: pipe
  *
  * This function checks for fifo underruns on GMCH platforms. This needs to be
  * done manually on modeset to make sure that we catch all underruns since they
  * do not generate an interrupt by themselves on these platforms.
  */
-void i9xx_check_fifo_underruns(struct drm_i915_private *dev_priv)
+static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
 {
-	struct intel_crtc *crtc;
-
-	spin_lock_irq(&dev_priv->irq_lock);
-
-	for_each_intel_crtc(dev_priv->dev, crtc) {
-		u32 reg = PIPESTAT(crtc->pipe);
-		u32 pipestat;
-
-		if (crtc->cpu_fifo_underrun_disabled)
-			continue;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 reg = PIPESTAT(crtc->pipe);
+	u32 pipestat = I915_READ(reg) & 0xffff0000;
 
-		pipestat = I915_READ(reg) & 0xffff0000;
-		if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
-			continue;
+	assert_spin_locked(&dev_priv->irq_lock);
 
-		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
-		POSTING_READ(reg);
+	if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
+		return;
 
-		DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
-	}
+	I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+	POSTING_READ(reg);
 
-	spin_unlock_irq(&dev_priv->irq_lock);
+	DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
 }
 
 static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
@@ -150,6 +141,23 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
 		ironlake_disable_display_irq(dev_priv, bit);
 }
 
+static void ivybridge_check_fifo_underruns(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	uint32_t err_int = I915_READ(GEN7_ERR_INT);
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	if ((err_int & ERR_INT_FIFO_UNDERRUN(pipe)) == 0)
+		return;
+
+	I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
+	POSTING_READ(GEN7_ERR_INT);
+
+	DRM_ERROR("fifo underrun on pipe %c\n", pipe_name(pipe));
+}
+
 static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 						  enum pipe pipe,
 						  bool enable, bool old)
@@ -202,6 +210,24 @@ static void ibx_set_fifo_underrun_reporting(struct drm_device *dev,
 		ibx_disable_display_interrupt(dev_priv, bit);
 }
 
+static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum transcoder pch_transcoder = (enum transcoder) crtc->pipe;
+	uint32_t serr_int = I915_READ(SERR_INT);
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
+		return;
+
+	I915_WRITE(SERR_INT, SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
+	POSTING_READ(SERR_INT);
+
+	DRM_ERROR("pch fifo underrun on pch transcoder %c\n",
+		  transcoder_name(pch_transcoder));
+}
+
 static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 					    enum transcoder pch_transcoder,
 					    bool enable, bool old)
@@ -375,3 +401,56 @@ void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 		DRM_ERROR("PCH transcoder %c FIFO underrun\n",
 			  transcoder_name(pch_transcoder));
 }
+
+/**
+ * intel_check_cpu_fifo_underruns - check for CPU fifo underruns immediately
+ * @dev_priv: i915 device instance
+ *
+ * Check for CPU fifo underruns immediately. Useful on IVB/HSW where the shared
+ * error interrupt may have been disabled, and so CPU fifo underruns won't
+ * necessarily raise an interrupt, and on GMCH platforms where underruns never
+ * raise an interrupt.
+ */
+void intel_check_cpu_fifo_underruns(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	for_each_intel_crtc(dev_priv->dev, crtc) {
+		if (crtc->cpu_fifo_underrun_disabled)
+			continue;
+
+		if (HAS_GMCH_DISPLAY(dev_priv))
+			i9xx_check_fifo_underruns(crtc);
+		else if (IS_GEN7(dev_priv))
+			ivybridge_check_fifo_underruns(crtc);
+	}
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+/**
+ * intel_check_pch_fifo_underruns - check for PCH fifo underruns immediately
+ * @dev_priv: i915 device instance
+ *
+ * Check for PCH fifo underruns immediately. Useful on CPT/PPT where the shared
+ * error interrupt may have been disabled, and so PCH fifo underruns won't
+ * necessarily raise an interrupt.
+ */
+void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	for_each_intel_crtc(dev_priv->dev, crtc) {
+		if (crtc->pch_fifo_underrun_disabled)
+			continue;
+
+		if (HAS_PCH_CPT(dev_priv))
+			cpt_check_pch_fifo_underruns(crtc);
+	}
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
-- 
2.4.10

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

Re: [PATCH 06/14] drm/i915: Check for FIFO underruns after modeset on IVB/HSW and CPT/PPT

[Daniel Vetter]

On Thu, Oct 29, 2015 at 09:25:55PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Due to the shared error interrupt on IVB/HSW and CPT/PPT we may not
> always get an interrupt on a FIFO underrun. But we can always do an
> explicit check (like we do on GMCH platforms that have no underrun
> interrupt).
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c       |   6 +-
>  drivers/gpu/drm/i915/intel_drv.h           |   3 +-
>  drivers/gpu/drm/i915/intel_fifo_underrun.c | 121 ++++++++++++++++++++++++-----
>  3 files changed, 105 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c7cd9f7..e820147 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4719,9 +4719,9 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  	if (IS_GEN2(dev))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  
> -	/* Underruns don't raise interrupts, so check manually. */
> -	if (HAS_GMCH_DISPLAY(dev))
> -		i9xx_check_fifo_underruns(dev_priv);
> +	/* Underruns don't always raise interrupts, so check manually. */
> +	intel_check_cpu_fifo_underruns(dev_priv);
> +	intel_check_pch_fifo_underruns(dev_priv);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1a3bbdc..72cc272 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -959,7 +959,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  					 enum pipe pipe);
>  void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  					 enum transcoder pch_transcoder);
> -void i9xx_check_fifo_underruns(struct drm_i915_private *dev_priv);
> +void intel_check_cpu_fifo_underruns(struct drm_i915_private *dev_priv);
> +void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv);
>  
>  /* i915_irq.c */
>  void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 54daa66..a546fc3 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -85,37 +85,28 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
>  }
>  
>  /**
> - * i9xx_check_fifo_underruns - check for fifo underruns
> - * @dev_priv: i915 device instance
> + * intel_check_cpu_fifo_underruns - check for fifo underruns
> + * @crtc: pipe
>   *
>   * This function checks for fifo underruns on GMCH platforms. This needs to be
>   * done manually on modeset to make sure that we catch all underruns since they
>   * do not generate an interrupt by themselves on these platforms.
>   */

Stale kerneldoc above, just delete it. With that fixed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -void i9xx_check_fifo_underruns(struct drm_i915_private *dev_priv)
> +static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>  {
> -	struct intel_crtc *crtc;
> -
> -	spin_lock_irq(&dev_priv->irq_lock);
> -
> -	for_each_intel_crtc(dev_priv->dev, crtc) {
> -		u32 reg = PIPESTAT(crtc->pipe);
> -		u32 pipestat;
> -
> -		if (crtc->cpu_fifo_underrun_disabled)
> -			continue;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 reg = PIPESTAT(crtc->pipe);
> +	u32 pipestat = I915_READ(reg) & 0xffff0000;
>  
> -		pipestat = I915_READ(reg) & 0xffff0000;
> -		if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> -			continue;
> +	assert_spin_locked(&dev_priv->irq_lock);
>  
> -		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> -		POSTING_READ(reg);
> +	if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> +		return;
>  
> -		DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
> -	}
> +	I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> +	POSTING_READ(reg);
>  
> -	spin_unlock_irq(&dev_priv->irq_lock);
> +	DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
>  }
>  
>  static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
> @@ -150,6 +141,23 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
>  		ironlake_disable_display_irq(dev_priv, bit);
>  }
>  
> +static void ivybridge_check_fifo_underruns(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	uint32_t err_int = I915_READ(GEN7_ERR_INT);
> +
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	if ((err_int & ERR_INT_FIFO_UNDERRUN(pipe)) == 0)
> +		return;
> +
> +	I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
> +	POSTING_READ(GEN7_ERR_INT);
> +
> +	DRM_ERROR("fifo underrun on pipe %c\n", pipe_name(pipe));
> +}
> +
>  static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
>  						  enum pipe pipe,
>  						  bool enable, bool old)
> @@ -202,6 +210,24 @@ static void ibx_set_fifo_underrun_reporting(struct drm_device *dev,
>  		ibx_disable_display_interrupt(dev_priv, bit);
>  }
>  
> +static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum transcoder pch_transcoder = (enum transcoder) crtc->pipe;
> +	uint32_t serr_int = I915_READ(SERR_INT);
> +
> +	assert_spin_locked(&dev_priv->irq_lock);
> +
> +	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
> +		return;
> +
> +	I915_WRITE(SERR_INT, SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> +	POSTING_READ(SERR_INT);
> +
> +	DRM_ERROR("pch fifo underrun on pch transcoder %c\n",
> +		  transcoder_name(pch_transcoder));
> +}
> +
>  static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>  					    enum transcoder pch_transcoder,
>  					    bool enable, bool old)
> @@ -375,3 +401,56 @@ void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  		DRM_ERROR("PCH transcoder %c FIFO underrun\n",
>  			  transcoder_name(pch_transcoder));
>  }
> +
> +/**
> + * intel_check_cpu_fifo_underruns - check for CPU fifo underruns immediately
> + * @dev_priv: i915 device instance
> + *
> + * Check for CPU fifo underruns immediately. Useful on IVB/HSW where the shared
> + * error interrupt may have been disabled, and so CPU fifo underruns won't
> + * necessarily raise an interrupt, and on GMCH platforms where underruns never
> + * raise an interrupt.
> + */
> +void intel_check_cpu_fifo_underruns(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_crtc *crtc;
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +
> +	for_each_intel_crtc(dev_priv->dev, crtc) {
> +		if (crtc->cpu_fifo_underrun_disabled)
> +			continue;
> +
> +		if (HAS_GMCH_DISPLAY(dev_priv))
> +			i9xx_check_fifo_underruns(crtc);
> +		else if (IS_GEN7(dev_priv))
> +			ivybridge_check_fifo_underruns(crtc);
> +	}
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
> +/**
> + * intel_check_pch_fifo_underruns - check for PCH fifo underruns immediately
> + * @dev_priv: i915 device instance
> + *
> + * Check for PCH fifo underruns immediately. Useful on CPT/PPT where the shared
> + * error interrupt may have been disabled, and so PCH fifo underruns won't
> + * necessarily raise an interrupt.
> + */
> +void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_crtc *crtc;
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +
> +	for_each_intel_crtc(dev_priv->dev, crtc) {
> +		if (crtc->pch_fifo_underrun_disabled)
> +			continue;
> +
> +		if (HAS_PCH_CPT(dev_priv))
> +			cpt_check_pch_fifo_underruns(crtc);
> +	}
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> -- 
> 2.4.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 06/14] drm/i915: Check for FIFO underruns after modeset on IVB/HSW and CPT/PPT

[ville.syrjala]

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

Due to the shared error interrupt on IVB/HSW and CPT/PPT we may not
always get an interrupt on a FIFO underrun. But we can always do an
explicit check (like we do on GMCH platforms that have no underrun
interrupt).

v2: Drop stale kerneldoc for i9xx_check_fifo_underruns() (Daniel)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c       |   6 +-
 drivers/gpu/drm/i915/intel_drv.h           |   3 +-
 drivers/gpu/drm/i915/intel_fifo_underrun.c | 125 ++++++++++++++++++++++-------
 3 files changed, 103 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92f8079..4066f7c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4719,9 +4719,9 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	if (IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 
-	/* Underruns don't raise interrupts, so check manually. */
-	if (HAS_GMCH_DISPLAY(dev))
-		i9xx_check_fifo_underruns(dev_priv);
+	/* Underruns don't always raise interrupts, so check manually. */
+	intel_check_cpu_fifo_underruns(dev_priv);
+	intel_check_pch_fifo_underruns(dev_priv);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1a3bbdc..72cc272 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -959,7 +959,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 					 enum pipe pipe);
 void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 					 enum transcoder pch_transcoder);
-void i9xx_check_fifo_underruns(struct drm_i915_private *dev_priv);
+void intel_check_cpu_fifo_underruns(struct drm_i915_private *dev_priv);
+void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv);
 
 /* i915_irq.c */
 void gen5_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 54daa66..af906c7 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -84,38 +84,21 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
 	return true;
 }
 
-/**
- * i9xx_check_fifo_underruns - check for fifo underruns
- * @dev_priv: i915 device instance
- *
- * This function checks for fifo underruns on GMCH platforms. This needs to be
- * done manually on modeset to make sure that we catch all underruns since they
- * do not generate an interrupt by themselves on these platforms.
- */
-void i9xx_check_fifo_underruns(struct drm_i915_private *dev_priv)
+static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
 {
-	struct intel_crtc *crtc;
-
-	spin_lock_irq(&dev_priv->irq_lock);
-
-	for_each_intel_crtc(dev_priv->dev, crtc) {
-		u32 reg = PIPESTAT(crtc->pipe);
-		u32 pipestat;
-
-		if (crtc->cpu_fifo_underrun_disabled)
-			continue;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 reg = PIPESTAT(crtc->pipe);
+	u32 pipestat = I915_READ(reg) & 0xffff0000;
 
-		pipestat = I915_READ(reg) & 0xffff0000;
-		if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
-			continue;
+	assert_spin_locked(&dev_priv->irq_lock);
 
-		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
-		POSTING_READ(reg);
+	if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
+		return;
 
-		DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
-	}
+	I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+	POSTING_READ(reg);
 
-	spin_unlock_irq(&dev_priv->irq_lock);
+	DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
 }
 
 static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
@@ -150,6 +133,23 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
 		ironlake_disable_display_irq(dev_priv, bit);
 }
 
+static void ivybridge_check_fifo_underruns(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	uint32_t err_int = I915_READ(GEN7_ERR_INT);
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	if ((err_int & ERR_INT_FIFO_UNDERRUN(pipe)) == 0)
+		return;
+
+	I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
+	POSTING_READ(GEN7_ERR_INT);
+
+	DRM_ERROR("fifo underrun on pipe %c\n", pipe_name(pipe));
+}
+
 static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 						  enum pipe pipe,
 						  bool enable, bool old)
@@ -202,6 +202,24 @@ static void ibx_set_fifo_underrun_reporting(struct drm_device *dev,
 		ibx_disable_display_interrupt(dev_priv, bit);
 }
 
+static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum transcoder pch_transcoder = (enum transcoder) crtc->pipe;
+	uint32_t serr_int = I915_READ(SERR_INT);
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
+		return;
+
+	I915_WRITE(SERR_INT, SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
+	POSTING_READ(SERR_INT);
+
+	DRM_ERROR("pch fifo underrun on pch transcoder %c\n",
+		  transcoder_name(pch_transcoder));
+}
+
 static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 					    enum transcoder pch_transcoder,
 					    bool enable, bool old)
@@ -375,3 +393,56 @@ void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 		DRM_ERROR("PCH transcoder %c FIFO underrun\n",
 			  transcoder_name(pch_transcoder));
 }
+
+/**
+ * intel_check_cpu_fifo_underruns - check for CPU fifo underruns immediately
+ * @dev_priv: i915 device instance
+ *
+ * Check for CPU fifo underruns immediately. Useful on IVB/HSW where the shared
+ * error interrupt may have been disabled, and so CPU fifo underruns won't
+ * necessarily raise an interrupt, and on GMCH platforms where underruns never
+ * raise an interrupt.
+ */
+void intel_check_cpu_fifo_underruns(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	for_each_intel_crtc(dev_priv->dev, crtc) {
+		if (crtc->cpu_fifo_underrun_disabled)
+			continue;
+
+		if (HAS_GMCH_DISPLAY(dev_priv))
+			i9xx_check_fifo_underruns(crtc);
+		else if (IS_GEN7(dev_priv))
+			ivybridge_check_fifo_underruns(crtc);
+	}
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
+/**
+ * intel_check_pch_fifo_underruns - check for PCH fifo underruns immediately
+ * @dev_priv: i915 device instance
+ *
+ * Check for PCH fifo underruns immediately. Useful on CPT/PPT where the shared
+ * error interrupt may have been disabled, and so PCH fifo underruns won't
+ * necessarily raise an interrupt.
+ */
+void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+
+	for_each_intel_crtc(dev_priv->dev, crtc) {
+		if (crtc->pch_fifo_underrun_disabled)
+			continue;
+
+		if (HAS_PCH_CPT(dev_priv))
+			cpt_check_pch_fifo_underruns(crtc);
+	}
+
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
-- 
2.4.10

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

[PATCH 07/14] drm/i915: Check for CPT and not !IBX in ironlake_disable_pch_transcoder()

[ville.syrjala]

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

ironlake_enaable_pch_transcoder() checks for CPT to see if it should
enable the timing override chicken bit, but
ironlake_disable_pch_transcoder() checks for !IBX to see if it should
clear the same bit. Change ironlake_disable_pch_transcoder() to check
for CPT as well to keep the two sides consistent.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e820147..0d87a4e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2068,7 +2068,7 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
 	if (wait_for((I915_READ(reg) & TRANS_STATE_ENABLE) == 0, 50))
 		DRM_ERROR("failed to disable transcoder %c\n", pipe_name(pipe));
 
-	if (!HAS_PCH_IBX(dev)) {
+	if (HAS_PCH_CPT(dev)) {
 		/* Workaround: Clear the timing override chicken bit again. */
 		reg = TRANS_CHICKEN2(pipe);
 		val = I915_READ(reg);
-- 
2.4.10

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

Re: [PATCH 07/14] drm/i915: Check for CPT and not !IBX in ironlake_disable_pch_transcoder()

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> ironlake_enaable_pch_transcoder() checks for CPT to see if it should
> enable the timing override chicken bit, but
> ironlake_disable_pch_transcoder() checks for !IBX to see if it should
> clear the same bit. Change ironlake_disable_pch_transcoder() to check
> for CPT as well to keep the two sides consistent.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e820147..0d87a4e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2068,7 +2068,7 @@ static void ironlake_disable_pch_transcoder(struct drm_i915_private *dev_priv,
>  	if (wait_for((I915_READ(reg) & TRANS_STATE_ENABLE) == 0, 50))
>  		DRM_ERROR("failed to disable transcoder %c\n", pipe_name(pipe));
>  
> -	if (!HAS_PCH_IBX(dev)) {
> +	if (HAS_PCH_CPT(dev)) {
>  		/* Workaround: Clear the timing override chicken bit again. */
>  		reg = TRANS_CHICKEN2(pipe);
>  		val = I915_READ(reg);
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/14] drm/i915: Disable FIFO underrun reporting around IBX transcoder B workaround

[ville.syrjala]

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

Doing the IBX transcoder B workaround causes underruns on
pipe/transcoder A. Just hide them by disabling underrun reporting for
pipe A around the workaround.

It might be possible to avoid the underruns by moving the workaround
to be applied only when enabling pipe A. But I was too lazy to try it
right now, and the current method has been proven to work, so didn't
want to change it too hastily.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   | 11 +++++++++++
 drivers/gpu/drm/i915/intel_drv.h  |  9 +++++++++
 drivers/gpu/drm/i915/intel_hdmi.c | 11 +++++++++++
 drivers/gpu/drm/i915/intel_sdvo.c | 11 +++++++++++
 4 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..4a0fb63 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3957,6 +3957,13 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	 * matching HDMI port to be enabled on transcoder A.
 	 */
 	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
+		/*
+		 * We get CPU/PCH FIFO underruns on the other pipe when
+		 * doing the workaround. Sweep them under the rug.
+		 */
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
 		/* always enable with pattern 1 (as per spec) */
 		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
 		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
@@ -3966,6 +3973,10 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 		DP &= ~DP_PORT_EN;
 		I915_WRITE(intel_dp->output_reg, DP);
 		POSTING_READ(intel_dp->output_reg);
+
+		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 	}
 
 	msleep(intel_dp->panel_power_down_delay);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 72cc272..35f1457 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1073,6 +1073,15 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
 {
 	drm_wait_one_vblank(dev, pipe);
 }
+static inline void
+intel_wait_for_vblank_if_active(struct drm_device *dev, int pipe)
+{
+	const struct intel_crtc *crtc =
+		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+
+	if (crtc->active)
+		intel_wait_for_vblank(dev, pipe);
+}
 int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
 			 struct intel_digital_port *dport,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 013bd7d..bccbe70 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1108,6 +1108,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 	 * matching DP port to be enabled on transcoder A.
 	 */
 	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
+		/*
+		 * We get CPU/PCH FIFO underruns on the other pipe when
+		 * doing the workaround. Sweep them under the rug.
+		 */
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
 		temp &= ~SDVO_PIPE_B_SELECT;
 		temp |= SDVO_ENABLE;
 		/*
@@ -1122,6 +1129,10 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 		temp &= ~SDVO_ENABLE;
 		I915_WRITE(intel_hdmi->hdmi_reg, temp);
 		POSTING_READ(intel_hdmi->hdmi_reg);
+
+		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 	}
 
 	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index c42b636..267e6cb 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1464,12 +1464,23 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
 	 * matching DP port to be enabled on transcoder A.
 	 */
 	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
+		/*
+		 * We get CPU/PCH FIFO underruns on the other pipe when
+		 * doing the workaround. Sweep them under the rug.
+		 */
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
 		temp &= ~SDVO_PIPE_B_SELECT;
 		temp |= SDVO_ENABLE;
 		intel_sdvo_write_sdvox(intel_sdvo, temp);
 
 		temp &= ~SDVO_ENABLE;
 		intel_sdvo_write_sdvox(intel_sdvo, temp);
+
+		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 	}
 }
 
-- 
2.4.10

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

Re: [PATCH 08/14] drm/i915: Disable FIFO underrun reporting around IBX transcoder B workaround

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Doing the IBX transcoder B workaround causes underruns on
> pipe/transcoder A. Just hide them by disabling underrun reporting for
> pipe A around the workaround.
> 
> It might be possible to avoid the underruns by moving the workaround
> to be applied only when enabling pipe A. But I was too lazy to try it
> right now, and the current method has been proven to work, so didn't
> want to change it too hastily.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  9 +++++++++
>  drivers/gpu/drm/i915/intel_hdmi.c | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_sdvo.c | 11 +++++++++++
>  4 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8287df4..4a0fb63 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3957,6 +3957,13 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	 * matching HDMI port to be enabled on transcoder A.
>  	 */
>  	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
> +		/*
> +		 * We get CPU/PCH FIFO underruns on the other pipe when
> +		 * doing the workaround. Sweep them under the rug.
> +		 */
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +
>  		/* always enable with pattern 1 (as per spec) */
>  		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
>  		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
> @@ -3966,6 +3973,10 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  		DP &= ~DP_PORT_EN;
>  		I915_WRITE(intel_dp->output_reg, DP);
>  		POSTING_READ(intel_dp->output_reg);
> +
> +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  
>  	msleep(intel_dp->panel_power_down_delay);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 72cc272..35f1457 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1073,6 +1073,15 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  {
>  	drm_wait_one_vblank(dev, pipe);
>  }
> +static inline void
> +intel_wait_for_vblank_if_active(struct drm_device *dev, int pipe)
> +{
> +	const struct intel_crtc *crtc =
> +		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +
> +	if (crtc->active)
> +		intel_wait_for_vblank(dev, pipe);
> +}
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
>  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>  			 struct intel_digital_port *dport,
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 013bd7d..bccbe70 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1108,6 +1108,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  	 * matching DP port to be enabled on transcoder A.
>  	 */
>  	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
> +		/*
> +		 * We get CPU/PCH FIFO underruns on the other pipe when
> +		 * doing the workaround. Sweep them under the rug.
> +		 */
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +
>  		temp &= ~SDVO_PIPE_B_SELECT;
>  		temp |= SDVO_ENABLE;
>  		/*
> @@ -1122,6 +1129,10 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  		temp &= ~SDVO_ENABLE;
>  		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  
>  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index c42b636..267e6cb 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1464,12 +1464,23 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
>  	 * matching DP port to be enabled on transcoder A.
>  	 */
>  	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
> +		/*
> +		 * We get CPU/PCH FIFO underruns on the other pipe when
> +		 * doing the workaround. Sweep them under the rug.
> +		 */
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +
>  		temp &= ~SDVO_PIPE_B_SELECT;
>  		temp |= SDVO_ENABLE;
>  		intel_sdvo_write_sdvox(intel_sdvo, temp);
>  
>  		temp &= ~SDVO_ENABLE;
>  		intel_sdvo_write_sdvox(intel_sdvo, temp);
> +
> +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  }
>  
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/14] drm/i915: Disable FIFO underrun reporting around IBX transcoder B workaround

[Jani Nikula]

On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Doing the IBX transcoder B workaround causes underruns on
> pipe/transcoder A. Just hide them by disabling underrun reporting for
> pipe A around the workaround.
>
> It might be possible to avoid the underruns by moving the workaround
> to be applied only when enabling pipe A. But I was too lazy to try it
> right now, and the current method has been proven to work, so didn't
> want to change it too hastily.

Is it possible this enables underrun reporting on pipe A even if it
wasn't enabled before?

BR,
Jani.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  9 +++++++++
>  drivers/gpu/drm/i915/intel_hdmi.c | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_sdvo.c | 11 +++++++++++
>  4 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8287df4..4a0fb63 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3957,6 +3957,13 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	 * matching HDMI port to be enabled on transcoder A.
>  	 */
>  	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
> +		/*
> +		 * We get CPU/PCH FIFO underruns on the other pipe when
> +		 * doing the workaround. Sweep them under the rug.
> +		 */
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +
>  		/* always enable with pattern 1 (as per spec) */
>  		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
>  		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
> @@ -3966,6 +3973,10 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  		DP &= ~DP_PORT_EN;
>  		I915_WRITE(intel_dp->output_reg, DP);
>  		POSTING_READ(intel_dp->output_reg);
> +
> +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  
>  	msleep(intel_dp->panel_power_down_delay);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 72cc272..35f1457 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1073,6 +1073,15 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  {
>  	drm_wait_one_vblank(dev, pipe);
>  }
> +static inline void
> +intel_wait_for_vblank_if_active(struct drm_device *dev, int pipe)
> +{
> +	const struct intel_crtc *crtc =
> +		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +
> +	if (crtc->active)
> +		intel_wait_for_vblank(dev, pipe);
> +}
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
>  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>  			 struct intel_digital_port *dport,
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 013bd7d..bccbe70 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1108,6 +1108,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  	 * matching DP port to be enabled on transcoder A.
>  	 */
>  	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
> +		/*
> +		 * We get CPU/PCH FIFO underruns on the other pipe when
> +		 * doing the workaround. Sweep them under the rug.
> +		 */
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +
>  		temp &= ~SDVO_PIPE_B_SELECT;
>  		temp |= SDVO_ENABLE;
>  		/*
> @@ -1122,6 +1129,10 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>  		temp &= ~SDVO_ENABLE;
>  		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>  		POSTING_READ(intel_hdmi->hdmi_reg);
> +
> +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  
>  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index c42b636..267e6cb 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1464,12 +1464,23 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
>  	 * matching DP port to be enabled on transcoder A.
>  	 */
>  	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
> +		/*
> +		 * We get CPU/PCH FIFO underruns on the other pipe when
> +		 * doing the workaround. Sweep them under the rug.
> +		 */
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> +
>  		temp &= ~SDVO_PIPE_B_SELECT;
>  		temp |= SDVO_ENABLE;
>  		intel_sdvo_write_sdvox(intel_sdvo, temp);
>  
>  		temp &= ~SDVO_ENABLE;
>  		intel_sdvo_write_sdvox(intel_sdvo, temp);
> +
> +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
>  	}
>  }

-- 
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 08/14] drm/i915: Disable FIFO underrun reporting around IBX transcoder B workaround

[Ville Syrjälä]

On Fri, Oct 30, 2015 at 12:11:45PM +0200, Jani Nikula wrote:
> On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Doing the IBX transcoder B workaround causes underruns on
> > pipe/transcoder A. Just hide them by disabling underrun reporting for
> > pipe A around the workaround.
> >
> > It might be possible to avoid the underruns by moving the workaround
> > to be applied only when enabling pipe A. But I was too lazy to try it
> > right now, and the current method has been proven to work, so didn't
> > want to change it too hastily.
> 
> Is it possible this enables underrun reporting on pipe A even if it
> wasn't enabled before?

Yes, it's possible. It would mean that pipe A is currently enabled, and
has already suffered an underrun (which is why the underrun reporting
got disabled). But I think that's OK. We would really want the underrun
reporting to rearm itself after a small delay anyway, but currently that
doesn't happen. I had a hacky patch for that at some point, but it would
probably need more work, and we should first fix up all the known bugs 
that can cause underruns ie. watermark code.

> 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c   | 11 +++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h  |  9 +++++++++
> >  drivers/gpu/drm/i915/intel_hdmi.c | 11 +++++++++++
> >  drivers/gpu/drm/i915/intel_sdvo.c | 11 +++++++++++
> >  4 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 8287df4..4a0fb63 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3957,6 +3957,13 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> >  	 * matching HDMI port to be enabled on transcoder A.
> >  	 */
> >  	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
> > +		/*
> > +		 * We get CPU/PCH FIFO underruns on the other pipe when
> > +		 * doing the workaround. Sweep them under the rug.
> > +		 */
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > +
> >  		/* always enable with pattern 1 (as per spec) */
> >  		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
> >  		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
> > @@ -3966,6 +3973,10 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> >  		DP &= ~DP_PORT_EN;
> >  		I915_WRITE(intel_dp->output_reg, DP);
> >  		POSTING_READ(intel_dp->output_reg);
> > +
> > +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> >  	}
> >  
> >  	msleep(intel_dp->panel_power_down_delay);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 72cc272..35f1457 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1073,6 +1073,15 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
> >  {
> >  	drm_wait_one_vblank(dev, pipe);
> >  }
> > +static inline void
> > +intel_wait_for_vblank_if_active(struct drm_device *dev, int pipe)
> > +{
> > +	const struct intel_crtc *crtc =
> > +		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> > +
> > +	if (crtc->active)
> > +		intel_wait_for_vblank(dev, pipe);
> > +}
> >  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
> >  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >  			 struct intel_digital_port *dport,
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 013bd7d..bccbe70 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1108,6 +1108,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> >  	 * matching DP port to be enabled on transcoder A.
> >  	 */
> >  	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
> > +		/*
> > +		 * We get CPU/PCH FIFO underruns on the other pipe when
> > +		 * doing the workaround. Sweep them under the rug.
> > +		 */
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > +
> >  		temp &= ~SDVO_PIPE_B_SELECT;
> >  		temp |= SDVO_ENABLE;
> >  		/*
> > @@ -1122,6 +1129,10 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> >  		temp &= ~SDVO_ENABLE;
> >  		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> >  		POSTING_READ(intel_hdmi->hdmi_reg);
> > +
> > +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> >  	}
> >  
> >  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index c42b636..267e6cb 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1464,12 +1464,23 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
> >  	 * matching DP port to be enabled on transcoder A.
> >  	 */
> >  	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
> > +		/*
> > +		 * We get CPU/PCH FIFO underruns on the other pipe when
> > +		 * doing the workaround. Sweep them under the rug.
> > +		 */
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
> > +
> >  		temp &= ~SDVO_PIPE_B_SELECT;
> >  		temp |= SDVO_ENABLE;
> >  		intel_sdvo_write_sdvox(intel_sdvo, temp);
> >  
> >  		temp &= ~SDVO_ENABLE;
> >  		intel_sdvo_write_sdvox(intel_sdvo, temp);
> > +
> > +		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
> >  	}
> >  }
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 08/14] drm/i915: Disable FIFO underrun reporting around IBX transcoder B workaround

[ville.syrjala]

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

Doing the IBX transcoder B workaround causes underruns on
pipe/transcoder A. Just hide them by disabling underrun reporting for
pipe A around the workaround.

It might be possible to avoid the underruns by moving the workaround
to be applied only when enabling pipe A. But I was too lazy to try it
right now, and the current method has been proven to work, so didn't
want to change it too hastily.

Note that this can re-enable underrun reporting on pipe A if was
already disabled due to a previous actual underrun. But that's OK, we
may just get a second underrun report if another real underron occurrs
on pipe A.

v2: Note that pipe A underruns can get re-enabled due to this (Jani)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
---
 drivers/gpu/drm/i915/intel_dp.c   | 11 +++++++++++
 drivers/gpu/drm/i915/intel_drv.h  |  9 +++++++++
 drivers/gpu/drm/i915/intel_hdmi.c | 11 +++++++++++
 drivers/gpu/drm/i915/intel_sdvo.c | 11 +++++++++++
 4 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1cb1f3f..3ad3405 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3957,6 +3957,13 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	 * matching HDMI port to be enabled on transcoder A.
 	 */
 	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B && port != PORT_A) {
+		/*
+		 * We get CPU/PCH FIFO underruns on the other pipe when
+		 * doing the workaround. Sweep them under the rug.
+		 */
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
 		/* always enable with pattern 1 (as per spec) */
 		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
 		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
@@ -3966,6 +3973,10 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 		DP &= ~DP_PORT_EN;
 		I915_WRITE(intel_dp->output_reg, DP);
 		POSTING_READ(intel_dp->output_reg);
+
+		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 	}
 
 	msleep(intel_dp->panel_power_down_delay);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 72cc272..35f1457 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1073,6 +1073,15 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
 {
 	drm_wait_one_vblank(dev, pipe);
 }
+static inline void
+intel_wait_for_vblank_if_active(struct drm_device *dev, int pipe)
+{
+	const struct intel_crtc *crtc =
+		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+
+	if (crtc->active)
+		intel_wait_for_vblank(dev, pipe);
+}
 int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
 			 struct intel_digital_port *dport,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 013bd7d..bccbe70 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1108,6 +1108,13 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 	 * matching DP port to be enabled on transcoder A.
 	 */
 	if (HAS_PCH_IBX(dev) && crtc->pipe == PIPE_B) {
+		/*
+		 * We get CPU/PCH FIFO underruns on the other pipe when
+		 * doing the workaround. Sweep them under the rug.
+		 */
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
 		temp &= ~SDVO_PIPE_B_SELECT;
 		temp |= SDVO_ENABLE;
 		/*
@@ -1122,6 +1129,10 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
 		temp &= ~SDVO_ENABLE;
 		I915_WRITE(intel_hdmi->hdmi_reg, temp);
 		POSTING_READ(intel_hdmi->hdmi_reg);
+
+		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 	}
 
 	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index c42b636..267e6cb 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1464,12 +1464,23 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
 	 * matching DP port to be enabled on transcoder A.
 	 */
 	if (HAS_PCH_IBX(dev_priv) && crtc->pipe == PIPE_B) {
+		/*
+		 * We get CPU/PCH FIFO underruns on the other pipe when
+		 * doing the workaround. Sweep them under the rug.
+		 */
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
 		temp &= ~SDVO_PIPE_B_SELECT;
 		temp |= SDVO_ENABLE;
 		intel_sdvo_write_sdvox(intel_sdvo, temp);
 
 		temp &= ~SDVO_ENABLE;
 		intel_sdvo_write_sdvox(intel_sdvo, temp);
+
+		intel_wait_for_vblank_if_active(dev_priv->dev, PIPE_A);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, PIPE_A, true);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, true);
 	}
 }
 
-- 
2.4.10

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

[PATCH 09/14] drm/i915: Hide underruns from eDP PLL and port enable on ILK

[ville.syrjala]

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

We get underruns on the other pipe when enabling the CPU eDP PLL and
port on ILK.

Bspec knows about the PLL issue, and recommends doing a vblank wait just
prior to enabling the PLL. That does seem to help, but unfortunately we
get another underrun when actually enabling the CPU eDP port. Bspec
doesn't mention that at all, and the same vblank wait trick doesn't
appear to be effective there.

Since I have no better clue how to deal with this, just hide the errors.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4a0fb63..0b9b440 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2575,6 +2575,8 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
+	enum port port = dp_to_dig_port(intel_dp)->port;
+	enum pipe pipe = crtc->pipe;
 
 	if (WARN_ON(dp_reg & DP_PORT_EN))
 		return;
@@ -2586,6 +2588,17 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 
 	intel_dp_enable_port(intel_dp);
 
+	if (port == PORT_A && IS_GEN5(dev_priv)) {
+		/*
+		 * Underrun reporting for the other pipe was disabled in
+		 * g4x_pre_enable_dp(). The eDP PLL and port have now been
+		 * enabled, so it's now safe to re-enable underrun reporting.
+		 */
+		intel_wait_for_vblank_if_active(dev_priv->dev, !pipe);
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, true);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, true);
+	}
+
 	edp_panel_vdd_on(intel_dp);
 	edp_panel_on(intel_dp);
 	edp_panel_vdd_off(intel_dp, true);
@@ -2608,7 +2621,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 
 	if (crtc->config->has_audio) {
 		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
-				 pipe_name(crtc->pipe));
+				 pipe_name(pipe));
 		intel_audio_codec_enable(encoder);
 	}
 }
@@ -2631,13 +2644,28 @@ static void vlv_enable_dp(struct intel_encoder *encoder)
 
 static void g4x_pre_enable_dp(struct intel_encoder *encoder)
 {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
-	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
+	enum port port = dp_to_dig_port(intel_dp)->port;
+	enum pipe pipe = to_intel_crtc(encoder->base.crtc)->pipe;
 
 	intel_dp_prepare(encoder);
 
+	if (port == PORT_A && IS_GEN5(dev_priv)) {
+		/*
+		 * We get FIFO underruns on the other pipe when
+		 * enabling the CPU eDP PLL, and when enabling CPU
+		 * eDP port. We could potentially avoid the PLL
+		 * underrun with a vblank wait just prior to enabling
+		 * the PLL, but that doesn't appear to help the port
+		 * enable case. Just sweep it all under the rug.
+		 */
+		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, false);
+		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, false);
+	}
+
 	/* Only ilk+ has port A */
-	if (dport->port == PORT_A) {
+	if (port == PORT_A) {
 		ironlake_set_pll_cpu_edp(intel_dp);
 		ironlake_edp_pll_on(intel_dp);
 	}
-- 
2.4.10

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

Re: [PATCH 09/14] drm/i915: Hide underruns from eDP PLL and port enable on ILK

[Jesse Barnes]

On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We get underruns on the other pipe when enabling the CPU eDP PLL and
> port on ILK.
> 
> Bspec knows about the PLL issue, and recommends doing a vblank wait just
> prior to enabling the PLL. That does seem to help, but unfortunately we
> get another underrun when actually enabling the CPU eDP port. Bspec
> doesn't mention that at all, and the same vblank wait trick doesn't
> appear to be effective there.
> 
> Since I have no better clue how to deal with this, just hide the errors.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4a0fb63..0b9b440 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2575,6 +2575,8 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> +	enum port port = dp_to_dig_port(intel_dp)->port;
> +	enum pipe pipe = crtc->pipe;
>  
>  	if (WARN_ON(dp_reg & DP_PORT_EN))
>  		return;
> @@ -2586,6 +2588,17 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  
>  	intel_dp_enable_port(intel_dp);
>  
> +	if (port == PORT_A && IS_GEN5(dev_priv)) {
> +		/*
> +		 * Underrun reporting for the other pipe was disabled in
> +		 * g4x_pre_enable_dp(). The eDP PLL and port have now been
> +		 * enabled, so it's now safe to re-enable underrun reporting.
> +		 */
> +		intel_wait_for_vblank_if_active(dev_priv->dev, !pipe);
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, true);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, true);
> +	}
> +
>  	edp_panel_vdd_on(intel_dp);
>  	edp_panel_on(intel_dp);
>  	edp_panel_vdd_off(intel_dp, true);
> @@ -2608,7 +2621,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  
>  	if (crtc->config->has_audio) {
>  		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> -				 pipe_name(crtc->pipe));
> +				 pipe_name(pipe));
>  		intel_audio_codec_enable(encoder);
>  	}
>  }
> @@ -2631,13 +2644,28 @@ static void vlv_enable_dp(struct intel_encoder *encoder)
>  
>  static void g4x_pre_enable_dp(struct intel_encoder *encoder)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> -	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
> +	enum port port = dp_to_dig_port(intel_dp)->port;
> +	enum pipe pipe = to_intel_crtc(encoder->base.crtc)->pipe;
>  
>  	intel_dp_prepare(encoder);
>  
> +	if (port == PORT_A && IS_GEN5(dev_priv)) {
> +		/*
> +		 * We get FIFO underruns on the other pipe when
> +		 * enabling the CPU eDP PLL, and when enabling CPU
> +		 * eDP port. We could potentially avoid the PLL
> +		 * underrun with a vblank wait just prior to enabling
> +		 * the PLL, but that doesn't appear to help the port
> +		 * enable case. Just sweep it all under the rug.
> +		 */
> +		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, false);
> +		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, false);
> +	}
> +
>  	/* Only ilk+ has port A */
> -	if (dport->port == PORT_A) {
> +	if (port == PORT_A) {
>  		ironlake_set_pll_cpu_edp(intel_dp);
>  		ironlake_edp_pll_on(intel_dp);
>  	}
> 

Wish we had a nice hook to hide the gen5 bits somewhere better, but it's
fine as is with the comment.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/14] drm/i915: Hide underruns from eDP PLL and port enable on ILK

[Ville Syrjälä]

On Thu, Oct 29, 2015 at 12:39:53PM -0700, Jesse Barnes wrote:
> On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We get underruns on the other pipe when enabling the CPU eDP PLL and
> > port on ILK.
> > 
> > Bspec knows about the PLL issue, and recommends doing a vblank wait just
> > prior to enabling the PLL. That does seem to help, but unfortunately we
> > get another underrun when actually enabling the CPU eDP port. Bspec
> > doesn't mention that at all, and the same vblank wait trick doesn't
> > appear to be effective there.
> > 
> > Since I have no better clue how to deal with this, just hide the errors.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 34 +++++++++++++++++++++++++++++++---
> >  1 file changed, 31 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 4a0fb63..0b9b440 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2575,6 +2575,8 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> >  	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> > +	enum port port = dp_to_dig_port(intel_dp)->port;
> > +	enum pipe pipe = crtc->pipe;
> >  
> >  	if (WARN_ON(dp_reg & DP_PORT_EN))
> >  		return;
> > @@ -2586,6 +2588,17 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  
> >  	intel_dp_enable_port(intel_dp);
> >  
> > +	if (port == PORT_A && IS_GEN5(dev_priv)) {
> > +		/*
> > +		 * Underrun reporting for the other pipe was disabled in
> > +		 * g4x_pre_enable_dp(). The eDP PLL and port have now been
> > +		 * enabled, so it's now safe to re-enable underrun reporting.
> > +		 */
> > +		intel_wait_for_vblank_if_active(dev_priv->dev, !pipe);
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, true);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, true);
> > +	}
> > +
> >  	edp_panel_vdd_on(intel_dp);
> >  	edp_panel_on(intel_dp);
> >  	edp_panel_vdd_off(intel_dp, true);
> > @@ -2608,7 +2621,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  
> >  	if (crtc->config->has_audio) {
> >  		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> > -				 pipe_name(crtc->pipe));
> > +				 pipe_name(pipe));
> >  		intel_audio_codec_enable(encoder);
> >  	}
> >  }
> > @@ -2631,13 +2644,28 @@ static void vlv_enable_dp(struct intel_encoder *encoder)
> >  
> >  static void g4x_pre_enable_dp(struct intel_encoder *encoder)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > -	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
> > +	enum port port = dp_to_dig_port(intel_dp)->port;
> > +	enum pipe pipe = to_intel_crtc(encoder->base.crtc)->pipe;
> >  
> >  	intel_dp_prepare(encoder);
> >  
> > +	if (port == PORT_A && IS_GEN5(dev_priv)) {
> > +		/*
> > +		 * We get FIFO underruns on the other pipe when
> > +		 * enabling the CPU eDP PLL, and when enabling CPU
> > +		 * eDP port. We could potentially avoid the PLL
> > +		 * underrun with a vblank wait just prior to enabling
> > +		 * the PLL, but that doesn't appear to help the port
> > +		 * enable case. Just sweep it all under the rug.
> > +		 */
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, false);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, false);
> > +	}
> > +
> >  	/* Only ilk+ has port A */
> > -	if (dport->port == PORT_A) {
> > +	if (port == PORT_A) {
> >  		ironlake_set_pll_cpu_edp(intel_dp);
> >  		ironlake_edp_pll_on(intel_dp);
> >  	}
> > 
> 
> Wish we had a nice hook to hide the gen5 bits somewhere better, but it's
> fine as is with the comment.

I did consider adding ilk_pre_enable_dp and ilk_enable_dp for this, but I
decided that it would be more confusing since then both pre-ilk (g4x) and
post-ilk (snb+) would use the g4x functions.

I did manage to confuse myself already once with the g4x vs. pch vs.
platform specific vs. totally generic hooks we have for some port
types. So it might make sense to take a small code duplication hit,
and just make sure there is always a full set of hook with the same
prefix, even if some of those do exactly the same thing.

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/14] drm/i915: s/DP_PLL_FREQ_160MHZ/DP_PLL_FREQ_162MHZ/

[ville.syrjala]

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

The DP link frequency is 162MHz, not 160MHz. Rename the ILK eDP PLL
defines to match.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  2 +-
 drivers/gpu/drm/i915/intel_dp.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8942532..d02e3c7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4199,7 +4199,7 @@ enum skl_disp_power_wells {
 
 /* eDP */
 #define   DP_PLL_FREQ_270MHZ		(0 << 16)
-#define   DP_PLL_FREQ_160MHZ		(1 << 16)
+#define   DP_PLL_FREQ_162MHZ		(1 << 16)
 #define   DP_PLL_FREQ_MASK		(3 << 16)
 
 /* locked once port is enabled */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0b9b440..55d5246 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1557,11 +1557,11 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
 
 	if (crtc->config->port_clock == 162000) {
 		/* For a long time we've carried around a ILK-DevA w/a for the
-		 * 160MHz clock. If we're really unlucky, it's still required.
+		 * 162MHz clock. If we're really unlucky, it's still required.
 		 */
-		DRM_DEBUG_KMS("160MHz cpu eDP clock, might need ilk devA w/a\n");
-		dpa_ctl |= DP_PLL_FREQ_160MHZ;
-		intel_dp->DP |= DP_PLL_FREQ_160MHZ;
+		DRM_DEBUG_KMS("162MHz cpu eDP clock, might need ilk devA w/a\n");
+		dpa_ctl |= DP_PLL_FREQ_162MHZ;
+		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
 	} else {
 		dpa_ctl |= DP_PLL_FREQ_270MHZ;
 		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
@@ -2324,7 +2324,7 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 	intel_dp_get_m_n(crtc, pipe_config);
 
 	if (port == PORT_A) {
-		if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ)
+		if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_162MHZ)
 			pipe_config->port_clock = 162000;
 		else
 			pipe_config->port_clock = 270000;
-- 
2.4.10

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

Re: [PATCH 10/14] drm/i915: s/DP_PLL_FREQ_160MHZ/DP_PLL_FREQ_162MHZ/

[Daniel Vetter]

On Thu, Oct 29, 2015 at 09:25:59PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The DP link frequency is 162MHz, not 160MHz. Rename the ILK eDP PLL
> defines to match.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

ocd ftw. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_reg.h |  2 +-
>  drivers/gpu/drm/i915/intel_dp.c | 10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8942532..d02e3c7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4199,7 +4199,7 @@ enum skl_disp_power_wells {
>  
>  /* eDP */
>  #define   DP_PLL_FREQ_270MHZ		(0 << 16)
> -#define   DP_PLL_FREQ_160MHZ		(1 << 16)
> +#define   DP_PLL_FREQ_162MHZ		(1 << 16)
>  #define   DP_PLL_FREQ_MASK		(3 << 16)
>  
>  /* locked once port is enabled */
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0b9b440..55d5246 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1557,11 +1557,11 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
>  
>  	if (crtc->config->port_clock == 162000) {
>  		/* For a long time we've carried around a ILK-DevA w/a for the
> -		 * 160MHz clock. If we're really unlucky, it's still required.
> +		 * 162MHz clock. If we're really unlucky, it's still required.
>  		 */
> -		DRM_DEBUG_KMS("160MHz cpu eDP clock, might need ilk devA w/a\n");
> -		dpa_ctl |= DP_PLL_FREQ_160MHZ;
> -		intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> +		DRM_DEBUG_KMS("162MHz cpu eDP clock, might need ilk devA w/a\n");
> +		dpa_ctl |= DP_PLL_FREQ_162MHZ;
> +		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
>  	} else {
>  		dpa_ctl |= DP_PLL_FREQ_270MHZ;
>  		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> @@ -2324,7 +2324,7 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>  	intel_dp_get_m_n(crtc, pipe_config);
>  
>  	if (port == PORT_A) {
> -		if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ)
> +		if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_162MHZ)
>  			pipe_config->port_clock = 162000;
>  		else
>  			pipe_config->port_clock = 270000;
> -- 
> 2.4.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/14] drm/i915: Remove ILK-A eDP PLL workaround notes

[ville.syrjala]

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

We don't care about ILK-A and the old w/a notes may just confuse
people, so get rid of them.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 55d5246..763b0ef 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1556,10 +1556,6 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
 	dpa_ctl &= ~DP_PLL_FREQ_MASK;
 
 	if (crtc->config->port_clock == 162000) {
-		/* For a long time we've carried around a ILK-DevA w/a for the
-		 * 162MHz clock. If we're really unlucky, it's still required.
-		 */
-		DRM_DEBUG_KMS("162MHz cpu eDP clock, might need ilk devA w/a\n");
 		dpa_ctl |= DP_PLL_FREQ_162MHZ;
 		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
 	} else {
-- 
2.4.10

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

Re: [PATCH 11/14] drm/i915: Remove ILK-A eDP PLL workaround notes

[Jesse Barnes]

On 10/29/2015 12:26 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We don't care about ILK-A and the old w/a notes may just confuse
> people, so get rid of them.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 55d5246..763b0ef 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1556,10 +1556,6 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
>  	dpa_ctl &= ~DP_PLL_FREQ_MASK;
>  
>  	if (crtc->config->port_clock == 162000) {
> -		/* For a long time we've carried around a ILK-DevA w/a for the
> -		 * 162MHz clock. If we're really unlucky, it's still required.
> -		 */
> -		DRM_DEBUG_KMS("162MHz cpu eDP clock, might need ilk devA w/a\n");
>  		dpa_ctl |= DP_PLL_FREQ_162MHZ;
>  		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
>  	} else {
> 

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/14] drm/i915: Clean up eDP PLL state asserts

[ville.syrjala]

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

Rewrite the eDP PLL state asserts to conform to our usual state assert
style.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 54 +++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 763b0ef..e259803 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2142,21 +2142,48 @@ static void intel_edp_backlight_power(struct intel_connector *connector,
 		_intel_edp_backlight_off(intel_dp);
 }
 
+static const char *state_string(bool enabled)
+{
+	return enabled ? "on" : "off";
+}
+
+static void assert_dp_port(struct intel_dp *intel_dp, bool state)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	bool cur_state = I915_READ(intel_dp->output_reg) & DP_PORT_EN;
+
+	I915_STATE_WARN(cur_state != state,
+			"DP port %c state assertion failure (expected %s, current %s)\n",
+			port_name(dig_port->port),
+			state_string(state), state_string(cur_state));
+}
+#define assert_dp_port_disabled(d) assert_dp_port((d), false)
+
+static void assert_edp_pll(struct drm_i915_private *dev_priv, bool state)
+{
+	bool cur_state = I915_READ(DP_A) & DP_PLL_ENABLE;
+
+	I915_STATE_WARN(cur_state != state,
+			"eDP PLL state assertion failure (expected %s, current %s)\n",
+			state_string(state), state_string(cur_state));
+}
+#define assert_edp_pll_enabled(d) assert_edp_pll((d), true)
+#define assert_edp_pll_disabled(d) assert_edp_pll((d), false)
+
 static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	u32 dpa_ctl;
 
-	assert_pipe_disabled(dev_priv,
-			     to_intel_crtc(crtc)->pipe);
+	assert_pipe_disabled(dev_priv, crtc->pipe);
+	assert_dp_port_disabled(intel_dp);
+	assert_edp_pll_disabled(dev_priv);
 
 	DRM_DEBUG_KMS("\n");
 	dpa_ctl = I915_READ(DP_A);
-	WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n");
-	WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
 
 	/* We don't adjust intel_dp->DP while tearing down the link, to
 	 * facilitate link retraining (e.g. after hotplug). Hence clear all
@@ -2171,18 +2198,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
 static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	u32 dpa_ctl;
 
-	assert_pipe_disabled(dev_priv,
-			     to_intel_crtc(crtc)->pipe);
+	assert_pipe_disabled(dev_priv, crtc->pipe);
+	assert_dp_port_disabled(intel_dp);
+	assert_edp_pll_enabled(dev_priv);
 
 	dpa_ctl = I915_READ(DP_A);
-	WARN((dpa_ctl & DP_PLL_ENABLE) == 0,
-	     "dp pll off, should be on\n");
-	WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
 
 	/* We can't rely on the value tracked for the DP register in
 	 * intel_dp->DP because link_down must not change that (otherwise link
-- 
2.4.10

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

Re: [PATCH 12/14] drm/i915: Clean up eDP PLL state asserts

[Daniel Vetter]

On Thu, Oct 29, 2015 at 09:26:01PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Rewrite the eDP PLL state asserts to conform to our usual state assert
> style.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 54 +++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 763b0ef..e259803 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2142,21 +2142,48 @@ static void intel_edp_backlight_power(struct intel_connector *connector,
>  		_intel_edp_backlight_off(intel_dp);
>  }
>  
> +static const char *state_string(bool enabled)
> +{
> +	return enabled ? "on" : "off";
> +}
> +
> +static void assert_dp_port(struct intel_dp *intel_dp, bool state)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	bool cur_state = I915_READ(intel_dp->output_reg) & DP_PORT_EN;
> +
> +	I915_STATE_WARN(cur_state != state,
> +			"DP port %c state assertion failure (expected %s, current %s)\n",
> +			port_name(dig_port->port),
> +			state_string(state), state_string(cur_state));
> +}
> +#define assert_dp_port_disabled(d) assert_dp_port((d), false)
> +
> +static void assert_edp_pll(struct drm_i915_private *dev_priv, bool state)
> +{
> +	bool cur_state = I915_READ(DP_A) & DP_PLL_ENABLE;
> +
> +	I915_STATE_WARN(cur_state != state,
> +			"eDP PLL state assertion failure (expected %s, current %s)\n",
> +			state_string(state), state_string(cur_state));
> +}
> +#define assert_edp_pll_enabled(d) assert_edp_pll((d), true)
> +#define assert_edp_pll_disabled(d) assert_edp_pll((d), false)
> +
>  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	u32 dpa_ctl;
>  
> -	assert_pipe_disabled(dev_priv,
> -			     to_intel_crtc(crtc)->pipe);
> +	assert_pipe_disabled(dev_priv, crtc->pipe);
> +	assert_dp_port_disabled(intel_dp);
> +	assert_edp_pll_disabled(dev_priv);
>  
>  	DRM_DEBUG_KMS("\n");
>  	dpa_ctl = I915_READ(DP_A);
> -	WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n");
> -	WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
>  
>  	/* We don't adjust intel_dp->DP while tearing down the link, to
>  	 * facilitate link retraining (e.g. after hotplug). Hence clear all
> @@ -2171,18 +2198,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>  static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	u32 dpa_ctl;
>  
> -	assert_pipe_disabled(dev_priv,
> -			     to_intel_crtc(crtc)->pipe);
> +	assert_pipe_disabled(dev_priv, crtc->pipe);
> +	assert_dp_port_disabled(intel_dp);
> +	assert_edp_pll_enabled(dev_priv);
>  
>  	dpa_ctl = I915_READ(DP_A);
> -	WARN((dpa_ctl & DP_PLL_ENABLE) == 0,
> -	     "dp pll off, should be on\n");
> -	WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
>  
>  	/* We can't rely on the value tracked for the DP register in
>  	 * intel_dp->DP because link_down must not change that (otherwise link
> -- 
> 2.4.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/14] drm/i915: Use intel_dp->DP in eDP PLL setup

[ville.syrjala]

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

Use intel_dp->DP in the eDP PLL setup, instead of doing RMWs.

To do this we need to move DP_AUDIO_OUTPUT_ENABLE setup to happen later,
so that we don't enable audio accidentally while configuring the PLL.

Also intel_dp_link_down() must be made to update intel_dp->DP so that we
don't re-enable the port by accident when turning off the PLL. This is
safe now that we don't call intel_dp_link_down() during link retraining.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e259803..63659e7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1548,23 +1548,18 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
 	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 dpa_ctl;
 
 	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
 		      crtc->config->port_clock);
-	dpa_ctl = I915_READ(DP_A);
-	dpa_ctl &= ~DP_PLL_FREQ_MASK;
 
-	if (crtc->config->port_clock == 162000) {
-		dpa_ctl |= DP_PLL_FREQ_162MHZ;
+	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
+
+	if (crtc->config->port_clock == 162000)
 		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
-	} else {
-		dpa_ctl |= DP_PLL_FREQ_270MHZ;
+	else
 		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
-	}
-
-	I915_WRITE(DP_A, dpa_ctl);
 
+	I915_WRITE(DP_A, intel_dp->DP);
 	POSTING_READ(DP_A);
 	udelay(500);
 }
@@ -1613,9 +1608,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
 	intel_dp->DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
 	intel_dp->DP |= DP_PORT_WIDTH(crtc->config->lane_count);
 
-	if (crtc->config->has_audio)
-		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
-
 	/* Split out the IBX/CPU vs CPT settings */
 
 	if (IS_GEN7(dev) && port == PORT_A) {
@@ -2176,20 +2168,14 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	u32 dpa_ctl;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 	assert_dp_port_disabled(intel_dp);
 	assert_edp_pll_disabled(dev_priv);
 
 	DRM_DEBUG_KMS("\n");
-	dpa_ctl = I915_READ(DP_A);
-
-	/* We don't adjust intel_dp->DP while tearing down the link, to
-	 * facilitate link retraining (e.g. after hotplug). Hence clear all
-	 * enable bits here to ensure that we don't enable too much. */
-	intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
 	intel_dp->DP |= DP_PLL_ENABLE;
+
 	I915_WRITE(DP_A, intel_dp->DP);
 	POSTING_READ(DP_A);
 	udelay(200);
@@ -2200,19 +2186,14 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	u32 dpa_ctl;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 	assert_dp_port_disabled(intel_dp);
 	assert_edp_pll_enabled(dev_priv);
 
-	dpa_ctl = I915_READ(DP_A);
+	intel_dp->DP &= ~DP_PLL_ENABLE;
 
-	/* We can't rely on the value tracked for the DP register in
-	 * intel_dp->DP because link_down must not change that (otherwise link
-	 * re-training will fail. */
-	dpa_ctl &= ~DP_PLL_ENABLE;
-	I915_WRITE(DP_A, dpa_ctl);
+	I915_WRITE(DP_A, intel_dp->DP);
 	POSTING_READ(DP_A);
 	udelay(200);
 }
@@ -2568,6 +2549,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc =
+		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 
 	/* enable with pattern 1 (as per spec) */
 	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
@@ -2583,6 +2566,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
 	 * fail when the power sequencer is freshly used for this port.
 	 */
 	intel_dp->DP |= DP_PORT_EN;
+	if (crtc->config->has_audio)
+		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
 
 	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
@@ -4028,6 +4013,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	}
 
 	msleep(intel_dp->panel_power_down_delay);
+
+	intel_dp->DP = DP;
 }
 
 static bool
-- 
2.4.10

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

Re: [PATCH 13/14] drm/i915: Use intel_dp->DP in eDP PLL setup

[Daniel Vetter]

On Thu, Oct 29, 2015 at 09:26:02PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use intel_dp->DP in the eDP PLL setup, instead of doing RMWs.
> 
> To do this we need to move DP_AUDIO_OUTPUT_ENABLE setup to happen later,
> so that we don't enable audio accidentally while configuring the PLL.
> 
> Also intel_dp_link_down() must be made to update intel_dp->DP so that we
> don't re-enable the port by accident when turning off the PLL. This is
> safe now that we don't call intel_dp_link_down() during link retraining.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++---------------------------
>  1 file changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e259803..63659e7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1548,23 +1548,18 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
>  	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 dpa_ctl;
>  
>  	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
>  		      crtc->config->port_clock);
> -	dpa_ctl = I915_READ(DP_A);
> -	dpa_ctl &= ~DP_PLL_FREQ_MASK;
>  
> -	if (crtc->config->port_clock == 162000) {
> -		dpa_ctl |= DP_PLL_FREQ_162MHZ;
> +	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
> +
> +	if (crtc->config->port_clock == 162000)
>  		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
> -	} else {
> -		dpa_ctl |= DP_PLL_FREQ_270MHZ;
> +	else
>  		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> -	}
> -
> -	I915_WRITE(DP_A, dpa_ctl);
>  
> +	I915_WRITE(DP_A, intel_dp->DP);
>  	POSTING_READ(DP_A);
>  	udelay(500);
>  }
> @@ -1613,9 +1608,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
>  	intel_dp->DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
>  	intel_dp->DP |= DP_PORT_WIDTH(crtc->config->lane_count);
>  
> -	if (crtc->config->has_audio)
> -		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
> -
>  	/* Split out the IBX/CPU vs CPT settings */
>  
>  	if (IS_GEN7(dev) && port == PORT_A) {
> @@ -2176,20 +2168,14 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 dpa_ctl;
>  
>  	assert_pipe_disabled(dev_priv, crtc->pipe);
>  	assert_dp_port_disabled(intel_dp);
>  	assert_edp_pll_disabled(dev_priv);
>  
>  	DRM_DEBUG_KMS("\n");
> -	dpa_ctl = I915_READ(DP_A);
> -
> -	/* We don't adjust intel_dp->DP while tearing down the link, to
> -	 * facilitate link retraining (e.g. after hotplug). Hence clear all
> -	 * enable bits here to ensure that we don't enable too much. */
> -	intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
>  	intel_dp->DP |= DP_PLL_ENABLE;
> +
>  	I915_WRITE(DP_A, intel_dp->DP);
>  	POSTING_READ(DP_A);
>  	udelay(200);
> @@ -2200,19 +2186,14 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 dpa_ctl;
>  
>  	assert_pipe_disabled(dev_priv, crtc->pipe);
>  	assert_dp_port_disabled(intel_dp);
>  	assert_edp_pll_enabled(dev_priv);
>  
> -	dpa_ctl = I915_READ(DP_A);
> +	intel_dp->DP &= ~DP_PLL_ENABLE;

I think we need to clear DP_AUDIO_OUTPUT_ENABLE here too, otherwise a dpms
off->on cycle will again enable audio too early in edp_pll_on.

>  
> -	/* We can't rely on the value tracked for the DP register in
> -	 * intel_dp->DP because link_down must not change that (otherwise link
> -	 * re-training will fail. */
> -	dpa_ctl &= ~DP_PLL_ENABLE;
> -	I915_WRITE(DP_A, dpa_ctl);
> +	I915_WRITE(DP_A, intel_dp->DP);
>  	POSTING_READ(DP_A);
>  	udelay(200);
>  }
> @@ -2568,6 +2549,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc =
> +		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>  
>  	/* enable with pattern 1 (as per spec) */
>  	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> @@ -2583,6 +2566,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
>  	 * fail when the power sequencer is freshly used for this port.
>  	 */
>  	intel_dp->DP |= DP_PORT_EN;
> +	if (crtc->config->has_audio)
> +		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;

If I read the code correctly before this patch the I915_WRITE right above
this one here already enabled the audio bit. Shouldn't matter, and
probably better, but must be mentioned in the commit message.

With these two things addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>  	POSTING_READ(intel_dp->output_reg);
> @@ -4028,6 +4013,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	}
>  
>  	msleep(intel_dp->panel_power_down_delay);
> +
> +	intel_dp->DP = DP;
>  }
>  
>  static bool
> -- 
> 2.4.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/14] drm/i915: Use intel_dp->DP in eDP PLL setup

[Ville Syrjälä]

On Fri, Oct 30, 2015 at 05:00:42PM +0100, Daniel Vetter wrote:
> On Thu, Oct 29, 2015 at 09:26:02PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use intel_dp->DP in the eDP PLL setup, instead of doing RMWs.
> > 
> > To do this we need to move DP_AUDIO_OUTPUT_ENABLE setup to happen later,
> > so that we don't enable audio accidentally while configuring the PLL.
> > 
> > Also intel_dp_link_down() must be made to update intel_dp->DP so that we
> > don't re-enable the port by accident when turning off the PLL. This is
> > safe now that we don't call intel_dp_link_down() during link retraining.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++---------------------------
> >  1 file changed, 14 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e259803..63659e7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1548,23 +1548,18 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
> >  	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	u32 dpa_ctl;
> >  
> >  	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
> >  		      crtc->config->port_clock);
> > -	dpa_ctl = I915_READ(DP_A);
> > -	dpa_ctl &= ~DP_PLL_FREQ_MASK;
> >  
> > -	if (crtc->config->port_clock == 162000) {
> > -		dpa_ctl |= DP_PLL_FREQ_162MHZ;
> > +	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
> > +
> > +	if (crtc->config->port_clock == 162000)
> >  		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
> > -	} else {
> > -		dpa_ctl |= DP_PLL_FREQ_270MHZ;
> > +	else
> >  		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> > -	}
> > -
> > -	I915_WRITE(DP_A, dpa_ctl);
> >  
> > +	I915_WRITE(DP_A, intel_dp->DP);
> >  	POSTING_READ(DP_A);
> >  	udelay(500);
> >  }
> > @@ -1613,9 +1608,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
> >  	intel_dp->DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
> >  	intel_dp->DP |= DP_PORT_WIDTH(crtc->config->lane_count);
> >  
> > -	if (crtc->config->has_audio)
> > -		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
> > -
> >  	/* Split out the IBX/CPU vs CPT settings */
> >  
> >  	if (IS_GEN7(dev) && port == PORT_A) {
> > @@ -2176,20 +2168,14 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	u32 dpa_ctl;
> >  
> >  	assert_pipe_disabled(dev_priv, crtc->pipe);
> >  	assert_dp_port_disabled(intel_dp);
> >  	assert_edp_pll_disabled(dev_priv);
> >  
> >  	DRM_DEBUG_KMS("\n");
> > -	dpa_ctl = I915_READ(DP_A);
> > -
> > -	/* We don't adjust intel_dp->DP while tearing down the link, to
> > -	 * facilitate link retraining (e.g. after hotplug). Hence clear all
> > -	 * enable bits here to ensure that we don't enable too much. */
> > -	intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
> >  	intel_dp->DP |= DP_PLL_ENABLE;
> > +
> >  	I915_WRITE(DP_A, intel_dp->DP);
> >  	POSTING_READ(DP_A);
> >  	udelay(200);
> > @@ -2200,19 +2186,14 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	u32 dpa_ctl;
> >  
> >  	assert_pipe_disabled(dev_priv, crtc->pipe);
> >  	assert_dp_port_disabled(intel_dp);
> >  	assert_edp_pll_enabled(dev_priv);
> >  
> > -	dpa_ctl = I915_READ(DP_A);
> > +	intel_dp->DP &= ~DP_PLL_ENABLE;
> 
> I think we need to clear DP_AUDIO_OUTPUT_ENABLE here too, otherwise a dpms
> off->on cycle will again enable audio too early in edp_pll_on.

intel_dp_prepare() starts from scratch:
"intel_dp->DP = I915_READ(intel_dp->output_reg) & DP_DETECTED;"

> 
> >  
> > -	/* We can't rely on the value tracked for the DP register in
> > -	 * intel_dp->DP because link_down must not change that (otherwise link
> > -	 * re-training will fail. */
> > -	dpa_ctl &= ~DP_PLL_ENABLE;
> > -	I915_WRITE(DP_A, dpa_ctl);
> > +	I915_WRITE(DP_A, intel_dp->DP);
> >  	POSTING_READ(DP_A);
> >  	udelay(200);
> >  }
> > @@ -2568,6 +2549,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *crtc =
> > +		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
> >  
> >  	/* enable with pattern 1 (as per spec) */
> >  	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> > @@ -2583,6 +2566,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
> >  	 * fail when the power sequencer is freshly used for this port.
> >  	 */
> >  	intel_dp->DP |= DP_PORT_EN;
> > +	if (crtc->config->has_audio)
> > +		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
> 
> If I read the code correctly before this patch the I915_WRITE right above
> this one here already enabled the audio bit. Shouldn't matter, and
> probably better, but must be mentioned in the commit message.

Indeed. The new order even makes more sense. I'll add a note.

> 
> With these two things addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> >  
> >  	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
> >  	POSTING_READ(intel_dp->output_reg);
> > @@ -4028,6 +4013,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> >  	}
> >  
> >  	msleep(intel_dp->panel_power_down_delay);
> > +
> > +	intel_dp->DP = DP;
> >  }
> >  
> >  static bool
> > -- 
> > 2.4.10
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/14] drm/i915: Use intel_dp->DP in eDP PLL setup

[Jani Nikula]

On Fri, 30 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Oct 30, 2015 at 05:00:42PM +0100, Daniel Vetter wrote:
>> On Thu, Oct 29, 2015 at 09:26:02PM +0200, ville.syrjala@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > 
>> > Use intel_dp->DP in the eDP PLL setup, instead of doing RMWs.
>> > 
>> > To do this we need to move DP_AUDIO_OUTPUT_ENABLE setup to happen later,
>> > so that we don't enable audio accidentally while configuring the PLL.
>> > 
>> > Also intel_dp_link_down() must be made to update intel_dp->DP so that we
>> > don't re-enable the port by accident when turning off the PLL. This is
>> > safe now that we don't call intel_dp_link_down() during link retraining.
>> > 
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++---------------------------
>> >  1 file changed, 14 insertions(+), 27 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index e259803..63659e7 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1548,23 +1548,18 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
>> >  	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
>> >  	struct drm_device *dev = crtc->base.dev;
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> > -	u32 dpa_ctl;
>> >  
>> >  	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
>> >  		      crtc->config->port_clock);
>> > -	dpa_ctl = I915_READ(DP_A);
>> > -	dpa_ctl &= ~DP_PLL_FREQ_MASK;
>> >  
>> > -	if (crtc->config->port_clock == 162000) {
>> > -		dpa_ctl |= DP_PLL_FREQ_162MHZ;
>> > +	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
>> > +
>> > +	if (crtc->config->port_clock == 162000)
>> >  		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
>> > -	} else {
>> > -		dpa_ctl |= DP_PLL_FREQ_270MHZ;
>> > +	else
>> >  		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
>> > -	}
>> > -
>> > -	I915_WRITE(DP_A, dpa_ctl);
>> >  
>> > +	I915_WRITE(DP_A, intel_dp->DP);
>> >  	POSTING_READ(DP_A);
>> >  	udelay(500);
>> >  }
>> > @@ -1613,9 +1608,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
>> >  	intel_dp->DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
>> >  	intel_dp->DP |= DP_PORT_WIDTH(crtc->config->lane_count);
>> >  
>> > -	if (crtc->config->has_audio)
>> > -		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
>> > -
>> >  	/* Split out the IBX/CPU vs CPT settings */
>> >  
>> >  	if (IS_GEN7(dev) && port == PORT_A) {
>> > @@ -2176,20 +2168,14 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> >  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > -	u32 dpa_ctl;
>> >  
>> >  	assert_pipe_disabled(dev_priv, crtc->pipe);
>> >  	assert_dp_port_disabled(intel_dp);
>> >  	assert_edp_pll_disabled(dev_priv);
>> >  
>> >  	DRM_DEBUG_KMS("\n");
>> > -	dpa_ctl = I915_READ(DP_A);
>> > -
>> > -	/* We don't adjust intel_dp->DP while tearing down the link, to
>> > -	 * facilitate link retraining (e.g. after hotplug). Hence clear all
>> > -	 * enable bits here to ensure that we don't enable too much. */
>> > -	intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
>> >  	intel_dp->DP |= DP_PLL_ENABLE;
>> > +
>> >  	I915_WRITE(DP_A, intel_dp->DP);
>> >  	POSTING_READ(DP_A);
>> >  	udelay(200);
>> > @@ -2200,19 +2186,14 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
>> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> >  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > -	u32 dpa_ctl;
>> >  
>> >  	assert_pipe_disabled(dev_priv, crtc->pipe);
>> >  	assert_dp_port_disabled(intel_dp);
>> >  	assert_edp_pll_enabled(dev_priv);
>> >  
>> > -	dpa_ctl = I915_READ(DP_A);
>> > +	intel_dp->DP &= ~DP_PLL_ENABLE;
>> 
>> I think we need to clear DP_AUDIO_OUTPUT_ENABLE here too, otherwise a dpms
>> off->on cycle will again enable audio too early in edp_pll_on.
>
> intel_dp_prepare() starts from scratch:
> "intel_dp->DP = I915_READ(intel_dp->output_reg) & DP_DETECTED;"

Yeah, Daniel's point here is invalid.

For this part,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



>
>> 
>> >  
>> > -	/* We can't rely on the value tracked for the DP register in
>> > -	 * intel_dp->DP because link_down must not change that (otherwise link
>> > -	 * re-training will fail. */
>> > -	dpa_ctl &= ~DP_PLL_ENABLE;
>> > -	I915_WRITE(DP_A, dpa_ctl);
>> > +	I915_WRITE(DP_A, intel_dp->DP);
>> >  	POSTING_READ(DP_A);
>> >  	udelay(200);
>> >  }
>> > @@ -2568,6 +2549,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
>> >  {
>> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> > +	struct intel_crtc *crtc =
>> > +		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>> >  
>> >  	/* enable with pattern 1 (as per spec) */
>> >  	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
>> > @@ -2583,6 +2566,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
>> >  	 * fail when the power sequencer is freshly used for this port.
>> >  	 */
>> >  	intel_dp->DP |= DP_PORT_EN;
>> > +	if (crtc->config->has_audio)
>> > +		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
>> 
>> If I read the code correctly before this patch the I915_WRITE right above
>> this one here already enabled the audio bit. Shouldn't matter, and
>> probably better, but must be mentioned in the commit message.
>
> Indeed. The new order even makes more sense. I'll add a note.
>
>> 
>> With these two things addressed:
>> 
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> 
>> >  
>> >  	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>> >  	POSTING_READ(intel_dp->output_reg);
>> > @@ -4028,6 +4013,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>> >  	}
>> >  
>> >  	msleep(intel_dp->panel_power_down_delay);
>> > +
>> > +	intel_dp->DP = DP;
>> >  }
>> >  
>> >  static bool
>> > -- 
>> > 2.4.10
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

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

[PATCH v2 13/14] drm/i915: Use intel_dp->DP in eDP PLL setup

[ville.syrjala]

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

Use intel_dp->DP in the eDP PLL setup, instead of doing RMWs.

To do this we need to move DP_AUDIO_OUTPUT_ENABLE setup to happen later,
so that we don't enable audio accidentally while configuring the PLL.

Note that actually we already enabled audio before the port due to
the double port register write magic required by VLV/CHV from
7b713f50d78b6 ("drm/i915: Fix eDP link training when switching pipes on VLV/CHV")
So that gets changed now to keep audio off as long as the port is off.

Also intel_dp_link_down() must be made to update intel_dp->DP so that we
don't re-enable the port by accident when turning off the PLL. This is
safe now that we don't call intel_dp_link_down() during link retraining.

v2: Add a note about the audio vs. port enable (Daniel)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 019283f..d93f9f3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1551,23 +1551,18 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
 	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 dpa_ctl;
 
 	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
 		      crtc->config->port_clock);
-	dpa_ctl = I915_READ(DP_A);
-	dpa_ctl &= ~DP_PLL_FREQ_MASK;
 
-	if (crtc->config->port_clock == 162000) {
-		dpa_ctl |= DP_PLL_FREQ_162MHZ;
+	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
+
+	if (crtc->config->port_clock == 162000)
 		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
-	} else {
-		dpa_ctl |= DP_PLL_FREQ_270MHZ;
+	else
 		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
-	}
-
-	I915_WRITE(DP_A, dpa_ctl);
 
+	I915_WRITE(DP_A, intel_dp->DP);
 	POSTING_READ(DP_A);
 	udelay(500);
 }
@@ -1616,9 +1611,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
 	intel_dp->DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
 	intel_dp->DP |= DP_PORT_WIDTH(crtc->config->lane_count);
 
-	if (crtc->config->has_audio)
-		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
-
 	/* Split out the IBX/CPU vs CPT settings */
 
 	if (IS_GEN7(dev) && port == PORT_A) {
@@ -2179,20 +2171,14 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	u32 dpa_ctl;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 	assert_dp_port_disabled(intel_dp);
 	assert_edp_pll_disabled(dev_priv);
 
 	DRM_DEBUG_KMS("\n");
-	dpa_ctl = I915_READ(DP_A);
-
-	/* We don't adjust intel_dp->DP while tearing down the link, to
-	 * facilitate link retraining (e.g. after hotplug). Hence clear all
-	 * enable bits here to ensure that we don't enable too much. */
-	intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
 	intel_dp->DP |= DP_PLL_ENABLE;
+
 	I915_WRITE(DP_A, intel_dp->DP);
 	POSTING_READ(DP_A);
 	udelay(200);
@@ -2203,19 +2189,14 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	u32 dpa_ctl;
 
 	assert_pipe_disabled(dev_priv, crtc->pipe);
 	assert_dp_port_disabled(intel_dp);
 	assert_edp_pll_enabled(dev_priv);
 
-	dpa_ctl = I915_READ(DP_A);
+	intel_dp->DP &= ~DP_PLL_ENABLE;
 
-	/* We can't rely on the value tracked for the DP register in
-	 * intel_dp->DP because link_down must not change that (otherwise link
-	 * re-training will fail. */
-	dpa_ctl &= ~DP_PLL_ENABLE;
-	I915_WRITE(DP_A, dpa_ctl);
+	I915_WRITE(DP_A, intel_dp->DP);
 	POSTING_READ(DP_A);
 	udelay(200);
 }
@@ -2571,6 +2552,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc =
+		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 
 	/* enable with pattern 1 (as per spec) */
 	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
@@ -2586,6 +2569,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
 	 * fail when the power sequencer is freshly used for this port.
 	 */
 	intel_dp->DP |= DP_PORT_EN;
+	if (crtc->config->has_audio)
+		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
 
 	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
@@ -3726,6 +3711,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	}
 
 	msleep(intel_dp->panel_power_down_delay);
+
+	intel_dp->DP = DP;
 }
 
 static bool
-- 
2.4.10

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

Re: [PATCH v2 13/14] drm/i915: Use intel_dp->DP in eDP PLL setup

[Jani Nikula]

On Tue, 10 Nov 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use intel_dp->DP in the eDP PLL setup, instead of doing RMWs.
>
> To do this we need to move DP_AUDIO_OUTPUT_ENABLE setup to happen later,
> so that we don't enable audio accidentally while configuring the PLL.
>
> Note that actually we already enabled audio before the port due to
> the double port register write magic required by VLV/CHV from
> 7b713f50d78b6 ("drm/i915: Fix eDP link training when switching pipes on VLV/CHV")
> So that gets changed now to keep audio off as long as the port is off.
>
> Also intel_dp_link_down() must be made to update intel_dp->DP so that we
> don't re-enable the port by accident when turning off the PLL. This is
> safe now that we don't call intel_dp_link_down() during link retraining.
>
> v2: Add a note about the audio vs. port enable (Daniel)
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



> ---
>  drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++---------------------------
>  1 file changed, 14 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 019283f..d93f9f3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1551,23 +1551,18 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
>  	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 dpa_ctl;
>  
>  	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
>  		      crtc->config->port_clock);
> -	dpa_ctl = I915_READ(DP_A);
> -	dpa_ctl &= ~DP_PLL_FREQ_MASK;
>  
> -	if (crtc->config->port_clock == 162000) {
> -		dpa_ctl |= DP_PLL_FREQ_162MHZ;
> +	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
> +
> +	if (crtc->config->port_clock == 162000)
>  		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
> -	} else {
> -		dpa_ctl |= DP_PLL_FREQ_270MHZ;
> +	else
>  		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> -	}
> -
> -	I915_WRITE(DP_A, dpa_ctl);
>  
> +	I915_WRITE(DP_A, intel_dp->DP);
>  	POSTING_READ(DP_A);
>  	udelay(500);
>  }
> @@ -1616,9 +1611,6 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
>  	intel_dp->DP |= DP_VOLTAGE_0_4 | DP_PRE_EMPHASIS_0;
>  	intel_dp->DP |= DP_PORT_WIDTH(crtc->config->lane_count);
>  
> -	if (crtc->config->has_audio)
> -		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
> -
>  	/* Split out the IBX/CPU vs CPT settings */
>  
>  	if (IS_GEN7(dev) && port == PORT_A) {
> @@ -2179,20 +2171,14 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 dpa_ctl;
>  
>  	assert_pipe_disabled(dev_priv, crtc->pipe);
>  	assert_dp_port_disabled(intel_dp);
>  	assert_edp_pll_disabled(dev_priv);
>  
>  	DRM_DEBUG_KMS("\n");
> -	dpa_ctl = I915_READ(DP_A);
> -
> -	/* We don't adjust intel_dp->DP while tearing down the link, to
> -	 * facilitate link retraining (e.g. after hotplug). Hence clear all
> -	 * enable bits here to ensure that we don't enable too much. */
> -	intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
>  	intel_dp->DP |= DP_PLL_ENABLE;
> +
>  	I915_WRITE(DP_A, intel_dp->DP);
>  	POSTING_READ(DP_A);
>  	udelay(200);
> @@ -2203,19 +2189,14 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 dpa_ctl;
>  
>  	assert_pipe_disabled(dev_priv, crtc->pipe);
>  	assert_dp_port_disabled(intel_dp);
>  	assert_edp_pll_enabled(dev_priv);
>  
> -	dpa_ctl = I915_READ(DP_A);
> +	intel_dp->DP &= ~DP_PLL_ENABLE;
>  
> -	/* We can't rely on the value tracked for the DP register in
> -	 * intel_dp->DP because link_down must not change that (otherwise link
> -	 * re-training will fail. */
> -	dpa_ctl &= ~DP_PLL_ENABLE;
> -	I915_WRITE(DP_A, dpa_ctl);
> +	I915_WRITE(DP_A, intel_dp->DP);
>  	POSTING_READ(DP_A);
>  	udelay(200);
>  }
> @@ -2571,6 +2552,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc =
> +		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>  
>  	/* enable with pattern 1 (as per spec) */
>  	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> @@ -2586,6 +2569,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
>  	 * fail when the power sequencer is freshly used for this port.
>  	 */
>  	intel_dp->DP |= DP_PORT_EN;
> +	if (crtc->config->has_audio)
> +		intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
>  
>  	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>  	POSTING_READ(intel_dp->output_reg);
> @@ -3726,6 +3711,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	}
>  
>  	msleep(intel_dp->panel_power_down_delay);
> +
> +	intel_dp->DP = DP;
>  }
>  
>  static bool

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

[PATCH 14/14] drm/i915: Configure eDP PLL freq from ironlake_edp_pll_on()

[ville.syrjala]

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

ironlake_set_pll_cpu_edp() only gets called just before
ironlake_edp_pll_on(), so just pull the code into ironlake_edp_pll_on().

Also toss in a debug print into ironlake_edp_pll_off() to match the one
we have in ironlake_edp_pll_on().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 45 +++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 63659e7..ba4cbf5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1542,28 +1542,6 @@ found:
 	return true;
 }
 
-static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
-		      crtc->config->port_clock);
-
-	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
-
-	if (crtc->config->port_clock == 162000)
-		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
-	else
-		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
-
-	I915_WRITE(DP_A, intel_dp->DP);
-	POSTING_READ(DP_A);
-	udelay(500);
-}
-
 void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      const struct intel_crtc_state *pipe_config)
 {
@@ -2173,7 +2151,20 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
 	assert_dp_port_disabled(intel_dp);
 	assert_edp_pll_disabled(dev_priv);
 
-	DRM_DEBUG_KMS("\n");
+	DRM_DEBUG_KMS("enabling eDP PLL for clock %d\n",
+		      crtc->config->port_clock);
+
+	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
+
+	if (crtc->config->port_clock == 162000)
+		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
+	else
+		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
+
+	I915_WRITE(DP_A, intel_dp->DP);
+	POSTING_READ(DP_A);
+	udelay(500);
+
 	intel_dp->DP |= DP_PLL_ENABLE;
 
 	I915_WRITE(DP_A, intel_dp->DP);
@@ -2191,6 +2182,8 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
 	assert_dp_port_disabled(intel_dp);
 	assert_edp_pll_enabled(dev_priv);
 
+	DRM_DEBUG_KMS("disabling eDP PLL\n");
+
 	intel_dp->DP &= ~DP_PLL_ENABLE;
 
 	I915_WRITE(DP_A, intel_dp->DP);
@@ -2390,6 +2383,8 @@ static void ilk_post_disable_dp(struct intel_encoder *encoder)
 	enum port port = dp_to_dig_port(intel_dp)->port;
 
 	intel_dp_link_down(intel_dp);
+
+	/* Only ilk+ has port A */
 	if (port == PORT_A)
 		ironlake_edp_pll_off(intel_dp);
 }
@@ -2670,10 +2665,8 @@ static void g4x_pre_enable_dp(struct intel_encoder *encoder)
 	}
 
 	/* Only ilk+ has port A */
-	if (port == PORT_A) {
-		ironlake_set_pll_cpu_edp(intel_dp);
+	if (port == PORT_A)
 		ironlake_edp_pll_on(intel_dp);
-	}
 }
 
 static void vlv_detach_power_sequencer(struct intel_dp *intel_dp)
-- 
2.4.10

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

Re: [PATCH 14/14] drm/i915: Configure eDP PLL freq from ironlake_edp_pll_on()

[Daniel Vetter]

On Thu, Oct 29, 2015 at 09:26:03PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> ironlake_set_pll_cpu_edp() only gets called just before
> ironlake_edp_pll_on(), so just pull the code into ironlake_edp_pll_on().
> 
> Also toss in a debug print into ironlake_edp_pll_off() to match the one
> we have in ironlake_edp_pll_on().
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 45 +++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 63659e7..ba4cbf5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1542,28 +1542,6 @@ found:
>  	return true;
>  }
>  
> -static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> -	struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
> -		      crtc->config->port_clock);
> -
> -	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
> -
> -	if (crtc->config->port_clock == 162000)
> -		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
> -	else
> -		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> -
> -	I915_WRITE(DP_A, intel_dp->DP);
> -	POSTING_READ(DP_A);
> -	udelay(500);
> -}
> -
>  void intel_dp_set_link_params(struct intel_dp *intel_dp,
>  			      const struct intel_crtc_state *pipe_config)
>  {
> @@ -2173,7 +2151,20 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>  	assert_dp_port_disabled(intel_dp);
>  	assert_edp_pll_disabled(dev_priv);
>  
> -	DRM_DEBUG_KMS("\n");
> +	DRM_DEBUG_KMS("enabling eDP PLL for clock %d\n",
> +		      crtc->config->port_clock);
> +
> +	intel_dp->DP &= ~DP_PLL_FREQ_MASK;
> +
> +	if (crtc->config->port_clock == 162000)
> +		intel_dp->DP |= DP_PLL_FREQ_162MHZ;
> +	else
> +		intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> +
> +	I915_WRITE(DP_A, intel_dp->DP);
> +	POSTING_READ(DP_A);
> +	udelay(500);
> +
>  	intel_dp->DP |= DP_PLL_ENABLE;
>  
>  	I915_WRITE(DP_A, intel_dp->DP);
> @@ -2191,6 +2182,8 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
>  	assert_dp_port_disabled(intel_dp);
>  	assert_edp_pll_enabled(dev_priv);
>  
> +	DRM_DEBUG_KMS("disabling eDP PLL\n");
> +
>  	intel_dp->DP &= ~DP_PLL_ENABLE;
>  
>  	I915_WRITE(DP_A, intel_dp->DP);
> @@ -2390,6 +2383,8 @@ static void ilk_post_disable_dp(struct intel_encoder *encoder)
>  	enum port port = dp_to_dig_port(intel_dp)->port;
>  
>  	intel_dp_link_down(intel_dp);
> +
> +	/* Only ilk+ has port A */
>  	if (port == PORT_A)
>  		ironlake_edp_pll_off(intel_dp);
>  }
> @@ -2670,10 +2665,8 @@ static void g4x_pre_enable_dp(struct intel_encoder *encoder)
>  	}
>  
>  	/* Only ilk+ has port A */
> -	if (port == PORT_A) {
> -		ironlake_set_pll_cpu_edp(intel_dp);
> +	if (port == PORT_A)
>  		ironlake_edp_pll_on(intel_dp);
> -	}
>  }
>  
>  static void vlv_detach_power_sequencer(struct intel_dp *intel_dp)
> -- 
> 2.4.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/14] drm/i915: FIFO underrun elimination for PCH platforms

[Jani Nikula]

On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This series eliminates all spurious PCH FIFO underrun reports on my
> machines during a BAT run ('-t basic -x reload -x suspend' actually).
> It also eliminates the non-spurious but expected underrun reports
> on ILK.
>
> I also embarked on a small scale cleanup of the CPU eDP PLL code while
> I was trying to get to the bottom of the underruns on ILK. So I figured
> I'd include that work here as well.
>
> I've tested this on ILK, SNB, two IVBs, and one HSW, with as many
> displays plugged in as possible. What I could actually test is HSW/BDW
> CRT output since I have no machine for that.
>
> The series is available here:
> git://github.com/vsyrjala/linux.git pch_fifo_underrun_fix_4

I spammed "a few" potentially relevant bugs with test requests:

https://bugs.freedesktop.org/show_bug.cgi?id=74102
https://bugs.freedesktop.org/show_bug.cgi?id=85906
https://bugs.freedesktop.org/show_bug.cgi?id=88977
https://bugs.freedesktop.org/show_bug.cgi?id=89518
https://bugs.freedesktop.org/show_bug.cgi?id=89806
https://bugs.freedesktop.org/show_bug.cgi?id=90239
https://bugs.freedesktop.org/show_bug.cgi?id=90307

BR,
Jani.


-- 
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 00/14] drm/i915: FIFO underrun elimination for PCH platforms

[Ville Syrjälä]

On Thu, Oct 29, 2015 at 09:25:49PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This series eliminates all spurious PCH FIFO underrun reports on my
> machines during a BAT run ('-t basic -x reload -x suspend' actually).
> It also eliminates the non-spurious but expected underrun reports
> on ILK.
> 
> I also embarked on a small scale cleanup of the CPU eDP PLL code while
> I was trying to get to the bottom of the underruns on ILK. So I figured
> I'd include that work here as well.
> 
> I've tested this on ILK, SNB, two IVBs, and one HSW, with as many
> displays plugged in as possible. What I could actually test is HSW/BDW
> CRT output since I have no machine for that.
> 
> The series is available here:
> git://github.com/vsyrjala/linux.git pch_fifo_underrun_fix_4
> 
> Ville Syrjälä (14):
>   drm/i915: Don't use intel_pipe_to_cpu_transcoder() when there's a pipe
>     config around
>   drm/i915: Set sync polarity from adjusted mode for TRANS_DP_CTL
>   drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB
>   drm/i915: Enable PCH FIFO underruns later on HSW+
>   drm/i915: Re-enable PCH FIO underrun reporting after pipe has been
>     disabled
>   drm/i915: Check for FIFO underruns after modeset on IVB/HSW and
>     CPT/PPT
>   drm/i915: Check for CPT and not !IBX in
>     ironlake_disable_pch_transcoder()
>   drm/i915: Disable FIFO underrun reporting around IBX transcoder B
>     workaround
>   drm/i915: Hide underruns from eDP PLL and port enable on ILK
>   drm/i915: s/DP_PLL_FREQ_160MHZ/DP_PLL_FREQ_162MHZ/
>   drm/i915: Remove ILK-A eDP PLL workaround notes
>   drm/i915: Clean up eDP PLL state asserts
>   drm/i915: Use intel_dp->DP in eDP PLL setup
>   drm/i915: Configure eDP PLL freq from ironlake_edp_pll_on()

Entire series applied. Thanks for the reviews.

> 
>  drivers/gpu/drm/i915/i915_reg.h            |   2 +-
>  drivers/gpu/drm/i915/intel_display.c       |  64 +++++++----
>  drivers/gpu/drm/i915/intel_dp.c            | 179 ++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h           |  12 +-
>  drivers/gpu/drm/i915/intel_fifo_underrun.c | 121 +++++++++++++++----
>  drivers/gpu/drm/i915/intel_hdmi.c          |  11 ++
>  drivers/gpu/drm/i915/intel_sdvo.c          |  11 ++
>  7 files changed, 285 insertions(+), 115 deletions(-)
> 
> -- 
> 2.4.10

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx