[PATCH] drm/imagination: Add support for trace points

[PATCH] drm/imagination: Add support for trace points

[Alexandru Dadu]

Add support for workload submission trace points.

Co-developed-by: Donald Robson <donald.robson@imgtec.com>
Signed-off-by: Donald Robson <donald.robson@imgtec.com>
Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
---
 drivers/gpu/drm/imagination/Makefile           |   3 +
 drivers/gpu/drm/imagination/pvr_drv.c          |   3 +
 drivers/gpu/drm/imagination/pvr_job.c          |   3 +
 drivers/gpu/drm/imagination/pvr_queue.c        |   5 ++
 drivers/gpu/drm/imagination/pvr_trace.h        | 113 +++++++++++++++++++++++++
 drivers/gpu/drm/imagination/pvr_trace_points.c |   7 ++
 6 files changed, 134 insertions(+)

diff --git a/drivers/gpu/drm/imagination/Makefile b/drivers/gpu/drm/imagination/Makefile
index 1222a14262e4..a5c5e386f16a 100644
--- a/drivers/gpu/drm/imagination/Makefile
+++ b/drivers/gpu/drm/imagination/Makefile
@@ -32,6 +32,9 @@ powervr-y := \
 powervr-$(CONFIG_DEBUG_FS) += \
 	pvr_debugfs.o
 
+powervr-$(CONFIG_TRACEPOINTS) += \
+	pvr_trace_points.o
+
 obj-$(CONFIG_DRM_POWERVR) += powervr.o
 
 obj-$(CONFIG_DRM_POWERVR_KUNIT_TEST) += pvr_test.o
diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 268900464ab6..b20c462bcba0 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -14,6 +14,7 @@
 #include "pvr_rogue_defs.h"
 #include "pvr_rogue_fwif_client.h"
 #include "pvr_rogue_fwif_shared.h"
+#include "pvr_trace.h"
 #include "pvr_vm.h"
 
 #include <uapi/drm/pvr_drm.h>
@@ -1150,6 +1151,8 @@ pvr_ioctl_submit_jobs(struct drm_device *drm_dev, void *raw_args,
 	int idx;
 	int err;
 
+	trace_pvr_job_submit_ioctl(pvr_dev, args->jobs.count);
+
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
diff --git a/drivers/gpu/drm/imagination/pvr_job.c b/drivers/gpu/drm/imagination/pvr_job.c
index dd9f5df01e08..b8a58d817009 100644
--- a/drivers/gpu/drm/imagination/pvr_job.c
+++ b/drivers/gpu/drm/imagination/pvr_job.c
@@ -14,6 +14,7 @@
 #include "pvr_stream.h"
 #include "pvr_stream_defs.h"
 #include "pvr_sync.h"
+#include "pvr_trace.h"
 
 #include <drm/drm_exec.h>
 #include <drm/drm_gem.h>
@@ -510,6 +511,8 @@ static int pvr_job_data_init(struct pvr_device *pvr_dev,
 		}
 
 		job_data_out[i].sync_op_count = job_args[i].sync_ops.count;
+
+		trace_pvr_job_create(pvr_dev, job_data_out[i].job, job_data_out[i].sync_op_count);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
index d10a13173f0f..3c2331e834fb 100644
--- a/drivers/gpu/drm/imagination/pvr_queue.c
+++ b/drivers/gpu/drm/imagination/pvr_queue.c
@@ -10,6 +10,7 @@
 #include "pvr_drv.h"
 #include "pvr_job.h"
 #include "pvr_queue.h"
+#include "pvr_trace.h"
 #include "pvr_vm.h"
 
 #include "pvr_rogue_fwif_client.h"
@@ -725,6 +726,8 @@ static void pvr_queue_submit_job_to_cccb(struct pvr_job *job)
 		cmd->partial_render_geom_frag_fence.value = job->done_fence->seqno - 1;
 	}
 
