diff mbox series

[v2,1/7] drm/i915: Iterate over pipe and skip the disabled one

Message ID 20200211172532.14287-2-anshuman.gupta@intel.com (mailing list archive)
State New, archived
Headers show
Series 3 display pipes combination system support | expand

Commit Message

Gupta, Anshuman Feb. 11, 2020, 5:25 p.m. UTC 
It should not be assumed that a disabled display pipe will be
always last the pipe.
for_each_pipe() should iterate over I915_MAX_PIPES and check
for the disabled pipe and skip that pipe so that it should not
initialize the intel crtc for any disabled pipes.

Below compilation error require to be handle due to change in
for_each_pipe() macro.
"suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]"

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.h | 5 +++--
 drivers/gpu/drm/i915/i915_irq.c              | 6 ++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä Feb. 12, 2020, 1:50 p.m. UTC | #1 
On Tue, Feb 11, 2020 at 10:55:26PM +0530, Anshuman Gupta wrote:
> It should not be assumed that a disabled display pipe will be
> always last the pipe.
> for_each_pipe() should iterate over I915_MAX_PIPES and check
> for the disabled pipe and skip that pipe so that it should not
> initialize the intel crtc for any disabled pipes.
> 
> Below compilation error require to be handle due to change in
> for_each_pipe() macro.
> "suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]"
> 
> 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.h | 5 +++--
>  drivers/gpu/drm/i915/i915_irq.c              | 6 ++++--
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 75438a136d58..7a531e485b53 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -313,10 +313,11 @@ enum phy_fia {
>  };
>  
>  #define for_each_pipe(__dev_priv, __p) \
> -	for ((__p) = 0; (__p) < INTEL_NUM_PIPES(__dev_priv); (__p)++)
> +	for ((__p) = 0; (__p) < I915_MAX_PIPES; (__p)++) \
> +		for_each_if((INTEL_INFO(__dev_priv)->pipe_mask) & BIT(__p))
>  
>  #define for_each_pipe_masked(__dev_priv, __p, __mask) \
> -	for ((__p) = 0; (__p) < INTEL_NUM_PIPES(__dev_priv); (__p)++) \
> +	for_each_pipe(__dev_priv, __p) \
>  		for_each_if((__mask) & BIT(__p))

You didn't address my comments for this stuff! Please don't leave
review comments unaddressed, it's just wasting everyone's time.

