[Intel-xe] [PATCH 11/17] drm/xe: Introduce xe_reg_t
Lucas De Marchi
lucas.demarchi at intel.com
Wed Apr 19 20:13:52 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().
but this is another topic that would be a follow up. Somewhere in the
commit messages I mentioned that the next step would be to make
xe_mmio_* strong typed
>
>Strong typing for i915_reg_t has positive score 9 on Rusty's API Design
>Manifesto [1].
here the discussion is having or not N types for the different types of
registers we have, instead of one type, xe_reg_t, and proper runtime
checks where needed.
Btw, note that we already rely on runtime checks for misuse of
i915_mcr_reg_t: when that type is used and there isn't a range for it
in xe_gt_mcr.c
Lucas De Marchi
>
>
>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
More information about the Intel-xe
mailing list