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

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx