Re: [PATCH 02/13] drm/i915: Clean up the cursor register defines

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 16 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Group the cursor register defines such that everything to
> do with one register is in one place.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

but a couple of nitpicks inline...

> ---
>  .../gpu/drm/i915/display/intel_cursor_regs.h  | 52 +++++++++----------
>  1 file changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor_regs.h b/drivers/gpu/drm/i915/display/intel_cursor_regs.h
> index c2190af1e9f5..270c26c2e6df 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cursor_regs.h
> @@ -9,6 +9,7 @@
>  #include "intel_display_reg_defs.h"
>  
>  #define _CURACNTR		0x70080
> +#define CURCNTR(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CURACNTR)

In addition to code movement, these add braces around (dev_priv) and
(pipe). While it makes review harder by breaking 'git show
--color-moved', I also think it's kind of unnecessary when they're only
passed on as parameters. Or is there some corner case where it matters?
Comma has the lowest precedence, and I don't think you could easily pass
in a value with a comma operator.

No need to change for this, it's not wrong either.

>  /* Old style CUR*CNTR flags (desktop 8xx) */
>  #define   CURSOR_ENABLE			REG_BIT(31)
>  #define   CURSOR_PIPE_GAMMA_ENABLE	REG_BIT(30)
> @@ -38,61 +39,60 @@
>  #define   MCURSOR_MODE_128_ARGB_AX	(0x20 | MCURSOR_MODE_128_32B_AX)
>  #define   MCURSOR_MODE_256_ARGB_AX	(0x20 | MCURSOR_MODE_256_32B_AX)
>  #define   MCURSOR_MODE_64_ARGB_AX	(0x20 | MCURSOR_MODE_64_32B_AX)
> +
>  #define _CURABASE		0x70084
> +#define CURBASE(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CURABASE)
> +
>  #define _CURAPOS		0x70088
> -#define _CURAPOS_ERLY_TPT	0x7008c
> +#define CURPOS(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CURAPOS)
>  #define   CURSOR_POS_Y_SIGN		REG_BIT(31)
>  #define   CURSOR_POS_Y_MASK		REG_GENMASK(30, 16)
>  #define   CURSOR_POS_Y(y)		REG_FIELD_PREP(CURSOR_POS_Y_MASK, (y))
>  #define   CURSOR_POS_X_SIGN		REG_BIT(15)
>  #define   CURSOR_POS_X_MASK		REG_GENMASK(14, 0)
>  #define   CURSOR_POS_X(x)		REG_FIELD_PREP(CURSOR_POS_X_MASK, (x))
> +
> +#define _CURAPOS_ERLY_TPT	0x7008c
> +#define CURPOS_ERLY_TPT(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CURAPOS_ERLY_TPT)
> +
>  #define _CURASIZE		0x700a0 /* 845/865 */
> +#define CURSIZE(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CURASIZE)
>  #define   CURSOR_HEIGHT_MASK		REG_GENMASK(21, 12)
>  #define   CURSOR_HEIGHT(h)		REG_FIELD_PREP(CURSOR_HEIGHT_MASK, (h))
>  #define   CURSOR_WIDTH_MASK		REG_GENMASK(9, 0)
>  #define   CURSOR_WIDTH(w)		REG_FIELD_PREP(CURSOR_WIDTH_MASK, (w))
> +
>  #define _CUR_FBC_CTL_A		0x700a0 /* ivb+ */
> +#define CUR_FBC_CTL(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CUR_FBC_CTL_A)
>  #define   CUR_FBC_EN			REG_BIT(31)
>  #define   CUR_FBC_HEIGHT_MASK		REG_GENMASK(7, 0)
>  #define   CUR_FBC_HEIGHT(h)		REG_FIELD_PREP(CUR_FBC_HEIGHT_MASK, (h))
> +
>  #define _CUR_CHICKEN_A		0x700a4 /* mtl+ */
> +#define CUR_CHICKEN(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CUR_CHICKEN_A)
> +
>  #define _CURASURFLIVE		0x700ac /* g4x+ */
> -#define _CURBCNTR		0x700c0
> -#define _CURBBASE		0x700c4
> -#define _CURBPOS		0x700c8
> -
> -#define _CURBCNTR_IVB		0x71080
> -#define _CURBBASE_IVB		0x71084
> -#define _CURBPOS_IVB		0x71088
> -
> -#define CURCNTR(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CURACNTR)
> -#define CURBASE(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CURABASE)
> -#define CURPOS(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CURAPOS)
> -#define CURPOS_ERLY_TPT(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CURAPOS_ERLY_TPT)
> -#define CURSIZE(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CURASIZE)
> -#define CUR_FBC_CTL(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CUR_FBC_CTL_A)
> -#define CUR_CHICKEN(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CUR_CHICKEN_A)
> -#define CURSURFLIVE(dev_priv, pipe) _MMIO_CURSOR2(dev_priv, pipe, _CURASURFLIVE)
> +#define CURSURFLIVE(dev_priv, pipe)	_MMIO_CURSOR2((dev_priv), (pipe), _CURASURFLIVE)
>  
>  /* skl+ */
>  #define _CUR_WM_A_0		0x70140
>  #define _CUR_WM_B_0		0x71140
> +#define CUR_WM(pipe, level)	_MMIO(_PIPE((pipe), _CUR_WM_A_0, _CUR_WM_B_0) + (level) * 4)
> +
>  #define _CUR_WM_SAGV_A		0x70158
>  #define _CUR_WM_SAGV_B		0x71158
> +#define CUR_WM_SAGV(pipe)	_MMIO_PIPE((pipe), _CUR_WM_SAGV_A, _CUR_WM_SAGV_B)
> +
>  #define _CUR_WM_SAGV_TRANS_A	0x7015C
>  #define _CUR_WM_SAGV_TRANS_B	0x7115C
> +#define CUR_WM_SAGV_TRANS(pipe)	_MMIO_PIPE((pipe), _CUR_WM_SAGV_TRANS_A, _CUR_WM_SAGV_TRANS_B)
> +
>  #define _CUR_WM_TRANS_A		0x70168
>  #define _CUR_WM_TRANS_B		0x71168
> -#define _CUR_WM_0(pipe) _PIPE(pipe, _CUR_WM_A_0, _CUR_WM_B_0)
> -#define CUR_WM(pipe, level) _MMIO(_CUR_WM_0(pipe) + ((4) * (level)))

