Re: [PATCH 10/16] drm/i915: Shuffle the skl+ plane register definitions

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

On Mon, May 13, 2024 at 02:28:11PM +0300, Jani Nikula wrote:
> On Fri, 10 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Rearrange the plane skl+ universal plane register definitions:
> > - keep everything related to the same register in one place
> > - sort based on register offset
> > - unify the whitespace/etc a bit
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../i915/display/skl_universal_plane_regs.h   | 502 ++++++++----------
> >  1 file changed, 207 insertions(+), 295 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h b/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h
> > index 0558d97614e1..0ad14727e334 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane_regs.h
> > @@ -9,8 +9,6 @@
> >  #include "intel_display_reg_defs.h"
> >  
> >  #define _PLANE_CTL_1_A				0x70180
> > -#define _PLANE_CTL_2_A				0x70280
> > -#define _PLANE_CTL_3_A				0x70380
> >  #define   PLANE_CTL_ENABLE			REG_BIT(31)
> >  #define   PLANE_CTL_ARB_SLOTS_MASK		REG_GENMASK(30, 28) /* icl+ */
> >  #define   PLANE_CTL_ARB_SLOTS(x)		REG_FIELD_PREP(PLANE_CTL_ARB_SLOTS_MASK, (x)) /* icl+ */
> > @@ -74,59 +72,132 @@
> >  #define   PLANE_CTL_ROTATE_90			REG_FIELD_PREP(PLANE_CTL_ROTATE_MASK, 1)
> >  #define   PLANE_CTL_ROTATE_180			REG_FIELD_PREP(PLANE_CTL_ROTATE_MASK, 2)
> >  #define   PLANE_CTL_ROTATE_270			REG_FIELD_PREP(PLANE_CTL_ROTATE_MASK, 3)
> 
> This is a painful patch to review (in part because some newline removals
> throw off --color-moved) so I want to check something first.
> 
> Shouldn't the above register *content* definitions be...
> 
> > +#define _PLANE_CTL_2_A				0x70280
> > +#define _PLANE_CTL_1_B				0x71180
> > +#define _PLANE_CTL_2_B				0x71280
> > +#define _PLANE_CTL_1(pipe)		_PIPE(pipe, _PLANE_CTL_1_A, _PLANE_CTL_1_B)
> > +#define _PLANE_CTL_2(pipe)		_PIPE(pipe, _PLANE_CTL_2_A, _PLANE_CTL_2_B)
> > +#define PLANE_CTL(pipe, plane)		_MMIO_PLANE(plane, _PLANE_CTL_1(pipe), _PLANE_CTL_2(pipe))
> 
> ...here after all the register *offset* definitions, not right after the
> plane 1 / pipe A register offset macro? Ditto for a bunch of the other
> changes here.

Shrug. I don't think we have any real consistency in how these
things are laid out. Sometimes the bits are defined after the
_FOO_A, sometimes after all the _FOO_?, and sometimes after FOO().

I guess we should try to standardize on one of those. And I suppose
it should be that last option of those three (which is what you
suggest as well) since we don't always have any intermediate _FOO
defines at all. I can respin with that.

-- 
Ville Syrjälä
Intel