Re: [PATCH v2 1/2] drm/i915/display: Use explicit cast in POWER_DOMAIN_*() macros

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 12, 2025 at 03:44:26PM -0300, Gustavo Sousa wrote:
> Quoting Gustavo Sousa (2025-02-12 14:59:28-03:00)
> >Quoting Ville Syrjälä (2025-02-12 14:52:19-03:00)
> >>On Wed, Feb 12, 2025 at 02:43:16PM -0300, Gustavo Sousa wrote:
> >>> Let the compiler know that we are intetionally using a different enum
> >>> type to perform arithmetic with enum intel_display_power_domain in the
> >>> POWER_DOMAIN_*(). Do that by explicitly casting the macro argument to
> >>> int.
> >>> 
> >>> Reported-by: kernel test robot <lkp@intel.com>
> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202502120809.XfmcqkBD-lkp@intel.com/
> >>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/display/intel_display_power.h | 6 +++---
> >>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> >>> index a3a5c1be8bab..3caa3f517a32 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> >>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> >>> @@ -117,12 +117,12 @@ enum intel_display_power_domain {
> >>>          POWER_DOMAIN_INVALID = POWER_DOMAIN_NUM,
> >>>  };
> >>>  
> >>> -#define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> >>> +#define POWER_DOMAIN_PIPE(pipe) ((int)(pipe) + POWER_DOMAIN_PIPE_A)
> >>>  #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \
> >>> -                ((pipe) + POWER_DOMAIN_PIPE_PANEL_FITTER_A)
> >>> +                ((int)(pipe) + POWER_DOMAIN_PIPE_PANEL_FITTER_A)
> >>>  #define POWER_DOMAIN_TRANSCODER(tran) \
> >>>          ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \
> >>> -         (tran) + POWER_DOMAIN_TRANSCODER_A)
> >>> +         (int)(tran) + POWER_DOMAIN_TRANSCODER_A)
> >>
> >>I've generally gone for the 
> >>POWER_DOMAIN_TRANSCODER_A + (tran) - TRANSCODER_A
> >>form for such things, to also make sure it works
> >>even if TRANSCODER_A isn't 0 anymore.
> >>Does that avoid the warning as well?
> >
> >Hm... That's a good idea; and I think it might avoid the warning indeed
> >(maybe we would need parentheses around (tran) - TRANSCODER_A).
> 
> I did a quick test and this also took care of removing the clang warning
> in my environment:
> 
>   diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
>   index e354051e8982..d46b35dbe018 100644
>   --- a/drivers/gpu/drm/i915/display/intel_display_power.h
>   +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
>   @@ -123,7 +123,7 @@ enum intel_display_power_domain {
>           ((enum intel_display_power_domain)((int)(pipe) + POWER_DOMAIN_PIPE_PANEL_FITTER_A))
>    #define POWER_DOMAIN_TRANSCODER(tran) \
>           ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \
>   -        (enum intel_display_power_domain)((int)(tran) + POWER_DOMAIN_TRANSCODER_A))
>   +        (enum intel_display_power_domain)(POWER_DOMAIN_TRANSCODER_A + ((tran) - TRANSCODER_A)))
>    
>    struct intel_power_domain_mask {
>           DECLARE_BITMAP(bits, POWER_DOMAIN_NUM);
> 
> The parentheses around (tran) - TRANSCODER_A were indeed necessary,
> probably for the compiler to see that as an int expression.
> 
> We can get rid of the parentheses if we do (tran) - TRANSCODER_A before
> adding POWER_DOMAIN_TRANSCODER_A:
> 
>   diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
>   index e354051e8982..b15eb4fd5062 100644
>   --- a/drivers/gpu/drm/i915/display/intel_display_power.h
>   +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
>   @@ -123,7 +123,7 @@ enum intel_display_power_domain {
>    	((enum intel_display_power_domain)((int)(pipe) + POWER_DOMAIN_PIPE_PANEL_FITTER_A))
>    #define POWER_DOMAIN_TRANSCODER(tran) \
>    	((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \
>   -	 (enum intel_display_power_domain)((int)(tran) + POWER_DOMAIN_TRANSCODER_A))
>   +	 (enum intel_display_power_domain)((tran) - TRANSCODER_A + POWER_DOMAIN_TRANSCODER_A))

Looks reasoanble enough to me. Do we still need the final cast?

>    
>    struct intel_power_domain_mask {
>    	DECLARE_BITMAP(bits, POWER_DOMAIN_NUM);
> 
> I'm tending more toward the second alternative.
> 
> --
> Gustavo Sousa
> 
> >
> >>
> >>Maybe these should even be functions rather than macros?
> >
> >Yeah. I actually considered this possibility, but went with the macros
> >to keep the change simple.
> >
> >If that's welcome, I could go ahead with turning those macros into
> >static inline functions.
> >
> >--
> >Gustavo Sousa
> >
> >>
> >>>  
> >>>  struct intel_power_domain_mask {
> >>>          DECLARE_BITMAP(bits, POWER_DOMAIN_NUM);
> >>> -- 
> >>> 2.48.1
> >>
> >>-- 
> >>Ville Syrjälä
> >>Intel

-- 
Ville Syrjälä
Intel