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@xxxxxxxxx> >> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202502120809.XfmcqkBD-lkp@xxxxxxxxx/ >> >>> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> >> >>> --- >> >>> 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