+	trace_pvr_job_submit_fw(job);
+
 	/* Submit job to FW */
 	pvr_cccb_write_command_with_header(cccb, job->fw_ccb_cmd_type, job->cmd_len, job->cmd,
 					   job->id, job->id);
@@ -1193,6 +1196,8 @@ void pvr_queue_job_cleanup(struct pvr_job *job)
 
 	if (job->base.s_fence)
 		drm_sched_job_cleanup(&job->base);
+
+	trace_pvr_job_done(job);
 }
 
 /**
diff --git a/drivers/gpu/drm/imagination/pvr_trace.h b/drivers/gpu/drm/imagination/pvr_trace.h
new file mode 100644
index 000000000000..ffbbde3b2bcc
--- /dev/null
+++ b/drivers/gpu/drm/imagination/pvr_trace.h
@@ -0,0 +1,113 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/* Copyright (c) 2026 Imagination Technologies Ltd. */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pvr
+
+#if !defined(PVR_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define PVR_TRACE_H
+
+#include "pvr_context.h"
+#include "pvr_device.h"
+#include "pvr_fw.h"
+#include "pvr_hwrt.h"
+#include "pvr_job.h"
+#include "pvr_sync.h"
+
+#include <linux/stringify.h>
+#include <linux/types.h>
+#include <linux/tracepoint.h>
+
+/*
+ * NOTE:
+ * When adding trace points, or extra data to existing ones - you must capture
+ * all the data in the TP_fast_assign section that you wish to use in the
+ * TP_printk section. This is because the printk is performed on demand from the
+ * captured data when you `cat /sys/kernel/tracing/trace` and by the time this
+ * happens any pointers you captured will likely no longer point to valid data.
+ */
+
+/* Job submit */
+
+TRACE_EVENT(pvr_job_submit_ioctl,
+	    TP_PROTO(struct pvr_device *pvr_dev, u32 count),
+	    TP_ARGS(pvr_dev, count),
+	    TP_STRUCT__entry(__field(struct pvr_device *, pvr_dev)
+			     __field(u32, count)),
+	    TP_fast_assign(__entry->pvr_dev = pvr_dev;
+			   __entry->count = count;),
+	    TP_printk("pvr_dev=%p count=%u",
+		      __entry->pvr_dev,
+		      __entry->count)
+);
+
+#define PVR_JOB_TYPE_TO_STR(val)                                         \
+	__print_symbolic(val,                                            \
+			 { DRM_PVR_JOB_TYPE_GEOMETRY, "geometry" },      \
+			 { DRM_PVR_JOB_TYPE_FRAGMENT, "fragment" },      \
+			 { DRM_PVR_JOB_TYPE_COMPUTE, "compute" },        \
+			 { DRM_PVR_JOB_TYPE_TRANSFER_FRAG, "transfer" })
+
+TRACE_EVENT(pvr_job_create,
+	    TP_PROTO(struct pvr_device *pvr_dev, struct pvr_job *job,
+		     u32 sync_op_count),
+	    TP_ARGS(pvr_dev, job, sync_op_count),
+	    TP_STRUCT__entry(__field(struct pvr_device *, pvr_dev)
+			     __field(struct pvr_context *, ctx)
+			     __field(struct pvr_fw_object *, fw_obj)
+			     __field(u32, fw_addr)
+			     __field(u32, hwrt_addr)
+			     __field(struct pvr_job *, job)
+			     __field(enum drm_pvr_job_type, job_type)
+			     __field(u32, sync_op_count)),
+	    TP_fast_assign(__entry->pvr_dev = pvr_dev;
+			   __entry->ctx = job->ctx;
+			   __entry->fw_obj = job->ctx->fw_obj;
+			   pvr_fw_object_get_fw_addr(job->ctx->fw_obj, &__entry->fw_addr);
+			   __entry->hwrt_addr = job->hwrt ?
+						job->hwrt->fw_obj->fw_addr_offset :
+						0;
+			   __entry->job = job;
+			   __entry->job_type = job->type;
+			   __entry->sync_op_count = sync_op_count;),
+	    TP_printk("pvr_dev=%p ctx=%p fw_obj=%p fw_addr=0x%x hwrt_addr=0x%x job=%p job_type=%s sync_op_count=%u",
+		      __entry->pvr_dev,
+		      __entry->ctx,
+		      __entry->fw_obj,
+		      __entry->fw_addr,
+		      __entry->hwrt_addr,
+		      __entry->job,
+		      PVR_JOB_TYPE_TO_STR(__entry->job_type),
+		      __entry->sync_op_count)
+);
+
+#undef PVR_JOB_TYPE_TO_STR
+
+TRACE_EVENT(pvr_job_submit_fw,
+	    TP_PROTO(struct pvr_job *job),
+	    TP_ARGS(job),
+	    TP_STRUCT__entry(__field(struct pvr_job *, job)
+			     __field(u32, done_seqno)),
+	    TP_fast_assign(__entry->job = job;
+			   __entry->done_seqno = job->done_fence->seqno;),
+	    TP_printk("job=%p done_seqno=%u",
+		      __entry->job,
+		      __entry->done_seqno)
+);
+
+TRACE_EVENT(pvr_job_done,
+	    TP_PROTO(struct pvr_job *job),
+	    TP_ARGS(job),
+	    TP_STRUCT__entry(__field(struct pvr_job *, job)),
+	    TP_fast_assign(__entry->job = job;),
+	    TP_printk("job=%p", __entry->job)
+);
+
+#endif /* PVR_TRACE_H */
+
+/* This part must be outside protection */
+#undef TRACE_INCLUDE_PATH
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_PATH ../../drivers/gpu/drm/imagination
+#define TRACE_INCLUDE_FILE pvr_trace
+#include <trace/define_trace.h>
diff --git a/drivers/gpu/drm/imagination/pvr_trace_points.c b/drivers/gpu/drm/imagination/pvr_trace_points.c
new file mode 100644
index 000000000000..71b53f267af0
--- /dev/null
+++ b/drivers/gpu/drm/imagination/pvr_trace_points.c
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/* Copyright (c) 2026 Imagination Technologies Ltd. */
+
+#ifndef __CHECKER__
+#define CREATE_TRACE_POINTS
+#include "pvr_trace.h"
+#endif