There's some unmentioned drive-by cleanup here too. No biggie, but I've
found 'git show --color-moved' to be such a powerful aid in reviewing
code movement patches that I'd prefer these to be separate. No need to
change now, because I already reviewed it. :)

BR,
Jani.

> -#define CUR_WM_SAGV(pipe) _MMIO_PIPE(pipe, _CUR_WM_SAGV_A, _CUR_WM_SAGV_B)
> -#define CUR_WM_SAGV_TRANS(pipe) _MMIO_PIPE(pipe, _CUR_WM_SAGV_TRANS_A, _CUR_WM_SAGV_TRANS_B)
> -#define CUR_WM_TRANS(pipe) _MMIO_PIPE(pipe, _CUR_WM_TRANS_A, _CUR_WM_TRANS_B)
> +#define CUR_WM_TRANS(pipe)	_MMIO_PIPE((pipe), _CUR_WM_TRANS_A, _CUR_WM_TRANS_B)
>  
> -/* skl+ */
> -#define _CUR_BUF_CFG_A				0x7017c
> -#define _CUR_BUF_CFG_B				0x7117c
> -#define CUR_BUF_CFG(pipe)	_MMIO_PIPE(pipe, _CUR_BUF_CFG_A, _CUR_BUF_CFG_B)
> +#define _CUR_BUF_CFG_A		0x7017c
> +#define _CUR_BUF_CFG_B		0x7117c
> +#define CUR_BUF_CFG(pipe)	_MMIO_PIPE((pipe), _CUR_BUF_CFG_A, _CUR_BUF_CFG_B)
>  
>  #endif /* __INTEL_CURSOR_REGS_H__ */

-- 
Jani Nikula, Intel