Le 25/05/2026 à 14:54, Liang, Prike a écrit :
AMD General

Regards,
       Prike

-----Original Message-----
From: Koenig, Christian <[email protected]>
Sent: Wednesday, May 20, 2026 9:44 PM
To: Pierre-Eric Pelloux-Prayer <[email protected]>; Liang, Prike
<[email protected]>; [email protected]
Cc: Deucher, Alexander <[email protected]>; Pelloux-Prayer, Pierre-
Eric <[email protected]>
Subject: Re: [PATCH 3/3] drm/amdgpu: add userq job and state transition trace
events

On 5/20/26 15:28, Pierre-Eric Pelloux-Prayer wrote:
Le 20/05/2026 à 14:33, Christian König a écrit :
On 5/20/26 14:25, Pierre-Eric Pelloux-Prayer wrote:


Le 20/05/2026 à 11:14, Christian König a écrit :
On 5/20/26 08:38, Prike Liang wrote:
From: Pierre-Eric Pelloux-Prayer
<[email protected]>

Add ftrace events for tracking the userq fence emit, signal and
queue state transition.

The queue trace points look good to me, but clear NAK to the fence trace
points those just duplicates the common trace points in the dma_fence framework.

The dma_fence trace points don't contain enough context to be usable from a
tool (no device, no client id at the very least).

The userqueue events are based on the gpu_scheduler traces and are what is
required for UMR to implement its Activity view.

In that case we should change umr to use the fence context instead of the client
id and/or put the client/doorbell in the fence descripton. That's what this is 
good for.

It *is* using the fence context. Having the client_id helps associating with
information available elsewhere (fdinfo for instance).


Creating new trace points to track userqueue usage and not using the standard
dma_fence onces is an absolutely clear NO-GO from my side, do we also do that 
for
the scheduler?


Yes, the gpu_scheduler trace events do the same thing.

Crap I completely missed that, I though that the scheduler trace points would 
expose
additional stuff and not superseet the dma_fence trace points.

Let's discuss tomorrow how to best handle that.

If we don't bind the userq client, doorbell offset, and queue context to the 
fence, then the dma-fence trace alone is sufficient for
tracking userq fence emit/signal/wait, in which case, should we drop the custom 
userq fence trace?

We should keep everything as is, except amdgpu_userq_job_done that can be removed because it's essentially a duplicate of dma_fence_signaled.

Pierre-Eric


Thanks,
Christian.


More below.

Regards,
Christian.


Pierre-Eric


Regards,
Christian.


Signed-off-by: Pierre-Eric Pelloux-Prayer
<[email protected]>
Signed-off-by: Prike Liang <[email protected]>
---
    drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h     | 129
++++++++++++++++++
    drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c     |  21 +++
    .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   |  13 +-
    3 files changed, 160 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 4ff8a4d7bb8b..32d8c36caaf3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -28,6 +28,8 @@
    #include <linux/types.h>
    #include <linux/tracepoint.h>
    +#include "amdgpu_userq_fence.h"
+
    #undef TRACE_SYSTEM
    #define TRACE_SYSTEM amdgpu
    #define TRACE_INCLUDE_FILE amdgpu_trace @@ -659,6 +661,133
