On Mon, May 13, 2024 at 02:28:11PM +0300, Jani Nikula wrote: > On Fri, 10 May 2024, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx> > > --- > > .../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
