| Message ID | 1434707535-13014-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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;
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(-)