[Intel-xe] [PATCH 11/17] drm/xe: Introduce xe_reg_t
Matt Roper
matthew.d.roper at intel.com
Wed Apr 19 20:13:59 UTC 2023
On Wed, Apr 19, 2023 at 10:49:41PM +0300, Jani Nikula wrote:
> On Wed, 19 Apr 2023, Matt Roper <matthew.d.roper at intel.com> wrote:
> > On Wed, Apr 19, 2023 at 12:44:34AM -0700, Lucas De Marchi wrote:
> >> Stop using i915 types for register our own xe_reg_t. Differently from
> >> i915, this will keep under this will keep under the register definition
> >> the knowledge for the different types of registers. For now, the "flags"
> >> are mcr and masked, although only the former is being used.
> >>
> >> Most of the driver is agnostic to the register differences. Convert the
> >> few places that care about that, namely xe_gt_mcr.c, to take the generic
> >> type and warn if the wrong register is used.
> >
> > The disadvantage of this approach is that we don't get the nice
> > type-checking that we have in i915 to catch register misuse at build
> > time. Instead we wind up with a bunch of run-time checks that only tell
> > you that you used the wrong register semantics after the fact. Wouldn't
> > it be better to keep the strict types and let the compiler find mistakes
> > for us at build time?
> >
> > The only real problem with using strict types is that there are a few
> > places where the driver is just trying to refer to registers/offsets
> > without actually accessing them (e.g., whitelists, OA register groups,
> > etc.) and the strict typing makes it harder to provide a heterogeneous
> > list of those registers. But I still feel the benefits of compile-time
> > checking outweighs the extra complexity we wind up with in a handful of
> > places.
>
> The history of i915_reg_t is that there were subtle bugs where the
> arguments to the register write call were accidentally swapped, they
> went unnoticed, and needed lengthy debugging to finally figure out.
>
> This can happen with xe_mmio_write32().
>
> Strong typing for i915_reg_t has positive score 9 on Rusty's API Design
> Manifesto [1].
Exactly. That's why I think we should stick with the strong typing for
MCR vs non-MCR as well, rather than just adding a bunch of runtime
assertions. It sounds like the proposal here would only earn a score of
5 by that website's metrics, whereas we have a 9 in i915.
Matt
>
>
> BR,
> Jani.
>
>
> [1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
>
>
>
> >
> >
> > Matt
> >
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> >> ---
> >> drivers/gpu/drm/xe/regs/xe_reg_defs.h | 15 +++++++++++++++
> >> drivers/gpu/drm/xe/xe_gt_mcr.c | 22 ++++++++++++++++------
> >> drivers/gpu/drm/xe/xe_gt_mcr.h | 8 ++++----
> >> drivers/gpu/drm/xe/xe_irq.c | 2 +-
> >> drivers/gpu/drm/xe/xe_mmio.c | 2 +-
> >> 5 files changed, 37 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/regs/xe_reg_defs.h b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> >> index b5c25e31b889..1e78508c737b 100644
> >> --- a/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> >> +++ b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> >> @@ -8,4 +8,19 @@
> >>
> >> #include "compat-i915-headers/i915_reg_defs.h"
> >>
> >> +typedef union {
> >> + struct {
> >> + u32 reg:30;
> >> + u32 mcr:1;
> >> + u32 masked:1;
> >> + };
> >> + u32 raw;
> >> +} xe_reg_t;
> >> +
> >> +/* TODO: remove these once the register declarations are not using them anymore */
> >> +#undef _MMIO
> >> +#undef MCR_REG
> >> +#define _MMIO(r) ((const xe_reg_t){ .reg = (r) })
> >> +#define MCR_REG(r) ((const xe_reg_t){ .reg = (r), .mcr = 1 })
> >> +
> >> #endif
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
> >> index aa04ba5a6dbe..b9631cfd5b81 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> >> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> >> @@ -360,11 +360,13 @@ void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt)
> >> * returned. Returns false if the caller need not perform any steering
> >> */
> >> static bool xe_gt_mcr_get_nonterminated_steering(struct xe_gt *gt,
> >> - i915_mcr_reg_t reg,
> >> + xe_reg_t reg,
> >> u8 *group, u8 *instance)
> >> {
> >> const struct xe_mmio_range *implicit_ranges;
> >>
> >> + drm_WARN_ON(>_to_xe(gt)->drm, !reg.mcr);
> >> +
> >> for (int type = 0; type < IMPLICIT_STEERING; type++) {
> >> if (!gt->steering[type].ranges)
> >> continue;
> >> @@ -436,11 +438,13 @@ static void mcr_unlock(struct xe_gt *gt) {
> >> *
> >> * Caller needs to make sure the relevant forcewake wells are up.
> >> */
> >> -static u32 rw_with_mcr_steering(struct xe_gt *gt, i915_mcr_reg_t reg, u8 rw_flag,
> >> +static u32 rw_with_mcr_steering(struct xe_gt *gt, xe_reg_t reg, u8 rw_flag,
> >> int group, int instance, u32 value)
> >> {
> >> u32 steer_reg, steer_val, val = 0;
> >>
> >> + drm_WARN_ON(>_to_xe(gt)->drm, !reg.mcr);
> >> +
> >> lockdep_assert_held(>->mcr_lock);
> >>
> >> if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270) {
> >> @@ -494,12 +498,14 @@ static u32 rw_with_mcr_steering(struct xe_gt *gt, i915_mcr_reg_t reg, u8 rw_flag
> >> *
> >> * Returns the value from a non-terminated instance of @reg.
> >> */
> >> -u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, i915_mcr_reg_t reg)
> >> +u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, xe_reg_t reg)
> >> {
> >> u8 group, instance;
> >> u32 val;
> >> bool steer;
> >>
> >> + drm_WARN_ON(>_to_xe(gt)->drm, !reg.mcr);
> >> +
> >> steer = xe_gt_mcr_get_nonterminated_steering(gt, reg, &group, &instance);
> >>
> >> if (steer) {
> >> @@ -525,11 +531,13 @@ u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, i915_mcr_reg_t reg)
> >> * group/instance.
> >> */
> >> u32 xe_gt_mcr_unicast_read(struct xe_gt *gt,
> >> - i915_mcr_reg_t reg,
> >> + xe_reg_t reg,
> >> int group, int instance)
> >> {
> >> u32 val;
> >>
> >> + drm_WARN_ON(>_to_xe(gt)->drm, !reg.mcr);
> >> +
> >> mcr_lock(gt);
> >> val = rw_with_mcr_steering(gt, reg, MCR_OP_READ, group, instance, 0);
> >> mcr_unlock(gt);
> >> @@ -548,9 +556,11 @@ u32 xe_gt_mcr_unicast_read(struct xe_gt *gt,
> >> * Write an MCR register in unicast mode after steering toward a specific
> >> * group/instance.
> >> */
> >> -void xe_gt_mcr_unicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value,
> >> +void xe_gt_mcr_unicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value,
> >> int group, int instance)
> >> {
> >> + drm_WARN_ON(>_to_xe(gt)->drm, !reg.mcr);
> >> +
> >> mcr_lock(gt);
> >> rw_with_mcr_steering(gt, reg, MCR_OP_WRITE, group, instance, value);
> >> mcr_unlock(gt);
> >> @@ -564,7 +574,7 @@ void xe_gt_mcr_unicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value,
> >> *
> >> * Write an MCR register in multicast mode to update all instances.
> >> */
> >> -void xe_gt_mcr_multicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value)
> >> +void xe_gt_mcr_multicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value)
> >> {
> >> /*
> >> * Synchronize with any unicast operations. Once we have exclusive
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
> >> index 2a6cd38c8cb7..492d9519784a 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_mcr.h
> >> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
> >> @@ -15,13 +15,13 @@ void xe_gt_mcr_init(struct xe_gt *gt);
> >>
> >> void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt);
> >>
> >> -u32 xe_gt_mcr_unicast_read(struct xe_gt *gt, i915_mcr_reg_t reg,
> >> +u32 xe_gt_mcr_unicast_read(struct xe_gt *gt, xe_reg_t reg,
> >> int group, int instance);
> >> -u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, i915_mcr_reg_t reg);
> >> +u32 xe_gt_mcr_unicast_read_any(struct xe_gt *gt, xe_reg_t reg);
> >>
> >> -void xe_gt_mcr_unicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value,
> >> +void xe_gt_mcr_unicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value,
> >> int group, int instance);
> >> -void xe_gt_mcr_multicast_write(struct xe_gt *gt, i915_mcr_reg_t reg, u32 value);
> >> +void xe_gt_mcr_multicast_write(struct xe_gt *gt, xe_reg_t reg, u32 value);
> >>
> >> void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p);
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> >> index 13f9f220bca0..8e5f8e7c16c8 100644
> >> --- a/drivers/gpu/drm/xe/xe_irq.c
> >> +++ b/drivers/gpu/drm/xe/xe_irq.c
> >> @@ -27,7 +27,7 @@
> >> #define IIR(offset) _MMIO(offset + 0x8)
> >> #define IER(offset) _MMIO(offset + 0xc)
> >>
> >> -static void assert_iir_is_zero(struct xe_gt *gt, i915_reg_t reg)
> >> +static void assert_iir_is_zero(struct xe_gt *gt, xe_reg_t reg)
> >> {
> >> u32 val = xe_mmio_read32(gt, reg.reg);
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> >> index a93838e23b7b..1029b9f27988 100644
> >> --- a/drivers/gpu/drm/xe/xe_mmio.c
> >> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> >> @@ -397,7 +397,7 @@ int xe_mmio_init(struct xe_device *xe)
> >> DRM_XE_MMIO_READ |\
> >> DRM_XE_MMIO_WRITE)
> >>
> >> -static const i915_reg_t mmio_read_whitelist[] = {
> >> +static const xe_reg_t mmio_read_whitelist[] = {
> >> RING_TIMESTAMP(RENDER_RING_BASE),
> >> };
> >>
> >> --
> >> 2.39.0
> >>
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list