| Message ID | 1485532626-20923-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 27, 2017 at 05:57:06PM +0200, Jani Nikula wrote: > This allows the use of more than 3 ports/pipes/whatever without tricks, > even if the register offsets are not evenly spaced. > > There's the risk of out of bounds access if we're not careful; currently > that would "just" lead to the wrong register offset being used. It might > be possible to add build bug ons for build time constant indexing. > > We already have ports defined up to E, not sure if we might have bugs > related to them and the current _PORT3() macro. > > text data bss dec hex filename > 1239868 46199 4096 1290163 13afb3 drivers/gpu/drm/i915/i915.ko > 1238828 46199 4096 1289123 13aba3 drivers/gpu/drm/i915/i915.ko > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Tvrtko Ursulin <tursulin@ursulin.net> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 672cb102f477..c6435a447300 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG); > } > > +#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index]) Very neat. The danger is that for a variable index, the compiler will plonk the array on the stack, for each invocation. Though for a constant the compiler will see through it and generate the right constant. https://godbolt.org/g/YCK1od Given that it looks like the compiler will get smarter, this looks like a much less error prone way of writing these. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Fri, Jan 27, 2017 at 05:57:06PM +0200, Jani Nikula wrote: > This allows the use of more than 3 ports/pipes/whatever without tricks, > even if the register offsets are not evenly spaced. > > There's the risk of out of bounds access if we're not careful; currently > that would "just" lead to the wrong register offset being used. It might > be possible to add build bug ons for build time constant indexing. > > We already have ports defined up to E, not sure if we might have bugs > related to them and the current _PORT3() macro. > > text data bss dec hex filename > 1239868 46199 4096 1290163 13afb3 drivers/gpu/drm/i915/i915.ko > 1238828 46199 4096 1289123 13aba3 drivers/gpu/drm/i915/i915.ko Fits in well with all the other preprocessor horrors we have ;) I'm convinced. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Tvrtko Ursulin <tursulin@ursulin.net> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 672cb102f477..c6435a447300 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG); > } > > +#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index]) > + > #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a))) > #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b)) > #define _PLANE(plane, a, b) _PIPE(plane, a, b) > @@ -56,14 +58,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) > #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b)) > #define _PORT(port, a, b) ((a) + (port)*((b)-(a))) > #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b)) > -#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \ > - (pipe) == PIPE_B ? (b) : (c)) > +#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__) > #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c)) > -#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \ > - (port) == PORT_B ? (b) : (c)) > +#define _PORT3(port, ...) _PICK(port, __VA_ARGS__) > #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c)) > -#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \ > - (phy) == DPIO_PHY1 ? (b) : (c)) > +#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__) > #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c)) > > #define _MASKED_FIELD(mask, value) ({ \ > -- > 2.1.4
On Fri, 27 Jan 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Jan 27, 2017 at 05:57:06PM +0200, Jani Nikula wrote: >> This allows the use of more than 3 ports/pipes/whatever without tricks, >> even if the register offsets are not evenly spaced. >> >> There's the risk of out of bounds access if we're not careful; currently >> that would "just" lead to the wrong register offset being used. It might >> be possible to add build bug ons for build time constant indexing. >> >> We already have ports defined up to E, not sure if we might have bugs >> related to them and the current _PORT3() macro. >> >> text data bss dec hex filename >> 1239868 46199 4096 1290163 13afb3 drivers/gpu/drm/i915/i915.ko >> 1238828 46199 4096 1289123 13aba3 drivers/gpu/drm/i915/i915.ko >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Tvrtko Ursulin <tursulin@ursulin.net> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 11 +++++------ >> 1 file changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 672cb102f477..c6435a447300 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) >> return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG); >> } >> >> +#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index]) > > Very neat. The danger is that for a variable index, the compiler will > plonk the array on the stack, for each invocation. Though for a constant > the compiler will see through it and generate the right constant. > > https://godbolt.org/g/YCK1od > > Given that it looks like the compiler will get smarter, this looks like > a much less error prone way of writing these. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thanks for the reviews, pushed to drm-intel-next-queued with Daniel's IRC ack added on top. BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 672cb102f477..c6435a447300 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG); } +#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index]) + #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a))) #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b)) #define _PLANE(plane, a, b) _PIPE(plane, a, b) @@ -56,14 +58,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b)) #define _PORT(port, a, b) ((a) + (port)*((b)-(a))) #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b)) -#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \ - (pipe) == PIPE_B ? (b) : (c)) +#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__) #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c)) -#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \ - (port) == PORT_B ? (b) : (c)) +#define _PORT3(port, ...) _PICK(port, __VA_ARGS__) #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c)) -#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \ - (phy) == DPIO_PHY1 ? (b) : (c)) +#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__) #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c)) #define _MASKED_FIELD(mask, value) ({ \
This allows the use of more than 3 ports/pipes/whatever without tricks, even if the register offsets are not evenly spaced. There's the risk of out of bounds access if we're not careful; currently that would "just" lead to the wrong register offset being used. It might be possible to add build bug ons for build time constant indexing. We already have ports defined up to E, not sure if we might have bugs related to them and the current _PORT3() macro. text data bss dec hex filename 1239868 46199 4096 1290163 13afb3 drivers/gpu/drm/i915/i915.ko 1238828 46199 4096 1289123 13aba3 drivers/gpu/drm/i915/i915.ko Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Tvrtko Ursulin <tursulin@ursulin.net> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)