[Intel-xe] [PATCH 1/3] drm/xe/reg_sr: Simplify check for masked registers

[Mika Kuoppala]

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;
+
        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;
+               }
+
                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