>  
>  #define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3d0cd0960bd2..a26f2bf1b6ea 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1739,11 +1739,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
>  	if (pch_iir & SDE_POISON)
>  		drm_err(&dev_priv->drm, "PCH poison interrupt\n");
>  
> -	if (pch_iir & SDE_FDI_MASK)
> +	if (pch_iir & SDE_FDI_MASK) {
>  		for_each_pipe(dev_priv, pipe)
>  			drm_dbg(&dev_priv->drm, "  pipe %c FDI IIR: 0x%08x\n",
>  				pipe_name(pipe),
>  				I915_READ(FDI_RX_IIR(pipe)));
> +	}
>  
>  	if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE))
>  		drm_dbg(&dev_priv->drm, "PCH transcoder CRC done interrupt\n");
> @@ -1823,11 +1824,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
>  	if (pch_iir & SDE_AUDIO_CP_CHG_CPT)
>  		drm_dbg(&dev_priv->drm, "Audio CP change interrupt\n");
>  
> -	if (pch_iir & SDE_FDI_MASK_CPT)
> +	if (pch_iir & SDE_FDI_MASK_CPT) {
>  		for_each_pipe(dev_priv, pipe)
>  			drm_dbg(&dev_priv->drm, "  pipe %c FDI IIR: 0x%08x\n",
>  				pipe_name(pipe),
>  				I915_READ(FDI_RX_IIR(pipe)));
> +	}
>  
>  	if (pch_iir & SDE_ERROR_CPT)
>  		cpt_serr_int_handler(dev_priv);
> -- 
> 2.24.0
Gupta, Anshuman Feb. 18, 2020, 4:41 p.m. UTC | #2 
On 2020-02-12 at 15:50:12 +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2020 at 10:55:26PM +0530, Anshuman Gupta wrote:
> > It should not be assumed that a disabled display pipe will be
> > always last the pipe.
> > for_each_pipe() should iterate over I915_MAX_PIPES and check
> > for the disabled pipe and skip that pipe so that it should not
> > initialize the intel crtc for any disabled pipes.
> > 
> > Below compilation error require to be handle due to change in
> > for_each_pipe() macro.
> > "suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]"
> > 
> > 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.h | 5 +++--
> >  drivers/gpu/drm/i915/i915_irq.c              | 6 ++++--
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > index 75438a136d58..7a531e485b53 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -313,10 +313,11 @@ enum phy_fia {
> >  };
> >  
> >  #define for_each_pipe(__dev_priv, __p) \
> > -	for ((__p) = 0; (__p) < INTEL_NUM_PIPES(__dev_priv); (__p)++)
> > +	for ((__p) = 0; (__p) < I915_MAX_PIPES; (__p)++) \
> > +		for_each_if((INTEL_INFO(__dev_priv)->pipe_mask) & BIT(__p))
> >  
> >  #define for_each_pipe_masked(__dev_priv, __p, __mask) \
> > -	for ((__p) = 0; (__p) < INTEL_NUM_PIPES(__dev_priv); (__p)++) \
> > +	for_each_pipe(__dev_priv, __p) \
> >  		for_each_if((__mask) & BIT(__p))
> 
> You didn't address my comments for this stuff! Please don't leave
> review comments unaddressed, it's just wasting everyone's time.
Unfortunately this email and the other email with actual review comment
delivered to my spam folder, i observe the pattern with my mailbox if
there were two email thread with similar msgid, it is considering them
as spam. I don't have any idea why i am getting dulplicated email
thread with same msgid, i will raise ticket to intel IT for this.
My apology for unaddressed review comment, i will send a patch for
this.
Thanks,
Anshuman Gupta.
> 
> >  
> >  #define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 3d0cd0960bd2..a26f2bf1b6ea 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1739,11 +1739,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
> >  	if (pch_iir & SDE_POISON)
> >  		drm_err(&dev_priv->drm, "PCH poison interrupt\n");
> >  
> > -	if (pch_iir & SDE_FDI_MASK)
> > +	if (pch_iir & SDE_FDI_MASK) {
> >  		for_each_pipe(dev_priv, pipe)
> >  			drm_dbg(&dev_priv->drm, "  pipe %c FDI IIR: 0x%08x\n",
> >  				pipe_name(pipe),
> >  				I915_READ(FDI_RX_IIR(pipe)));
> > +	}
> >  
> >  	if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE))
> >  		drm_dbg(&dev_priv->drm, "PCH transcoder CRC done interrupt\n");
> > @@ -1823,11 +1824,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
> >  	if (pch_iir & SDE_AUDIO_CP_CHG_CPT)
> >  		drm_dbg(&dev_priv->drm, "Audio CP change interrupt\n");
> >  
> > -	if (pch_iir & SDE_FDI_MASK_CPT)
> > +	if (pch_iir & SDE_FDI_MASK_CPT) {
> >  		for_each_pipe(dev_priv, pipe)
> >  			drm_dbg(&dev_priv->drm, "  pipe %c FDI IIR: 0x%08x\n",
> >  				pipe_name(pipe),
> >  				I915_READ(FDI_RX_IIR(pipe)));
> > +	}
> >  
> >  	if (pch_iir & SDE_ERROR_CPT)
> >  		cpt_serr_int_handler(dev_priv);
> > -- 
> > 2.24.0
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 75438a136d58..7a531e485b53 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -313,10 +313,11 @@  enum phy_fia {
 };
 
 #define for_each_pipe(__dev_priv, __p) \
-	for ((__p) = 0; (__p) < INTEL_NUM_PIPES(__dev_priv); (__p)++)
+	for ((__p) = 0; (__p) < I915_MAX_PIPES; (__p)++) \
+		for_each_if((INTEL_INFO(__dev_priv)->pipe_mask) & BIT(__p))
 
 #define for_each_pipe_masked(__dev_priv, __p, __mask) \
-	for ((__p) = 0; (__p) < INTEL_NUM_PIPES(__dev_priv); (__p)++) \
+	for_each_pipe(__dev_priv, __p) \
 		for_each_if((__mask) & BIT(__p))
 
 #define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3d0cd0960bd2..a26f2bf1b6ea 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1739,11 +1739,12 @@  static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
 	if (pch_iir & SDE_POISON)
 		drm_err(&dev_priv->drm, "PCH poison interrupt\n");
 
-	if (pch_iir & SDE_FDI_MASK)
+	if (pch_iir & SDE_FDI_MASK) {
 		for_each_pipe(dev_priv, pipe)
 			drm_dbg(&dev_priv->drm, "  pipe %c FDI IIR: 0x%08x\n",
 				pipe_name(pipe),
 				I915_READ(FDI_RX_IIR(pipe)));
+	}
 
 	if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE))
 		drm_dbg(&dev_priv->drm, "PCH transcoder CRC done interrupt\n");
@@ -1823,11 +1824,12 @@  static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir)
 	if (pch_iir & SDE_AUDIO_CP_CHG_CPT)
 		drm_dbg(&dev_priv->drm, "Audio CP change interrupt\n");
 
-	if (pch_iir & SDE_FDI_MASK_CPT)
+	if (pch_iir & SDE_FDI_MASK_CPT) {
 		for_each_pipe(dev_priv, pipe)
 			drm_dbg(&dev_priv->drm, "  pipe %c FDI IIR: 0x%08x\n",
 				pipe_name(pipe),
 				I915_READ(FDI_RX_IIR(pipe)));
+	}
 
 	if (pch_iir & SDE_ERROR_CPT)
 		cpt_serr_int_handler(dev_priv);