| Message ID | 20250217203722.87152-2-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve type-safety on POWER_DOMAIN_*() macros | expand |
On Mon, Feb 17, 2025 at 05:34:26PM -0300, Gustavo Sousa wrote: > Although we have comments in intel_display_limits.h saying that the > code expects PIPE_A and TRANSCODER_A to be zero, it doesn't hurt to add > them as explicit base values for calculating the power domain offset in > POWER_DOMAIN_*() macros. > > On the plus side, we have that this: > > * Fixes a warning reported by kernel test robot <lkp@intel.com> > about doing arithmetic with two different enum types. > * Makes the code arguably more robust (in the unlikely event of those > bases becoming non-zero). > > v2: > - Prefer using explicit base values instead of simply casting the > macro argument to int. (Ville) > - Update commit message to match the new approach (for reference, the > old message subject was "drm/i915/display: Use explicit cast in > POWER_DOMAIN_*() macros"). > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > 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> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.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..4ad35bd4b040 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) ((pipe) - PIPE_A + POWER_DOMAIN_PIPE_A) > #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \ > - ((pipe) + POWER_DOMAIN_PIPE_PANEL_FITTER_A) > + ((pipe) - PIPE_A + POWER_DOMAIN_PIPE_PANEL_FITTER_A) > #define POWER_DOMAIN_TRANSCODER(tran) \ > ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ Btw looks like we could drop this edp special case. The enums do seem to match even for this, and we appear to rely on that matching also for the DSI transcoders. So special casing just EDP is a bit weird. Either that or perhaps we need to special case DSI as well. If we do want to depend on the enums matching then one random idea that came to mind is something like: enum intel_display_power_domain { _POWER_DOMAIN_PIPES, POWER_DOMAIN_PIPE_A = _POWER_DOMAIN_PIPES + PIPE_A, POWER_DOMAIN_PIPE_B = _POWER_DOMAIN_PIPES + PIPE_B, ... _POWER_DOMAIN_TRANSCODERS, POWER_DOMAIN_TRANSCODER_A = _POWER_DOMAIN_TRANSCODERS + TRANSCODER_A, POWER_DOMAIN_TRANSCODER_B = _POWER_DOMAIN_TRANSCODERS + TRANSCODER_B, ... which should semi-guarantee that things keep working even if we accidentally introduces holes into enum pipe/transcoder/etc. Though this would still be slightly vulnerable against ordering differences since the _POWER_DOMAIN_* value would be based on the previous value. But maybe this is just pointless paranoia since we've not screwed up the enums so far... > - (tran) + POWER_DOMAIN_TRANSCODER_A) > + (tran) - TRANSCODER_A + POWER_DOMAIN_TRANSCODER_A) > > struct intel_power_domain_mask { > DECLARE_BITMAP(bits, POWER_DOMAIN_NUM); > -- > 2.48.1
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h index a3a5c1be8bab..4ad35bd4b040 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) ((pipe) - PIPE_A + POWER_DOMAIN_PIPE_A) #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \ - ((pipe) + POWER_DOMAIN_PIPE_PANEL_FITTER_A) + ((pipe) - PIPE_A + POWER_DOMAIN_PIPE_PANEL_FITTER_A) #define POWER_DOMAIN_TRANSCODER(tran) \ ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ - (tran) + POWER_DOMAIN_TRANSCODER_A) + (tran) - TRANSCODER_A + POWER_DOMAIN_TRANSCODER_A) struct intel_power_domain_mask { DECLARE_BITMAP(bits, POWER_DOMAIN_NUM);
Although we have comments in intel_display_limits.h saying that the code expects PIPE_A and TRANSCODER_A to be zero, it doesn't hurt to add them as explicit base values for calculating the power domain offset in POWER_DOMAIN_*() macros. On the plus side, we have that this: * Fixes a warning reported by kernel test robot <lkp@intel.com> about doing arithmetic with two different enum types. * Makes the code arguably more robust (in the unlikely event of those bases becoming non-zero). v2: - Prefer using explicit base values instead of simply casting the macro argument to int. (Ville) - Update commit message to match the new approach (for reference, the old message subject was "drm/i915/display: Use explicit cast in POWER_DOMAIN_*() macros"). Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> 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(-)