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

[Sripada, Radhakrishna]

Hi Gustavo,

> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa at intel.com>
> Sent: Monday, June 3, 2024 8:45 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> xe at lists.freedesktop.org
> Cc: De Marchi, Lucas <lucas.demarchi at intel.com>; Sripada, Radhakrishna
> <radhakrishna.sripada at intel.com>
> Subject: Re: [PATCH v3 6/6] drm/xe: Add reg read/write trace
> 
> Quoting Radhakrishna Sripada (2024-05-30 12:13:13-03:00)
> >This will help debug register read/writes and provides
> >a way to trace all the mmio transactions.
> >
> >v2: Fix kunit error
> >v3: Print devid to help in multi-gpu setup
> >
> >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> >---
> > drivers/gpu/drm/xe/xe_mmio.c  | 22 ++++++++++++++++++----
> > drivers/gpu/drm/xe/xe_trace.h | 30 ++++++++++++++++++++++++++++++
> > 2 files changed, 48 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> >index 248e93ec6df7..a44baa349109 100644
> >--- a/drivers/gpu/drm/xe/xe_mmio.c
> >+++ b/drivers/gpu/drm/xe/xe_mmio.c
> >@@ -27,6 +27,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)
> >@@ -455,16 +456,24 @@ u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg
> reg)
> > {
> >         struct xe_tile *tile = gt_to_tile(gt);
> >         u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
> >+        u8 val;
> >
> >-        return readb((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> >+        val = readb((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> >+        trace_xe_reg_rw(dev_name(gt_to_xe(gt)->drm.dev), false, addr, 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);
> >         u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
> >+        u16 val;
> >+
> >+        val = readw((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> >+        trace_xe_reg_rw(dev_name(gt_to_xe(gt)->drm.dev), false, addr, val,
> sizeof(val), true);
> >
> >-        return readw((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> >+        return val;
> > }
> >
> > void xe_mmio_write32(struct xe_gt *gt, struct xe_reg reg, u32 val)
> >@@ -472,6 +481,7 @@ void xe_mmio_write32(struct xe_gt *gt, struct xe_reg
> reg, u32 val)
> >         struct xe_tile *tile = gt_to_tile(gt);
> >         u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
> >
> >+        trace_xe_reg_rw(dev_name(gt_to_xe(gt)->drm.dev), true, addr, val,
> sizeof(val), true);
> >         writel(val, (reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> > }
> >
> >@@ -479,11 +489,15 @@ u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg
> reg)
> > {
> >         struct xe_tile *tile = gt_to_tile(gt);
> >         u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
> >+        u32 val;
> >
> >         if (!reg.vf && IS_SRIOV_VF(gt_to_xe(gt)))
> >-                return xe_gt_sriov_vf_read32(gt, reg);
> >+                val = xe_gt_sriov_vf_read32(gt, reg);
> >+
> >+        val = readl((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> 
> This readl() call needs to be inside an "else" statement, right?
Ah nice catch. Will fix that.
> 
> >+        trace_xe_reg_rw(dev_name(gt_to_xe(gt)->drm.dev), false, addr, val,
> sizeof(val), true);
> >
> >-        return readl((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> >+        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 41e0b3365876..d56e073560ee 100644
> >--- a/drivers/gpu/drm/xe/xe_trace.h
> >+++ b/drivers/gpu/drm/xe/xe_trace.h
> >@@ -342,6 +342,36 @@ DEFINE_EVENT(xe_hw_fence, xe_hw_fence_free,
> >              TP_ARGS(fence)
> > );
> >
> >+TRACE_EVENT_CONDITION(xe_reg_rw,
> >+        TP_PROTO(const char *devid, bool write, u32 reg, u64 val, int len, bool
> trace),
> >+
> >+        TP_ARGS(devid, write, reg, val, len, trace),
> >+
> >+        TP_CONDITION(trace),
> 
> Looks like trace is always true. Are there plans to have a false value?
I don’t think we have a no trace variant for any calls. So I will simply omit this argument.

-- Radhakrishna Sripada
> 
> --
> Gustavo Sousa
> 
> >+
> >+        TP_STRUCT__entry(
> >+                __array(char, devid, 12)
> >+                __field(u64, val)
> >+                __field(u32, reg)
> >+                __field(u16, write)
> >+                __field(u16, len)
> >+                ),
> >+
> >+        TP_fast_assign(
> >+                memcpy(__entry->devid, devid, 12);
> >+                __entry->val = (u64)val;
> >+                __entry->reg = reg;
> >+                __entry->write = write;
> >+                __entry->len = len;
> >+                ),
> >+
> >+        TP_printk("dev: %s: %s reg=0x%x, len=%d, val=(0x%x, 0x%x)",
> >+                  __entry->devid, __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
> >