diff mbox series

[1/5] media: v4l2: print the fh, during qbuf/dqbuf tracing

Message ID 20210517183801.1255496-2-emil.l.velikov@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/5] media: v4l2: print the fh, during qbuf/dqbuf tracing | expand

Commit Message

Emil Velikov May 17, 2021, 6:37 p.m. UTC 
From: Emil Velikov <emil.velikov@collabora.com>

To correlate the buffer handling with specific jobs, we need to provide
the file handle (pointer) used.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++++++++--
 include/trace/events/v4l2.h          | 22 ++++++++++++----------
 2 files changed, 20 insertions(+), 12 deletions(-)

Comments

Ezequiel Garcia June 3, 2021, 6:16 p.m. UTC | #1 
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@collabora.com>
> 
> 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@collabora.com>
> ---
>  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
diff mbox series

Patch

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;
+		__entry->fh = fh;
 		__entry->index = buf->index;
 		__entry->type = buf->type;
 		__entry->bytesused = buf->bytesused;
@@ -144,12 +146,12 @@  DECLARE_EVENT_CLASS(v4l2_event_class,
 		__entry->sequence = buf->sequence;
 	),
 
-	TP_printk("minor = %d, index = %u, type = %s, bytesused = %u, "
+	TP_printk("minor = %d, fh = %p, index = %u, type = %s, bytesused = %u, "
 		  "flags = %s, field = %s, timestamp = %llu, "
 		  "timecode = { type = %s, flags = %s, frames = %u, "
 		  "seconds = %u, minutes = %u, hours = %u, "
 		  "userbits = { %u %u %u %u } }, sequence = %u", __entry->minor,
-		  __entry->index, show_type(__entry->type),
+		  __entry->fh, __entry->index, show_type(__entry->type),
 		  __entry->bytesused,
 		  show_flags(__entry->flags),
 		  show_field(__entry->field),
@@ -169,13 +171,13 @@  DECLARE_EVENT_CLASS(v4l2_event_class,
 )
 
 DEFINE_EVENT(v4l2_event_class, v4l2_dqbuf,
-	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)
 );
 
 DEFINE_EVENT(v4l2_event_class, v4l2_qbuf,
-	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)
 );
 
 DECLARE_EVENT_CLASS(vb2_v4l2_event_class,