[PATCH] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[PATCH] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[ville.syrjala]

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

LPT/WPT only have transcoder A, so we shouldn't look at FIFO underruns
for transocoder B/C. And more importnatnly we should not consider
the state of underrun reporting for transcoders B/C when checking
whether we can enable the south error interrupt.

The whole thing is a bit of mess since we store the underrun reporting
state for transcoder A under intel_crtc for pipe A, irrespective of
which pipe may actually be driving the transcoder. But I figured trying
to change that would result in more churn.

Caveat: totally untested due to lack of hw, but might explain some of
the HSW/BDW BAT fails...

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

diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index bda526660e20..04bf625a1b6c 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -48,6 +48,13 @@
  * The code also supports underrun detection on the PCH transcoder.
  */
 
+static bool cpt_transcoder_exists(struct drm_i915_private *dev_priv,
+				  enum transcoder pch_transcoder)
+{
+	/* HSW/BDW have only transcoder A */
+	return !HAS_DDI(dev_priv) || pch_transcoder == TRANSCODER_A;
+}
+
 static bool ivb_can_enable_err_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -69,13 +76,16 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
 static bool cpt_can_enable_serr_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	struct intel_crtc *crtc;
+	enum transcoder pch_transcoder;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	for_each_pipe(dev_priv, pipe) {
-		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+	for_each_pipe(dev_priv, pch_transcoder) {
+		struct intel_crtc *crtc =
+			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pch_transcoder]);
+
+		if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
+			continue;
 
 		if (crtc->pch_fifo_underrun_disabled)
 			return false;
@@ -206,6 +216,9 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
+		return;
+
 	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
 		return;
 