---
base-commit: 921578cf373981eba92774a27f7644a11282cf89
change-id: 20260512-b4-pvr-trace-points-4d3b66116e15

Best regards,
-- 
Alexandru Dadu <alexandru.dadu@imgtec.com>

Claude review: drm/imagination: Add support for trace points

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imagination: Add support for trace points
Author: Alexandru Dadu <alexandru.dadu@imgtec.com>
Patches: 1
Reviewed: 2026-05-16T12:08:16.677256

---

This is a single-patch series adding ftrace tracepoints to the PowerVR (Imagination) DRM driver for workload submission debugging. The boilerplate (Makefile, `pvr_trace_points.c`, trace header structure) follows established kernel conventions correctly, and the `__CHECKER__` guard matches other DRM drivers (i915, xe, vc4, etc.).

There are several issues worth addressing, ranging from a potential information leak to questionable trace placement and unnecessary includes.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: Add support for trace points

[Claude Code Review Bot]

Patch Review

**Storing raw kernel pointers in trace entries (`pvr_trace.h:35-41, 55-61, 89, 101`)**

The `pvr_job_submit_ioctl`, `pvr_job_create`, `pvr_job_submit_fw`, and `pvr_job_done` tracepoints all store raw kernel pointers (`struct pvr_device *`, `struct pvr_context *`, `struct pvr_fw_object *`, `struct pvr_job *`) and print them with `%p`. While `%p` in the kernel will hash the pointer (so the trace output is safe), storing the raw pointer in the trace ring buffer entry struct is still questionable — it takes 8 bytes but carries no useful information since the printed value is hashed. Consider using `%pK` if actual addresses are needed for privileged debugging, or better yet, store meaningful scalar identifiers instead (e.g., `job->id` for the job, `ctx->ctx_id` or similar for the context). That would make the trace output actually useful for correlating events rather than printing opaque hashed values.

**`pvr_job_done` placed after fence cleanup (`pvr_queue.c:1200`)**

```c
void pvr_queue_job_cleanup(struct pvr_job *job)
{
    pvr_queue_fence_put(job->done_fence);
    pvr_queue_fence_put(job->cccb_fence);
    pvr_kccb_fence_put(job->kccb_fence);

    if (job->base.s_fence)
        drm_sched_job_cleanup(&job->base);

    trace_pvr_job_done(job);
}
```

The `trace_pvr_job_done` is placed *after* fences and the scheduler job have been cleaned up. While the current trace only records `job` (a pointer), this is semantically backwards — it fires at "cleanup" time, not "done" time. The function name is `pvr_queue_job_cleanup`, and the caller in `pvr_job.c:35` is `pvr_job_release` (the kref release handler), so this tracepoint fires when the last reference is dropped, not when the GPU finishes the job. This is misleading; a "job done" trace would be expected when the `done_fence` signals, not at reference-count teardown. At minimum, consider renaming to `trace_pvr_job_release` or `trace_pvr_job_cleanup`, or moving it to the actual completion path.

Additionally, if this tracepoint is ever extended to log fence data (as the comment in the header warns about), the data will already be freed.

**`pvr_job_submit_ioctl` placed before validation (`pvr_drv.c:1154`)**

```c
trace_pvr_job_submit_ioctl(pvr_dev, args->jobs.count);

if (!drm_dev_enter(drm_dev, &idx))
    return -EIO;
```

The trace fires before `drm_dev_enter()`, meaning it traces submissions that may immediately fail. This could confuse analysis — a matching `pvr_job_submit_ioctl` event doesn't mean submission actually proceeded. Moving the trace after `drm_dev_enter()` would make it more meaningful.

**Unnecessary include of `pvr_sync.h` (`pvr_trace.h:15`)**

The trace header includes `pvr_sync.h` but none of the tracepoints reference any type from it. This adds unnecessary coupling and slows compilation. Remove it.

**`#undef PVR_JOB_TYPE_TO_STR` prevents use by future tracepoints (`pvr_trace.h:84`)**

```c
#undef PVR_JOB_TYPE_TO_STR
```

The `#undef` after `pvr_job_create` means any subsequent tracepoint added to this header cannot use the macro. Other kernel trace headers typically leave such helper macros defined within the guard. If the intent is namespace cleanliness, it's fine, but it's worth noting this will need to be reorganized when more job-type-aware traces are added.

**Makefile: `CONFIG_TRACEPOINTS` guard (`Makefile:123-124`)**

```makefile
powervr-$(CONFIG_TRACEPOINTS) += \
	pvr_trace_points.o
```

This is correct — when `CONFIG_TRACEPOINTS` is not set, the trace header macros expand to no-ops, and the `pvr_trace_points.c` compilation unit (which defines `CREATE_TRACE_POINTS`) is excluded. This follows standard practice.

**Minor: `pvr_trace_points.c` is clean and follows convention**

The `__CHECKER__` guard and `CREATE_TRACE_POINTS` pattern match other DRM drivers exactly. No issues here.

**Summary of requested changes:**
1. **Move `trace_pvr_job_done`** to before the fence cleanup, or rename it to reflect what it actually traces (job release/cleanup, not completion).
2. **Move `trace_pvr_job_submit_ioctl`** after `drm_dev_enter()` so it only traces submissions that will proceed.
3. **Remove `#include "pvr_sync.h"`** from `pvr_trace.h` — it's unused.
4. **Consider replacing raw pointer fields** with scalar identifiers (like `job->id`) to make trace output actionable rather than printing hashed pointers.

---
Generated by Claude Code Patch Reviewer