@@
DEFINE_EVENT(amdgpu_userq_eviction_fence,
amdgpu_userq_eviction_fence_enable_sig
    DEFINE_EVENT(amdgpu_userq_eviction_fence,
amdgpu_userq_eviction_fence_signal,
            TP_PROTO(u64 context, u64 seqno),
            TP_ARGS(context, seqno));
+TRACE_EVENT(amdgpu_userq_job_run,
+        TP_PROTO(struct device *device, struct
+amdgpu_usermode_queue *queue, struct amdgpu_userq_fence *fence),
+        TP_ARGS(device, queue, fence),
+        TP_STRUCT__entry(
+                 __field(u64, fence_context)
+                 __field(u64, fence_seqno)

In the context of userq, these fields are similar to dma_fence_init.

+                 __string(dev, dev_name(device))
+                 __field(u64, doorbell_index)
+                 __field(u64, client_id)
+                 __field(u32, queue_type)

These 4 are missing in dma_fence_init and useful for UMR. eg: neither
dma_fence_init nor dma_fence_signalled trace the device. They only trace the
timeline which is not unique on a system.

+                 ),
+        TP_fast_assign(
+               __entry->fence_context = fence->base.context;
+               __entry->fence_seqno = fence->base.seqno;
+               __assign_str(dev);
+               __entry->doorbell_index = queue->doorbell_index;
+               __entry->client_id =
+queue->userq_mgr->file->client_id;
+               __entry->queue_type = queue->queue_type;
+               ),
+        TP_printk("dev=%s, client_id=%llu, type=%u,
+doorbell=%llu, fence=%llu:%llu",
+              __get_str(dev), __entry->client_id,
+__entry->queue_type, __entry->doorbell_index,
+              __entry->fence_context,
+              __entry->fence_seqno) );
+
+TRACE_EVENT(amdgpu_userq_job_done,
+        TP_PROTO(struct amdgpu_userq_fence *fence),
+        TP_ARGS(fence),
+        TP_STRUCT__entry(
+                 __field(u64, fence_context)
+                 __field(u64, fence_seqno)
+                 ),
+        TP_fast_assign(
+               __entry->fence_context = fence->base.context;
+               __entry->fence_seqno = fence->base.seqno;
+               ),
+        TP_printk("fence=%llu:%llu",
+              __entry->fence_context,
+              __entry->fence_seqno)

This one is indeed a duplicate of dma_fence_signaled.
It exists so we have similar events as gpu_scheduler but we can get rid of it 
if you
want.
(the only caveat is that dma_fence_signaled traces context and seqno as 32bit
integers).

The other events below have no dma_fence events equivalent so are they fine?

Pierre-Eric

+);
+
+TRACE_EVENT(amdgpu_userq_job_queue,
+        TP_PROTO(struct device *device,
+             struct amdgpu_usermode_queue *queue),
+        TP_ARGS(device, queue),
+        TP_STRUCT__entry(__field(u64, context)
+                 __string(dev, dev_name(device))
+                 __field(u64, doorbell_index)
+                 __field(u64, client_id)
+                 __field(u32, queue_type)
+                 ),
+        TP_fast_assign(__assign_str(dev);
+               __entry->doorbell_index = queue->doorbell_index;
+               __entry->queue_type = queue->queue_type;
+               __entry->client_id =
+queue->userq_mgr->file->client_id;
+               __entry->context = queue->fence_drv->context;
+              ),
+        TP_printk("dev=%s, client_id=%llu, type=%u,
+doorbell=%llu, context=%llu",
+              __get_str(dev), __entry->client_id,
+__entry->queue_type,
+              __entry->doorbell_index, __entry->context) );
+
+TRACE_EVENT(amdgpu_userq_job_add_dep,
+        TP_PROTO(struct device *device, struct
+amdgpu_usermode_queue *queue, struct amdgpu_userq_fence *dep),
+        TP_ARGS(device, queue, dep),
+        TP_STRUCT__entry(
+                 __field(u64, context)
+                 __field(u64, dep_context)
+                 __field(u64, dep_seqno)
+                 __string(dev, dev_name(device))
+                 __field(u64, doorbell_index)
+                 __field(u64, client_id)
+                 __field(u32, queue_type)
+                 ),
+        TP_fast_assign(
+               __assign_str(dev);
+               __entry->doorbell_index = queue->doorbell_index;
+               __entry->queue_type = queue->queue_type;
+               __entry->client_id =
+queue->userq_mgr->file->client_id;
+               __entry->context = queue->fence_drv->context;
+               __entry->dep_context = dep->base.context;
+               __entry->dep_seqno = dep->base.seqno;
+               ),
+        TP_printk("dev=%s, client_id=%llu, type=%u,
+doorbell=%llu, context=%llu depends on fence=%llu:%llu",
+              __get_str(dev), __entry->client_id,
+__entry->queue_type, __entry->doorbell_index, __entry->context,
+              __entry->dep_context,
+              __entry->dep_seqno) );
+
+TRACE_EVENT(amdgpu_userq_state_start,
+        TP_PROTO(struct amdgpu_usermode_queue *queue),
+        TP_ARGS(queue),
+        TP_STRUCT__entry(
+                 __field(u64, doorbell_index)
+                 __field(u64, client_id)
+                 __field(u32, queue_type)
+                 __field(u32, from)
+                 ),
+        TP_fast_assign(
+               __entry->doorbell_index = queue->doorbell_index;
+               __entry->queue_type = queue->queue_type;
+               __entry->client_id =
+queue->userq_mgr->file->client_id;
+               __entry->from = queue->state;
+               ),
+        TP_printk("client_id=%llu, type=%u, doorbell=%llu,
+from=%d",
+              __entry->client_id, __entry->queue_type,
+__entry->doorbell_index, __entry->from) );
+
+TRACE_EVENT(amdgpu_userq_state_changed,
+        TP_PROTO(struct amdgpu_usermode_queue *queue, enum
+amdgpu_userq_state new_state),
+        TP_ARGS(queue, new_state),
+        TP_STRUCT__entry(
+                 __field(u64, doorbell_index)
+                 __field(u64, client_id)
+                 __field(u32, queue_type)
+                 __field(u32, to)
+                 ),
+        TP_fast_assign(
+               __entry->doorbell_index = queue->doorbell_index;
+               __entry->queue_type = queue->queue_type;
+               __entry->client_id =
+queue->userq_mgr->file->client_id;
+               __entry->to = new_state;
+               ),
+        TP_printk("client_id=%llu, type=%u, doorbell=%llu,
+to=%d",
+              __entry->client_id, __entry->queue_type,
+__entry->doorbell_index, __entry->to) );
+
    #undef AMDGPU_JOB_GET_TIMELINE_NAME
    #endif
    diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index e27f9a76f986..60d1186af286 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -329,11 +329,15 @@ static int
amdgpu_userq_preempt_helper(struct amdgpu_usermode_queue *queue)
        int r;
          if (queue->state == AMDGPU_USERQ_STATE_MAPPED) {
+        trace_amdgpu_userq_state_start(queue);
+
            r = userq_funcs->preempt(queue);
            if (r) {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_HUNG);
                queue->state = AMDGPU_USERQ_STATE_HUNG;
                return r;
            } else {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_PREEMPTED);
                queue->state = AMDGPU_USERQ_STATE_PREEMPTED;
            }
        }
