Re: [PATCH] v4l: doc: clarify v4l2_mbus_fmt height definition

[Hans Verkuil <hverkuil@xs4all.nl>]

On 01/08/2018 05:11 PM, Niklas Söderlund wrote:
> On 2018-01-08 16:48:35 +0100, 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 <kieran.bingham+renesas@ideasonboard.com>
>>>>> ---
>>>>>  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:
>>>
>>>    "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.
>>
>>>
>>> But there are no such clear definition of the height field for 
>>> v4l2_mbus_framefmt. This have cased some confusion for us which would be 
>>> nice to clarify. I think it would be a good thing to add to the 
>>> documentation if the height in v4l2_mbus_framefmt should describe the 
>>> height of a frame or field. And if it should represent the frame height 
>>> then v4l2_fill_pix_format() and v4l2_fill_mbus_format() should be 
>>> updated to support converting from the two different formats for height.
>>
>> And that makes no sense as it would make things even more difficult.
>>
>> So I believe it's best to be clear about it and talk about image height
>> instead of frame height.
> 
> And to be clear, image height for ALTERNATE should be half the frame 
> height?

Right.

> 
>>
>> Just double check first in the code if there are any subdevs that support
>> TOP/BOTTOM/ALTERNATE and how they fill in the height, I don't think we
>> have any at the moment, but I'm not 100% certain.
> 
> A quick look and I find no subdevs which supports TOP or BOTTOM but two 
> who supports ALTERNATE, adv748x and tvp5150. And of course they handle 
> this differently.
> 
> * adv748x - adv748x_afe_fill_format()
> 
>   fmt->field = V4L2_FIELD_ALTERNATE;
>   fmt->height = afe->curr_norm & V4L2_STD_525_60 ? 480 : 576;
>   fmt->height /= 2;
> 
> * tvp5150 - tvp5150_fill_fmt()
> 
>   f->height = decoder->rect.height;
>   f->field = V4L2_FIELD_ALTERNATE;
> 
>   Where rect is from tvp5150_probe() and tvp5150_s_std()
> 
>   #define TVP5150_V_MAX_525_60    480U
>   #define TVP5150_V_MAX_OTHERS    576U
> 
>   if (tvp5150_read_std(sd) & V4L2_STD_525_60)
>           core->rect.height = TVP5150_V_MAX_525_60;
>   else
>           core->rect.height = TVP5150_V_MAX_OTHERS
> 
>   But it can be changed to any value < TVP5150_V_MAX_OTHERS in 
>   tvp5150_set_selection(). And interestingly enough it is reported as 
>   half size in .enum_frame_size() - tvp5150_enum_frame_size():
> 
>   fse->min_height = decoder->rect.height / 2;
>   fse->max_height = decoder->rect.height / 2;
> 
> Not sure how this two different solutions best should be handled and 
> which one is correct.

The first (adv748x) is correct.

The tvp5150.c should divide the height by 2in fill_fmt.

Luckily, nobody is using either get_fmt or set_fmt ops from this driver,
so the changes shouldn't affect anything.

Note that the rectangle used by the crop selection API uses *frame* coordinates,
not image coordinates. This driver handles that correctly already.

tvp5150_enum_frame_size should be removed: it is not used and in general
it makes no sense for video receivers. I think it is broken as well since
this initial check makes no sense:

       if (fse->index >= 8 || fse->code != MEDIA_BUS_FMT_UYVY8_2X8)
                return -EINVAL;

No idea where 'fse->index >= 8' comes from. It appears it just enumerates
8 identical formats!

Talking about removing functions: tvp5150_g_mbus_config should also be
dropped. It's unused by em28xx and I want to get rid of that callback since
this belongs in the device tree or usb/pci card information.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>>
>>>>>   * @code:	data format code (from enum v4l2_mbus_pixelcode)
>>>>>   * @field:	used interlacing type (from enum v4l2_field)
>>>>>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
>>>>
>>>> -- 
>>>> Regards,
>>>>
>>>> Sakari Ailus
>>>> e-mail: sakari.ailus@iki.fi
>>>
>>
>