[RFC] drm/i915: Introduce documentation for register subfields
diff mbox

Message ID 1434707535-13014-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson June 19, 2015, 9:52 a.m. UTC
An interesting point was raised in some recent patches to document the
various widths of the register subfields. I disagreed with that patch in
that it transformed illegal values into some random potentially harmful
valid value (and not transforming an invalid value would end up writing
bits in other subfields, so equally bad). Most of our register usage is
with static values so unlikely to be of concern, but for those we can
document the register subfields and provide compile time checking.

The resulting error message is fairly impententrable though:

n file included from include/uapi/linux/stddef.h:1:0,
                 from include/linux/stddef.h:4,
                 from ./include/uapi/linux/posix_types.h:4,
                 from include/uapi/linux/types.h:13,
                 from include/linux/types.h:5,
                 from include/linux/mod_devicetable.h:11,
                 from include/linux/i2c.h:29,
                 from drivers/gpu/drm/i915/intel_dp.c:28:
In function ‘intel_dp_autotest_edid’,
    inlined from ‘intel_dp_handle_test_request’ at drivers/gpu/drm/i915/intel_dp.c:4230:14,
    inlined from ‘intel_dp_detect’ at drivers/gpu/drm/i915/intel_dp.c:4635:4:
include/linux/compiler.h:429:38: error: call to ‘__compiletime_assert_4173’ declared with attribute error: BUILD_BUG_ON failed: (5) & -(1<<2)
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                      ^
include/linux/compiler.h:412:4: note: in definition of macro ‘__compiletime_assert’
    prefix ## suffix();    \
    ^
include/linux/compiler.h:429:2: note: in expansion of macro ‘_compiletime_assert’
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
  ^
include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’
 #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                     ^
include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
  ^
drivers/gpu/drm/i915/intel_dp.c:45:68: note: in expansion of macro ‘BUILD_BUG_ON’
 #define __FIELD__(x, shift, width) ({ if (__builtin_constant_p(x)) BUILD_BUG_ON((x) & -(1<<width)); ((x) << (shift)); })
                                                                    ^
drivers/gpu/drm/i915/intel_dp.c:47:32: note: in expansion of macro ‘__FIELD__’
 #define INTEL_DP_RESOLUTION(x) __FIELD__(x, INTEL_DP_RESOLUTION_SHIFT, 2)
                                ^
drivers/gpu/drm/i915/intel_dp.c:51:36: note: in expansion of macro ‘INTEL_DP_RESOLUTION’
 #define INTEL_DP_RESOLUTION_BROKEN INTEL_DP_RESOLUTION(5)
                                    ^
drivers/gpu/drm/i915/intel_dp.c:4173:36: note: in expansion of macro ‘INTEL_DP_RESOLUTION_BROKEN’
   intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_BROKEN;

We can extend the macro to WARN_ON for instances of runtime violation -
maybe useful, but it surely would bulk our code out dramatically.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Lespiau, Damien June 19, 2015, 10:44 a.m. UTC | #1
On Fri, Jun 19, 2015 at 10:52:15AM +0100, Chris Wilson wrote:
> An interesting point was raised in some recent patches to document the
> various widths of the register subfields. I disagreed with that patch in
> that it transformed illegal values into some random potentially harmful
> valid value (and not transforming an invalid value would end up writing
> bits in other subfields, so equally bad). Most of our register usage is
> with static values so unlikely to be of concern, but for those we can
> document the register subfields and provide compile time checking.

I've thought about things like this in the past as well, we can even
automatically generate all of that from the specs!

I had other per-register metadata in mind, like:
  - is the register is a masked one or not (and then warn if we're
    trying to write it wihtout the _MASKED_*() macros)
  - is the register part of the render context (and then warn if we try
    to write it with a MMIO write).

#define FOO			0x1234
#define FOO_IS_MASKED		1
#define FOO_IS_IN_RENDER_CTX	1

