| 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 |
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
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 --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);
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(-)