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

[Claude Code Review Bot <claude-review@example.com>]

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