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

[Sripada, Radhakrishna]

Hi Jani,

After this patch the following will be the main trace declarations
xe_gt_tlb_invalidation_fence, xe_bo, xe_exec_queue, xe_sched_job, xe_sched_msg,
xe_hw_fence, xe_vma, xe_vm, xe_guc_ct_flow_control, xe_guc_ctb, xe_reg_rw

Would it be ok if we can group them like the following.

xe_trace_bo.h : xe_bo, xe_vma, xe_vm
xe_trace_gt.h: xe_gt_tlb_invalidation_fence, xe_exec_queue, xe_sched_job, xe_sched_msg, xe_hw_fence, xe_reg_rw
xe_trace_guc.h: xe_guc_ct_flow_control, xe_guc_ctb

Does this grouping seem ok?

Regards,
Radhakrishna Sripada 

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Friday, April 19, 2024 4:46 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> xe at lists.freedesktop.org
> Cc: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; De Marchi, Lucas
> <lucas.demarchi at intel.com>
> Subject: Re: [PATCH] drm/xe: Add reg read/write trace
> 
> On Thu, 18 Apr 2024, Radhakrishna Sripada <radhakrishna.sripada at 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 at 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