From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bear.ext.ti.com ([192.94.94.41]:37718 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755782AbaDKJDY (ORCPT ); Fri, 11 Apr 2014 05:03:24 -0400 Message-ID: <5347AF96.8030008@ti.com> Date: Fri, 11 Apr 2014 14:32:14 +0530 From: Archit Taneja MIME-Version: 1.0 To: Hans Verkuil , CC: , , , , Hans Verkuil Subject: Re: [REVIEWv3 PATCH 07/13] vb2: reject output buffers with V4L2_FIELD_ALTERNATE References: <1397203879-37443-1-git-send-email-hverkuil@xs4all.nl> <1397203879-37443-8-git-send-email-hverkuil@xs4all.nl> <5347AAF5.30704@ti.com> <5347AEC5.6050700@xs4all.nl> In-Reply-To: <5347AEC5.6050700@xs4all.nl> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On Friday 11 April 2014 02:28 PM, Hans Verkuil wrote: > On 04/11/2014 10:42 AM, Archit Taneja wrote: >> On Friday 11 April 2014 01:41 PM, Hans Verkuil wrote: >>> From: Hans Verkuil >>> >>> This is not allowed by the spec and does in fact not make any sense. >>> Return -EINVAL if this is the case. >>> >>> Signed-off-by: Hans Verkuil >>> Acked-by: Pawel Osciak >>> Acked-by: Sakari Ailus >>> --- >>> drivers/media/v4l2-core/videobuf2-core.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >>> index 6e05495..f8c0247 100644 >>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> @@ -1511,6 +1511,19 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) >>> dprintk(1, "plane parameters verification failed: %d\n", ret); >>> return ret; >>> } >>> + if (b->field == V4L2_FIELD_ALTERNATE && V4L2_TYPE_IS_OUTPUT(q->type)) { >>> + /* >>> + * If the format's field is ALTERNATE, then the buffer's field >>> + * should be either TOP or BOTTOM, not ALTERNATE since that >>> + * makes no sense. The driver has to know whether the >>> + * buffer represents a top or a bottom field in order to >>> + * program any DMA correctly. Using ALTERNATE is wrong, since >>> + * that just says that it is either a top or a bottom field, >>> + * but not which of the two it is. >>> + */ >>> + dprintk(1, "the field is incorrectly set to ALTERNATE for an output buffer\n"); >>> + return -EINVAL; >>> + } >> >> If vb2_queue had a field parameter, which tells the format's field type. >> We could have returned an error if the field format was ALTERNATE, and >> the buffer field was not TOP or BOTTOM. >> >> I don't know whether having a field parameter in vb2_queue is a good >> idea or not. > > The predecessor of vb2, videobuf, had that actually. > > I am not sure myself if it is a good idea or not to do the same for vb2. > For now I think we should leave it as is. There are very few drivers that > support FIELD_ALTERNATE although this should become more common for > drivers supporting interlaced HDTV formats. When we see more drivers that > support this, then we can see if it makes sense to move part of the handling > to vb2. > Sure, queue_setup vb op might be a good place to populate it. But as you said, there aren't many drivers that use FIELD_ALTERNATE, and there doesn't seem to be any other advantage for having 'field' in vb2_queue. So, it can be left for later. Archit