[v2,3/7] drm/i915: Fix broken transcoder err state
diff mbox series

Message ID 20200211172532.14287-4-anshuman.gupta@intel.com
State New
Headers show
Series
  • 3 display pipes combination system support
Related show

Commit Message

Anshuman Gupta Feb. 11, 2020, 5:25 p.m. UTC
Skip the transcoder whose pipe is disabled while
initializing transcoder error state in 3 non-contiguous
display pipe system.

v2:
- Don't skip EDP_TRANSCODER error state. [Ville]
- Use a helper has_transcoder(). [Ville]

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c       |  2 +-
 drivers/gpu/drm/i915/display/intel_display_types.h | 14 ++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h                    |  2 ++
 3 files changed, 17 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä Feb. 20, 2020, 5:16 p.m. UTC | #1
On Tue, Feb 11, 2020 at 10:55:28PM +0530, Anshuman Gupta wrote:
> Skip the transcoder whose pipe is disabled while
> initializing transcoder error state in 3 non-contiguous
> display pipe system.
> 
> v2:
> - Don't skip EDP_TRANSCODER error state. [Ville]
> - Use a helper has_transcoder(). [Ville]
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c       |  2 +-
>  drivers/gpu/drm/i915/display/intel_display_types.h | 14 ++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h                    |  2 ++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 5333f7a7db42..a3649020ea97 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -19051,7 +19051,7 @@ intel_display_capture_error_state(struct drm_i915_private *dev_priv)
>  	for (i = 0; i < ARRAY_SIZE(error->transcoder); i++) {
>  		enum transcoder cpu_transcoder = transcoders[i];
>  
> -		if (!INTEL_INFO(dev_priv)->trans_offsets[cpu_transcoder])
> +		if (!has_transcoder(dev_priv, cpu_transcoder))
>  			continue;
>  
>  		error->transcoder[i].available = true;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 14e3d78fef7c..d359f1636ba8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1626,4 +1626,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
>  	return i915_ggtt_offset(state->vma);
>  }
>  
> +static inline bool
> +has_transcoder(struct drm_i915_private *dev_priv, enum transcoder transcoder) {

{ is in the wrong place.

> +	switch (transcoder) {
> +	case TRANSCODER_EDP:
> +		return HAS_TRANSCODER_EDP(dev_priv);
> +	case TRANSCODER_DSI_0:
> +		return HAS_TRANSCODER_DSI0(dev_priv);
> +	case TRANSCODER_DSI_1:
> +		return HAS_TRANSCODER_DSI1(dev_priv);

The error capture so far doesn't care about DSI, so I wouldn't bother
with these for now.

> +	default:
> +		return INTEL_INFO(dev_priv)->pipe_mask & BIT(transcoder);
> +	}
> +}

This functions has one user so no point in putting it into a header.

> +
>  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index da509d9b8895..17bbaf7f0844 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1674,6 +1674,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->has_fpga_dbg)
>  #define HAS_PSR(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_psr)
>  #define HAS_TRANSCODER_EDP(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_EDP] != 0)
> +#define HAS_TRANSCODER_DSI0(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_DSI_0] != 0)
> +#define HAS_TRANSCODER_DSI1(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_DSI_1] != 0)
>  
>  #define HAS_RC6(dev_priv)		 (INTEL_INFO(dev_priv)->has_rc6)
>  #define HAS_RC6p(dev_priv)		 (INTEL_INFO(dev_priv)->has_rc6p)
> -- 
> 2.24.0
Ville Syrjälä Feb. 20, 2020, 8:02 p.m. UTC | #2
On Thu, Feb 20, 2020 at 07:16:32PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2020 at 10:55:28PM +0530, Anshuman Gupta wrote:
> > Skip the transcoder whose pipe is disabled while
> > initializing transcoder error state in 3 non-contiguous
> > display pipe system.
> > 
> > v2:
> > - Don't skip EDP_TRANSCODER error state. [Ville]
> > - Use a helper has_transcoder(). [Ville]
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c       |  2 +-
> >  drivers/gpu/drm/i915/display/intel_display_types.h | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.h                    |  2 ++
> >  3 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 5333f7a7db42..a3649020ea97 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -19051,7 +19051,7 @@ intel_display_capture_error_state(struct drm_i915_private *dev_priv)
> >  	for (i = 0; i < ARRAY_SIZE(error->transcoder); i++) {
> >  		enum transcoder cpu_transcoder = transcoders[i];
> >  
> > -		if (!INTEL_INFO(dev_priv)->trans_offsets[cpu_transcoder])
> > +		if (!has_transcoder(dev_priv, cpu_transcoder))
> >  			continue;
> >  
> >  		error->transcoder[i].available = true;
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 14e3d78fef7c..d359f1636ba8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1626,4 +1626,18 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> >  	return i915_ggtt_offset(state->vma);
> >  }
> >  
> > +static inline bool
> > +has_transcoder(struct drm_i915_private *dev_priv, enum transcoder transcoder) {
> 
> { is in the wrong place.

Oh and 'cpu_transcoder' is the variable name used everwhere else. Pls
stick to established patterns if possible.

> 
> > +	switch (transcoder) {
> > +	case TRANSCODER_EDP:
> > +		return HAS_TRANSCODER_EDP(dev_priv);
> > +	case TRANSCODER_DSI_0:
> > +		return HAS_TRANSCODER_DSI0(dev_priv);
> > +	case TRANSCODER_DSI_1:
> > +		return HAS_TRANSCODER_DSI1(dev_priv);
> 
> The error capture so far doesn't care about DSI, so I wouldn't bother
> with these for now.
> 
> > +	default:
> > +		return INTEL_INFO(dev_priv)->pipe_mask & BIT(transcoder);
> > +	}
> > +}
> 
> This functions has one user so no point in putting it into a header.
> 
> > +
> >  #endif /*  __INTEL_DISPLAY_TYPES_H__ */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index da509d9b8895..17bbaf7f0844 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1674,6 +1674,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >  #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->has_fpga_dbg)
> >  #define HAS_PSR(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_psr)
> >  #define HAS_TRANSCODER_EDP(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_EDP] != 0)
> > +#define HAS_TRANSCODER_DSI0(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_DSI_0] != 0)
> > +#define HAS_TRANSCODER_DSI1(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_DSI_1] != 0)
> >  
> >  #define HAS_RC6(dev_priv)		 (INTEL_INFO(dev_priv)->has_rc6)
> >  #define HAS_RC6p(dev_priv)		 (INTEL_INFO(dev_priv)->has_rc6p)
> > -- 
> > 2.24.0
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 5333f7a7db42..a3649020ea97 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -19051,7 +19051,7 @@  intel_display_capture_error_state(struct drm_i915_private *dev_priv)
 	for (i = 0; i < ARRAY_SIZE(error->transcoder); i++) {
 		enum transcoder cpu_transcoder = transcoders[i];
 
-		if (!INTEL_INFO(dev_priv)->trans_offsets[cpu_transcoder])
+		if (!has_transcoder(dev_priv, cpu_transcoder))
 			continue;
 
 		error->transcoder[i].available = true;
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 14e3d78fef7c..d359f1636ba8 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1626,4 +1626,18 @@  static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
 	return i915_ggtt_offset(state->vma);
 }
 
+static inline bool
+has_transcoder(struct drm_i915_private *dev_priv, enum transcoder transcoder) {
+	switch (transcoder) {
+	case TRANSCODER_EDP:
+		return HAS_TRANSCODER_EDP(dev_priv);
+	case TRANSCODER_DSI_0:
+		return HAS_TRANSCODER_DSI0(dev_priv);
+	case TRANSCODER_DSI_1:
+		return HAS_TRANSCODER_DSI1(dev_priv);
+	default:
+		return INTEL_INFO(dev_priv)->pipe_mask & BIT(transcoder);
+	}
+}
+
 #endif /*  __INTEL_DISPLAY_TYPES_H__ */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index da509d9b8895..17bbaf7f0844 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1674,6 +1674,8 @@  IS_SUBPLATFORM(const struct drm_i915_private *i915,
 #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->has_fpga_dbg)
 #define HAS_PSR(dev_priv)		 (INTEL_INFO(dev_priv)->display.has_psr)
 #define HAS_TRANSCODER_EDP(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_EDP] != 0)
+#define HAS_TRANSCODER_DSI0(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_DSI_0] != 0)
+#define HAS_TRANSCODER_DSI1(dev_priv)	 (INTEL_INFO(dev_priv)->trans_offsets[TRANSCODER_DSI_1] != 0)
 
 #define HAS_RC6(dev_priv)		 (INTEL_INFO(dev_priv)->has_rc6)
 #define HAS_RC6p(dev_priv)		 (INTEL_INFO(dev_priv)->has_rc6p)