Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755180AbeAHSHG (ORCPT + 1 other); Mon, 8 Jan 2018 13:07:06 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:45785 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754560AbeAHSHE (ORCPT ); Mon, 8 Jan 2018 13:07:04 -0500 Subject: Re: [PATCH] v4l: doc: clarify v4l2_mbus_fmt height definition To: Hans Verkuil , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , Sakari Ailus Cc: linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Kieran Bingham , Mauro Carvalho Chehab , Archit Taneja , Sean Paul , Neil Armstrong , open list References: <1515422746-5971-1-git-send-email-kieran.bingham@ideasonboard.com> <20180108151353.zn2ee2tbdq2yragp@valkosipuli.retiisi.org.uk> <20180108153247.GA23075@bigcity.dyn.berto.se> <7c5ad9a4-c218-45f4-e21e-7aa9935cfd41@xs4all.nl> From: Kieran Bingham Organization: Ideas on Board Message-ID: <9002632f-4ce3-e063-ffbc-a164a348c0f8@ideasonboard.com> Date: Mon, 8 Jan 2018 18:06:58 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <7c5ad9a4-c218-45f4-e21e-7aa9935cfd41@xs4all.nl> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi Hans, Niklas, Sakari, Thank you for the very prompt reviews! I fired the patch - disappeared to teach code club, and came back to the answers :-D - very streamlined! On 08/01/18 15:48, Hans Verkuil wrote: > On 01/08/2018 04:32 PM, Niklas Söderlund wrote: >> Hi, >> >> Thanks for your patch. >> >> On 2018-01-08 17:13:53 +0200, Sakari Ailus wrote: >>> Hi Kieran, >>> >>> On Mon, Jan 08, 2018 at 02:45:49PM +0000, Kieran Bingham wrote: >>>> The v4l2_mbus_fmt width and height corresponds directly with the >>>> v4l2_pix_format definitions, yet the differences in documentation make >>>> it ambiguous what to do in the event of field heights. >>>> >>>> Clarify this by referencing the v4l2_pix_format which is explicit on the >>>> matter, and by matching the terminology of 'image height' rather than >>>> the misleading 'frame height'. >> >> Nice that this relationship is documented as it have contributed to some >> confusion on my side in the past! >> >>>> >>>> Signed-off-by: Kieran Bingham >>>> --- >>>> Documentation/media/uapi/v4l/subdev-formats.rst | 6 ++++-- >>>> include/uapi/linux/v4l2-mediabus.h | 4 ++-- >>>> 2 files changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst >>>> index b1eea44550e1..a2a00202b430 100644 >>>> --- a/Documentation/media/uapi/v4l/subdev-formats.rst >>>> +++ b/Documentation/media/uapi/v4l/subdev-formats.rst >>>> @@ -16,10 +16,12 @@ Media Bus Formats >>>> >>>> * - __u32 >>>> - ``width`` >>>> - - Image width, in pixels. >>>> + - Image width in pixels. See struct >>>> + :c:type:`v4l2_pix_format`. >>>> * - __u32 >>>> - ``height`` >>>> - - Image height, in pixels. >>>> + - Image height in pixels. See struct >>>> + :c:type:`v4l2_pix_format`. >>>> * - __u32 >>>> - ``code`` >>>> - Format code, from enum >>>> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h >>>> index 6e20de63ec59..6b34108d0338 100644 >>>> --- a/include/uapi/linux/v4l2-mediabus.h >>>> +++ b/include/uapi/linux/v4l2-mediabus.h >>>> @@ -18,8 +18,8 @@ >>>> >>>> /** >>>> * struct v4l2_mbus_framefmt - frame format on the media bus >>>> - * @width: frame width >>>> - * @height: frame height >>>> + * @width: image width >>>> + * @height: image height (see struct v4l2_pix_format) >>> >>> Hmm. This is the media bus format and it has no direct relation to >>> v4l2_pix_format. So no, I can't see what would be the point in making such >>> a reference. >> >> Well we have functions like v4l2_fill_pix_format() that do >> >> pix_fmt->width = mbus_fmt->width; >> pix_fmt->height = mbus_fmt->height; >> >> So I think there at least is an implicit relation between the two >> structs. The issue I think Kieran is trying to address is in the case of >> TOP, BOTTOM and ALTERNATE field formats. From the v4l2_pix_format >> documentation on the height field: Yes, it was this relationship which made me feel it was appropriate to just reference in the same way that the subdevice version did. >> "Image height in pixels. If field is one of V4L2_FIELD_TOP, >> V4L2_FIELD_BOTTOM or V4L2_FIELD_ALTERNATE then height refers to the >> number of lines in the field, otherwise it refers to the number of >> lines in the frame (which is twice the field height for interlaced >> formats)." > > Right, and I'd just copy this text to subdev-formats.rst rather than referring > to it. Ok - Copied for V2. Thanks Kieran