Re: [PATCH] drm/xe: Add reg read/write trace

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 18, 2024 at 03:22:43PM -0700, Radhakrishna Sripada wrote:
> This will help debug register read/writes and provides
> a way to trace all the mmio transactions.
> 
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_mmio.c  | 20 +++++++++++++++++---
>  drivers/gpu/drm/xe/xe_trace.h | 28 ++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 334637511e75..659c19d4f0a6 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -22,6 +22,7 @@
>  #include "xe_module.h"
>  #include "xe_sriov.h"
>  #include "xe_tile.h"
> +#include "xe_trace.h"
>  
>  #define XEHP_MTCFG_ADDR		XE_REG(0x101800)
>  #define TILE_COUNT		REG_GENMASK(15, 8)
> @@ -423,21 +424,29 @@ int xe_mmio_init(struct xe_device *xe)
>  u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
>  {
>  	struct xe_tile *tile = gt_to_tile(gt);
> +	u8 val;
>  
>  	if (reg.addr < gt->mmio.adj_limit)
>  		reg.addr += gt->mmio.adj_offset;
>  
> -	return readb((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + reg.addr);
> +	val = readb((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + reg.addr);
> +	trace_xe_reg_rw(false, reg, val, sizeof(val), true);
> +
> +	return val;
>  }
>  
>  u16 xe_mmio_read16(struct xe_gt *gt, struct xe_reg reg)
>  {
>  	struct xe_tile *tile = gt_to_tile(gt);
> +	u16 val;
>  
>  	if (reg.addr < gt->mmio.adj_limit)
>  		reg.addr += gt->mmio.adj_offset;
>  
> -	return readw((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + reg.addr);
> +	val = readw((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + reg.addr);
> +	trace_xe_reg_rw(false, reg, val, sizeof(val), true);
> +
> +	return val;
>  }
>  
>  void xe_mmio_write32(struct xe_gt *gt, struct xe_reg reg, u32 val)
> @@ -447,17 +456,22 @@ void xe_mmio_write32(struct xe_gt *gt, struct xe_reg reg, u32 val)
>  	if (reg.addr < gt->mmio.adj_limit)
>  		reg.addr += gt->mmio.adj_offset;
>  
> +	trace_xe_reg_rw(true, reg, val, sizeof(val), true);
>  	writel(val, (reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + reg.addr);
>  }
>  
>  u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
>  {
>  	struct xe_tile *tile = gt_to_tile(gt);
> +	u32 val;
>  
>  	if (reg.addr < gt->mmio.adj_limit)
>  		reg.addr += gt->mmio.adj_offset;
>  
> -	return readl((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + reg.addr);
> +	val = readl((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + reg.addr);
> +	trace_xe_reg_rw(false, reg, val, sizeof(val), true);
> +
> +	return val;
>  }
>  
>  u32 xe_mmio_rmw32(struct xe_gt *gt, struct xe_reg reg, u32 clr, u32 set)
> diff --git a/drivers/gpu/drm/xe/xe_trace.h b/drivers/gpu/drm/xe/xe_trace.h
> index 2d56cfc09e42..41d778dd43c6 100644
> --- a/drivers/gpu/drm/xe/xe_trace.h
> +++ b/drivers/gpu/drm/xe/xe_trace.h
> @@ -621,6 +621,34 @@ DEFINE_EVENT_PRINT(xe_guc_ctb, xe_guc_ctb_g2h,
>  
>  );
>  
> +TRACE_EVENT_CONDITION(xe_reg_rw,
> +	TP_PROTO(bool write, u32 reg, u64 val, int len, bool trace),
> +
> +	TP_ARGS(write, reg, val, len, trace),
> +
> +	TP_CONDITION(trace),
> +
> +	TP_STRUCT__entry(
> +		__field(u64, val)
> +		__field(u32, reg)
> +		__field(u16, write)
> +		__field(u16, len)
> +		),
> +
> +	TP_fast_assign(
> +		__entry->val = (u64)val;
> +		__entry->reg = reg;
> +		__entry->write = write;
> +		__entry->len = len;
> +		),
> +
> +	TP_printk("%s reg=0x%x, len=%d, val=(0x%x, 0x%x)",

Someone should do something about all the i915 and xe
tracepoints that lack the device information. Currently
those are somewhat useless on any multi-gpu system (unless
you can guarantee only gpu is actually doing something).

> +		__entry->write ? "write" : "read",
> +		__entry->reg, __entry->len,
> +		(u32)(__entry->val & 0xffffffff),
> +		(u32)(__entry->val >> 32))
> +);
> +
>  #endif
>  
>  /* This part must be outside protection */
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel