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

[Gustavo Sousa]

Quoting Ville Syrjälä (2025-02-12 15:55:59-03:00)
>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 at intel.com>
>> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202502120809.XfmcqkBD-lkp@intel.com/
>> >>> Signed-off-by: Gustavo Sousa <gustavo.sousa at 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?

The final cast is just an extra thing (actually done in patch #2) to
have the result of the macro to be seen as an enum
intel_display_power_domain from the outside. Nothing really required,
just something to make its usage a bit more type safe.

If we went with implementing those macros as functions, their return
type as enum intel_display_power_domain would basically have the same
effect, I think.

--
Gustavo Sousa

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