Re: [PATCH] v4l2: move tracepoints to video_usercopy
Hans Verkuil wrote:
The (d)qbuf ioctls were traced in the low-level v4l2 ioctl function. The
trace was outside the serialization lock, so that can affect the usefulness
of the timing. In addition, the __user pointer was expected instead of a
proper kernel pointer.
By moving the tracepoints to video_usercopy we ensure that the trace calls
use the correct kernel pointer, and that it happens right after the ioctl
call to the driver, so certainly inside the serialization lock.
In addition, we only trace if the call was successful.
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
Looks good to me.
Acked-by: Wade Farnsworth wade_farnswo...@mentor.com
---
drivers/media/v4l2-core/v4l2-dev.c | 9 -
drivers/media/v4l2-core/v4l2-ioctl.c | 9 +
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-dev.c
b/drivers/media/v4l2-core/v4l2-dev.c
index 1cc1749..b5c 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -31,10 +31,6 @@
#include media/v4l2-device.h
#include media/v4l2-ioctl.h
-
-#define CREATE_TRACE_POINTS
-#include trace/events/v4l2.h
-
#define VIDEO_NUM_DEVICES256
#define VIDEO_NAME video4linux
@@ -395,11 +391,6 @@ static long v4l2_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
} else
ret = -ENOTTY;
- if (cmd == VIDIOC_DQBUF)
- trace_v4l2_dqbuf(vdev-minor, (struct v4l2_buffer *)arg);
- else if (cmd == VIDIOC_QBUF)
- trace_v4l2_qbuf(vdev-minor, (struct v4l2_buffer *)arg);
-
return ret;
}
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 68e6b5e..707aef7 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -28,6 +28,9 @@
#include media/v4l2-device.h
#include media/videobuf2-core.h
+#define CREATE_TRACE_POINTS
+#include trace/events/v4l2.h
+
/* Zero out the end of the struct pointed to by p. Everything after, but
* not including, the specified field is cleared. */
#define CLEAR_AFTER_FIELD(p, field) \
@@ -2338,6 +2341,12 @@ video_usercopy(struct file *file, unsigned int cmd,
unsigned long arg,
err = func(file, cmd, parg);
if (err == -ENOIOCTLCMD)
err = -ENOTTY;
+ if (err == 0) {
+ if (cmd == VIDIOC_DQBUF)
+ trace_v4l2_dqbuf(video_devdata(file)-minor, parg);
+ else if (cmd == VIDIOC_QBUF)
+ trace_v4l2_qbuf(video_devdata(file)-minor, parg);
+ }
if (has_array_args) {
*kernel_ptr = user_ptr;
--
Wade Farnsworth | Sr. Embedded Linux Dev. Engr.
Mentor Embedded™
Nucleus® | Linux® | Android™ | Services | UI | Multi-OS
Android is a trademark of Google Inc. Use of this trademark is subject
to Google Permissions.
Linux is the registered trademark of Linus Torvalds in the U.S. and
other countries
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
Hans Verkuil wrote:
On 12/11/2013 03:53 PM, Wade Farnsworth wrote:
Hi Hans,
Hans Verkuil wrote:
On 12/11/13 13:44, Mauro Carvalho Chehab wrote:
Em Wed, 11 Dec 2013 12:53:55 +0100
Hans Verkuil hverk...@xs4all.nl escreveu:
Hi Mauro,
On 12/11/13 11:44, Mauro Carvalho Chehab wrote:
Hans,
Em Wed, 11 Dec 2013 08:29:58 +0100
Hans Verkuil hverk...@xs4all.nl escreveu:
On 12/10/2013 09:53 PM, Mauro Carvalho Chehab wrote:
Em Sat, 23 Nov 2013 17:54:48 +0100
Hans Verkuil hverk...@xs4all.nl escreveu:
On 11/23/2013 05:30 PM, Sylwester Nawrocki wrote:
Hi,
On 11/23/2013 12:25 PM, Hans Verkuil wrote:
Hi Wade,
On 11/22/2013 08:48 PM, Wade Farnsworth wrote:
Add tracepoints to the QBUF and DQBUF ioctls to enable rudimentary
performance measurements using standard kernel tracers.
Signed-off-by: Wade Farnsworthwade_farnswo...@mentor.com
---
This is the update to the RFC patch I posted a few weeks back.
I've added
several bits of metadata to the tracepoint output per Mauro's
suggestion.
I don't like this. All v4l2 ioctls can already be traced by doing
e.g.
echo 1 (or echo 2)/sys/class/video4linux/video0/debug.
So this code basically duplicates that functionality. It would be
nice to be able
to tie in the existing tracing code (v4l2-ioctl.c) into tracepoints.
I think it would be really nice to have this kind of support for
standard
traces at the v4l2 subsystem. Presumably it could even gradually
replace
the v4l2 custom debug infrastructure.
If I understand things correctly, the current tracing/profiling
infrastructure
is much less invasive than inserting printks all over, which may
cause
changes
in control flow. I doubt the system could be reliably profiled by
enabling all
those debug prints.
So my vote would be to add support for standard tracers, like in
other
subsystems in the kernel.
The reason for the current system is to trace which ioctls are called
in
what order by a misbehaving application. It's very useful for that,
especially when trying to debug user problems.
I don't mind switching to tracepoints as long as this functionality is
kept one way or another.
I agree with Sylwester: we should move to tracepoints, and this is a
good
start.
As I mentioned, I don't mind switching to tracepoints, but not in the
way the
current patch does it. I certainly don't agree with you merging this
patch
as-is without further discussion.
Let's not mix the subjects here. There are two different demands that
can be
either fulfilled by the same solution or by different ones:
1) To have a way to enable debugging all parameters passed from/to
userspace;
2) To be able to measure the driver (and core) performance while
streaming.
This patch's scope is to solve (2), by allowing userspace to hook the two
ioctls that queues/dequeues streaming buffers.
With that regards, what's wrong on this patch? I couldn't see anything
bad
there. It is not meant to solve (1).
I have two problems with it:
1) Right now it is just checking for two ioctls, but as soon as we add
more
tracepoints you get another switch on the ioctl command, something I've
tried
to avoid by creating the table. So a table-oriented implementation would
be
much preferred.
From performance measurement PoV, I don't see any reason to add it to any
other ioctl.
But perhaps this makes sense as well for read() and write()? Possibly
poll()?
Of course if we revisit it and other traces become needed,
then we should use a table approach instead.
2) It duplicates the already existing code to dump the v4l2_buffer struct.
Is there a way to have just one copy of that? I was hoping Wade could look
into that, and if not, then it is something I can explore (although
finding
time is an issue).
Having implemented tracepoints myself, I'd say that the answer is no.
Let me give you an example.
The ras:aer_event trace is a simple tracepoint that also uses the
__print_flags() macro. It is implemented at include/trace/events/ras.h
as:
#define aer_correctable_errors \
{BIT(0),Receiver Error}, \
{BIT(6),Bad TLP}, \
{BIT(7),Bad DLLP},\
{BIT(8),RELAY_NUM Rollover}, \
{BIT(12), Replay Timer Timeout},\
{BIT(13), Advisory Non-Fatal}
#define aer_uncorrectable_errors \
{BIT(4),Data Link Protocol}, \
{BIT(12), Poisoned TLP},\
{BIT(13), Flow Control Protocol}, \
{BIT(14), Completion Timeout}, \
{BIT(15), Completer Abort}, \
{BIT(16), Unexpected Completion}, \
{BIT(17), Receiver Overflow}, \
{BIT(18), Malformed TLP}, \
{BIT(19), ECRC},\
{BIT(20), Unsupported Request}
TRACE_EVENT(aer_event,
TP_PROTO
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
Hi Hans,
Hans Verkuil wrote:
On 12/11/13 13:44, Mauro Carvalho Chehab wrote:
Em Wed, 11 Dec 2013 12:53:55 +0100
Hans Verkuil hverk...@xs4all.nl escreveu:
Hi Mauro,
On 12/11/13 11:44, Mauro Carvalho Chehab wrote:
Hans,
Em Wed, 11 Dec 2013 08:29:58 +0100
Hans Verkuil hverk...@xs4all.nl escreveu:
On 12/10/2013 09:53 PM, Mauro Carvalho Chehab wrote:
Em Sat, 23 Nov 2013 17:54:48 +0100
Hans Verkuil hverk...@xs4all.nl escreveu:
On 11/23/2013 05:30 PM, Sylwester Nawrocki wrote:
Hi,
On 11/23/2013 12:25 PM, Hans Verkuil wrote:
Hi Wade,
On 11/22/2013 08:48 PM, Wade Farnsworth wrote:
Add tracepoints to the QBUF and DQBUF ioctls to enable rudimentary
performance measurements using standard kernel tracers.
Signed-off-by: Wade Farnsworthwade_farnswo...@mentor.com
---
This is the update to the RFC patch I posted a few weeks back. I've
added
several bits of metadata to the tracepoint output per Mauro's
suggestion.
I don't like this. All v4l2 ioctls can already be traced by doing e.g.
echo 1 (or echo 2)/sys/class/video4linux/video0/debug.
So this code basically duplicates that functionality. It would be
nice to be able
to tie in the existing tracing code (v4l2-ioctl.c) into tracepoints.
I think it would be really nice to have this kind of support for
standard
traces at the v4l2 subsystem. Presumably it could even gradually
replace
the v4l2 custom debug infrastructure.
If I understand things correctly, the current tracing/profiling
infrastructure
is much less invasive than inserting printks all over, which may cause
changes
in control flow. I doubt the system could be reliably profiled by
enabling all
those debug prints.
So my vote would be to add support for standard tracers, like in other
subsystems in the kernel.
The reason for the current system is to trace which ioctls are called in
what order by a misbehaving application. It's very useful for that,
especially when trying to debug user problems.
I don't mind switching to tracepoints as long as this functionality is
kept one way or another.
I agree with Sylwester: we should move to tracepoints, and this is a good
start.
As I mentioned, I don't mind switching to tracepoints, but not in the way
the
current patch does it. I certainly don't agree with you merging this patch
as-is without further discussion.
Let's not mix the subjects here. There are two different demands that can
be
either fulfilled by the same solution or by different ones:
1) To have a way to enable debugging all parameters passed from/to
userspace;
2) To be able to measure the driver (and core) performance while
streaming.
This patch's scope is to solve (2), by allowing userspace to hook the two
ioctls that queues/dequeues streaming buffers.
With that regards, what's wrong on this patch? I couldn't see anything bad
there. It is not meant to solve (1).
I have two problems with it:
1) Right now it is just checking for two ioctls, but as soon as we add more
tracepoints you get another switch on the ioctl command, something I've
tried
to avoid by creating the table. So a table-oriented implementation would be
much preferred.
From performance measurement PoV, I don't see any reason to add it to any
other ioctl.
But perhaps this makes sense as well for read() and write()? Possibly poll()?
Of course if we revisit it and other traces become needed,
then we should use a table approach instead.
2) It duplicates the already existing code to dump the v4l2_buffer struct.
Is there a way to have just one copy of that? I was hoping Wade could look
into that, and if not, then it is something I can explore (although finding
time is an issue).
Having implemented tracepoints myself, I'd say that the answer is no.
Let me give you an example.
The ras:aer_event trace is a simple tracepoint that also uses the
__print_flags() macro. It is implemented at include/trace/events/ras.h
as:
#define aer_correctable_errors \
{BIT(0),Receiver Error}, \
{BIT(6),Bad TLP}, \
{BIT(7),Bad DLLP},\
{BIT(8),RELAY_NUM Rollover}, \
{BIT(12), Replay Timer Timeout},\
{BIT(13), Advisory Non-Fatal}
#define aer_uncorrectable_errors \
{BIT(4),Data Link Protocol}, \
{BIT(12), Poisoned TLP},\
{BIT(13), Flow Control Protocol}, \
{BIT(14), Completion Timeout}, \
{BIT(15), Completer Abort}, \
{BIT(16), Unexpected Completion}, \
{BIT(17), Receiver Overflow}, \
{BIT(18), Malformed TLP}, \
{BIT(19), ECRC},\
{BIT(20), Unsupported Request}
TRACE_EVENT(aer_event,
TP_PROTO(const char *dev_name,
const u32
[PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
Add tracepoints to the QBUF and DQBUF ioctls to enable rudimentary
performance measurements using standard kernel tracers.
Signed-off-by: Wade Farnsworth wade_farnswo...@mentor.com
---
This is the update to the RFC patch I posted a few weeks back. I've added
several bits of metadata to the tracepoint output per Mauro's suggestion.
drivers/media/v4l2-core/v4l2-dev.c |9 ++
include/trace/events/v4l2.h| 157
2 files changed, 166 insertions(+), 0 deletions(-)
create mode 100644 include/trace/events/v4l2.h
diff --git a/drivers/media/v4l2-core/v4l2-dev.c
b/drivers/media/v4l2-core/v4l2-dev.c
index b5c..1cc1749 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -31,6 +31,10 @@
#include media/v4l2-device.h
#include media/v4l2-ioctl.h
+
+#define CREATE_TRACE_POINTS
+#include trace/events/v4l2.h
+
#define VIDEO_NUM_DEVICES 256
#define VIDEO_NAME video4linux
@@ -391,6 +395,11 @@ static long v4l2_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
} else
ret = -ENOTTY;
+ if (cmd == VIDIOC_DQBUF)
+ trace_v4l2_dqbuf(vdev-minor, (struct v4l2_buffer *)arg);
+ else if (cmd == VIDIOC_QBUF)
+ trace_v4l2_qbuf(vdev-minor, (struct v4l2_buffer *)arg);
+
return ret;
}
diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
new file mode 100644
index 000..0b7d6cb
--- /dev/null
+++ b/include/trace/events/v4l2.h
@@ -0,0 +1,157 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM v4l2
+
+#if !defined(_TRACE_V4L2_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_V4L2_H
+
+#include linux/tracepoint.h
+
+#define show_type(type)
\
+ __print_symbolic(type, \
+ { V4L2_BUF_TYPE_VIDEO_CAPTURE,VIDEO_CAPTURE }, \
+ { V4L2_BUF_TYPE_VIDEO_OUTPUT, VIDEO_OUTPUT },\
+ { V4L2_BUF_TYPE_VIDEO_OVERLAY,VIDEO_OVERLAY }, \
+ { V4L2_BUF_TYPE_VBI_CAPTURE, VBI_CAPTURE }, \
+ { V4L2_BUF_TYPE_VBI_OUTPUT, VBI_OUTPUT }, \
+ { V4L2_BUF_TYPE_SLICED_VBI_CAPTURE, SLICED_VBI_CAPTURE }, \
+ { V4L2_BUF_TYPE_SLICED_VBI_OUTPUT,SLICED_VBI_OUTPUT }, \
+ { V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY, VIDEO_OUTPUT_OVERLAY },\
+ { V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, VIDEO_CAPTURE_MPLANE },\
+ { V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, VIDEO_OUTPUT_MPLANE }, \
+ { V4L2_BUF_TYPE_PRIVATE, PRIVATE })
+
+#define show_field(field) \
+ __print_symbolic(field, \
+ { V4L2_FIELD_ANY, ANY },\
+ { V4L2_FIELD_NONE, NONE }, \
+ { V4L2_FIELD_TOP, TOP },\
+ { V4L2_FIELD_BOTTOM,BOTTOM }, \
+ { V4L2_FIELD_INTERLACED,INTERLACED }, \
+ { V4L2_FIELD_SEQ_TB,SEQ_TB }, \
+ { V4L2_FIELD_SEQ_BT,SEQ_BT }, \
+ { V4L2_FIELD_ALTERNATE, ALTERNATE }, \
+ { V4L2_FIELD_INTERLACED_TB, INTERLACED_TB }, \
+ { V4L2_FIELD_INTERLACED_BT, INTERLACED_BT })
+
+#define show_timecode_type(type) \
+ __print_symbolic(type, \
+ { V4L2_TC_TYPE_24FPS, 24FPS }, \
+ { V4L2_TC_TYPE_25FPS, 25FPS }, \
+ { V4L2_TC_TYPE_30FPS, 30FPS }, \
+ { V4L2_TC_TYPE_50FPS, 50FPS }, \
+ { V4L2_TC_TYPE_60FPS, 60FPS })
+
+#define show_flags(flags)\
+ __print_flags(flags, |, \
+ { V4L2_BUF_FLAG_MAPPED, MAPPED }, \
+ { V4L2_BUF_FLAG_QUEUED, QUEUED }, \
+ { V4L2_BUF_FLAG_DONE,DONE },\
+ { V4L2_BUF_FLAG_KEYFRAME,KEYFRAME },\
+ { V4L2_BUF_FLAG_PFRAME, PFRAME }, \
+ { V4L2_BUF_FLAG_BFRAME, BFRAME }, \
+ { V4L2_BUF_FLAG_ERROR, ERROR }, \
+ { V4L2_BUF_FLAG_TIMECODE,TIMECODE },\
+ { V4L2_BUF_FLAG_PREPARED,PREPARED
[RFC][PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
Greetings, We've found this patch helpful for making some simple performance measurements on V4L2 systems using the standard Linux tracers (FTRACE, LTTng, etc.), and were wondering if the larger community would find it useful to have this in mainline as well. This patch adds two tracepoints for the VIDIOC_DQBUF and VIDIOC_QBUF ioctls. We've identified two ways we can use this information to measure performance, though this is likely not an exhaustive list: 1. Measuring the difference in timestamps between QBUF events on a display device provides a throughput (framerate) measurement for the display. 2. Measuring the difference between the timestamps on a DQBUF event for a capture device and a corresponding timestamp on a QBUF event on a display device provides a rough approximation of the latency of that particular frame. However, this tends to only work for simple video pipelines where captured and displayed frames can be correlated in such a fashion. Comments are welcome. If there is interest, I'll post another patch suitable for merge. Signed-off-by: Wade Farnsworth wade_farnswo...@mentor.com --- drivers/media/v4l2-core/v4l2-dev.c |9 ++ include/trace/events/v4l2.h| 48 2 files changed, 57 insertions(+), 0 deletions(-) create mode 100644 include/trace/events/v4l2.h diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index b5c..1cc1749 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -31,6 +31,10 @@ #include media/v4l2-device.h #include media/v4l2-ioctl.h + +#define CREATE_TRACE_POINTS +#include trace/events/v4l2.h + #define VIDEO_NUM_DEVICES 256 #define VIDEO_NAME video4linux @@ -391,6 +395,11 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) } else ret = -ENOTTY; + if (cmd == VIDIOC_DQBUF) + trace_v4l2_dqbuf(vdev-minor, (struct v4l2_buffer *)arg); + else if (cmd == VIDIOC_QBUF) + trace_v4l2_qbuf(vdev-minor, (struct v4l2_buffer *)arg); + return ret; } diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h new file mode 100644 index 000..1697441 --- /dev/null +++ b/include/trace/events/v4l2.h @@ -0,0 +1,48 @@ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM v4l2 + +#if !defined(_TRACE_V4L2_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_V4L2_H + +#include linux/tracepoint.h + +TRACE_EVENT(v4l2_dqbuf, + TP_PROTO(int minor, struct v4l2_buffer *buf), + + TP_ARGS(minor, buf), + + TP_STRUCT__entry( + __field(int, minor) + __field(s64, ts) + ), + + TP_fast_assign( + __entry-minor = minor; + __entry-ts = timeval_to_ns(buf-timestamp); + ), + + TP_printk(%d [%lld], __entry-minor, __entry-ts) +); + +TRACE_EVENT(v4l2_qbuf, + TP_PROTO(int minor, struct v4l2_buffer *buf), + + TP_ARGS(minor, buf), + + TP_STRUCT__entry( + __field(int, minor) + __field(s64, ts) + ), + + TP_fast_assign( + __entry-minor = minor; + __entry-ts = timeval_to_ns(buf-timestamp); + ), + + TP_printk(%d [%lld], __entry-minor, __entry-ts) +); + +#endif /* if !defined(_TRACE_V4L2_H) || defined(TRACE_HEADER_MULTI_READ) */ + +/* This part must be outside protection */ +#include trace/define_trace.h -- 1.7.0.4 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
