[Intel-xe] [PATCH 1/3] drm/xe/reg_sr: Simplify check for masked registers
Lucas De Marchi
lucas.demarchi at intel.com
Tue Sep 5 16:14:21 UTC 2023
On Tue, Sep 05, 2023 at 06:39:31PM +0300, Mika Kuoppala wrote:
>Lucas De Marchi <lucas.demarchi at intel.com> writes:
>
>> For all RTP actions, clr_bits is a superset of the bits being modified.
>> That's also why the check for "changing all bits" can be done with
>> `clr_bits + 1`. So always use clr_bits for setting the upper bits of a
>> masked register.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_reg_sr.c | 8 ++++----
>> drivers/gpu/drm/xe/xe_rtp_types.h | 5 ++++-
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
>> index 7c88352636d2..264520015861 100644
>> --- a/drivers/gpu/drm/xe/xe_reg_sr.c
>> +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
>> @@ -153,15 +153,15 @@ static void apply_one_mmio(struct xe_gt *gt, struct xe_reg_sr_entry *entry)
>> u32 val;
>>
>> /*
>> - * If this is a masked register, need to figure what goes on the upper
>> - * 16 bits: it's either the clr_bits (when using FIELD_SET and WR) or
>> - * the set_bits, when using SET.
>> + * If this is a masked register, need to set the upper 16 bits.
>> + * Set them to clr_bits since that is always a superset of the bits
>> + * being modified.
>> *
>> * When it's not masked, we have to read it from hardware, unless we are
>> * supposed to set all bits.
>> */
>> if (reg.masked)
>> - val = (entry->clr_bits ?: entry->set_bits) << 16;
>> + val = entry->clr_bits << 16;
>
>Makes sense but should we consider also making sure in the
>setup side that the set_bits stays withing low if masked is set.
>So that we dont accidantally trample the upper parts.
>
>+++ b/drivers/gpu/drm/xe/xe_reg_sr.c
>@@ -80,6 +80,9 @@ static bool compatible_entries(const struct
>xe_reg_sr_entry *e1,
> if (e1->reg.raw != e2->reg.raw)
> return false;
>
>+ if (e1->reg.masked != e2->reg.masked)
>+ return false;
this is covered by the check above. The raw member is a union that
covers the entire register - address, masked, mcr.
>+
> return true;
> }
>
>@@ -104,6 +107,14 @@ int xe_reg_sr_add(struct xe_reg_sr *sr,
> goto fail;
> }
>
>+ if (XE_WARN_ON(e->reg.masked &&
>+ upper_32_bits(e->clr_bits |
>+ e->set_bits |
>+ e->read_mask))) {
>+ ret = -EINVAL;
>+ goto fail;
>+ }
yeah, we can add something like that in a separate patch. I'm also
thinking on renaming s/clr_bits/mod_bits/ to make sure it's captured
that those bits are always a superset of set_bits.
Lucas De Marchi
>+
> pentry->clr_bits |= e->clr_bits;
>
>Something like this?
>
>-Mika
>
>
>> else if (entry->clr_bits + 1)
>> val = (reg.mcr ?
>> xe_gt_mcr_unicast_read_any(gt, reg_mcr) :
>> diff --git a/drivers/gpu/drm/xe/xe_rtp_types.h b/drivers/gpu/drm/xe/xe_rtp_types.h
>> index d170532a98a5..637acc7626a4 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp_types.h
>> +++ b/drivers/gpu/drm/xe/xe_rtp_types.h
>> @@ -22,7 +22,10 @@ struct xe_gt;
>> struct xe_rtp_action {
>> /** @reg: Register */
>> struct xe_reg reg;
>> - /** @clr_bits: bits to clear when updating register */
>> + /**
>> + * @clr_bits: bits to clear when updating register. It's always a
>> + * superset of bits being modified
>> + */
>> u32 clr_bits;
>> /** @set_bits: bits to set when updating register */
>> u32 set_bits;
>> --
>> 2.40.1
More information about the Intel-xe
mailing list