@@ -222,6 +235,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
+		return;
+
 	if (enable) {
 		I915_WRITE(SERR_INT,
 			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
-- 
2.4.10

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

Re: [PATCH] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[Ville Syrjälä]

On Wed, Nov 25, 2015 at 05:11:23PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> LPT/WPT only have transcoder A, so we shouldn't look at FIFO underruns
> for transocoder B/C. And more importnatnly we should not consider
> the state of underrun reporting for transcoders B/C when checking
> whether we can enable the south error interrupt.
> 
> The whole thing is a bit of mess since we store the underrun reporting
> state for transcoder A under intel_crtc for pipe A, irrespective of
> which pipe may actually be driving the transcoder. But I figured trying
> to change that would result in more churn.
> 
> Caveat: totally untested due to lack of hw, but might explain some of
> the HSW/BDW BAT fails...

Hmm. Actually it can't since we don't use the CRCs that get signalled
via the south error interrupt. Back to the drawing board...

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fifo_underrun.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index bda526660e20..04bf625a1b6c 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -48,6 +48,13 @@
>   * The code also supports underrun detection on the PCH transcoder.
>   */
>  
> +static bool cpt_transcoder_exists(struct drm_i915_private *dev_priv,
> +				  enum transcoder pch_transcoder)
> +{
> +	/* HSW/BDW have only transcoder A */
> +	return !HAS_DDI(dev_priv) || pch_transcoder == TRANSCODER_A;
> +}
> +
>  static bool ivb_can_enable_err_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -69,13 +76,16 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
>  static bool cpt_can_enable_serr_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum pipe pipe;
> -	struct intel_crtc *crtc;
> +	enum transcoder pch_transcoder;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	for_each_pipe(dev_priv, pipe) {
> -		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +	for_each_pipe(dev_priv, pch_transcoder) {
> +		struct intel_crtc *crtc =
> +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pch_transcoder]);
> +
> +		if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> +			continue;
>  
>  		if (crtc->pch_fifo_underrun_disabled)
>  			return false;
> @@ -206,6 +216,9 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> +		return;
> +
>  	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
>  		return;
>  
> @@ -222,6 +235,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> +		return;
> +
>  	if (enable) {
>  		I915_WRITE(SERR_INT,
>  			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> -- 
> 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

[PATCH 1/2] drm/i915: Add HAS_PCH_LPT_H()

[ville.syrjala]

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

We have HAS_PCH_LPT_LP() already, so add HAS_PCH_LPT_H() and use it
where appropriate.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bc865e234df2..9ab3e25ddf38 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2615,6 +2615,7 @@ struct drm_i915_cmd_table {
 #define HAS_PCH_SPT(dev) (INTEL_PCH_TYPE(dev) == PCH_SPT)
 #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT)
 #define HAS_PCH_LPT_LP(dev) (__I915__(dev)->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE)
+#define HAS_PCH_LPT_H(dev) (__I915__(dev)->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE)
 #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
 #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
 #define HAS_PCH_NOP(dev) (INTEL_PCH_TYPE(dev) == PCH_NOP)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 634dba423ae9..644e2fc9c67d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -711,7 +711,7 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 		if (index)
 			return 0;
 		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
-	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
+	} else if (HAS_PCH_LPT_H(dev_priv)) {
 		/* Workaround for non-ULT HSW */
 		switch (index) {
 		case 0: return 63;
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a24df35e11e7..d4c81df318b6 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1300,7 +1300,7 @@ static u32 lpt_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
 	else
 		mul = 128;
 
-	if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE)
+	if (HAS_PCH_LPT_H(dev_priv))
 		clock = MHz(135); /* LPT:H */
 	else
 		clock = MHz(24); /* LPT:LP */
-- 
2.4.10

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

[PATCH v2 2/2] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[ville.syrjala]

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

LPT/WPT only have transcoder A, so we shouldn't look at FIFO underruns
for transocoder B/C. And more importnatnly we should not consider
the state of underrun reporting for transcoders B/C when checking
whether we can enable the south error interrupt.

The whole thing is a bit of mess since we store the underrun reporting
state for transcoder A under intel_crtc for pipe A, irrespective of
which pipe may actually be driving the transcoder. But I figured trying
to change that would result in more churn.

Caveat: Still untested

v2: Use HAS_PCH_LPT_H instead of HAS_DDI
    Use cpt_check_pch_fifo_underruns() on LPT-H/WPT-H too

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

diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index bda526660e20..3d3acc8b8367 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -48,6 +48,14 @@
  * The code also supports underrun detection on the PCH transcoder.
  */
 
+static bool cpt_transcoder_exists(struct drm_i915_private *dev_priv,
+				  enum transcoder pch_transcoder)
+{
+	/* LPT-H/WPT-H have only transcoder A */
+	return HAS_PCH_CPT(dev_priv) ||
+		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
+}
+
 static bool ivb_can_enable_err_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -69,13 +77,16 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
 static bool cpt_can_enable_serr_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	struct intel_crtc *crtc;
+	enum transcoder pch_transcoder;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	for_each_pipe(dev_priv, pipe) {
-		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+	for_each_pipe(dev_priv, pch_transcoder) {
+		struct intel_crtc *crtc =
+			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pch_transcoder]);
+
+		if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
+			continue;
 
 		if (crtc->pch_fifo_underrun_disabled)
 			return false;
@@ -206,6 +217,9 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
+		return;
+
 	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
 		return;
 
@@ -222,6 +236,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
+		return;
+
 	if (enable) {
 		I915_WRITE(SERR_INT,
 			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
@@ -436,7 +453,7 @@ void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
 		if (crtc->pch_fifo_underrun_disabled)
 			continue;
 
-		if (HAS_PCH_CPT(dev_priv))
+		if (HAS_PCH_CPT(dev_priv) || HAS_PCH_LPT_H(dev_priv))
 			cpt_check_pch_fifo_underruns(crtc);
 	}
 
-- 
2.4.10

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

Re: [PATCH v2 2/2] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[Daniel Vetter]

On Thu, Nov 26, 2015 at 10:55:53PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> LPT/WPT only have transcoder A, so we shouldn't look at FIFO underruns
> for transocoder B/C. And more importnatnly we should not consider
> the state of underrun reporting for transcoders B/C when checking
> whether we can enable the south error interrupt.
> 
> The whole thing is a bit of mess since we store the underrun reporting
> state for transcoder A under intel_crtc for pipe A, irrespective of
> which pipe may actually be driving the transcoder. But I figured trying
> to change that would result in more churn.
> 
> Caveat: Still untested
> 
> v2: Use HAS_PCH_LPT_H instead of HAS_DDI
>     Use cpt_check_pch_fifo_underruns() on LPT-H/WPT-H too
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've battled hsw lpt fifo underrun issues last week and never seen a fifo
underrun on pipe B/C. Have you seen them anywhere really?

If not I think we can skip this patch here, since I think I tracked it all
down.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fifo_underrun.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index bda526660e20..3d3acc8b8367 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -48,6 +48,14 @@
>   * The code also supports underrun detection on the PCH transcoder.
>   */
>  
> +static bool cpt_transcoder_exists(struct drm_i915_private *dev_priv,
> +				  enum transcoder pch_transcoder)
> +{
> +	/* LPT-H/WPT-H have only transcoder A */
> +	return HAS_PCH_CPT(dev_priv) ||
> +		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
> +}
> +
>  static bool ivb_can_enable_err_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -69,13 +77,16 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
>  static bool cpt_can_enable_serr_int(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum pipe pipe;
> -	struct intel_crtc *crtc;
> +	enum transcoder pch_transcoder;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	for_each_pipe(dev_priv, pipe) {
> -		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +	for_each_pipe(dev_priv, pch_transcoder) {
> +		struct intel_crtc *crtc =
> +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pch_transcoder]);
> +
> +		if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> +			continue;
>  
>  		if (crtc->pch_fifo_underrun_disabled)
>  			return false;
> @@ -206,6 +217,9 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> +		return;
> +
>  	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
>  		return;
>  
> @@ -222,6 +236,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> +		return;
> +
>  	if (enable) {
>  		I915_WRITE(SERR_INT,
>  			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> @@ -436,7 +453,7 @@ void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
>  		if (crtc->pch_fifo_underrun_disabled)
>  			continue;
>  
> -		if (HAS_PCH_CPT(dev_priv))
> +		if (HAS_PCH_CPT(dev_priv) || HAS_PCH_LPT_H(dev_priv))
>  			cpt_check_pch_fifo_underruns(crtc);
>  	}
>  
> -- 
> 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 v2 2/2] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[Ville Syrjälä]

On Mon, Nov 30, 2015 at 09:25:03AM +0100, Daniel Vetter wrote:
> On Thu, Nov 26, 2015 at 10:55:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > LPT/WPT only have transcoder A, so we shouldn't look at FIFO underruns
> > for transocoder B/C. And more importnatnly we should not consider
> > the state of underrun reporting for transcoders B/C when checking
> > whether we can enable the south error interrupt.
> > 
> > The whole thing is a bit of mess since we store the underrun reporting
> > state for transcoder A under intel_crtc for pipe A, irrespective of
> > which pipe may actually be driving the transcoder. But I figured trying
> > to change that would result in more churn.
> > 
> > Caveat: Still untested
> > 
> > v2: Use HAS_PCH_LPT_H instead of HAS_DDI
> >     Use cpt_check_pch_fifo_underruns() on LPT-H/WPT-H too
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I've battled hsw lpt fifo underrun issues last week and never seen a fifo
> underrun on pipe B/C. Have you seen them anywhere really?

No, I've not seen it. We mark underrun detection as disabled for active
pipes in intel_sanitize_crtc(), and we never turn it back on except for
transcoder A, so I'm thinking we'll never actually enable the south
error interrup on LPT if a non-pch port is enabled on boot.

Note that so far I didn't check if that's actually the case, but based
on the code I can't come to any other conclusion.

> 
> If not I think we can skip this patch here, since I think I tracked it all
> down.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_fifo_underrun.c | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > index bda526660e20..3d3acc8b8367 100644
> > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > @@ -48,6 +48,14 @@
> >   * The code also supports underrun detection on the PCH transcoder.
> >   */
> >  
> > +static bool cpt_transcoder_exists(struct drm_i915_private *dev_priv,
> > +				  enum transcoder pch_transcoder)
> > +{
> > +	/* LPT-H/WPT-H have only transcoder A */
> > +	return HAS_PCH_CPT(dev_priv) ||
> > +		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
> > +}
> > +
> >  static bool ivb_can_enable_err_int(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -69,13 +77,16 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
> >  static bool cpt_can_enable_serr_int(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	enum pipe pipe;
> > -	struct intel_crtc *crtc;
> > +	enum transcoder pch_transcoder;
> >  
> >  	assert_spin_locked(&dev_priv->irq_lock);
> >  
> > -	for_each_pipe(dev_priv, pipe) {
> > -		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > +	for_each_pipe(dev_priv, pch_transcoder) {
> > +		struct intel_crtc *crtc =
> > +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pch_transcoder]);
> > +
> > +		if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > +			continue;
> >  
> >  		if (crtc->pch_fifo_underrun_disabled)
> >  			return false;
> > @@ -206,6 +217,9 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
> >  
> >  	assert_spin_locked(&dev_priv->irq_lock);
> >  
> > +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > +		return;
> > +
> >  	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
> >  		return;
> >  
> > @@ -222,6 +236,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > +		return;
> > +
> >  	if (enable) {
> >  		I915_WRITE(SERR_INT,
> >  			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> > @@ -436,7 +453,7 @@ void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
> >  		if (crtc->pch_fifo_underrun_disabled)
> >  			continue;
> >  
> > -		if (HAS_PCH_CPT(dev_priv))
> > +		if (HAS_PCH_CPT(dev_priv) || HAS_PCH_LPT_H(dev_priv))
> >  			cpt_check_pch_fifo_underruns(crtc);
> >  	}
> >  
> > -- 
> > 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 v2 2/2] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[Daniel Vetter]

On Mon, Nov 30, 2015 at 04:08:34PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 30, 2015 at 09:25:03AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 26, 2015 at 10:55:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > LPT/WPT only have transcoder A, so we shouldn't look at FIFO underruns
> > > for transocoder B/C. And more importnatnly we should not consider
> > > the state of underrun reporting for transcoders B/C when checking
> > > whether we can enable the south error interrupt.
> > > 
> > > The whole thing is a bit of mess since we store the underrun reporting
> > > state for transcoder A under intel_crtc for pipe A, irrespective of
> > > which pipe may actually be driving the transcoder. But I figured trying
> > > to change that would result in more churn.
> > > 
> > > Caveat: Still untested
> > > 
> > > v2: Use HAS_PCH_LPT_H instead of HAS_DDI
> > >     Use cpt_check_pch_fifo_underruns() on LPT-H/WPT-H too
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I've battled hsw lpt fifo underrun issues last week and never seen a fifo
> > underrun on pipe B/C. Have you seen them anywhere really?
> 
> No, I've not seen it. We mark underrun detection as disabled for active
> pipes in intel_sanitize_crtc(), and we never turn it back on except for
> transcoder A, so I'm thinking we'll never actually enable the south
> error interrup on LPT if a non-pch port is enabled on boot.

Hm right this is broken for pch ... I guess we need an IS_HSW(dev) : PCH_A
: pipe in there to get this right.

I do get plenty pch fifo underruns here, but I guess my bios boots with
vga on pipe A and hence I get away with this bug ;-)
 
> Note that so far I didn't check if that's actually the case, but based
> on the code I can't come to any other conclusion.

Yeah seems indeed broken, but I think simpler to fix in the crtc sanitize
code.
-Daniel

> 
> > 
> > If not I think we can skip this patch here, since I think I tracked it all
> > down.
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_fifo_underrun.c | 27 ++++++++++++++++++++++-----
> > >  1 file changed, 22 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > index bda526660e20..3d3acc8b8367 100644
> > > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > @@ -48,6 +48,14 @@
> > >   * The code also supports underrun detection on the PCH transcoder.
> > >   */
> > >  
> > > +static bool cpt_transcoder_exists(struct drm_i915_private *dev_priv,
> > > +				  enum transcoder pch_transcoder)
> > > +{
> > > +	/* LPT-H/WPT-H have only transcoder A */
> > > +	return HAS_PCH_CPT(dev_priv) ||
> > > +		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
> > > +}
> > > +
> > >  static bool ivb_can_enable_err_int(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -69,13 +77,16 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
> > >  static bool cpt_can_enable_serr_int(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > -	enum pipe pipe;
> > > -	struct intel_crtc *crtc;
> > > +	enum transcoder pch_transcoder;
> > >  
> > >  	assert_spin_locked(&dev_priv->irq_lock);
> > >  
> > > -	for_each_pipe(dev_priv, pipe) {
> > > -		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > > +	for_each_pipe(dev_priv, pch_transcoder) {
> > > +		struct intel_crtc *crtc =
> > > +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pch_transcoder]);
> > > +
> > > +		if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > > +			continue;
> > >  
> > >  		if (crtc->pch_fifo_underrun_disabled)
> > >  			return false;
> > > @@ -206,6 +217,9 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
> > >  
> > >  	assert_spin_locked(&dev_priv->irq_lock);
> > >  
> > > +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > > +		return;
> > > +
> > >  	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
> > >  		return;
> > >  
> > > @@ -222,6 +236,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > > +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > > +		return;
> > > +
> > >  	if (enable) {
> > >  		I915_WRITE(SERR_INT,
> > >  			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> > > @@ -436,7 +453,7 @@ void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
> > >  		if (crtc->pch_fifo_underrun_disabled)
> > >  			continue;
> > >  
> > > -		if (HAS_PCH_CPT(dev_priv))
> > > +		if (HAS_PCH_CPT(dev_priv) || HAS_PCH_LPT_H(dev_priv))
> > >  			cpt_check_pch_fifo_underruns(crtc);
> > >  	}
> > >  
> > > -- 
> > > 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

-- 
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