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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jun 28, 2018 at 10:45:02AM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-06-28 às 15:03 +0300, Jani Nikula escreveu:
> > On Wed, 27 Jun 2018, Chris Wilson <chris@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@inte
> > > > l.com>  
> > > > wrote:
> > > > 
> > > > > There's already some BIT() usage here and there, embrace it.
> > > > > 
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Since I'm CC'd I guess my opinion counts here :)
> 
> 
> > > > > Signed-off-by: Jani Nikula <jani.nikula@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.

+1 for the Consistency.

> 
> > > > 
> > > > >   *  #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)

I fully agree with them, so I'm actually dropping my ack while
we don't find rough agreement on how to move forward without more consistency.

> 
> 
> > 
> > BR,
> > Jani.
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx