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