[PATCH v3 1/2] media: uapi: add MEDIA_BUS_FMT_METADATA_FIXED media bus format.

Thu Nov 5 09:44:51 EST 2020

On Thu, Nov 05, 2020 at 03:21:45PM +0100, Dafna Hirschfeld wrote:
> Hi
> 
> Am 05.11.20 um 13:45 schrieb Sakari Ailus:
> > On Thu, Nov 05, 2020 at 01:35:00PM +0100, Hans Verkuil wrote:
> > > On 04/11/2020 15:54, Sakari Ailus wrote:
> > > > Hi Hans,
> > > > 
> > > > On Wed, Nov 04, 2020 at 02:46:39PM +0100, Hans Verkuil wrote:
> > > > > On 04/11/2020 13:32, Sakari Ailus wrote:
> > > > > > Hi Hans,
> > > > > > 
> > > > > > On Wed, Nov 04, 2020 at 01:16:03PM +0100, Hans Verkuil wrote:
> > > > > > > On 30/10/2020 15:02, Sakari Ailus wrote:
> > > > > > > > Hi Dafna,
> > > > > > > > 
> > > > > > > > On Fri, Oct 30, 2020 at 02:46:08PM +0100, Dafna Hirschfeld wrote:
> > > > > > > > > MEDIA_BUS_FMT_METADATA_FIXED should be used when
> > > > > > > > > the same driver handles both sides of the link and
> > > > > > > > > the bus format is a fixed metadata format that is
> > > > > > > > > not configurable from userspace.
> > > > > > > > > The width and height will be set to 0 for this format.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
> > > > > > > > > Acked-by: Helen Koike <helen.koike at collabora.com>
> > > > > > > > > ---
> > > > > > > > > changes since v2:
> > > > > > > > > added documentation in subdev-formats.rst
> > > > > > > > > changes since v1:
> > > > > > > > > 1. replace "This format may have 0 height and width."
> > > > > > > > > with "Width and height will be set to 0 for this format."
> > > > > > > > > and add it also to the commit log
> > > > > > > > > 2. s/meida:/media:/ in the patch subject line
> > > > > > > > > 
> > > > > > > > >   .../media/v4l/subdev-formats.rst              | 27 +++++++++++++++++++
> > > > > > > > >   include/uapi/linux/media-bus-format.h         |  8 ++++++
> > > > > > > > >   2 files changed, 35 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > > > > > > > index c9b7bb3ca089..7f16cbe46e5c 100644
> > > > > > > > > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > > > > > > > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > > > > > > > @@ -7899,3 +7899,30 @@ formats.
> > > > > > > > >         - 0x5001
> > > > > > > > >         - Interleaved raw UYVY and JPEG image format with embedded meta-data
> > > > > > > > >   	used by Samsung S3C73MX camera sensors.
> > > > > > > > > +
> > > > > > > > > +.. _v4l2-mbus-metadata-fmts:
> > > > > > > > > +
> > > > > > > > > +Metadata Formats
> > > > > > > > > +^^^^^^^^^^^^^^^^
> > > > > > > > > +
> > > > > > > > > +This section lists all metadata formats.
> > > > > > > > > +
> > > > > > > > > +The following table lists the existing metadata formats.
> > > > > > > > > +
> > > > > > > > > +.. tabularcolumns:: |p{8.0cm}|p{1.4cm}|p{7.7cm}|
> > > > > > > > > +
> > > > > > > > > +.. flat-table:: Metadata formats
> > > > > > > > > +    :header-rows:  1
> > > > > > > > > +    :stub-columns: 0
> > > > > > > > > +
> > > > > > > > > +    * - Identifier
> > > > > > > > > +      - Code
> > > > > > > > > +      - Comments
> > > > > > > > > +    * .. _MEDIA-BUS-FMT-METADATA-FIXED:
> > > > > > > > > +
> > > > > > > > > +      - MEDIA_BUS_FMT_METADATA_FIXED
> > > > > > > > > +      - 0x7001
> > > > > > > > > +      - This format should be used when the same driver handles
> > > > > > > > > +	both sides of the link and the bus format is a fixed
> > > > > > > > > +	metadata format that is not configurable from userspace.
> > > > > > > > > +	Width and height will be set to 0 for this format.
> > > > > > > > > diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
> > > > > > > > > index 84fa53ffb13f..2ce3d891d344 100644
> > > > > > > > > --- a/include/uapi/linux/media-bus-format.h
> > > > > > > > > +++ b/include/uapi/linux/media-bus-format.h
> > > > > > > > > @@ -156,4 +156,12 @@
> > > > > > > > >   /* HSV - next is	0x6002 */
> > > > > > > > >   #define MEDIA_BUS_FMT_AHSV8888_1X32		0x6001
> > > > > > > > > +/*
> > > > > > > > > + * This format should be used when the same driver handles
> > > > > > > > > + * both sides of the link and the bus format is a fixed
> > > > > > > > > + * metadata format that is not configurable from userspace.
> > > > > > > > > + * Width and height will be set to 0 for this format.
> > > > > > > > > + */
> > > > > > > > 
> > > > > > > > Does this mean that metadata with dimensions should not use
> > > > > > > > MEDIA_BUS_FMT_METADATA_FIXED? I guess that's not the intention? For some
> > > > > > > > formats the dimensions would be relevant but for others not. I'd thus
> > > > > > > > replace "will" by "may". Same for the documentation.
> > > > > > > 
> > > > > > > struct v4l2_meta_format as used with VIDIOC_G/S/TRY_FMT doesn't have
> > > > > > > a width or height either. Supporting width and height for metadata
> > > > > > > doesn't really make sense for me for metadata.
> > > > > > > 
> > > > > > > Explicitly specifying the width and height here indicates that the
> > > > > > > data is basically an array of width x height of some sort which makes
> > > > > > > sense for video devices.
> > > > > > > 
> > > > > > > Metadata is more complex data that cannot be represented like that.
> > > > > > > If metadata is actually an array, then I'm not sure I would call it
> > > > > > > metadata, I would probably see it as video with its own pixelformat
> > > > > > > that contains non-video data.
> > > > > > 
> > > > > > Let's say the metadata is laid out in a similar way than an image; you have
> > > > > > lines of data, followed by some padding at the end, and a line has length
> > > > > > and a buffer has a number of lines. Sensor metadata falls into this
> > > > > > description.
> > > > > > 
> > > > > > Would you then use struct v4l2_pix_format for it, and use
> > > > > > V4L2_BUF_TYPE_VIDEO_CAPTURE for it?
> > > > > 
> > > > > Yes. It's still metadata, but it has the same 'format' as video data.
> > > > > We have similar situations such as with v4l-touch devices: the data
> > > > > is formatted like video, but it is actually pressure values from a
> > > > > touch pad. But it is 'video-like' in its behavior.
> > > > > 
> > > > > > 
> > > > > > That would make a few things easier but this is still metadata, not video
> > > > > > data. Albeit I guess it's not important to be so strict about that
> > > > > > interface-wise, indeed this is not a bad fit for such metadata. Still some
> > > > > > fields such as colourspace and quantisation are not relevant, but that
> > > > > > holds also for some pixel formats.
> > > > > > 
> > > > > 
> > > > > So are you OK with setting width and height to 0 for MEDIA_BUS_FMT_METADATA_*?
> > > > 
> > > > One more question.
> > > > 
> > > > What do you do if a link can carry both metadata (as in
> > > > V4L2_BUF_TYPE_METADATA_CAPTURE) as well as pixel data, but with a fixed
> > > > format?
> > > > 
> > > > I'm not sure we have any such case at the moment but it's not
> > > > inconceivable.
> > > > 
> > > 
> > > This should be reflected in the mediabus format. So METADATA_FIXED if it carries
> > > metadata, or a video format if it carries video. Userspace could configure this
> > > with VIDIOC_SUBDEV_S_FMT if it is something that userspace can actually change.
> > 
> > Works for me.
> > 
> > Acked-by: Sakari Ailus <sakari.ailus at linux.intel.com>
> > 
> 
> Hi,
> There is another patch sent by jmondi that adds csi-2 embedded data format:
> 
> https://patchwork.kernel.org/project/linux-media/patch/20201102165258.408049-3-jacopo@jmondi.org/
> 
> Maybe it worth thinking already how those two patches should fit together.
> Currently they both try to acquire 0x7001 code.

I'll reply to Jacopo. This will at least require discussion, as not all
metadata is created equal.

-- 
Sakari Ailus