Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
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(const char
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,
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
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). Before this patch, all we have is (1), as a non-dynamic printk is too intrusive to be used for performance measurement. So, there's no way to measure how much time a buffer takes to be filled. In a matter of fact, in the case you didn't noticed, we are already somewhat moving to traces, as several drivers are now using dynamic_printk for debug messages, Yet, lots of drivers still use either their own normal printk-based debug macros. Now, you're proposing to use the same solution for (1) too. Ok, we can go on that direction, but this is a separate issue. Converting the v4l2-ioctl to use tracepoints is the easiest part. However, there are several things to consider, if we decide to use it for debug purposes: a) It will likely require to convert all printks to dynamic ones, as we want to have all debug messages serialized. Let me explain it better: if some debug messages come via the standard printk mechanism while others come via traces, we loose the capability of receiving the messages at the same order that they were produced, and it could be harder if not impossible to synchronize them into the right order. b) It may make sense to write an userspace tool similar to the rasdaemon [1] to allow controlling the debug output. That tool directly reads the events from the raw tracing queues, formatting the data on userspace and hooking only the events that are needed. Currently, the rasdaemon has inside part of the code used by a perf tool copied on it (as this code is not inside any library yet). However, that code is being prepared to be used as a library. So, it should be even easier to write such tool in a near future. [1] https://git.fedorahosted.org/cgit/rasdaemon.git/ c) I don't think that tracepoints are the right mechanism for a post-mortem analysis[2]. E. g. if the Kernel crashes, a printk-based mechanism will output something, but I'm not sure if doing the same is possible with traces (e. g. forcing the trace messages to go to to tty). You should notice that, with the dynamic_printk macros, it is possible to compile the Kernel to use normal printk macros for debug macros. However, if we convert the ioctl printk's to tracepoints, we may still need to have a fallback printk mechanism for ioctl debug
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
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. 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). Bottom line as far as I am concerned: it was merged while I have outstanding questions. If there are good technical reasons why the two problems stated above can't be easily resolved, then I will reconsider, but for now I think it is too early to merge. For the record, I don't think I mentioned the first problem in my original reply, it's something I noticed yesterday while looking at the patch again. Before this patch, all we have is (1), as a non-dynamic printk is too intrusive to be used for performance measurement. So, there's no way to measure how much time a buffer takes to be filled. Having tracepoints here doesn't tell you anything about that. At best it gives you information about the jitter. How are these tracepoints supposed to be used? What information will they provide? Are tracepoints expected to be limited to QBUF/DQBUF? Or other ioctls/fops as well? If so, which ones? I just have too many questions and I'd like to see some answers before something like this is merged in the v4l2 core. Rereading my older replies I see that I wasn't particularly clear about what my objections to the patch were, for which I apologies. Nevertheless, I stand by my opinion that this patch needs more discussion. Wade, I appreciate it if you can give some more insights into how you are using this and what it gives you. In a matter of fact, in the case you didn't noticed, we are already somewhat moving to traces, as several drivers are now using dynamic_printk for debug messages, Yet, lots of drivers still use either their own normal printk-based debug macros. Now, you're proposing to use the same solution for (1) too. Ok, we can go on that direction, but this is a separate
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
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. 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 status,
const u8 severity),
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
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 status,
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
Em Wed, 11 Dec 2013 14:15:23 +0100
Hans Verkuil hverk...@xs4all.nl escreveu:
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()?
Could be, but, in this case, it won't be at the ioctl table.
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},
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
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
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. Regards, Mauro -- 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
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. To make it official: Nacked-by: Hans Verkuil hans.verk...@cisco.com If we do tracepoints, then we do it right and for all ioctls, not in this half-baked manner. Please revert. Regards, Hans -- 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
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 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.
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.
Regards,
Hans
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_DEVICES256
#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,
Re: [PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
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. -- Regards, Sylwester -- 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
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. Regards, Hans -- 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
[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 },\
+ {
Re: [RFC][PATCH] v4l2-dev: Add tracepoints for QBUF and DQBUF
Hi Wade, Em Fri, 18 Oct 2013 08:03:21 -0700 Wade Farnsworth wade_farnswo...@mentor.com escreveu: 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. I like the idea of adding those tracepoints. See my comments below. 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_DEVICES256 #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); On both events, you're only decoding the buffer timestamp. I think you should also decode the other metadata there. For example, on some devices, the dqbuf order is different than the qbuf one. One may need to associate both, in order to be able to time how much time a buffer takes to be filled. + ), + + 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 -- Cheers, Mauro -- 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
[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
