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

[Gustavo Sousa]

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