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

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

[Jani Nikula]

There's already some BIT() usage here and there, embrace it.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
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)
  *  #define   FOO_MODE_MASK             (0xf << 16)
  *  #define   FOO_MODE_SHIFT            16
  *  #define   FOO_MODE_BAR              (0 << 16)
-- 
2.11.0

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

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

[Michal Wajdeczko]

On Wed, 27 Jun 2018 16:41:13 +0200, Jani Nikula <jani.nikula@intel.com>  
wrote:

> There's already some BIT() usage here and there, embrace it.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 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 ..

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

Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Michal Wajdeczko (2018-06-27 16:51:42)
> On Wed, 27 Jun 2018 16:41:13 +0200, Jani Nikula <jani.nikula@intel.com>  
> wrote:
> 
> > There's already some BIT() usage here and there, embrace it.
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 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 ..
> 
> >   *  #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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: encourage BIT() macro usage in register definitions

[Patchwork]

== Series Details ==

Series: drm/i915: encourage BIT() macro usage in register definitions
URL   : https://patchwork.freedesktop.org/series/45498/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4388 -> Patchwork_9446 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45498/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9446 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       FAIL (fdo#103841) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#107054, fdo#103927) -> PASS

    
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#107054 https://bugs.freedesktop.org/show_bug.cgi?id=107054


== Participating hosts (45 -> 40) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4388 -> Patchwork_9446

  CI_DRM_4388: b3654d40e9004f17bb612e71aee129347ea2c4aa @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9446: d22f87f4e6805dea2e32f6b9e0a78f1717cff8f4 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d22f87f4e680 drm/i915: encourage BIT() macro usage in register definitions

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9446/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: encourage BIT() macro usage in register definitions

[Patchwork]

== Series Details ==

Series: drm/i915: encourage BIT() macro usage in register definitions
URL   : https://patchwork.freedesktop.org/series/45498/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4388_full -> Patchwork_9446_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9446_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9446_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9446_full:

  === IGT changes ===

    ==== Warnings ====

    igt@kms_cursor_legacy@cursora-vs-flipb-atomic:
      shard-hsw:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9446_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_await@wide-contexts:
      shard-glk:          PASS -> FAIL (fdo#105900)

    igt@gem_exec_schedule@pi-ringfull-blt:
      {shard-glk9}:       NOTRUN -> FAIL (fdo#103158)

    igt@gem_exec_schedule@pi-ringfull-vebox:
      shard-glk:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_mmap_gtt@coherency:
      shard-glk:          NOTRUN -> FAIL (fdo#100587)

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          NOTRUN -> FAIL (fdo#106509, fdo#105454)

    igt@kms_flip@2x-plain-flip-fb-recreate:
      {shard-glk9}:       NOTRUN -> FAIL (fdo#100368)

    igt@kms_flip@plain-flip-fb-recreate:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-to-x-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724, fdo#103822)

    igt@kms_flip_tiling@flip-y-tiled:
      {shard-glk9}:       NOTRUN -> FAIL (fdo#104724)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    igt@testdisplay:
      shard-glk:          NOTRUN -> INCOMPLETE (fdo#103359, k.org#198133)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> PASS
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@drv_suspend@shrink:
      shard-snb:          FAIL (fdo#107054, fdo#106886) -> PASS

    igt@gem_ctx_switch@basic-all-light:
      shard-hsw:          INCOMPLETE (fdo#103540) -> PASS

    igt@gem_softpin@noreloc-s3:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_flip@dpms-vs-vblank-race:
      shard-kbl:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#100587 https://bugs.freedesktop.org/show_bug.cgi?id=100587
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107054 https://bugs.freedesktop.org/show_bug.cgi?id=107054
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4388 -> Patchwork_9446

  CI_DRM_4388: b3654d40e9004f17bb612e71aee129347ea2c4aa @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9446: d22f87f4e6805dea2e32f6b9e0a78f1717cff8f4 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9446/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Wed, Jun 27, 2018 at 05:41:13PM +0300, Jani Nikula wrote:
> There's already some BIT() usage here and there, embrace it.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 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)

Oh! I'm sure that I will miss the (1 << n) notation...
and right now we had just converted everything to the documented style...

any chance of get a script modifying all at once?

Anyways:

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>   *  #define   FOO_MODE_MASK             (0xf << 16)
>   *  #define   FOO_MODE_SHIFT            16
>   *  #define   FOO_MODE_BAR              (0 << 16)
> -- 
> 2.11.0
> 
> _______________________________________________
> 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

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

[Jani Nikula]

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@intel.com>  
>> wrote:
>> 
>> > There's already some BIT() usage here and there, embrace it.
>> >
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > 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 ..
>> 
>> >   *  #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.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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.

> > > 
> > > >   *  #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.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

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