On 23-09-15 03:37, Junghak Sung wrote:
>
>
> On 09/22/2015 11:48 PM, Hans Verkuil wrote:
>> Hi Junghak,
>>
>> This looks pretty good!
>>
>> I have a few small comments below, but overall it is much improved.
>>
>> On 22-09-15 15:30, Junghak Sung wrote:
>>> Prepare to divide videobuf2
>>> - Separate vb2 trace events from v4l2 trace event.
>>> - Make wrapper functions that will move to v4l2-side
>>> - Make vb2_core_* functions that remain in vb2 core side
>>> - Rename internal functions as vb2_*
>>>
>>> Signed-off-by: Junghak Sung <jh1009.sung@xxxxxxxxxxx>
>>> Signed-off-by: Geunyoung Kim <nenggun.kim@xxxxxxxxxxx>
>>> Acked-by: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
>>> Acked-by: Inki Dae <inki.dae@xxxxxxxxxxx>
>>> ---
>>> drivers/media/v4l2-core/Makefile | 2 +-
>>> drivers/media/v4l2-core/v4l2-trace.c | 8 +-
>>> drivers/media/v4l2-core/vb2-trace.c | 9 +
>>> drivers/media/v4l2-core/videobuf2-core.c | 556 ++++++++++++++++++++----------
>>> include/trace/events/v4l2.h | 28 +-
>>> include/trace/events/vb2.h | 65 ++++
>>> 6 files changed, 457 insertions(+), 211 deletions(-)
>>> create mode 100644 drivers/media/v4l2-core/vb2-trace.c
>>> create mode 100644 include/trace/events/vb2.h
>>>
>>> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>>> index ad07401..1dc8bba 100644
>>> --- a/drivers/media/v4l2-core/Makefile
>>> +++ b/drivers/media/v4l2-core/Makefile
>>> @@ -14,7 +14,7 @@ ifeq ($(CONFIG_OF),y)
>>> videodev-objs += v4l2-of.o
>>> endif
>>> ifeq ($(CONFIG_TRACEPOINTS),y)
>>> - videodev-objs += v4l2-trace.o
>>> + videodev-objs += vb2-trace.o v4l2-trace.o
>>> endif
>>>
>>> obj-$(CONFIG_VIDEO_V4L2) += videodev.o
>>> diff --git a/drivers/media/v4l2-core/v4l2-trace.c b/drivers/media/v4l2-core/v4l2-trace.c
>>> index 4004814..7416010 100644
>>> --- a/drivers/media/v4l2-core/v4l2-trace.c
>>> +++ b/drivers/media/v4l2-core/v4l2-trace.c
>>> @@ -5,7 +5,7 @@
>>> #define CREATE_TRACE_POINTS
>>> #include <trace/events/v4l2.h>
>>>
>>> -EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_done);
>>> -EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_queue);
>>> -EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_dqbuf);
>>> -EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_qbuf);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_buf_done);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_buf_queue);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_dqbuf);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_qbuf);
>>> diff --git a/drivers/media/v4l2-core/vb2-trace.c b/drivers/media/v4l2-core/vb2-trace.c
>>> new file mode 100644
>>> index 0000000..61e74f5
>>> --- /dev/null
>>> +++ b/drivers/media/v4l2-core/vb2-trace.c
>>> @@ -0,0 +1,9 @@
>>> +#include <media/videobuf2-core.h>
>>> +
>>> +#define CREATE_TRACE_POINTS
>>> +#include <trace/events/vb2.h>
>>> +
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_done);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_queue);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_dqbuf);
>>> +EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_qbuf);
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 32fa425..380536d 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -30,7 +30,7 @@
>>> #include <media/v4l2-common.h>
>>> #include <media/videobuf2-v4l2.h>
>>>
>>> -#include <trace/events/v4l2.h>
>>> +#include <trace/events/vb2.h>
>>>
>>> static int debug;
>>> module_param(debug, int, 0644);
>>> @@ -612,10 +612,10 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>> }
>>>
>>> /**
>>> - * __buffer_in_use() - return true if the buffer is in use and
>>> + * vb2_buffer_in_use() - return true if the buffer is in use and
>>> * the queue cannot be freed (by the means of REQBUFS(0)) call
>>> */
>>> -static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
>>> +static bool vb2_buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
>>> {
>>> unsigned int plane;
>>> for (plane = 0; plane < vb->num_planes; ++plane) {
>>> @@ -640,7 +640,7 @@ static bool __buffers_in_use(struct vb2_queue *q)
>>> {
>>> unsigned int buffer;
>>> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
>>> - if (__buffer_in_use(q, q->bufs[buffer]))
>>> + if (vb2_buffer_in_use(q, q->bufs[buffer]))
>>> return true;
>>> }
>>> return false;
>>> @@ -650,8 +650,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
>>> * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
>>> * returned to userspace
>>> */
>>> -static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>>> +static int __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>>
>> Why use a void * here? Wouldn't a struct vb2_buffer pointer be better? That way it all
>> remains type-safe. Ditto elsewhere in this patch, of course.
>
> I disagree with this idea, IMHO.
> This function is for filling struct v4l2_buffer with information to be
> returned to userspace. So, if the void pointer are replaced with
> struct vb2_buffer pointer, a additional function will be needed in order
> to translate the vb2_buffer to v4l2_buffer and the function should be
> duplicated with __fill_v4l2_buffer() by meaning of functionality.
Oops, my fault. Disregard my comment, I confused v4l2_buffer with vb2_v4l2_buffer.
What can I say? It was late in the day when I did the review :-)
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
