On Tue, Oct 04, 2022 at 04:00:57PM +0300, Jani Nikula wrote:
> On Tue, 04 Oct 2022, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > On Fri, 30 Sep 2022, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
> >> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
> >> index 8f486f77609f..e823869b9afd 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> >> @@ -104,22 +104,16 @@ typedef struct {
> >>
> >> #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
> >>
> >> -#define INVALID_MMIO_REG _MMIO(0)
> >> -
> >> -static __always_inline u32 i915_mmio_reg_offset(i915_reg_t reg)
> >> -{
> >> - return reg.reg;
> >> -}
> >> +typedef struct {
> >> + u32 reg;
> >> +} i915_mcr_reg_t;
> >>
> >> -static inline bool i915_mmio_reg_equal(i915_reg_t a, i915_reg_t b)
> >> -{
> >> - return i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b);
> >> -}
> >> +#define INVALID_MMIO_REG _MMIO(0)
> >>
> >> -static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >> -{
> >> - return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
> >> -}
> >> +/* These macros can be used on either i915_reg_t or i915_mcr_reg_t */
> >> +#define i915_mmio_reg_offset(r) (r.reg)
> >> +#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b))
> >> +#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))
> >>
> >
> > I don't really like losing the type safety here. The whole and only
> > purpose of typedeffing i915_reg_t as a struct instead of just u32 was
> > the strict type safety.
>
> PS. Changes like this should really be highlighted better, in the commit
> subject and title. Now it feels like it's hidden within a big commit
> within a big series, without even mentioning it in the commit message.
This patch adds the type safety we've been missing until now. The only
thing that's slightly less safe is the i915_mmio_reg_offset() macro
itself since it allows either of the two split register types to be
passed to the same macro for simplicity. In theory if you had some
other structure that also had a 'reg' member it could sneak through
here, but most type mistakes (e.g., passing an integer) would still
cause a build failure.
If we want to make this specific macro more bullet-proof, while still
minimizing thrash elsewhere, we could re-write this as
#define i915_mmio_reg_offset(r) \
_Generic((r), i915_reg_t: (r).reg, i915_mcr_reg_t: (r).reg)
which I believe should result in a build failure if anything other than
an i915_reg_t or i915_mcr_reg_t is passed in.
We could also just split this out into separate MCR vs non-MCR offset
lookup functions as we've done with other things in this series. But if
I recall correctly that leads to annoying thrash in stuff like
cmdparser, gvt, perf, etc. that never actually wanted registers to begin
with, but rather lists/ranges of MMIO offsets without a care for the
type of register that lives at the offset.
Matt
>
>
> BR,
> Jani.
>
>
> >
> > BR,
> > Jani.
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
