[RFC,1/6] drm/i915: Iterate over pipe and skip the disabled one
diff mbox series

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

Commit Message

Anshuman Gupta Jan. 23, 2020, 1:26 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.

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

Comments

Jani Nikula Jan. 23, 2020, 1:48 p.m. UTC | #1
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);
Anshuman Gupta Jan. 24, 2020, 11:59 a.m. UTC | #2
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
Jani Nikula Jan. 24, 2020, 12:15 p.m. UTC | #3
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
Anshuman Gupta Jan. 24, 2020, 12:19 p.m. UTC | #4
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
Jani Nikula Jan. 24, 2020, 1:34 p.m. UTC | #5
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

Patch
diff mbox series

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