should be enough for the compile-time warns like you do here.
Daniel Vetter June 22, 2015, 2:05 p.m. UTC | #2
On Fri, Jun 19, 2015 at 10:52:15AM +0100, Chris Wilson wrote:
> An interesting point was raised in some recent patches to document the
> various widths of the register subfields. I disagreed with that patch in
> that it transformed illegal values into some random potentially harmful
> valid value (and not transforming an invalid value would end up writing
> bits in other subfields, so equally bad). Most of our register usage is
> with static values so unlikely to be of concern, but for those we can
> document the register subfields and provide compile time checking.
> 
> The resulting error message is fairly impententrable though:
> 
> n file included from include/uapi/linux/stddef.h:1:0,
>                  from include/linux/stddef.h:4,
>                  from ./include/uapi/linux/posix_types.h:4,
>                  from include/uapi/linux/types.h:13,
>                  from include/linux/types.h:5,
>                  from include/linux/mod_devicetable.h:11,
>                  from include/linux/i2c.h:29,
>                  from drivers/gpu/drm/i915/intel_dp.c:28:
> In function ‘intel_dp_autotest_edid’,
>     inlined from ‘intel_dp_handle_test_request’ at drivers/gpu/drm/i915/intel_dp.c:4230:14,
>     inlined from ‘intel_dp_detect’ at drivers/gpu/drm/i915/intel_dp.c:4635:4:
> include/linux/compiler.h:429:38: error: call to ‘__compiletime_assert_4173’ declared with attribute error: BUILD_BUG_ON failed: (5) & -(1<<2)
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>                                       ^
> include/linux/compiler.h:412:4: note: in definition of macro ‘__compiletime_assert’
>     prefix ## suffix();    \
>     ^
> include/linux/compiler.h:429:2: note: in expansion of macro ‘_compiletime_assert’
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>   ^
> include/linux/bug.h:50:37: note: in expansion of macro ‘compiletime_assert’
>  #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>                                      ^
> include/linux/bug.h:74:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
>   BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>   ^
> drivers/gpu/drm/i915/intel_dp.c:45:68: note: in expansion of macro ‘BUILD_BUG_ON’
>  #define __FIELD__(x, shift, width) ({ if (__builtin_constant_p(x)) BUILD_BUG_ON((x) & -(1<<width)); ((x) << (shift)); })
>                                                                     ^
> drivers/gpu/drm/i915/intel_dp.c:47:32: note: in expansion of macro ‘__FIELD__’
>  #define INTEL_DP_RESOLUTION(x) __FIELD__(x, INTEL_DP_RESOLUTION_SHIFT, 2)
>                                 ^
> drivers/gpu/drm/i915/intel_dp.c:51:36: note: in expansion of macro ‘INTEL_DP_RESOLUTION’
>  #define INTEL_DP_RESOLUTION_BROKEN INTEL_DP_RESOLUTION(5)
>                                     ^
> drivers/gpu/drm/i915/intel_dp.c:4173:36: note: in expansion of macro ‘INTEL_DP_RESOLUTION_BROKEN’
>    intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_BROKEN;
> 
> We can extend the macro to WARN_ON for instances of runtime violation -
> maybe useful, but it surely would bulk our code out dramatically.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f52eef138247..fe44b06adeb4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -42,10 +42,12 @@
>  #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
>  
>  /* Compliance test status bits  */
> -#define INTEL_DP_RESOLUTION_SHIFT_MASK	0
> -#define INTEL_DP_RESOLUTION_PREFERRED	(1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> -#define INTEL_DP_RESOLUTION_STANDARD	(2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> -#define INTEL_DP_RESOLUTION_FAILSAFE	(3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
> +#define __FIELD__(x, shift, width) ({ if (__builtin_constant_p(x)) BUILD_BUG_ON((x) & -(1<<width)); ((x) << (shift)); })
> +#define INTEL_DP_RESOLUTION_SHIFT 0
> +#define INTEL_DP_RESOLUTION(x) __FIELD__(x, INTEL_DP_RESOLUTION_SHIFT, 2)
> +#define INTEL_DP_RESOLUTION_PREFERRED	INTEL_DP_RESOLUTION(1)
> +#define INTEL_DP_RESOLUTION_STANDARD	INTEL_DP_RESOLUTION(2)
> +#define INTEL_DP_RESOLUTION_FAILSAFE	INTEL_DP_RESOLUTION(3)

One issue with this is that our hw engineers love to extend bitfields at
both ends, and by hiding shifts it's kinda hard to see what's going on
exactly. Or they reuse bits used for other things on older platforms,
which again makes it harder to spot them if they're hidden behind neat
macros.

Not sure where to wheigh my concern against the obvious benefit of
checking our bitfield register code. Iirc that kind of historic madness
was also what killed the idea of using the Bspec xml. AMD had to write a
complete new driver to be able to do this, and they have a hw team which
seems rather good at obeying sensible compatability guarantees when reving
their hw ip blocks. We're definitely not in that dream land.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f52eef138247..fe44b06adeb4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -42,10 +42,12 @@ 
 #define DP_LINK_CHECK_TIMEOUT	(10 * 1000)
 
 /* Compliance test status bits  */
-#define INTEL_DP_RESOLUTION_SHIFT_MASK	0
-#define INTEL_DP_RESOLUTION_PREFERRED	(1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
-#define INTEL_DP_RESOLUTION_STANDARD	(2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
-#define INTEL_DP_RESOLUTION_FAILSAFE	(3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
+#define __FIELD__(x, shift, width) ({ if (__builtin_constant_p(x)) BUILD_BUG_ON((x) & -(1<<width)); ((x) << (shift)); })
+#define INTEL_DP_RESOLUTION_SHIFT 0
+#define INTEL_DP_RESOLUTION(x) __FIELD__(x, INTEL_DP_RESOLUTION_SHIFT, 2)
+#define INTEL_DP_RESOLUTION_PREFERRED	INTEL_DP_RESOLUTION(1)
+#define INTEL_DP_RESOLUTION_STANDARD	INTEL_DP_RESOLUTION(2)
+#define INTEL_DP_RESOLUTION_FAILSAFE	INTEL_DP_RESOLUTION(3)
 
 struct dp_link_dpll {
 	int link_bw;