Re: [PATCH v3 1/3] drm/i915/display: Use explicit base values in POWER_DOMAIN_*() macros

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel