Re: [PATCH 2/4] drm/xe/rtp: Add FIELD_SET_FUNC RTP action

[Gustavo Sousa <gustavo.sousa@intel.com>]

Matt Roper <matthew.d.roper@intel.com> writes:

> Most of our RTP programming involves programming constant values into
> register fields.  However there are a few cases (e.g., RING_CMD_CCTL
> programming) that rely on dynamic per-GT or per-engine checks to decide
> what value will be programmed.  Add a FIELD_SET_FUNC RTP action which
> will call the provided function pointer once at RTP processing time to
> determine the appropriate value.

That's a great idea! I think it would be nice if we added a test case
for this in rtp_to_sr_cases[], for completeness.

More feedback below...

>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_rtp.c       | 10 ++++++++--
>  drivers/gpu/drm/xe/xe_rtp.h       | 21 +++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_rtp_types.h | 17 +++++++++++++++--
>  3 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
> index 83a40e1f9528..6a8d6ea68f25 100644
> --- a/drivers/gpu/drm/xe/xe_rtp.c
> +++ b/drivers/gpu/drm/xe/xe_rtp.c
> @@ -227,17 +227,23 @@ static bool rule_matches(const struct xe_device *xe,
>  
>  static void rtp_add_sr_entry(const struct xe_rtp_action *action,
>  			     struct xe_gt *gt,
> +			     struct xe_hw_engine *hwe,
>  			     u32 mmio_base,
>  			     struct xe_reg_sr *sr)
>  {
>  	struct xe_reg_sr_entry sr_entry = {
>  		.reg = action->reg,
>  		.clr_bits = action->clr_bits,
> -		.set_bits = action->set_bits,
>  		.read_mask = action->read_mask,
>  	};
>  
> +	if (action->use_func)
> +		sr_entry.set_bits = action->set_func(gt, hwe);
> +	else
> +		sr_entry.set_bits = action->set_bits;
> +
>  	sr_entry.reg.addr += mmio_base;
> +
>  	xe_reg_sr_add(sr, &sr_entry, gt);
>  }
>  
> @@ -259,7 +265,7 @@ static bool rtp_process_one_sr(const struct xe_rtp_entry_sr *entry,
>  		else
>  			mmio_base = 0;
>  
> -		rtp_add_sr_entry(action, gt, mmio_base, sr);
> +		rtp_add_sr_entry(action, gt, hwe, mmio_base, sr);
>  	}
>  
>  	return true;
> diff --git a/drivers/gpu/drm/xe/xe_rtp.h b/drivers/gpu/drm/xe/xe_rtp.h
> index 2cc65053cd07..e374b57a066c 100644
> --- a/drivers/gpu/drm/xe/xe_rtp.h
> +++ b/drivers/gpu/drm/xe/xe_rtp.h
> @@ -322,6 +322,27 @@ struct xe_reg_sr;
>  	  .clr_bits = (mask_bits_), .set_bits = (val_),				\
>  	  .read_mask = 0, ##__VA_ARGS__ }
>  
> +/**
> + * XE_RTP_ACTION_FIELD_SET_FUNC: Set a bit range to the value returned by a function
> + * @reg_: Register
> + * @mask_bits_: Mask of bits to be changed in the register, forming a field
> + * @func_: Function that returns value to set in the field denoted by @mask_bits_
> + * @...: Additional fields to override in the struct xe_rtp_action entry
> + *
> + * @func_ will only be called a single time, when the RTP table is being
> + * processed.  After processing, the value in the reg_sr entry is fixed and
> + * will not be re-evaluated.
> + *
> + * For masked registers this translates to a single write, while for other
> + * registers it's a RMW. The correspondent bspec notation is:
> + *
> + *	REGNAME[<end>:<start>] = FUNC(GT, HWE)

I think we could avoid duplicating information in the doc by referring
to XE_RTP_ACTION_FIELD_SET().

So, before "@func_ will only be called (...)", we could add a paragraph
along the lines of:

      "This macro works like XE_RTP_ACTION_FIELD_SET(), except that the
       field value is evaluated by the time the RTP table is processed."

Then we can skip the last paragraph, which is a copy/paste from
XE_RTP_ACTION_FIELD_SET.

The change itself looks fine to me, so, unconditionally:

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

> + */
> +#define XE_RTP_ACTION_FIELD_SET_FUNC(reg_, mask_bits_, func_, ...)		\
> +	{ .reg = XE_RTP_DROP_CAST(reg_),					\
> +	  .clr_bits = mask_bits_, .set_func = func_, .use_func = 1,		\
> +	  .read_mask = mask_bits_, ##__VA_ARGS__ }
> +
>  /**
>   * XE_RTP_ACTION_WHITELIST - Add register to userspace whitelist
>   * @reg_: Register
> diff --git a/drivers/gpu/drm/xe/xe_rtp_types.h b/drivers/gpu/drm/xe/xe_rtp_types.h
> index 1d7c63d0ae94..b78092fa06e0 100644
> --- a/drivers/gpu/drm/xe/xe_rtp_types.h
> +++ b/drivers/gpu/drm/xe/xe_rtp_types.h
> @@ -30,8 +30,14 @@ struct xe_rtp_action {
>  	 */
>  	u32 clr_bits;
>  
> -	/** @set_bits: bits to set when updating register */
> -	u32 set_bits;
> +	union {
> +		/** @set_bits: bits to set when updating register */
> +		u32 set_bits;
> +
> +		/** @set_func: function to provide bits to set when updating register */
> +		u32 (*set_func)(struct xe_gt *gt,
> +				struct xe_hw_engine *hwe);
> +	};
>  
>  #define XE_RTP_NOCHECK		.read_mask = 0
>  	/** @read_mask: mask for bits to consider when reading value back */
> @@ -40,6 +46,13 @@ struct xe_rtp_action {
>  #define XE_RTP_ACTION_FLAG_ENGINE_BASE		BIT(0)
>  	/** @flags: flags to apply on rule evaluation or action */
>  	u8 flags;
> +
> +	/**
> +	 * @use_func:
> +	 *   Internal flag indicating @set_func should be called instead of
> +	 *   using @set_bits.
> +	 */
> +	u8 use_func:1;
>  };
>  
>  enum {
>
> -- 
> 2.54.0