@@ -349,10 +353,14 @@ static int
amdgpu_userq_restore_helper(struct amdgpu_usermode_queue *queue)
        int r = 0;
          if (queue->state == AMDGPU_USERQ_STATE_PREEMPTED) {
+        trace_amdgpu_userq_state_start(queue);
+
            r = userq_funcs->restore(queue);
            if (r) {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_HUNG);
                queue->state = AMDGPU_USERQ_STATE_HUNG;
            } else {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_MAPPED);
                queue->state = AMDGPU_USERQ_STATE_MAPPED;
            }
        }
@@ -370,12 +378,15 @@ static int amdgpu_userq_unmap_helper(struct
amdgpu_usermode_queue *queue)
          if ((queue->state == AMDGPU_USERQ_STATE_MAPPED) ||
            (queue->state == AMDGPU_USERQ_STATE_PREEMPTED)) {
+        trace_amdgpu_userq_state_start(queue);
              r = userq_funcs->unmap(queue);
            if (r) {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_HUNG);
                queue->state = AMDGPU_USERQ_STATE_HUNG;
                return r;
            } else {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_UNMAPPED);
                queue->state = AMDGPU_USERQ_STATE_UNMAPPED;
            }
        }
@@ -392,11 +403,15 @@ static int amdgpu_userq_map_helper(struct
amdgpu_usermode_queue *queue)
        int r;
          if (queue->state == AMDGPU_USERQ_STATE_UNMAPPED) {
+        trace_amdgpu_userq_state_start(queue);
+
            r = userq_funcs->map(queue);
            if (r) {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_HUNG);
                queue->state = AMDGPU_USERQ_STATE_HUNG;
                return r;
            } else {
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_MAPPED);
                queue->state = AMDGPU_USERQ_STATE_MAPPED;
            }
        }
@@ -1007,6 +1022,7 @@ amdgpu_userq_restore_all(struct
amdgpu_userq_mgr *uq_mgr)
            if (!amdgpu_userq_buffer_vas_mapped(queue)) {
                drm_file_err(uq_mgr->file,
                         "trying restore queue without va
mapping\n");
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_INVALID_VA);
                queue->state = AMDGPU_USERQ_STATE_INVALID_VA;
                continue;
            }
