From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mailout2.samsung.com ([203.254.224.25]:54197 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752805AbbIWBhJ (ORCPT ); Tue, 22 Sep 2015 21:37:09 -0400 Received: from epcpsbgr4.samsung.com (u144.gpu120.samsung.co.kr [203.254.230.144]) by mailout2.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NV301B41WHVEH30@mailout2.samsung.com> for linux-media@vger.kernel.org; Wed, 23 Sep 2015 10:37:07 +0900 (KST) Message-id: <56020243.8090602@samsung.com> Date: Wed, 23 Sep 2015 10:37:07 +0900 From: Junghak Sung MIME-version: 1.0 To: Hans Verkuil , linux-media@vger.kernel.org, mchehab@osg.samsung.com, laurent.pinchart@ideasonboard.com, sakari.ailus@iki.fi, pawel@osciak.com Cc: inki.dae@samsung.com, sw0312.kim@samsung.com, nenggun.kim@samsung.com, sangbae90.lee@samsung.com, rany.kwon@samsung.com Subject: Re: [RFC PATCH v5 7/8] media: videobuf2: Prepare to divide videobuf2 References: <1442928636-3589-1-git-send-email-jh1009.sung@samsung.com> <1442928636-3589-8-git-send-email-jh1009.sung@samsung.com> <56016A4E.50804@xs4all.nl> In-reply-to: <56016A4E.50804@xs4all.nl> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: 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 >> Signed-off-by: Geunyoung Kim >> Acked-by: Seung-Woo Kim >> Acked-by: Inki Dae >> --- >> 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 >> >> -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 >> + >> +#define CREATE_TRACE_POINTS >> +#include >> + >> +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 >> #include >> >> -#include >> +#include >> >> 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. > >> { >> + struct v4l2_buffer *b = pb; >> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> struct vb2_queue *q = vb->vb2_queue; >> unsigned int plane; >> @@ -742,8 +743,28 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) >> break; >> } >> >> - if (__buffer_in_use(q, vb)) >> + if (vb2_buffer_in_use(q, vb)) >> b->flags |= V4L2_BUF_FLAG_MAPPED; >> + >> + return 0; >> +} >> + >> +/** >> + * vb2_core_querybuf() - query video buffer information >> + * @q: videobuf queue >> + * @index: id number of the buffer >> + * @pb: buffer struct passed from userspace >> + * >> + * Should be called from vidioc_querybuf ioctl handler in driver. >> + * The passed buffer should have been verified. >> + * This function fills the relevant information for the userspace. >> + * >> + * The return values from this function are intended to be directly returned >> + * from vidioc_querybuf handler in driver. >> + */ >> +static int vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb) >> +{ >> + return __fill_v4l2_buffer(q->bufs[index], pb); >> } >> >> /** >> @@ -775,11 +796,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b) >> } >> vb = q->bufs[b->index]; >> ret = __verify_planes_array(vb, b); >> - if (!ret) >> - __fill_v4l2_buffer(vb, b); >> - return ret; >> + >> + return ret ? ret : vb2_core_querybuf(q, b->index, b); >> } >> -EXPORT_SYMBOL(vb2_querybuf); >> +EXPORT_SYMBOL_GPL(vb2_querybuf); > > I prefer that such license changes are done in a separate patch. Now it is hidden > in a large patch, and others might want to jump in whether or not this is a > good thing. > OK. I will revert this change. >> >> /** >> * __verify_userptr_ops() - verify that all memory operations required for > > > >> @@ -2121,7 +2211,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking) >> * Will sleep if required for nonblocking == false. >> */ >> static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb, >> - struct v4l2_buffer *b, int nonblocking) >> + int nonblocking) >> { >> unsigned long flags; >> int ret; >> @@ -2142,10 +2232,10 @@ static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb, >> /* >> * Only remove the buffer from done_list if v4l2_buffer can handle all >> * the planes. >> + * Verifing planes is NOT necessary since it aleady has been checked > > s/Verifing/Verifying/ > s/aleady/already/ > >> + * before the buffer is queued/prepared. So it can never fails > > s/fails/fail./ > Oh, ridiculous mistakes. I will correct those typos. Regards, Junghak >> */ >> - ret = __verify_planes_array(*vb, b); >> - if (!ret) >> - list_del(&(*vb)->done_entry); >> + list_del(&(*vb)->done_entry); >> spin_unlock_irqrestore(&q->done_lock, flags); >> >> return ret; > > > > Regards, > > Hans >