[Intel-gfx] [PATCH] drm/i915: encourage BIT() macro usage in register definitions

[Paulo Zanoni]

Em Qui, 2018-06-28 às 15:03 +0300, Jani Nikula escreveu:
> On Wed, 27 Jun 2018, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > Quoting Michal Wajdeczko (2018-06-27 16:51:42)
> > > On Wed, 27 Jun 2018 16:41:13 +0200, Jani Nikula <jani.nikula at inte
> > > l.com>  
> > > wrote:
> > > 
> > > > There's already some BIT() usage here and there, embrace it.
> > > > 
> > > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>

Since I'm CC'd I guess my opinion counts here :)


> > > > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h  
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 476118f46cf3..64b9c270045d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -65,9 +65,10 @@
> > > >   * but do note that the macros may be needed to read as well
> > > > as write  
> > > > the
> > > >   * register contents.
> > > >   *
> > > > - * Define bits using ``(1 << N)`` instead of ``BIT(N)``. We
> > > > may change  
> > > > this in
> > > > - * the future, but this is the prevailing style. Do **not**
> > > > add  
> > > > ``_BIT`` suffix
> > > > - * to the name.
> > > > + * Define bits using ``BIT(N)`` instead of ``(1 << N)``. Do
> > > > **not** add  
> > > > ``_BIT``
> > > > + * suffix to the name. Exception to ``BIT()`` usage: Value 1
> > > > for a bit  
> > > > field
> > > > + * should be defined using ``(1 << N)`` to be in line with
> > > > other values  
> > > > such as
> > > > + * ``(2 << N)`` for the same field.
> > > >   *
> > > >   * Group the register and its contents together without blank
> > > > lines,  
> > > > separate
> > > >   * from other registers and their contents with one blank
> > > > line.
> > > > @@ -105,7 +106,7 @@
> > > >   *  #define _FOO_A                      0xf000
> > > >   *  #define _FOO_B                      0xf001
> > > >   *  #define FOO(pipe)                   _MMIO_PIPE(pipe,
> > > > _FOO_A, _FOO_B)
> > > > - *  #define   FOO_ENABLE                (1 << 31)
> > > > + *  #define   FOO_ENABLE                BIT(31)
> > > 
> > > hmm, this breaks nice consistency between one- and multi-bit
> > > fields ..

I'll agree with Michal and Chris here: I'm not a huge fan of mixing
BIT() and (x << y), I would prefer to keep the current standard,
especially since BIT() is easily blacklistable. Or fully embrace the
helper macros and abolish all sorts of (x << y). Consistency wins IMHO.

> > > 
> > > >   *  #define   FOO_MODE_MASK             (0xf << 16)
> > > 
> > > .. but if you want to use macro for single bit, then maybe you
> > > should
> > > also consider other existing macro for the mask definition:
> > > 
> > >         #define   FOO_MODE_MASK             GENMASK(19, 16)
> > > 
> > > >   *  #define   FOO_MODE_SHIFT            16
> > > >   *  #define   FOO_MODE_BAR              (0 << 16)
> > > 
> > > .. but we still don't have any macro for defining multi-bit
> > > values
> > > so I'm not sure if this change will make code really easier to
> > > read
> > 
> > #include <linux/bitfield.h>
> > 
> > I'm not sure if I'm ready to embrace that yet, but it does seem to
> > be
> > the direction we should be heading in. Primarily to check the
> > invalid
> > range checking & usage.
> 
> I guess there are two things here. Using bitfield.h macros to define
> our
> own stuff is one thing, like so:
> 
> #define   FOO_ENABLE                BIT(31)
> #define   FOO_MODE_MASK             GENMASK(19, 16)
> #define   FOO_MODE_SHIFT            16
> #define   FOO_MODE_BAR              FIELD_PREP(FOO_MODE_MASK, 0)
> #define   FOO_MODE_BAZ              FIELD_PREP(FOO_MODE_MASK, 1)
> #define   FOO_MODE_QUX_SNB          FIELD_PREP(FOO_MODE_MASK, 2)
> 
> The range checking is indeed an advantage.
> 
> Using FIELD_PREP() or FIELD_GET() in code is another, because we
> currently don't define the *unshifted* field values. Everything is
> defined with the shift. Defining everything unshifted and then moving
> the FIELD_PREP() and FIELD_GET() in code would be quite the change.

Can't we create simple macros that cover our cases then?

(yes, that would perhaps be more divergence from the Kernel coding
standards, which could be worse than using (x << y) that are not hidden
by nonstandard macros)


> 
> BR,
> Jani.
>