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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 18 Apr 2024, Radhakrishna Sripada <radhakrishna.sripada@intel.com> 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)",
> +		__entry->write ? "write" : "read",
> +		__entry->reg, __entry->len,
> +		(u32)(__entry->val & 0xffffffff),
> +		(u32)(__entry->val >> 32))
> +);
> +

Btw xe_trace.h already has all the makings of a catch all header that
has to include everything to get stuff done, and then all the places
doing tracing end up depending on everything. Change some innocuous
header somewhere, and you need to rebuild the entire driver. We have
that in i915, and it was slightly mitigated by separating display
traces, but it's still bad.

It might be a good idea to split this up to xe_<feature>_trace.h files
instead. Failing at that, maybe stop adding new stuff to it?


BR,
Jani.


>  #endif
>  
>  /* This part must be outside protection */

-- 
Jani Nikula, Intel