Hi Emil, Thanks a lot for the series, I think it's great to start discussing some generic tracing for the media subsytem. First of all, looks like you should fix your submission process, the cover letter didn't hit patchwork. See: https://patchwork.linuxtv.org/project/linux-media/list/?series=5446 Unsure what's going on, but please take a look. Some feedback about this patch. On Mon, 2021-05-17 at 19:37 +0100, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > > To correlate the buffer handling with specific jobs, we need to provide > the file handle (pointer) used. > In general, it will be useful to have more information here. For instance, you are changing traces, so e.g. a before/after could be better. Not just for this patch, but in general, I think we'd like to have more documentation. > Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++++++++-- > include/trace/events/v4l2.h | 22 ++++++++++++---------- > 2 files changed, 20 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 31d1342e61e8..4b56493a1bae 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -3343,10 +3343,16 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, > } > > if (err == 0) { > + struct video_device *vdev = video_devdata(file); > + struct v4l2_fh *fh = NULL; > + > + if (test_bit(V4L2_FL_USES_V4L2_FH, &vdev->flags)) > + fh = file->private_data; > + > if (cmd == VIDIOC_DQBUF) > - trace_v4l2_dqbuf(video_devdata(file)->minor, parg); > + trace_v4l2_dqbuf(fh, parg); > else if (cmd == VIDIOC_QBUF) > - trace_v4l2_qbuf(video_devdata(file)->minor, parg); > + trace_v4l2_qbuf(fh, parg); > } > > if (has_array_args) { > diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h > index 248bc09bfc99..e07311cfe5ca 100644 > --- a/include/trace/events/v4l2.h > +++ b/include/trace/events/v4l2.h > @@ -7,6 +7,7 @@ > > #include <linux/tracepoint.h> > #include <media/videobuf2-v4l2.h> > +#include <media/v4l2-device.h> > > /* Enums require being exported to userspace, for user tool parsing */ > #undef EM > @@ -98,12 +99,12 @@ SHOW_FIELD > { V4L2_TC_USERBITS_8BITCHARS, "USERBITS_8BITCHARS" }) > > DECLARE_EVENT_CLASS(v4l2_event_class, > - TP_PROTO(int minor, struct v4l2_buffer *buf), > - > - TP_ARGS(minor, buf), > + TP_PROTO(struct v4l2_fh *fh, struct v4l2_buffer *buf), > + TP_ARGS(fh, buf), > > TP_STRUCT__entry( > __field(int, minor) > + __field(struct v4l2_fh *, fh) > __field(u32, index) > __field(u32, type) > __field(u32, bytesused) > @@ -124,7 +125,8 @@ DECLARE_EVENT_CLASS(v4l2_event_class, > ), > > TP_fast_assign( > - __entry->minor = minor; > + __entry->minor = fh ? fh->vdev->minor : -1; I think this is a regression now, if the driver isn't using V4L2_FL_USES_V4L2_FH, then minor field will be -1? Maybe we could leave this ioctl trace, and drop this patch, and instead do the tracing at the mem2mem level in v4l2_m2m_qbuf. Thanks, Ezequiel