@@ -1502,12 +1518,14 @@ void amdgpu_userq_pre_reset(struct
amdgpu_device *adev)
            if (queue->state != AMDGPU_USERQ_STATE_MAPPED)
                continue;
    +        trace_amdgpu_userq_state_start(queue);
            userq_funcs = adev->userq_funcs[queue->queue_type];
            userq_funcs->unmap(queue);
            /* just mark all queues as hung at this point.
             * if unmap succeeds, we could map again
             * in amdgpu_userq_post_reset() if vram is not lost
             */
+        trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_HUNG);
            queue->state = AMDGPU_USERQ_STATE_HUNG;
            amdgpu_userq_fence_driver_force_completion(queue);
        }
@@ -1526,6 +1544,8 @@ int amdgpu_userq_post_reset(struct
amdgpu_device *adev, bool vram_lost)
          xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
            if (queue->state == AMDGPU_USERQ_STATE_HUNG &&
!vram_lost) {
+            trace_amdgpu_userq_state_start(queue);
+
                userq_funcs = adev->userq_funcs[queue->queue_type];
                /* Re-map queue */
                r = userq_funcs->map(queue); @@ -1533,6 +1553,7 @@
int amdgpu_userq_post_reset(struct amdgpu_device *adev, bool
vram_lost)
                    dev_err(adev->dev, "Failed to remap queue
%ld\n", queue_id);
                    continue;
                }
+            trace_amdgpu_userq_state_changed(queue,
+AMDGPU_USERQ_STATE_MAPPED);
                queue->state = AMDGPU_USERQ_STATE_MAPPED;
            }
        }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index 008330a0d852..00cc7194321c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -30,7 +30,7 @@
    #include <drm/drm_syncobj.h>
      #include "amdgpu.h"
-#include "amdgpu_userq_fence.h"
+#include "amdgpu_trace.h"
      #define AMDGPU_USERQ_MAX_HANDLES    (1U << 16)
    @@ -169,6 +169,7 @@ amdgpu_userq_fence_driver_process(struct
amdgpu_userq_fence_driver *fence_drv)
            fence = &userq_fence->base;
            list_del_init(&userq_fence->link);
            dma_fence_signal(fence);
+        trace_amdgpu_userq_job_done(userq_fence);
            /* Drop fence_drv_array outside fence_list_lock
             * to avoid the recursion lock.
             */
@@ -528,6 +529,8 @@ int amdgpu_userq_signal_ioctl(struct
drm_device *dev, void *data,
        /* Create the new fence */
        amdgpu_userq_fence_init(queue, fence, wptr);
    +    trace_amdgpu_userq_job_run(dev->dev, queue, fence);
+
        mutex_unlock(&userq_mgr->userq_mutex);
          /*
@@ -701,7 +704,7 @@ amdgpu_userq_wait_add_fence(struct
drm_amdgpu_userq_wait *wait_info,
    }
      static int
-amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
+amdgpu_userq_wait_return_fence_info(struct drm_device *dev,
+struct drm_file *filp,
                        struct drm_amdgpu_userq_wait *wait_info,
                        u32 *syncobj_handles, u32 *timeline_points,
                        u32 *timeline_handles, @@ -835,6 +838,8 @@
amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
            goto free_fences;
        }
    +    trace_amdgpu_userq_job_queue(dev->dev, waitq);
+
        for (i = 0, cnt = 0; i < num_fences; i++) {
            struct amdgpu_userq_fence_driver *fence_drv;
            struct amdgpu_userq_fence *userq_fence; @@ -869,6
+874,8 @@ amdgpu_userq_wait_return_fence_info(struct drm_file
*filp,
              amdgpu_userq_fence_driver_get(fence_drv);
    +        trace_amdgpu_userq_job_add_dep(dev->dev, waitq,
userq_fence);
+
            /* Store drm syncobj's gpu va address and value */
            fence_info[cnt].va = fence_drv->va;
            fence_info[cnt].value = fences[i]->seqno; @@ -968,7
+975,7 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void
*data,
                               gobj_write,
                               gobj_read);
        } else {
-        r = amdgpu_userq_wait_return_fence_info(filp, wait_info,
+        r = amdgpu_userq_wait_return_fence_info(dev, filp,
+wait_info,
                                syncobj_handles,
                                timeline_points,
                                timeline_handles,


Reply via email to