| Message ID | 20240213155112.156537-7-pierre-eric.pelloux-prayer@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-fence, drm, amdgpu new trace events | expand |
On Tue, 13 Feb 2024 16:50:31 +0100 Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> wrote: > @@ -1503,6 +1504,24 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > drm_mode_object_put(obj); > } > > + if (trace_drm_mode_atomic_commit_enabled()) { > + struct drm_crtc_state *crtc_state; > + struct drm_crtc *crtc; > + int *crtcs; > + int i, num_crtcs; > + > + crtcs = kcalloc(dev->mode_config.num_crtc, sizeof(int), > + GFP_KERNEL); If the above allocation fails, this will cause a NULL kernel dereference. -- Steve > + > + num_crtcs = 0; > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) > + crtcs[num_crtcs++] = drm_crtc_index(crtc); > + > + trace_drm_mode_atomic_commit(file_priv, crtcs, num_crtcs, arg->flags); > + > + kfree(crtcs); > + } > + > ret = prepare_signaling(dev, state, arg, file_priv, &fence_state, > &num_fences); > if (ret)
On Tue, Feb 13, 2024 at 11:20:17AM -0500, Steven Rostedt wrote: > On Tue, 13 Feb 2024 16:50:31 +0100 > Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> wrote: > > > @@ -1503,6 +1504,24 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > drm_mode_object_put(obj); > > } > > > > + if (trace_drm_mode_atomic_commit_enabled()) { > > + struct drm_crtc_state *crtc_state; > > + struct drm_crtc *crtc; > > + int *crtcs; > > + int i, num_crtcs; > > + > > + crtcs = kcalloc(dev->mode_config.num_crtc, sizeof(int), > > + GFP_KERNEL); > > If the above allocation fails, this will cause a NULL kernel dereference. Yeah can't we somehow iterate directly into the trace subsystem? If nothing else works I guess just a per-crtc event should do. The more fundamental issue: I don't get how this works. For atomic we have: - explicitly handed in in-fences as dependencies with the IN_FENCE property - dependencies that drivers fish out of the dma_resv object of the underlying gem buffer objects for each framebuffer. That has become pretty much entirely generic code since everyone uses the same, and so imo the dependency tracking should be fully generic too - atomic has an out-fence too, so we could even do the full fence->fence dependency tracking. It's just not created as a userspace object if all userspace asks for is a drm vblank event, but it is very much there. And I think if you want fence tracking, we really should have fence tracking :-) Also the out-fence should be 100% generic (or it's a driver bug) because the driver functions hide the differences between generating a vblank event and signalling a dma_fence. Cheers, Sima > > -- Steve > > > + > > + num_crtcs = 0; > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) > > + crtcs[num_crtcs++] = drm_crtc_index(crtc); > > + > > + trace_drm_mode_atomic_commit(file_priv, crtcs, num_crtcs, arg->flags); > > + > > + kfree(crtcs); > > + } > > + > > ret = prepare_signaling(dev, state, arg, file_priv, &fence_state, > > &num_fences); > > if (ret)
On Fri, 16 Feb 2024 17:37:23 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > > > @@ -1503,6 +1504,24 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, > > > drm_mode_object_put(obj); > > > } > > > > > > + if (trace_drm_mode_atomic_commit_enabled()) { > > > + struct drm_crtc_state *crtc_state; > > > + struct drm_crtc *crtc; > > > + int *crtcs; > > > + int i, num_crtcs; > > > + > > > + crtcs = kcalloc(dev->mode_config.num_crtc, sizeof(int), > > > + GFP_KERNEL); > > > > If the above allocation fails, this will cause a NULL kernel dereference. > > Yeah can't we somehow iterate directly into the trace subsystem? If > nothing else works I guess just a per-crtc event should do. You mean like this? https://lore.kernel.org/all/20240216105934.7b81eae9@gandalf.local.home/ ;-) -- Steve
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 29d4940188d4..0d3767cd155a 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -41,6 +41,7 @@ #include <linux/file.h> #include "drm_crtc_internal.h" +#include "drm_trace.h" /** * DOC: overview @@ -1503,6 +1504,24 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, drm_mode_object_put(obj); } + if (trace_drm_mode_atomic_commit_enabled()) { + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + int *crtcs; + int i, num_crtcs; + + crtcs = kcalloc(dev->mode_config.num_crtc, sizeof(int), + GFP_KERNEL); + + num_crtcs = 0; + for_each_new_crtc_in_state(state, crtc, crtc_state, i) + crtcs[num_crtcs++] = drm_crtc_index(crtc); + + trace_drm_mode_atomic_commit(file_priv, crtcs, num_crtcs, arg->flags); + + kfree(crtcs); + } + ret = prepare_signaling(dev, state, arg, file_priv, &fence_state, &num_fences); if (ret) diff --git a/drivers/gpu/drm/drm_trace.h b/drivers/gpu/drm/drm_trace.h index 11c6dd577e8e..b62a44cb1270 100644 --- a/drivers/gpu/drm/drm_trace.h +++ b/drivers/gpu/drm/drm_trace.h @@ -62,8 +62,32 @@ TRACE_EVENT(drm_vblank_event_delivered, __entry->crtc = crtc; __entry->seq = seq; ), - TP_printk("file=%p, crtc=%d, seq=%u", __entry->file, __entry->crtc, \ - __entry->seq) + TP_printk("file=%p, crtc=%d, seq=%u, pid=%8d", \ + __entry->file, __entry->crtc, __entry->seq, \ + pid_nr(__entry->file->pid)) +); + +TRACE_EVENT(drm_mode_atomic_commit, + TP_PROTO(struct drm_file *file, int *crtcs, int ncrtcs, uint32_t flags), + TP_ARGS(file, crtcs, ncrtcs, flags), + TP_STRUCT__entry( + __field(struct drm_file *, file) + __dynamic_array(u32, crtcs, ncrtcs) + __field(uint32_t, ncrtcs) + __field(uint32_t, flags) + ), + TP_fast_assign( + unsigned int i; + + __entry->file = file; + for (i = 0; i < ncrtcs; i++) + ((u32 *)__get_dynamic_array(crtcs))[i] = crtcs[i]; + __entry->ncrtcs = ncrtcs; + __entry->flags = flags; + ), + TP_printk("file=%p, pid=%8d, flags=%08x, crtcs=%s", __entry->file, \ + pid_nr(__entry->file->pid), __entry->flags, \ + __print_array(__get_dynamic_array(crtcs), __entry->ncrtcs, 4)) ); #endif /* _DRM_TRACE_H_ */
With this and the dma_fence_sync_to event, a tool can draw the relationship between the compositing draw, the atomic commit, and vblank. An example on a 2 monitors system look like this: gnome-shell-1638 [018] ..... 2571.905124: drm_mode_atomic_commit: file=00000000245c3f0c, pid= 1165, flags=00000201, crtcs={0x1} gnome-shell-1638 [018] ..... 2571.905147: dma_fence_sync_to: driver=drm_sched timeline=gfx_0.0.0 context=270 seqno=73240 reason=dma_fence_chain_init gnome-shell-1638 [018] ..... 2571.913226: drm_mode_atomic_commit: file=00000000245c3f0c, pid= 1165, flags=00000201, crtcs={0x0} gnome-shell-1638 [018] ..... 2571.913250: dma_fence_sync_to: driver=drm_sched timeline=gfx_0.0.0 context=270 seqno=73241 reason=dma_fence_chain_init <idle>-0 [018] d.h3. 2571.915687: drm_vblank_event: crtc=1, seq=155747, time=2571916093743, high-prec=true <idle>-0 [018] d.h3. 2571.915968: drm_vblank_event: crtc=0, seq=153862, time=2571916377180, high-prec=true Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com> --- drivers/gpu/drm/drm_atomic_uapi.c | 19 +++++++++++++++++++ drivers/gpu/drm/drm_trace.h | 28 ++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 2 deletions(-)