| Message ID | 20200123132659.725-2-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 3 display pipes combination system support | expand |
On Thu, 23 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> 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. > > Few compilation error needed to handle accordingly due to > change in for_each_pipe() macro. Really? Please paste. > > 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 028aab728514..47813a50add4 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.h > +++ b/drivers/gpu/drm/i915/display/intel_display.h > @@ -312,10 +312,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)++) \ Originally I was envisioning using for_each_set_bit() from bitops.h for this. It's probably more efficient, however I'm not sure if efficiency matters much here. The ugly part is that for_each_set_bit() requires an explicit cast to unsigned long *. Perhaps this is just as well, it's not wrong, and can always be updated later. > + 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 94cb25ac504d..22ecd5bc407e 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1735,11 +1735,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) > if (pch_iir & SDE_POISON) > DRM_ERROR("PCH poison interrupt\n"); > > - if (pch_iir & SDE_FDI_MASK) > + if (pch_iir & SDE_FDI_MASK) { > for_each_pipe(dev_priv, pipe) > DRM_DEBUG_DRIVER(" pipe %c FDI IIR: 0x%08x\n", > pipe_name(pipe), > I915_READ(FDI_RX_IIR(pipe))); > + } Are the brace changes really needed? This is what the for_each_if hack is supposed to tackle. > > if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE)) > DRM_DEBUG_DRIVER("PCH transcoder CRC done interrupt\n"); > @@ -1818,11 +1819,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) > if (pch_iir & SDE_AUDIO_CP_CHG_CPT) > DRM_DEBUG_DRIVER("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_DEBUG_DRIVER(" 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);
On 2020-01-23 at 15:48:05 +0200, Jani Nikula wrote: > On Thu, 23 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> 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. > > > > Few compilation error needed to handle accordingly due to > > change in for_each_pipe() macro. > > Really? Please paste. It is dangling-else warning at couple of places. drivers/gpu/drm/i915/i915_irq.c:1861:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] 1861 | if (pch_iir & SDE_FDI_MASK) drivers/gpu/drm/i915/i915_irq.c:1944:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] 1944 | if (pch_iir & SDE_FDI_MASK_CPT) > > > > > 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 028aab728514..47813a50add4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.h > > +++ b/drivers/gpu/drm/i915/display/intel_display.h > > @@ -312,10 +312,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)++) \ > > Originally I was envisioning using for_each_set_bit() from bitops.h for > this. It's probably more efficient, however I'm not sure if efficiency > matters much here. The ugly part is that for_each_set_bit() requires an > explicit cast to unsigned long *. > > Perhaps this is just as well, it's not wrong, and can always be updated > later. > > > + 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 94cb25ac504d..22ecd5bc407e 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -1735,11 +1735,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) > > if (pch_iir & SDE_POISON) > > DRM_ERROR("PCH poison interrupt\n"); > > > > - if (pch_iir & SDE_FDI_MASK) > > + if (pch_iir & SDE_FDI_MASK) { > > for_each_pipe(dev_priv, pipe) > > DRM_DEBUG_DRIVER(" pipe %c FDI IIR: 0x%08x\n", > > pipe_name(pipe), > > I915_READ(FDI_RX_IIR(pipe))); > > + } > > Are the brace changes really needed? This is what the for_each_if hack > is supposed to tackle. IMHO it was dangling-else compilation, warning that requires braces. please correct me if i am wrong. Thanks, Anshuman > > > > > if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE)) > > DRM_DEBUG_DRIVER("PCH transcoder CRC done interrupt\n"); > > @@ -1818,11 +1819,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) > > if (pch_iir & SDE_AUDIO_CP_CHG_CPT) > > DRM_DEBUG_DRIVER("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_DEBUG_DRIVER(" 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); > > -- > Jani Nikula, Intel Open Source Graphics Center
On Fri, 24 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote: > On 2020-01-23 at 15:48:05 +0200, Jani Nikula wrote: >> On Thu, 23 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> 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. >> > >> > Few compilation error needed to handle accordingly due to >> > change in for_each_pipe() macro. >> >> Really? Please paste. > It is dangling-else warning at couple of places. > drivers/gpu/drm/i915/i915_irq.c:1861:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] > 1861 | if (pch_iir & SDE_FDI_MASK) > drivers/gpu/drm/i915/i915_irq.c:1944:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] > 1944 | if (pch_iir & SDE_FDI_MASK_CPT) Right, I suppose this is caused by the nesting of the for loops with if-else. Perhaps the right course of action is to *not* reuse for_each_pipe() in for_each_pipe_masked(). Just combine the conditions into one. BR, Jani. >> >> > >> > 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 028aab728514..47813a50add4 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display.h >> > +++ b/drivers/gpu/drm/i915/display/intel_display.h >> > @@ -312,10 +312,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)++) \ >> >> Originally I was envisioning using for_each_set_bit() from bitops.h for >> this. It's probably more efficient, however I'm not sure if efficiency >> matters much here. The ugly part is that for_each_set_bit() requires an >> explicit cast to unsigned long *. >> >> Perhaps this is just as well, it's not wrong, and can always be updated >> later. >> >> > + 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 94cb25ac504d..22ecd5bc407e 100644 >> > --- a/drivers/gpu/drm/i915/i915_irq.c >> > +++ b/drivers/gpu/drm/i915/i915_irq.c >> > @@ -1735,11 +1735,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) >> > if (pch_iir & SDE_POISON) >> > DRM_ERROR("PCH poison interrupt\n"); >> > >> > - if (pch_iir & SDE_FDI_MASK) >> > + if (pch_iir & SDE_FDI_MASK) { >> > for_each_pipe(dev_priv, pipe) >> > DRM_DEBUG_DRIVER(" pipe %c FDI IIR: 0x%08x\n", >> > pipe_name(pipe), >> > I915_READ(FDI_RX_IIR(pipe))); >> > + } >> >> Are the brace changes really needed? This is what the for_each_if hack >> is supposed to tackle. > IMHO it was dangling-else compilation, warning that requires braces. > please correct me if i am wrong. > Thanks, > Anshuman >> >> > >> > if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE)) >> > DRM_DEBUG_DRIVER("PCH transcoder CRC done interrupt\n"); >> > @@ -1818,11 +1819,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) >> > if (pch_iir & SDE_AUDIO_CP_CHG_CPT) >> > DRM_DEBUG_DRIVER("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_DEBUG_DRIVER(" 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); >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On 2020-01-24 at 14:15:30 +0200, Jani Nikula wrote: > On Fri, 24 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > On 2020-01-23 at 15:48:05 +0200, Jani Nikula wrote: > >> On Thu, 23 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> 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. > >> > > >> > Few compilation error needed to handle accordingly due to > >> > change in for_each_pipe() macro. > >> > >> Really? Please paste. > > It is dangling-else warning at couple of places. > > drivers/gpu/drm/i915/i915_irq.c:1861:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] > > 1861 | if (pch_iir & SDE_FDI_MASK) > > drivers/gpu/drm/i915/i915_irq.c:1944:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] > > 1944 | if (pch_iir & SDE_FDI_MASK_CPT) > > Right, I suppose this is caused by the nesting of the for loops with > if-else. > > Perhaps the right course of action is to *not* reuse for_each_pipe() in > for_each_pipe_masked(). Just combine the conditions into one. This is not caused by for_each_pipe_masked, this is caused by for_each_pipe itself, if (foo) /*for_each_pipe()*/ for(;;) if (condition) {} else > > BR, > Jani. > > > > >> > >> > > >> > 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 028aab728514..47813a50add4 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_display.h > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.h > >> > @@ -312,10 +312,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)++) \ > >> > >> Originally I was envisioning using for_each_set_bit() from bitops.h for > >> this. It's probably more efficient, however I'm not sure if efficiency > >> matters much here. The ugly part is that for_each_set_bit() requires an > >> explicit cast to unsigned long *. > >> > >> Perhaps this is just as well, it's not wrong, and can always be updated > >> later. > >> > >> > + 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 94cb25ac504d..22ecd5bc407e 100644 > >> > --- a/drivers/gpu/drm/i915/i915_irq.c > >> > +++ b/drivers/gpu/drm/i915/i915_irq.c > >> > @@ -1735,11 +1735,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) > >> > if (pch_iir & SDE_POISON) > >> > DRM_ERROR("PCH poison interrupt\n"); > >> > > >> > - if (pch_iir & SDE_FDI_MASK) > >> > + if (pch_iir & SDE_FDI_MASK) { > >> > for_each_pipe(dev_priv, pipe) > >> > DRM_DEBUG_DRIVER(" pipe %c FDI IIR: 0x%08x\n", > >> > pipe_name(pipe), > >> > I915_READ(FDI_RX_IIR(pipe))); > >> > + } > >> > >> Are the brace changes really needed? This is what the for_each_if hack > >> is supposed to tackle. > > IMHO it was dangling-else compilation, warning that requires braces. > > please correct me if i am wrong. > > Thanks, > > Anshuman > >> > >> > > >> > if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE)) > >> > DRM_DEBUG_DRIVER("PCH transcoder CRC done interrupt\n"); > >> > @@ -1818,11 +1819,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) > >> > if (pch_iir & SDE_AUDIO_CP_CHG_CPT) > >> > DRM_DEBUG_DRIVER("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_DEBUG_DRIVER(" 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); > >> > >> -- > >> Jani Nikula, Intel Open Source Graphics Center > > -- > Jani Nikula, Intel Open Source Graphics Center
On Fri, 24 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote: > On 2020-01-24 at 14:15:30 +0200, Jani Nikula wrote: >> On Fri, 24 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote: >> > On 2020-01-23 at 15:48:05 +0200, Jani Nikula wrote: >> >> On Thu, 23 Jan 2020, Anshuman Gupta <anshuman.gupta@intel.com> 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. >> >> > >> >> > Few compilation error needed to handle accordingly due to >> >> > change in for_each_pipe() macro. >> >> >> >> Really? Please paste. >> > It is dangling-else warning at couple of places. >> > drivers/gpu/drm/i915/i915_irq.c:1861:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] >> > 1861 | if (pch_iir & SDE_FDI_MASK) >> > drivers/gpu/drm/i915/i915_irq.c:1944:5: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] >> > 1944 | if (pch_iir & SDE_FDI_MASK_CPT) >> >> Right, I suppose this is caused by the nesting of the for loops with >> if-else. >> >> Perhaps the right course of action is to *not* reuse for_each_pipe() in >> for_each_pipe_masked(). Just combine the conditions into one. > This is not caused by for_each_pipe_masked, this is caused by for_each_pipe itself, > if (foo) > /*for_each_pipe()*/ > for(;;) > if (condition) {} > else Sorry for the noise, please carry on with the patch. *blush* BR, Jani. >> >> BR, >> Jani. >> >> >> >> >> >> >> > >> >> > 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 028aab728514..47813a50add4 100644 >> >> > --- a/drivers/gpu/drm/i915/display/intel_display.h >> >> > +++ b/drivers/gpu/drm/i915/display/intel_display.h >> >> > @@ -312,10 +312,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)++) \ >> >> >> >> Originally I was envisioning using for_each_set_bit() from bitops.h for >> >> this. It's probably more efficient, however I'm not sure if efficiency >> >> matters much here. The ugly part is that for_each_set_bit() requires an >> >> explicit cast to unsigned long *. >> >> >> >> Perhaps this is just as well, it's not wrong, and can always be updated >> >> later. >> >> >> >> > + 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 94cb25ac504d..22ecd5bc407e 100644 >> >> > --- a/drivers/gpu/drm/i915/i915_irq.c >> >> > +++ b/drivers/gpu/drm/i915/i915_irq.c >> >> > @@ -1735,11 +1735,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) >> >> > if (pch_iir & SDE_POISON) >> >> > DRM_ERROR("PCH poison interrupt\n"); >> >> > >> >> > - if (pch_iir & SDE_FDI_MASK) >> >> > + if (pch_iir & SDE_FDI_MASK) { >> >> > for_each_pipe(dev_priv, pipe) >> >> > DRM_DEBUG_DRIVER(" pipe %c FDI IIR: 0x%08x\n", >> >> > pipe_name(pipe), >> >> > I915_READ(FDI_RX_IIR(pipe))); >> >> > + } >> >> >> >> Are the brace changes really needed? This is what the for_each_if hack >> >> is supposed to tackle. >> > IMHO it was dangling-else compilation, warning that requires braces. >> > please correct me if i am wrong. >> > Thanks, >> > Anshuman >> >> >> >> > >> >> > if (pch_iir & (SDE_TRANSB_CRC_DONE | SDE_TRANSA_CRC_DONE)) >> >> > DRM_DEBUG_DRIVER("PCH transcoder CRC done interrupt\n"); >> >> > @@ -1818,11 +1819,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) >> >> > if (pch_iir & SDE_AUDIO_CP_CHG_CPT) >> >> > DRM_DEBUG_DRIVER("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_DEBUG_DRIVER(" 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); >> >> >> >> -- >> >> Jani Nikula, Intel Open Source Graphics Center >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index 028aab728514..47813a50add4 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -312,10 +312,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 94cb25ac504d..22ecd5bc407e 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1735,11 +1735,12 @@ static void ibx_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) if (pch_iir & SDE_POISON) DRM_ERROR("PCH poison interrupt\n"); - if (pch_iir & SDE_FDI_MASK) + if (pch_iir & SDE_FDI_MASK) { for_each_pipe(dev_priv, pipe) DRM_DEBUG_DRIVER(" 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_DEBUG_DRIVER("PCH transcoder CRC done interrupt\n"); @@ -1818,11 +1819,12 @@ static void cpt_irq_handler(struct drm_i915_private *dev_priv, u32 pch_iir) if (pch_iir & SDE_AUDIO_CP_CHG_CPT) DRM_DEBUG_DRIVER("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_DEBUG_DRIVER(" 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. Few compilation error needed to handle accordingly due to change in for_each_pipe() macro. 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(-)