Re: [PATCH v3 05/10] media: uapi: Document which mbus format fields are valid for metadata

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Aug 14, 2023 at 10:23:59AM +0000, Sakari Ailus wrote:
> On Thu, Aug 10, 2023 at 05:19:02PM +0200, Jacopo Mondi wrote:
> > On Tue, Aug 08, 2023 at 10:55:33AM +0300, Sakari Ailus wrote:
> > > Now that metadata mbus formats have been added, it is necessary to define
> > > which fields in struct v4l2_mbus_format are applicable to them (not many).
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  include/uapi/linux/v4l2-mediabus.h | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> > > index 6b07b73473b5..3cadb3b58b85 100644
> > > --- a/include/uapi/linux/v4l2-mediabus.h
> > > +++ b/include/uapi/linux/v4l2-mediabus.h
> > > @@ -19,12 +19,18 @@
> > >   * @width:	image width
> > >   * @height:	image height
> > >   * @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)
> > > - * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
> > > - * @hsv_enc:	HSV encoding of the data (from enum v4l2_hsv_encoding)
> > > - * @quantization: quantization of the data (from enum v4l2_quantization)
> > > - * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
> > > + * @field:	used interlacing type (from enum v4l2_field), not applicable
> > > + *		to metadata mbus codes
> > 
> > "not applicable" is a bit geeric. Should this be set to
> > V4L2_FIELD_NONE (for metadata, and progressive image formats maybe ?)
> 
> I actually intended to have the same wording here than for the other fields
> but missed changing this.
> 
> 0 corresponds to V4L2_FIELD_ANY.
> 
> > > + * @colorspace:	colorspace of the data (from enum v4l2_colorspace), zero on
> > > + *		metadata mbus codes
> > > + * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding), zero
> > > + *		on metadata mbus codes
> > > + * @hsv_enc:	HSV encoding of the data (from enum v4l2_hsv_encoding), zero on
> > > + *		metadata mbus codes
> > 
> > Can this be zero ?
> > 
> > enum v4l2_hsv_encoding {
> > 
> > 	/* Hue mapped to 0 - 179 */
> > 	V4L2_HSV_ENC_180		= 128,
> > 
> > 	/* Hue mapped to 0-255 */
> > 	V4L2_HSV_ENC_256		= 129,
> > };
> 
> Good question. Neither value is meaningful for metadata.
> 
> The values appear to be such in the enum to avoid colliding with ycbcr
> encoding values (another field above).
> 
> Generally, what doesn't matter will be zero. These fields have been added
> at some point and a lot of drivers do not set them, even for pixel data.
> 
> I wonder what Hans and Laurent think.

ycbcr_enc and hsv_enc are stored in an unnamed union.

> > > + * @quantization: quantization of the data (from enum v4l2_quantization), zero
> > > + *		on metadata mbus codes
> > > + * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func), zero
> > > + *		on metadata mbus codes
> > >   * @flags:	flags (V4L2_MBUS_FRAMEFMT_*)
> > >   * @reserved:  reserved bytes that can be later used
> > >   */

-- 
Regards,

Laurent Pinchart