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

From: Niklas Söderlund
Date: Mon Jan 08 2018 - 11:11:59 EST


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@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> 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?

>
> 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.

>
> 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@xxxxxx
> >
>

--
Regards,
Niklas Söderlund