[RFC,18/20] lib: image-formats: Add v4l2 formats support

Message ID	c97024b97d3261dcf41aad3c8bc1c5d9906f33c9.1553032382.git-series.maxime.ripard@bootlin.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-media-owner@kernel.org> sender: maxime.ripard@bootlin.com) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 55FF2E0007; Tue, 19 Mar 2019 21:58:15 +0000 (UTC) From: Maxime Ripard <maxime.ripard@bootlin.com> To: Daniel Vetter <daniel.vetter@intel.com>, David Airlie <airlied@linux.ie>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Sean Paul <seanpaul@chromium.org>, Maxime Ripard <maxime.ripard@bootlin.com>, Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>, Hans Verkuil <hans.verkuil@cisco.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Paul Kocialkowski <paul.kocialkowski@bootlin.com>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org Subject: [RFC PATCH 18/20] lib: image-formats: Add v4l2 formats support Date: Tue, 19 Mar 2019 22:57:23 +0100 Message-Id: <c97024b97d3261dcf41aad3c8bc1c5d9906f33c9.1553032382.git-series.maxime.ripard@bootlin.com> In-Reply-To: <cover.92acdec88ee4c280cb74e08ea22f0075e5fa055c.1553032382.git-series.maxime.ripard@bootlin.com> References: <cover.92acdec88ee4c280cb74e08ea22f0075e5fa055c.1553032382.git-series.maxime.ripard@bootlin.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk
Series	drm: Split out the formats API and move it to a common place \| expand [RFC,00/20] drm: Split out the formats API and move it to a common place [RFC,01/20] drm: Remove users of drm_format_num_planes [RFC,02/20] drm: Remove users of drm_format_(horz\|vert)_chroma_subsampling [RFC,03/20] drm/fourcc: Pass the format_info pointer to drm_format_plane_cpp [RFC,04/20] drm/fourcc: Pass the format_info pointer to drm_format_plane_width/height [RFC,05/20] drm: Replace instances of drm_format_info by drm_get_format_info [RFC,06/20] lib: Add video format information library [RFC,07/20] drm/fb: Move from drm_format_info to image_format_info [RFC,08/20] drm/malidp: Convert to generic image format library [RFC,09/20] drm/client: Convert to generic image format library [RFC,10/20] drm/exynos: Convert to generic image format library [RFC,11/20] drm/i915: Convert to generic image format library [RFC,12/20] drm/ipuv3: Convert to generic image format library [RFC,13/20] drm/msm: Convert to generic image format library [RFC,14/20] drm/omap: Convert to generic image format library [RFC,15/20] drm/rockchip: Convert to generic image format library [RFC,16/20] drm/tegra: Convert to generic image format library [RFC,17/20] drm/fourcc: Remove old DRM format API [RFC,18/20] lib: image-formats: Add v4l2 formats support [RFC,19/20] lib: image-formats: Add more functions [RFC,20/20] media: sun6i: Convert to the image format API

Maxime Ripard March 19, 2019, 9:57 p.m. UTC

V4L2 uses different fourcc's than DRM, and has a different set of formats.
For now, let's add the v4l2 fourcc's for the already existing formats.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 include/linux/image-formats.h |  9 +++++-
 lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 76 insertions(+)

Nicolas Dufresne March 19, 2019, 11:29 p.m. UTC | #1

Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> V4L2 uses different fourcc's than DRM, and has a different set of formats.
> For now, let's add the v4l2 fourcc's for the already existing formats.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  include/linux/image-formats.h |  9 +++++-
>  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 76 insertions(+)
> 
> diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> index 53fd73a71b3d..fbc3a4501ebd 100644
> --- a/include/linux/image-formats.h
> +++ b/include/linux/image-formats.h
> @@ -26,6 +26,13 @@ struct image_format_info {
>  	};
>  
>  	/**
> +	 * @v4l2_fmt:
> +	 *
> +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> +	 */
> +	u32 v4l2_fmt;
> +
> +	/**
>  	 * @depth:
>  	 *
>  	 * Color depth (number of bits per pixel excluding padding bits),
> @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
>  
>  const struct image_format_info *__image_format_drm_lookup(u32 drm);
>  const struct image_format_info *image_format_drm_lookup(u32 drm);
> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
>  unsigned int image_format_plane_cpp(const struct image_format_info *format,
>  				    int plane);
>  unsigned int image_format_plane_width(int width,
> diff --git a/lib/image-formats.c b/lib/image-formats.c
> index 9b9a73220c5d..39f1d38ae861 100644
> --- a/lib/image-formats.c
> +++ b/lib/image-formats.c
> @@ -8,6 +8,7 @@
>  static const struct image_format_info formats[] = {
>  	{
>  		.drm_fmt = DRM_FORMAT_C8,
> +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
>  		.depth = 8,
>  		.num_planes = 1,
>  		.cpp = { 1, 0, 0 },
> @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB332,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
>  		.depth = 8,
>  		.num_planes = 1,
>  		.cpp = { 1, 0, 0 },
> @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB4444,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB4444,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB1555,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
>  		.depth = 15,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB1555,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
>  		.depth = 15,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB565,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
>  		.depth = 16,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB888,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 3, 0, 0 },
> @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_BGR888,
> +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 3, 0, 0 },
> @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB8888,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,


All RGB mapping should be surrounded by ifdef, because many (not all)
DRM formats represent the order of component when placed in a CPU
register, unlike V4L2 which uses memory order. I've pick this one
randomly, but this one on most system, little endian, will match
V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
multiple places, notably in GStreamer:

https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45

>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 4, 0, 0 },
> @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB8888,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
>  		.depth = 32,
>  		.num_planes = 1,
>  		.cpp = { 4, 0, 0 },
> @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV410,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU410,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV420,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU420,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV422,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU422,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV444,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU444,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV12,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV21,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV16,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV61,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV24,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV42,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUYV,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVYU,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_UYVY,
> +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_VYUY,
> +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
>  EXPORT_SYMBOL(image_format_drm_lookup);
>  
>  /**
> + * __image_format_v4l2_lookup - query information for a given format
> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> +{
> +	return __image_format_lookup(v4l2_fmt, v4l2);
> +}
> +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> +
> +/**
> + * image_format_v4l2_lookup - query information for a given format
> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + * Unsupported pixel formats will generate a warning in the kernel log.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> +{
> +	const struct image_format_info *format;
> +
> +	format = __image_format_v4l2_lookup(v4l2);
> +
> +	WARN_ON(!format);
> +	return format;
> +}
> +EXPORT_SYMBOL(image_format_v4l2_lookup);
> +
> +/**
>   * image_format_plane_cpp - determine the bytes per pixel value
>   * @format: pointer to the image_format
>   * @plane: plane index

Ville Syrjälä March 20, 2019, 2:27 p.m. UTC | #2

On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > For now, let's add the v4l2 fourcc's for the already existing formats.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> >  include/linux/image-formats.h |  9 +++++-
> >  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 76 insertions(+)
> > 
> > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > index 53fd73a71b3d..fbc3a4501ebd 100644
> > --- a/include/linux/image-formats.h
> > +++ b/include/linux/image-formats.h
> > @@ -26,6 +26,13 @@ struct image_format_info {
> >  	};
> >  
> >  	/**
> > +	 * @v4l2_fmt:
> > +	 *
> > +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> > +	 */
> > +	u32 v4l2_fmt;
> > +
> > +	/**
> >  	 * @depth:
> >  	 *
> >  	 * Color depth (number of bits per pixel excluding padding bits),
> > @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> >  
> >  const struct image_format_info *__image_format_drm_lookup(u32 drm);
> >  const struct image_format_info *image_format_drm_lookup(u32 drm);
> > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
> >  unsigned int image_format_plane_cpp(const struct image_format_info *format,
> >  				    int plane);
> >  unsigned int image_format_plane_width(int width,
> > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > index 9b9a73220c5d..39f1d38ae861 100644
> > --- a/lib/image-formats.c
> > +++ b/lib/image-formats.c
> > @@ -8,6 +8,7 @@
> >  static const struct image_format_info formats[] = {
> >  	{
> >  		.drm_fmt = DRM_FORMAT_C8,
> > +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
> >  		.depth = 8,
> >  		.num_planes = 1,
> >  		.cpp = { 1, 0, 0 },
> > @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
> >  		.vsub = 1,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_RGB332,
> > +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
> >  		.depth = 8,
> >  		.num_planes = 1,
> >  		.cpp = { 1, 0, 0 },
> > @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
> >  		.vsub = 1,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_XRGB4444,
> > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> >  		.depth = 0,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
> >  		.vsub = 1,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_ARGB4444,
> > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> >  		.depth = 0,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
> >  		.has_alpha = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_XRGB1555,
> > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> >  		.depth = 15,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
> >  		.vsub = 1,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_ARGB1555,
> > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> >  		.depth = 15,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> >  		.has_alpha = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_RGB565,
> > +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> >  		.depth = 16,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
> >  		.vsub = 1,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_RGB888,
> > +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> >  		.depth = 24,
> >  		.num_planes = 1,
> >  		.cpp = { 3, 0, 0 },
> > @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
> >  		.vsub = 1,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_BGR888,
> > +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> >  		.depth = 24,
> >  		.num_planes = 1,
> >  		.cpp = { 3, 0, 0 },
> > @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
> >  		.vsub = 1,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_XRGB8888,
> > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> 
> 
> All RGB mapping should be surrounded by ifdef, because many (not all)
> DRM formats represent the order of component when placed in a CPU
> register, unlike V4L2 which uses memory order. I've pick this one

DRM formats are explicitly defined as little endian.

> randomly, but this one on most system, little endian, will match
> V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> multiple places, notably in GStreamer:
> 
> https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
> 
> >  		.depth = 24,
> >  		.num_planes = 1,
> >  		.cpp = { 4, 0, 0 },
> > @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
> >  		.has_alpha = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_ARGB8888,
> > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
> >  		.depth = 32,
> >  		.num_planes = 1,
> >  		.cpp = { 4, 0, 0 },
> > @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
> >  		.has_alpha = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YUV410,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YVU410,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YUV420,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YVU420,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YUV422,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YVU422,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YUV444,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YVU444,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
> >  		.depth = 0,
> >  		.num_planes = 3,
> >  		.cpp = { 1, 1, 1 },
> > @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_NV12,
> > +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
> >  		.depth = 0,
> >  		.num_planes = 2,
> >  		.cpp = { 1, 2, 0 },
> > @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_NV21,
> > +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
> >  		.depth = 0,
> >  		.num_planes = 2,
> >  		.cpp = { 1, 2, 0 },
> > @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_NV16,
> > +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
> >  		.depth = 0,
> >  		.num_planes = 2,
> >  		.cpp = { 1, 2, 0 },
> > @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_NV61,
> > +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
> >  		.depth = 0,
> >  		.num_planes = 2,
> >  		.cpp = { 1, 2, 0 },
> > @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_NV24,
> > +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> >  		.depth = 0,
> >  		.num_planes = 2,
> >  		.cpp = { 1, 2, 0 },
> > @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_NV42,
> > +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
> >  		.depth = 0,
> >  		.num_planes = 2,
> >  		.cpp = { 1, 2, 0 },
> > @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YUYV,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
> >  		.depth = 0,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_YVYU,
> > +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
> >  		.depth = 0,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_UYVY,
> > +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
> >  		.depth = 0,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
> >  		.is_yuv = true,
> >  	}, {
> >  		.drm_fmt = DRM_FORMAT_VYUY,
> > +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
> >  		.depth = 0,
> >  		.num_planes = 1,
> >  		.cpp = { 2, 0, 0 },
> > @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
> >  EXPORT_SYMBOL(image_format_drm_lookup);
> >  
> >  /**
> > + * __image_format_v4l2_lookup - query information for a given format
> > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > + *
> > + * The caller should only pass a supported pixel format to this function.
> > + *
> > + * Returns:
> > + * The instance of struct image_format_info that describes the pixel format, or
> > + * NULL if the format is unsupported.
> > + */
> > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> > +{
> > +	return __image_format_lookup(v4l2_fmt, v4l2);
> > +}
> > +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> > +
> > +/**
> > + * image_format_v4l2_lookup - query information for a given format
> > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > + *
> > + * The caller should only pass a supported pixel format to this function.
> > + * Unsupported pixel formats will generate a warning in the kernel log.
> > + *
> > + * Returns:
> > + * The instance of struct image_format_info that describes the pixel format, or
> > + * NULL if the format is unsupported.
> > + */
> > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> > +{
> > +	const struct image_format_info *format;
> > +
> > +	format = __image_format_v4l2_lookup(v4l2);
> > +
> > +	WARN_ON(!format);
> > +	return format;
> > +}
> > +EXPORT_SYMBOL(image_format_v4l2_lookup);
> > +
> > +/**
> >   * image_format_plane_cpp - determine the bytes per pixel value
> >   * @format: pointer to the image_format
> >   * @plane: plane index
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Nicolas Dufresne March 20, 2019, 3:51 p.m. UTC | #3

Le mercredi 20 mars 2019 à 16:27 +0200, Ville Syrjälä a écrit :
> On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> > Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > > For now, let's add the v4l2 fourcc's for the already existing formats.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > ---
> > >  include/linux/image-formats.h |  9 +++++-
> > >  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 76 insertions(+)
> > > 
> > > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > > index 53fd73a71b3d..fbc3a4501ebd 100644
> > > --- a/include/linux/image-formats.h
> > > +++ b/include/linux/image-formats.h
> > > @@ -26,6 +26,13 @@ struct image_format_info {
> > >  	};
> > >  
> > >  	/**
> > > +	 * @v4l2_fmt:
> > > +	 *
> > > +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> > > +	 */
> > > +	u32 v4l2_fmt;
> > > +
> > > +	/**
> > >  	 * @depth:
> > >  	 *
> > >  	 * Color depth (number of bits per pixel excluding padding bits),
> > > @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> > >  
> > >  const struct image_format_info *__image_format_drm_lookup(u32 drm);
> > >  const struct image_format_info *image_format_drm_lookup(u32 drm);
> > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
> > >  unsigned int image_format_plane_cpp(const struct image_format_info *format,
> > >  				    int plane);
> > >  unsigned int image_format_plane_width(int width,
> > > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > > index 9b9a73220c5d..39f1d38ae861 100644
> > > --- a/lib/image-formats.c
> > > +++ b/lib/image-formats.c
> > > @@ -8,6 +8,7 @@
> > >  static const struct image_format_info formats[] = {
> > >  	{
> > >  		.drm_fmt = DRM_FORMAT_C8,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
> > >  		.depth = 8,
> > >  		.num_planes = 1,
> > >  		.cpp = { 1, 0, 0 },
> > > @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
> > >  		.vsub = 1,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_RGB332,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
> > >  		.depth = 8,
> > >  		.num_planes = 1,
> > >  		.cpp = { 1, 0, 0 },
> > > @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
> > >  		.vsub = 1,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_XRGB4444,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> > >  		.depth = 0,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
> > >  		.vsub = 1,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_ARGB4444,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> > >  		.depth = 0,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
> > >  		.has_alpha = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_XRGB1555,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> > >  		.depth = 15,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
> > >  		.vsub = 1,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_ARGB1555,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> > >  		.depth = 15,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> > >  		.has_alpha = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_RGB565,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> > >  		.depth = 16,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
> > >  		.vsub = 1,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_RGB888,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> > >  		.depth = 24,
> > >  		.num_planes = 1,
> > >  		.cpp = { 3, 0, 0 },
> > > @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
> > >  		.vsub = 1,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_BGR888,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> > >  		.depth = 24,
> > >  		.num_planes = 1,
> > >  		.cpp = { 3, 0, 0 },
> > > @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
> > >  		.vsub = 1,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_XRGB8888,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> > 
> > All RGB mapping should be surrounded by ifdef, because many (not all)
> > DRM formats represent the order of component when placed in a CPU
> > register, unlike V4L2 which uses memory order. I've pick this one
> 
> DRM formats are explicitly defined as little endian.

Yes, that means the same thing. The mapping has nothing to do with the
buffer bytes order, unlike V4L2 (and most streaming stack) do. Though
the mapping in DRM isn't consistent. Again, this mapping is not
correct, it will result in swapped colors.

> 
> > randomly, but this one on most system, little endian, will match
> > V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> > multiple places, notably in GStreamer:
> > 
> > https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
> > 
> > >  		.depth = 24,
> > >  		.num_planes = 1,
> > >  		.cpp = { 4, 0, 0 },
> > > @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
> > >  		.has_alpha = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_ARGB8888,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
> > >  		.depth = 32,
> > >  		.num_planes = 1,
> > >  		.cpp = { 4, 0, 0 },
> > > @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
> > >  		.has_alpha = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YUV410,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YVU410,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YUV420,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YVU420,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YUV422,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YVU422,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YUV444,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YVU444,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
> > >  		.depth = 0,
> > >  		.num_planes = 3,
> > >  		.cpp = { 1, 1, 1 },
> > > @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_NV12,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
> > >  		.depth = 0,
> > >  		.num_planes = 2,
> > >  		.cpp = { 1, 2, 0 },
> > > @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_NV21,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
> > >  		.depth = 0,
> > >  		.num_planes = 2,
> > >  		.cpp = { 1, 2, 0 },
> > > @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_NV16,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
> > >  		.depth = 0,
> > >  		.num_planes = 2,
> > >  		.cpp = { 1, 2, 0 },
> > > @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_NV61,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
> > >  		.depth = 0,
> > >  		.num_planes = 2,
> > >  		.cpp = { 1, 2, 0 },
> > > @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_NV24,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> > >  		.depth = 0,
> > >  		.num_planes = 2,
> > >  		.cpp = { 1, 2, 0 },
> > > @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_NV42,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
> > >  		.depth = 0,
> > >  		.num_planes = 2,
> > >  		.cpp = { 1, 2, 0 },
> > > @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YUYV,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
> > >  		.depth = 0,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_YVYU,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
> > >  		.depth = 0,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_UYVY,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
> > >  		.depth = 0,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
> > >  		.is_yuv = true,
> > >  	}, {
> > >  		.drm_fmt = DRM_FORMAT_VYUY,
> > > +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
> > >  		.depth = 0,
> > >  		.num_planes = 1,
> > >  		.cpp = { 2, 0, 0 },
> > > @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
> > >  EXPORT_SYMBOL(image_format_drm_lookup);
> > >  
> > >  /**
> > > + * __image_format_v4l2_lookup - query information for a given format
> > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > + *
> > > + * The caller should only pass a supported pixel format to this function.
> > > + *
> > > + * Returns:
> > > + * The instance of struct image_format_info that describes the pixel format, or
> > > + * NULL if the format is unsupported.
> > > + */
> > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> > > +{
> > > +	return __image_format_lookup(v4l2_fmt, v4l2);
> > > +}
> > > +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> > > +
> > > +/**
> > > + * image_format_v4l2_lookup - query information for a given format
> > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > + *
> > > + * The caller should only pass a supported pixel format to this function.
> > > + * Unsupported pixel formats will generate a warning in the kernel log.
> > > + *
> > > + * Returns:
> > > + * The instance of struct image_format_info that describes the pixel format, or
> > > + * NULL if the format is unsupported.
> > > + */
> > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> > > +{
> > > +	const struct image_format_info *format;
> > > +
> > > +	format = __image_format_v4l2_lookup(v4l2);
> > > +
> > > +	WARN_ON(!format);
> > > +	return format;
> > > +}
> > > +EXPORT_SYMBOL(image_format_v4l2_lookup);
> > > +
> > > +/**
> > >   * image_format_plane_cpp - determine the bytes per pixel value
> > >   * @format: pointer to the image_format
> > >   * @plane: plane index
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Nicolas Dufresne March 20, 2019, 4:30 p.m. UTC | #4

Le mercredi 20 mars 2019 à 18:09 +0200, Ville Syrjälä a écrit :
> On Wed, Mar 20, 2019 at 11:51:58AM -0400, Nicolas Dufresne wrote:
> > Le mercredi 20 mars 2019 à 16:27 +0200, Ville Syrjälä a écrit :
> > > On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> > > > Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > > > > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > > > > For now, let's add the v4l2 fourcc's for the already existing formats.
> > > > > 
> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > ---
> > > > >  include/linux/image-formats.h |  9 +++++-
> > > > >  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
> > > > >  2 files changed, 76 insertions(+)
> > > > > 
> > > > > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > > > > index 53fd73a71b3d..fbc3a4501ebd 100644
> > > > > --- a/include/linux/image-formats.h
> > > > > +++ b/include/linux/image-formats.h
> > > > > @@ -26,6 +26,13 @@ struct image_format_info {
> > > > >  	};
> > > > >  
> > > > >  	/**
> > > > > +	 * @v4l2_fmt:
> > > > > +	 *
> > > > > +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> > > > > +	 */
> > > > > +	u32 v4l2_fmt;
> > > > > +
> > > > > +	/**
> > > > >  	 * @depth:
> > > > >  	 *
> > > > >  	 * Color depth (number of bits per pixel excluding padding bits),
> > > > > @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> > > > >  
> > > > >  const struct image_format_info *__image_format_drm_lookup(u32 drm);
> > > > >  const struct image_format_info *image_format_drm_lookup(u32 drm);
> > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
> > > > >  unsigned int image_format_plane_cpp(const struct image_format_info *format,
> > > > >  				    int plane);
> > > > >  unsigned int image_format_plane_width(int width,
> > > > > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > > > > index 9b9a73220c5d..39f1d38ae861 100644
> > > > > --- a/lib/image-formats.c
> > > > > +++ b/lib/image-formats.c
> > > > > @@ -8,6 +8,7 @@
> > > > >  static const struct image_format_info formats[] = {
> > > > >  	{
> > > > >  		.drm_fmt = DRM_FORMAT_C8,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
> > > > >  		.depth = 8,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 1, 0, 0 },
> > > > > @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.vsub = 1,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_RGB332,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
> > > > >  		.depth = 8,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 1, 0, 0 },
> > > > > @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.vsub = 1,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_XRGB4444,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.vsub = 1,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_ARGB4444,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.has_alpha = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_XRGB1555,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> > > > >  		.depth = 15,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.vsub = 1,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_ARGB1555,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> > > > >  		.depth = 15,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.has_alpha = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_RGB565,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> > > > >  		.depth = 16,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.vsub = 1,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_RGB888,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> > > > >  		.depth = 24,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 3, 0, 0 },
> > > > > @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.vsub = 1,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_BGR888,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> > > > >  		.depth = 24,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 3, 0, 0 },
> > > > > @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.vsub = 1,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_XRGB8888,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> > > > 
> > > > All RGB mapping should be surrounded by ifdef, because many (not all)
> > > > DRM formats represent the order of component when placed in a CPU
> > > > register, unlike V4L2 which uses memory order. I've pick this one
> > > 
> > > DRM formats are explicitly defined as little endian.
> > 
> > Yes, that means the same thing. The mapping has nothing to do with the
> > buffer bytes order, unlike V4L2 (and most streaming stack) do.
> 
> It has everything to do with byte order. Little endian means the least
> significant byte is stored in the first byte in memory.
> 
> Based on https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/pixfmt-packed-rgb.html
> drm XRGB888 is actually v4l XBGR32, not XRGB32 as claimed by this patch.

That's basically what I said, as it's define for Little Endian rather
then buffer order, you have to make the mapping conditional. It
basically mean that in memory, the DRM format physically differ
depending on CPU endian. Last time we have run this on PPC / Big
Endian, the mapping was XRGB/XRGB, we checked that up multiple time
with the DRM maintainers and was told this is exactly what it's suppose
to do, hence this endian dependant mapping all over the place. If you
make up that this isn't right, you are breaking userspace, and people
don't like that.

So the mapping should be:
Little: DRM XRGB / V4L2 XBGR
Big:    DRM XRGB / V4L2 XRGB

> 
> > > > randomly, but this one on most system, little endian, will match
> > > > V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> > > > multiple places, notably in GStreamer:
> > > > 
> > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
> > > > 
> > > > >  		.depth = 24,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 4, 0, 0 },
> > > > > @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.has_alpha = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_ARGB8888,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
> > > > >  		.depth = 32,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 4, 0, 0 },
> > > > > @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.has_alpha = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YUV410,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YVU410,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YUV420,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YVU420,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YUV422,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YVU422,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YUV444,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YVU444,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 3,
> > > > >  		.cpp = { 1, 1, 1 },
> > > > > @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_NV12,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 2,
> > > > >  		.cpp = { 1, 2, 0 },
> > > > > @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_NV21,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 2,
> > > > >  		.cpp = { 1, 2, 0 },
> > > > > @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_NV16,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 2,
> > > > >  		.cpp = { 1, 2, 0 },
> > > > > @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_NV61,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 2,
> > > > >  		.cpp = { 1, 2, 0 },
> > > > > @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_NV24,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 2,
> > > > >  		.cpp = { 1, 2, 0 },
> > > > > @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_NV42,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 2,
> > > > >  		.cpp = { 1, 2, 0 },
> > > > > @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YUYV,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_YVYU,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_UYVY,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
> > > > >  		.is_yuv = true,
> > > > >  	}, {
> > > > >  		.drm_fmt = DRM_FORMAT_VYUY,
> > > > > +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
> > > > >  		.depth = 0,
> > > > >  		.num_planes = 1,
> > > > >  		.cpp = { 2, 0, 0 },
> > > > > @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
> > > > >  EXPORT_SYMBOL(image_format_drm_lookup);
> > > > >  
> > > > >  /**
> > > > > + * __image_format_v4l2_lookup - query information for a given format
> > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > + *
> > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > + *
> > > > > + * Returns:
> > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > + * NULL if the format is unsupported.
> > > > > + */
> > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> > > > > +{
> > > > > +	return __image_format_lookup(v4l2_fmt, v4l2);
> > > > > +}
> > > > > +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> > > > > +
> > > > > +/**
> > > > > + * image_format_v4l2_lookup - query information for a given format
> > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > + *
> > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > + * Unsupported pixel formats will generate a warning in the kernel log.
> > > > > + *
> > > > > + * Returns:
> > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > + * NULL if the format is unsupported.
> > > > > + */
> > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> > > > > +{
> > > > > +	const struct image_format_info *format;
> > > > > +
> > > > > +	format = __image_format_v4l2_lookup(v4l2);
> > > > > +
> > > > > +	WARN_ON(!format);
> > > > > +	return format;
> > > > > +}
> > > > > +EXPORT_SYMBOL(image_format_v4l2_lookup);
> > > > > +
> > > > > +/**
> > > > >   * image_format_plane_cpp - determine the bytes per pixel value
> > > > >   * @format: pointer to the image_format
> > > > >   * @plane: plane index
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
>

Brian Starkey March 20, 2019, 6:15 p.m. UTC | #5

On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> All RGB mapping should be surrounded by ifdef, because many (not all)
> DRM formats represent the order of component when placed in a CPU
> register, unlike V4L2 which uses memory order. I've pick this one
> randomly, but this one on most system, little endian, will match
> V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> multiple places, notably in GStreamer:
> 
> https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
> 

I do sort-of wonder if it's worth trying to switch to common fourccs
between DRM and V4L2 (and whatever else there is).

The V4L2 formats list is quite incomplete and a little quirky in
places (V4L2_PIX_FORMAT_XBGR32 and V4L2_PIX_FORMAT_XRGB32 naming
inconsistency being one. 'X' isn't even next to 'B' in XBGR32).

At least for newly-added formats, not using a common definition
doesn't make a lot of sense to me. Longer term, I also don't really
see any downsides to unification.

-Brian

Nicolas Dufresne March 20, 2019, 6:27 p.m. UTC | #6

Le mercredi 20 mars 2019 à 18:41 +0200, Ville Syrjälä a écrit :
> On Wed, Mar 20, 2019 at 12:30:25PM -0400, Nicolas Dufresne wrote:
> > Le mercredi 20 mars 2019 à 18:09 +0200, Ville Syrjälä a écrit :
> > > On Wed, Mar 20, 2019 at 11:51:58AM -0400, Nicolas Dufresne wrote:
> > > > Le mercredi 20 mars 2019 à 16:27 +0200, Ville Syrjälä a écrit :
> > > > > On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> > > > > > Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > > > > > > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > > > > > > For now, let's add the v4l2 fourcc's for the already existing formats.
> > > > > > > 
> > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > > ---
> > > > > > >  include/linux/image-formats.h |  9 +++++-
> > > > > > >  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
> > > > > > >  2 files changed, 76 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > > > > > > index 53fd73a71b3d..fbc3a4501ebd 100644
> > > > > > > --- a/include/linux/image-formats.h
> > > > > > > +++ b/include/linux/image-formats.h
> > > > > > > @@ -26,6 +26,13 @@ struct image_format_info {
> > > > > > >  	};
> > > > > > >  
> > > > > > >  	/**
> > > > > > > +	 * @v4l2_fmt:
> > > > > > > +	 *
> > > > > > > +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> > > > > > > +	 */
> > > > > > > +	u32 v4l2_fmt;
> > > > > > > +
> > > > > > > +	/**
> > > > > > >  	 * @depth:
> > > > > > >  	 *
> > > > > > >  	 * Color depth (number of bits per pixel excluding padding bits),
> > > > > > > @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> > > > > > >  
> > > > > > >  const struct image_format_info *__image_format_drm_lookup(u32 drm);
> > > > > > >  const struct image_format_info *image_format_drm_lookup(u32 drm);
> > > > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> > > > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
> > > > > > >  unsigned int image_format_plane_cpp(const struct image_format_info *format,
> > > > > > >  				    int plane);
> > > > > > >  unsigned int image_format_plane_width(int width,
> > > > > > > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > > > > > > index 9b9a73220c5d..39f1d38ae861 100644
> > > > > > > --- a/lib/image-formats.c
> > > > > > > +++ b/lib/image-formats.c
> > > > > > > @@ -8,6 +8,7 @@
> > > > > > >  static const struct image_format_info formats[] = {
> > > > > > >  	{
> > > > > > >  		.drm_fmt = DRM_FORMAT_C8,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
> > > > > > >  		.depth = 8,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.vsub = 1,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_RGB332,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
> > > > > > >  		.depth = 8,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.vsub = 1,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB4444,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.vsub = 1,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB4444,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.has_alpha = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB1555,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> > > > > > >  		.depth = 15,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.vsub = 1,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB1555,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> > > > > > >  		.depth = 15,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.has_alpha = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_RGB565,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> > > > > > >  		.depth = 16,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.vsub = 1,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_RGB888,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> > > > > > >  		.depth = 24,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.vsub = 1,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_BGR888,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> > > > > > >  		.depth = 24,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.vsub = 1,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB8888,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> > > > > > 
> > > > > > All RGB mapping should be surrounded by ifdef, because many (not all)
> > > > > > DRM formats represent the order of component when placed in a CPU
> > > > > > register, unlike V4L2 which uses memory order. I've pick this one
> > > > > 
> > > > > DRM formats are explicitly defined as little endian.
> > > > 
> > > > Yes, that means the same thing. The mapping has nothing to do with the
> > > > buffer bytes order, unlike V4L2 (and most streaming stack) do.
> > > 
> > > It has everything to do with byte order. Little endian means the least
> > > significant byte is stored in the first byte in memory.
> > > 
> > > Based on https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/pixfmt-packed-rgb.html
> > > drm XRGB888 is actually v4l XBGR32, not XRGB32 as claimed by this patch.
> > 
> > That's basically what I said, as it's define for Little Endian rather
> > then buffer order, you have to make the mapping conditional. It
> > basically mean that in memory, the DRM format physically differ
> > depending on CPU endian.
> 
> No. It is always little endian no matter what the CPU is.

I'm sorry, this is in your ABI, we don't add layers of ifdef in
userspace code just for the fun of it. If you redefine this now you are
breaking userspace. I agree there is very little to no Big Endian on
DRM side anymore, but what historically was mapped per CPU cannot be
changed by you now.

> 
> > Last time we have run this on PPC / Big
> > Endian, the mapping was XRGB/XRGB, we checked that up multiple time
> > with the DRM maintainers and was told this is exactly what it's suppose
> > to do, hence this endian dependant mapping all over the place. If you
> > make up that this isn't right, you are breaking userspace, and people
> > don't like that.
> 
> Someone recently added those DRM_FORMAT_HOST variants to allow
> the legacy addfb1 to create pick the format based on host
> endianness. I thought that was the only conclusion from the
> little vs. big endian drm fourcc wars.
> 
> > So the mapping should be:
> > Little: DRM XRGB / V4L2 XBGR
> > Big:    DRM XRGB / V4L2 XRGB
> > 
> > > > > > randomly, but this one on most system, little endian, will match
> > > > > > V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> > > > > > multiple places, notably in GStreamer:
> > > > > > 
> > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
> > > > > > 
> > > > > > >  		.depth = 24,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 4, 0, 0 },
> > > > > > > @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.has_alpha = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB8888,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
> > > > > > >  		.depth = 32,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 4, 0, 0 },
> > > > > > > @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.has_alpha = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YUV410,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YVU410,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YUV420,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YVU420,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YUV422,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YVU422,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YUV444,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YVU444,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 3,
> > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_NV12,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 2,
> > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_NV21,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 2,
> > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_NV16,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 2,
> > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_NV61,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 2,
> > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_NV24,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 2,
> > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_NV42,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 2,
> > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YUYV,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_YVYU,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_UYVY,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
> > > > > > >  		.is_yuv = true,
> > > > > > >  	}, {
> > > > > > >  		.drm_fmt = DRM_FORMAT_VYUY,
> > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
> > > > > > >  		.depth = 0,
> > > > > > >  		.num_planes = 1,
> > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
> > > > > > >  EXPORT_SYMBOL(image_format_drm_lookup);
> > > > > > >  
> > > > > > >  /**
> > > > > > > + * __image_format_v4l2_lookup - query information for a given format
> > > > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > > > + *
> > > > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > > > + *
> > > > > > > + * Returns:
> > > > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > > > + * NULL if the format is unsupported.
> > > > > > > + */
> > > > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> > > > > > > +{
> > > > > > > +	return __image_format_lookup(v4l2_fmt, v4l2);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * image_format_v4l2_lookup - query information for a given format
> > > > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > > > + *
> > > > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > > > + * Unsupported pixel formats will generate a warning in the kernel log.
> > > > > > > + *
> > > > > > > + * Returns:
> > > > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > > > + * NULL if the format is unsupported.
> > > > > > > + */
> > > > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> > > > > > > +{
> > > > > > > +	const struct image_format_info *format;
> > > > > > > +
> > > > > > > +	format = __image_format_v4l2_lookup(v4l2);
> > > > > > > +
> > > > > > > +	WARN_ON(!format);
> > > > > > > +	return format;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(image_format_v4l2_lookup);
> > > > > > > +
> > > > > > > +/**
> > > > > > >   * image_format_plane_cpp - determine the bytes per pixel value
> > > > > > >   * @format: pointer to the image_format
> > > > > > >   * @plane: plane index
> > > > > > 
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
>

Ville Syrjälä March 20, 2019, 6:39 p.m. UTC | #7

On Wed, Mar 20, 2019 at 02:27:59PM -0400, Nicolas Dufresne wrote:
> Le mercredi 20 mars 2019 à 18:41 +0200, Ville Syrjälä a écrit :
> > On Wed, Mar 20, 2019 at 12:30:25PM -0400, Nicolas Dufresne wrote:
> > > Le mercredi 20 mars 2019 à 18:09 +0200, Ville Syrjälä a écrit :
> > > > On Wed, Mar 20, 2019 at 11:51:58AM -0400, Nicolas Dufresne wrote:
> > > > > Le mercredi 20 mars 2019 à 16:27 +0200, Ville Syrjälä a écrit :
> > > > > > On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> > > > > > > Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > > > > > > > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > > > > > > > For now, let's add the v4l2 fourcc's for the already existing formats.
> > > > > > > > 
> > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > > > ---
> > > > > > > >  include/linux/image-formats.h |  9 +++++-
> > > > > > > >  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
> > > > > > > >  2 files changed, 76 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > > > > > > > index 53fd73a71b3d..fbc3a4501ebd 100644
> > > > > > > > --- a/include/linux/image-formats.h
> > > > > > > > +++ b/include/linux/image-formats.h
> > > > > > > > @@ -26,6 +26,13 @@ struct image_format_info {
> > > > > > > >  	};
> > > > > > > >  
> > > > > > > >  	/**
> > > > > > > > +	 * @v4l2_fmt:
> > > > > > > > +	 *
> > > > > > > > +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> > > > > > > > +	 */
> > > > > > > > +	u32 v4l2_fmt;
> > > > > > > > +
> > > > > > > > +	/**
> > > > > > > >  	 * @depth:
> > > > > > > >  	 *
> > > > > > > >  	 * Color depth (number of bits per pixel excluding padding bits),
> > > > > > > > @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> > > > > > > >  
> > > > > > > >  const struct image_format_info *__image_format_drm_lookup(u32 drm);
> > > > > > > >  const struct image_format_info *image_format_drm_lookup(u32 drm);
> > > > > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> > > > > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
> > > > > > > >  unsigned int image_format_plane_cpp(const struct image_format_info *format,
> > > > > > > >  				    int plane);
> > > > > > > >  unsigned int image_format_plane_width(int width,
> > > > > > > > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > > > > > > > index 9b9a73220c5d..39f1d38ae861 100644
> > > > > > > > --- a/lib/image-formats.c
> > > > > > > > +++ b/lib/image-formats.c
> > > > > > > > @@ -8,6 +8,7 @@
> > > > > > > >  static const struct image_format_info formats[] = {
> > > > > > > >  	{
> > > > > > > >  		.drm_fmt = DRM_FORMAT_C8,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
> > > > > > > >  		.depth = 8,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > > @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.vsub = 1,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB332,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
> > > > > > > >  		.depth = 8,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > > @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.vsub = 1,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB4444,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.vsub = 1,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB4444,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.has_alpha = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB1555,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> > > > > > > >  		.depth = 15,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.vsub = 1,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB1555,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> > > > > > > >  		.depth = 15,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.has_alpha = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB565,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> > > > > > > >  		.depth = 16,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.vsub = 1,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB888,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> > > > > > > >  		.depth = 24,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > > @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.vsub = 1,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_BGR888,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> > > > > > > >  		.depth = 24,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > > @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.vsub = 1,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB8888,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> > > > > > > 
> > > > > > > All RGB mapping should be surrounded by ifdef, because many (not all)
> > > > > > > DRM formats represent the order of component when placed in a CPU
> > > > > > > register, unlike V4L2 which uses memory order. I've pick this one
> > > > > > 
> > > > > > DRM formats are explicitly defined as little endian.
> > > > > 
> > > > > Yes, that means the same thing. The mapping has nothing to do with the
> > > > > buffer bytes order, unlike V4L2 (and most streaming stack) do.
> > > > 
> > > > It has everything to do with byte order. Little endian means the least
> > > > significant byte is stored in the first byte in memory.
> > > > 
> > > > Based on https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/pixfmt-packed-rgb.html
> > > > drm XRGB888 is actually v4l XBGR32, not XRGB32 as claimed by this patch.
> > > 
> > > That's basically what I said, as it's define for Little Endian rather
> > > then buffer order, you have to make the mapping conditional. It
> > > basically mean that in memory, the DRM format physically differ
> > > depending on CPU endian.
> > 
> > No. It is always little endian no matter what the CPU is.
> 
> I'm sorry, this is in your ABI, we don't add layers of ifdef in
> userspace code just for the fun of it. If you redefine this now you are
> breaking userspace. I agree there is very little to no Big Endian on
> DRM side anymore, but what historically was mapped per CPU cannot be
> changed by you now.

It was always little endian.

> 
> > 
> > > Last time we have run this on PPC / Big
> > > Endian, the mapping was XRGB/XRGB, we checked that up multiple time
> > > with the DRM maintainers and was told this is exactly what it's suppose
> > > to do, hence this endian dependant mapping all over the place. If you
> > > make up that this isn't right, you are breaking userspace, and people
> > > don't like that.
> > 
> > Someone recently added those DRM_FORMAT_HOST variants to allow
> > the legacy addfb1 to create pick the format based on host
> > endianness. I thought that was the only conclusion from the
> > little vs. big endian drm fourcc wars.
> > 
> > > So the mapping should be:
> > > Little: DRM XRGB / V4L2 XBGR
> > > Big:    DRM XRGB / V4L2 XRGB
> > > 
> > > > > > > randomly, but this one on most system, little endian, will match
> > > > > > > V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> > > > > > > multiple places, notably in GStreamer:
> > > > > > > 
> > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
> > > > > > > 
> > > > > > > >  		.depth = 24,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 4, 0, 0 },
> > > > > > > > @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.has_alpha = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB8888,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
> > > > > > > >  		.depth = 32,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 4, 0, 0 },
> > > > > > > > @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.has_alpha = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV410,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU410,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV420,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU420,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV422,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU422,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV444,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU444,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 3,
> > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_NV12,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 2,
> > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_NV21,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 2,
> > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_NV16,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 2,
> > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_NV61,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 2,
> > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_NV24,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 2,
> > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_NV42,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 2,
> > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YUYV,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_YVYU,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_UYVY,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
> > > > > > > >  		.is_yuv = true,
> > > > > > > >  	}, {
> > > > > > > >  		.drm_fmt = DRM_FORMAT_VYUY,
> > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
> > > > > > > >  		.depth = 0,
> > > > > > > >  		.num_planes = 1,
> > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
> > > > > > > >  EXPORT_SYMBOL(image_format_drm_lookup);
> > > > > > > >  
> > > > > > > >  /**
> > > > > > > > + * __image_format_v4l2_lookup - query information for a given format
> > > > > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > > > > + *
> > > > > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > > > > + *
> > > > > > > > + * Returns:
> > > > > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > > > > + * NULL if the format is unsupported.
> > > > > > > > + */
> > > > > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> > > > > > > > +{
> > > > > > > > +	return __image_format_lookup(v4l2_fmt, v4l2);
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * image_format_v4l2_lookup - query information for a given format
> > > > > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > > > > + *
> > > > > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > > > > + * Unsupported pixel formats will generate a warning in the kernel log.
> > > > > > > > + *
> > > > > > > > + * Returns:
> > > > > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > > > > + * NULL if the format is unsupported.
> > > > > > > > + */
> > > > > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> > > > > > > > +{
> > > > > > > > +	const struct image_format_info *format;
> > > > > > > > +
> > > > > > > > +	format = __image_format_v4l2_lookup(v4l2);
> > > > > > > > +
> > > > > > > > +	WARN_ON(!format);
> > > > > > > > +	return format;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL(image_format_v4l2_lookup);
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > >   * image_format_plane_cpp - determine the bytes per pixel value
> > > > > > > >   * @format: pointer to the image_format
> > > > > > > >   * @plane: plane index
> > > > > > > 
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> >

Maxime Ripard March 21, 2019, 3:47 p.m. UTC | #8

On Wed, Mar 20, 2019 at 06:15:54PM +0000, Brian Starkey wrote:
> On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> > All RGB mapping should be surrounded by ifdef, because many (not all)
> > DRM formats represent the order of component when placed in a CPU
> > register, unlike V4L2 which uses memory order. I've pick this one
> > randomly, but this one on most system, little endian, will match
> > V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> > multiple places, notably in GStreamer:
> >
> > https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
>
> I do sort-of wonder if it's worth trying to switch to common fourccs
> between DRM and V4L2 (and whatever else there is).
>
> The V4L2 formats list is quite incomplete and a little quirky in
> places (V4L2_PIX_FORMAT_XBGR32 and V4L2_PIX_FORMAT_XRGB32 naming
> inconsistency being one. 'X' isn't even next to 'B' in XBGR32).
>
> At least for newly-added formats, not using a common definition
> doesn't make a lot of sense to me. Longer term, I also don't really
> see any downsides to unification.

Eventually, I agree that that his where we should be heading. Moving
the existing formats support to a common place will help with that.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Paul Kocialkowski March 21, 2019, 4:04 p.m. UTC | #9

Hi,

Le mercredi 20 mars 2019 à 20:39 +0200, Ville Syrjälä a écrit :
> On Wed, Mar 20, 2019 at 02:27:59PM -0400, Nicolas Dufresne wrote:
> > Le mercredi 20 mars 2019 à 18:41 +0200, Ville Syrjälä a écrit :
> > > On Wed, Mar 20, 2019 at 12:30:25PM -0400, Nicolas Dufresne wrote:
> > > > Le mercredi 20 mars 2019 à 18:09 +0200, Ville Syrjälä a écrit :
> > > > > On Wed, Mar 20, 2019 at 11:51:58AM -0400, Nicolas Dufresne wrote:
> > > > > > Le mercredi 20 mars 2019 à 16:27 +0200, Ville Syrjälä a écrit :
> > > > > > > On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> > > > > > > > Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > > > > > > > > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > > > > > > > > For now, let's add the v4l2 fourcc's for the already existing formats.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > > > > ---
> > > > > > > > >  include/linux/image-formats.h |  9 +++++-
> > > > > > > > >  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
> > > > > > > > >  2 files changed, 76 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > > > > > > > > index 53fd73a71b3d..fbc3a4501ebd 100644
> > > > > > > > > --- a/include/linux/image-formats.h
> > > > > > > > > +++ b/include/linux/image-formats.h
> > > > > > > > > @@ -26,6 +26,13 @@ struct image_format_info {
> > > > > > > > >  	};
> > > > > > > > >  
> > > > > > > > >  	/**
> > > > > > > > > +	 * @v4l2_fmt:
> > > > > > > > > +	 *
> > > > > > > > > +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> > > > > > > > > +	 */
> > > > > > > > > +	u32 v4l2_fmt;
> > > > > > > > > +
> > > > > > > > > +	/**
> > > > > > > > >  	 * @depth:
> > > > > > > > >  	 *
> > > > > > > > >  	 * Color depth (number of bits per pixel excluding padding bits),
> > > > > > > > > @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> > > > > > > > >  
> > > > > > > > >  const struct image_format_info *__image_format_drm_lookup(u32 drm);
> > > > > > > > >  const struct image_format_info *image_format_drm_lookup(u32 drm);
> > > > > > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> > > > > > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
> > > > > > > > >  unsigned int image_format_plane_cpp(const struct image_format_info *format,
> > > > > > > > >  				    int plane);
> > > > > > > > >  unsigned int image_format_plane_width(int width,
> > > > > > > > > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > > > > > > > > index 9b9a73220c5d..39f1d38ae861 100644
> > > > > > > > > --- a/lib/image-formats.c
> > > > > > > > > +++ b/lib/image-formats.c
> > > > > > > > > @@ -8,6 +8,7 @@
> > > > > > > > >  static const struct image_format_info formats[] = {
> > > > > > > > >  	{
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_C8,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
> > > > > > > > >  		.depth = 8,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > > > @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.vsub = 1,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB332,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
> > > > > > > > >  		.depth = 8,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > > > @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.vsub = 1,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB4444,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.vsub = 1,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB4444,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.has_alpha = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB1555,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> > > > > > > > >  		.depth = 15,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.vsub = 1,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB1555,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> > > > > > > > >  		.depth = 15,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.has_alpha = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB565,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> > > > > > > > >  		.depth = 16,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.vsub = 1,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB888,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> > > > > > > > >  		.depth = 24,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > > > @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.vsub = 1,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_BGR888,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> > > > > > > > >  		.depth = 24,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > > > @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.vsub = 1,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB8888,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> > > > > > > > 
> > > > > > > > All RGB mapping should be surrounded by ifdef, because many (not all)
> > > > > > > > DRM formats represent the order of component when placed in a CPU
> > > > > > > > register, unlike V4L2 which uses memory order. I've pick this one
> > > > > > > 
> > > > > > > DRM formats are explicitly defined as little endian.
> > > > > > 
> > > > > > Yes, that means the same thing. The mapping has nothing to do with the
> > > > > > buffer bytes order, unlike V4L2 (and most streaming stack) do.
> > > > > 
> > > > > It has everything to do with byte order. Little endian means the least
> > > > > significant byte is stored in the first byte in memory.
> > > > > 
> > > > > Based on https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/pixfmt-packed-rgb.html
> > > > > drm XRGB888 is actually v4l XBGR32, not XRGB32 as claimed by this patch.
> > > > 
> > > > That's basically what I said, as it's define for Little Endian rather
> > > > then buffer order, you have to make the mapping conditional. It
> > > > basically mean that in memory, the DRM format physically differ
> > > > depending on CPU endian.
> > > 
> > > No. It is always little endian no matter what the CPU is.
> > 
> > I'm sorry, this is in your ABI, we don't add layers of ifdef in
> > userspace code just for the fun of it. If you redefine this now you are
> > breaking userspace. I agree there is very little to no Big Endian on
> > DRM side anymore, but what historically was mapped per CPU cannot be
> > changed by you now.
> 
> It was always little endian.

I'm not sure what it's worth, but there is a "pixel format guide"
project that is all about matching formats from one API to another: 
https://afrantzis.com/pixel-format-guide/ (and it has an associated
tool too).

On the page about DRM, it seems that they got the word that DRM formats
are the native pixel order in little-endian systems:
https://afrantzis.com/pixel-format-guide/drm.html

Perhaps some drivers have been abusing the format definitions, leading
to the inconsistencies that Nicolas could witness?

Cheers,

Paul

> > > > Last time we have run this on PPC / Big
> > > > Endian, the mapping was XRGB/XRGB, we checked that up multiple time
> > > > with the DRM maintainers and was told this is exactly what it's suppose
> > > > to do, hence this endian dependant mapping all over the place. If you
> > > > make up that this isn't right, you are breaking userspace, and people
> > > > don't like that.
> > > 
> > > Someone recently added those DRM_FORMAT_HOST variants to allow
> > > the legacy addfb1 to create pick the format based on host
> > > endianness. I thought that was the only conclusion from the
> > > little vs. big endian drm fourcc wars.
> > > 
> > > > So the mapping should be:
> > > > Little: DRM XRGB / V4L2 XBGR
> > > > Big:    DRM XRGB / V4L2 XRGB
> > > > 
> > > > > > > > randomly, but this one on most system, little endian, will match
> > > > > > > > V4L2_PIX_FMT_XBGR32. This type of complex mapping can be found in
> > > > > > > > multiple places, notably in GStreamer:
> > > > > > > > 
> > > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/sys/kms/gstkmsutils.c#L45
> > > > > > > > 
> > > > > > > > >  		.depth = 24,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 4, 0, 0 },
> > > > > > > > > @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.has_alpha = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB8888,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
> > > > > > > > >  		.depth = 32,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 4, 0, 0 },
> > > > > > > > > @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.has_alpha = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV410,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU410,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV420,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU420,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV422,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU422,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YUV444,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YVU444,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 3,
> > > > > > > > >  		.cpp = { 1, 1, 1 },
> > > > > > > > > @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_NV12,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 2,
> > > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > > @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_NV21,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 2,
> > > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > > @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_NV16,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 2,
> > > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > > @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_NV61,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 2,
> > > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > > @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_NV24,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 2,
> > > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > > @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_NV42,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 2,
> > > > > > > > >  		.cpp = { 1, 2, 0 },
> > > > > > > > > @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YUYV,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_YVYU,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_UYVY,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > >  		.is_yuv = true,
> > > > > > > > >  	}, {
> > > > > > > > >  		.drm_fmt = DRM_FORMAT_VYUY,
> > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
> > > > > > > > >  		.depth = 0,
> > > > > > > > >  		.num_planes = 1,
> > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
> > > > > > > > >  EXPORT_SYMBOL(image_format_drm_lookup);
> > > > > > > > >  
> > > > > > > > >  /**
> > > > > > > > > + * __image_format_v4l2_lookup - query information for a given format
> > > > > > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > > > > > + *
> > > > > > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > > > > > + *
> > > > > > > > > + * Returns:
> > > > > > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > > > > > + * NULL if the format is unsupported.
> > > > > > > > > + */
> > > > > > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> > > > > > > > > +{
> > > > > > > > > +	return __image_format_lookup(v4l2_fmt, v4l2);
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * image_format_v4l2_lookup - query information for a given format
> > > > > > > > > + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> > > > > > > > > + *
> > > > > > > > > + * The caller should only pass a supported pixel format to this function.
> > > > > > > > > + * Unsupported pixel formats will generate a warning in the kernel log.
> > > > > > > > > + *
> > > > > > > > > + * Returns:
> > > > > > > > > + * The instance of struct image_format_info that describes the pixel format, or
> > > > > > > > > + * NULL if the format is unsupported.
> > > > > > > > > + */
> > > > > > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> > > > > > > > > +{
> > > > > > > > > +	const struct image_format_info *format;
> > > > > > > > > +
> > > > > > > > > +	format = __image_format_v4l2_lookup(v4l2);
> > > > > > > > > +
> > > > > > > > > +	WARN_ON(!format);
> > > > > > > > > +	return format;
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL(image_format_v4l2_lookup);
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > >   * image_format_plane_cpp - determine the bytes per pixel value
> > > > > > > > >   * @format: pointer to the image_format
> > > > > > > > >   * @plane: plane index
> > > > > > > > 
> > > > > > > > _______________________________________________
> > > > > > > > dri-devel mailing list
> > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
>

Ville Syrjälä March 21, 2019, 4:35 p.m. UTC | #10

On Thu, Mar 21, 2019 at 05:04:19PM +0100, Paul Kocialkowski wrote:
> Hi,
> 
> Le mercredi 20 mars 2019 à 20:39 +0200, Ville Syrjälä a écrit :
> > On Wed, Mar 20, 2019 at 02:27:59PM -0400, Nicolas Dufresne wrote:
> > > Le mercredi 20 mars 2019 à 18:41 +0200, Ville Syrjälä a écrit :
> > > > On Wed, Mar 20, 2019 at 12:30:25PM -0400, Nicolas Dufresne wrote:
> > > > > Le mercredi 20 mars 2019 à 18:09 +0200, Ville Syrjälä a écrit :
> > > > > > On Wed, Mar 20, 2019 at 11:51:58AM -0400, Nicolas Dufresne wrote:
> > > > > > > Le mercredi 20 mars 2019 à 16:27 +0200, Ville Syrjälä a écrit :
> > > > > > > > On Tue, Mar 19, 2019 at 07:29:18PM -0400, Nicolas Dufresne wrote:
> > > > > > > > > Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > > > > > > > > > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > > > > > > > > > For now, let's add the v4l2 fourcc's for the already existing formats.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > > > > > > ---
> > > > > > > > > >  include/linux/image-formats.h |  9 +++++-
> > > > > > > > > >  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
> > > > > > > > > >  2 files changed, 76 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> > > > > > > > > > index 53fd73a71b3d..fbc3a4501ebd 100644
> > > > > > > > > > --- a/include/linux/image-formats.h
> > > > > > > > > > +++ b/include/linux/image-formats.h
> > > > > > > > > > @@ -26,6 +26,13 @@ struct image_format_info {
> > > > > > > > > >  	};
> > > > > > > > > >  
> > > > > > > > > >  	/**
> > > > > > > > > > +	 * @v4l2_fmt:
> > > > > > > > > > +	 *
> > > > > > > > > > +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> > > > > > > > > > +	 */
> > > > > > > > > > +	u32 v4l2_fmt;
> > > > > > > > > > +
> > > > > > > > > > +	/**
> > > > > > > > > >  	 * @depth:
> > > > > > > > > >  	 *
> > > > > > > > > >  	 * Color depth (number of bits per pixel excluding padding bits),
> > > > > > > > > > @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
> > > > > > > > > >  
> > > > > > > > > >  const struct image_format_info *__image_format_drm_lookup(u32 drm);
> > > > > > > > > >  const struct image_format_info *image_format_drm_lookup(u32 drm);
> > > > > > > > > > +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> > > > > > > > > > +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
> > > > > > > > > >  unsigned int image_format_plane_cpp(const struct image_format_info *format,
> > > > > > > > > >  				    int plane);
> > > > > > > > > >  unsigned int image_format_plane_width(int width,
> > > > > > > > > > diff --git a/lib/image-formats.c b/lib/image-formats.c
> > > > > > > > > > index 9b9a73220c5d..39f1d38ae861 100644
> > > > > > > > > > --- a/lib/image-formats.c
> > > > > > > > > > +++ b/lib/image-formats.c
> > > > > > > > > > @@ -8,6 +8,7 @@
> > > > > > > > > >  static const struct image_format_info formats[] = {
> > > > > > > > > >  	{
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_C8,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
> > > > > > > > > >  		.depth = 8,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > > > > @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.vsub = 1,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB332,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
> > > > > > > > > >  		.depth = 8,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 1, 0, 0 },
> > > > > > > > > > @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.vsub = 1,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB4444,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> > > > > > > > > >  		.depth = 0,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > > @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.vsub = 1,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB4444,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> > > > > > > > > >  		.depth = 0,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > > @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.has_alpha = true,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB1555,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> > > > > > > > > >  		.depth = 15,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > > @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.vsub = 1,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_ARGB1555,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> > > > > > > > > >  		.depth = 15,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > > @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.has_alpha = true,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB565,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> > > > > > > > > >  		.depth = 16,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 2, 0, 0 },
> > > > > > > > > > @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.vsub = 1,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_RGB888,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> > > > > > > > > >  		.depth = 24,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > > > > @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.vsub = 1,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_BGR888,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> > > > > > > > > >  		.depth = 24,
> > > > > > > > > >  		.num_planes = 1,
> > > > > > > > > >  		.cpp = { 3, 0, 0 },
> > > > > > > > > > @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
> > > > > > > > > >  		.vsub = 1,
> > > > > > > > > >  	}, {
> > > > > > > > > >  		.drm_fmt = DRM_FORMAT_XRGB8888,
> > > > > > > > > > +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> > > > > > > > > 
> > > > > > > > > All RGB mapping should be surrounded by ifdef, because many (not all)
> > > > > > > > > DRM formats represent the order of component when placed in a CPU
> > > > > > > > > register, unlike V4L2 which uses memory order. I've pick this one
> > > > > > > > 
> > > > > > > > DRM formats are explicitly defined as little endian.
> > > > > > > 
> > > > > > > Yes, that means the same thing. The mapping has nothing to do with the
> > > > > > > buffer bytes order, unlike V4L2 (and most streaming stack) do.
> > > > > > 
> > > > > > It has everything to do with byte order. Little endian means the least
> > > > > > significant byte is stored in the first byte in memory.
> > > > > > 
> > > > > > Based on https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/pixfmt-packed-rgb.html
> > > > > > drm XRGB888 is actually v4l XBGR32, not XRGB32 as claimed by this patch.
> > > > > 
> > > > > That's basically what I said, as it's define for Little Endian rather
> > > > > then buffer order, you have to make the mapping conditional. It
> > > > > basically mean that in memory, the DRM format physically differ
> > > > > depending on CPU endian.
> > > > 
> > > > No. It is always little endian no matter what the CPU is.
> > > 
> > > I'm sorry, this is in your ABI, we don't add layers of ifdef in
> > > userspace code just for the fun of it. If you redefine this now you are
> > > breaking userspace. I agree there is very little to no Big Endian on
> > > DRM side anymore, but what historically was mapped per CPU cannot be
> > > changed by you now.
> > 
> > It was always little endian.
> 
> I'm not sure what it's worth, but there is a "pixel format guide"
> project that is all about matching formats from one API to another: 
> https://afrantzis.com/pixel-format-guide/ (and it has an associated
> tool too).
> 
> On the page about DRM, it seems that they got the word that DRM formats
> are the native pixel order in little-endian systems:
> https://afrantzis.com/pixel-format-guide/drm.html

Looks consistent with the official word in drm_fourcc.h.

$ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
Format: V4L2_PIX_FMT_XBGR32
Is compatible on all systems with:
        DRM_FORMAT_XRGB8888
Is compatible on little-endian systems with:
Is compatible on big-endian systems with:

$ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
Format: DRM_FORMAT_XRGB8888
Is compatible on all systems with:
        V4L2_PIX_FMT_XBGR32
Is compatible on little-endian systems with:
Is compatible on big-endian systems with:

Even works both ways :)

> 
> Perhaps some drivers have been abusing the format definitions, leading
> to the inconsistencies that Nicolas could witness?

That is quite possible, perhaps even likely. No one really
seems interested in making sure big endian systems actually
work properly. I believe the usual approach is to hack
around semi-rnadomly until the correct colors accidentally
appear on the screen.

Nicolas Dufresne March 21, 2019, 7:14 p.m. UTC | #11

Le jeudi 21 mars 2019 à 18:35 +0200, Ville Syrjälä a écrit :
> > I'm not sure what it's worth, but there is a "pixel format guide"
> > project that is all about matching formats from one API to another: 
> > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > tool too).
> > 
> > On the page about DRM, it seems that they got the word that DRM formats
> > are the native pixel order in little-endian systems:
> > https://afrantzis.com/pixel-format-guide/drm.html
> 
> Looks consistent with the official word in drm_fourcc.h.
> 
> $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> Format: V4L2_PIX_FMT_XBGR32
> Is compatible on all systems with:
>         DRM_FORMAT_XRGB8888
> Is compatible on little-endian systems with:
> Is compatible on big-endian systems with:
> 
> $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> Format: DRM_FORMAT_XRGB8888
> Is compatible on all systems with:
>         V4L2_PIX_FMT_XBGR32
> Is compatible on little-endian systems with:
> Is compatible on big-endian systems with:
> 
> Even works both ways :)
> 
> > Perhaps some drivers have been abusing the format definitions, leading
> > to the inconsistencies that Nicolas could witness?
> 
> That is quite possible, perhaps even likely. No one really
> seems interested in making sure big endian systems actually
> work properly. I believe the usual approach is to hack
> around semi-rnadomly until the correct colors accidentally
> appear on the screen.

We did not hack around randomly. The code in GStreamer is exactly what
the DRM and Wayland dev told us to do (they provided the initial
patches to make it work). These are initially patches from Intel for
what it's worth (see the wlvideoformat.c header [0]). Even though I
just notice that in this file, it seems that we ended up with a
different mapping order for WL and DRM format in 24bit RGB (no
padding), clearly is a bug. That being said, there is no logical
meaning for little endian 24bit RGB, you can't load a pixel on CPU in a
single op. Just saying since I would not know which one of the two
mapping here is right. I would probably have to go testing what DRM
drivers do, which may not mean anything. I would also ask and get
contradictory answers.

I have never myself tested these on big endian drivers though, as you
say, nobody seems to care about graphics on those anymore. So the easy
statement is to say they are little endian, like you just did, and
ignore the legacy, but there is some catch. YUV formats has been added
without applying this rules. So V4L2 YUYV match YUYV in DRM format name
instead of little endian UYVY. (at least 4 tested drivers implements it
this way). Same for NV12, for which little endian CPU representation
would swap the UV paid on a 16bit word.

To me, all the YUV stuff is the right way, because this is what the
rest of the world is doing, it's not ambiguous.

[0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/ext/wayland/wlvideoformat.c#L86



Nicolas

Ville Syrjälä March 21, 2019, 9:44 p.m. UTC | #12

On Thu, Mar 21, 2019 at 03:14:06PM -0400, Nicolas Dufresne wrote:
> Le jeudi 21 mars 2019 à 18:35 +0200, Ville Syrjälä a écrit :
> > > I'm not sure what it's worth, but there is a "pixel format guide"
> > > project that is all about matching formats from one API to another: 
> > > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > > tool too).
> > > 
> > > On the page about DRM, it seems that they got the word that DRM formats
> > > are the native pixel order in little-endian systems:
> > > https://afrantzis.com/pixel-format-guide/drm.html
> > 
> > Looks consistent with the official word in drm_fourcc.h.
> > 
> > $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> > Format: V4L2_PIX_FMT_XBGR32
> > Is compatible on all systems with:
> >         DRM_FORMAT_XRGB8888
> > Is compatible on little-endian systems with:
> > Is compatible on big-endian systems with:
> > 
> > $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> > Format: DRM_FORMAT_XRGB8888
> > Is compatible on all systems with:
> >         V4L2_PIX_FMT_XBGR32
> > Is compatible on little-endian systems with:
> > Is compatible on big-endian systems with:
> > 
> > Even works both ways :)
> > 
> > > Perhaps some drivers have been abusing the format definitions, leading
> > > to the inconsistencies that Nicolas could witness?
> > 
> > That is quite possible, perhaps even likely. No one really
> > seems interested in making sure big endian systems actually
> > work properly. I believe the usual approach is to hack
> > around semi-rnadomly until the correct colors accidentally
> > appear on the screen.
> 
> We did not hack around randomly. The code in GStreamer is exactly what
> the DRM and Wayland dev told us to do (they provided the initial
> patches to make it work). These are initially patches from Intel for
> what it's worth (see the wlvideoformat.c header [0]). Even though I
> just notice that in this file, it seems that we ended up with a
> different mapping order for WL and DRM format in 24bit RGB (no
> padding), clearly is a bug. That being said, there is no logical
> meaning for little endian 24bit RGB, you can't load a pixel on CPU in a
> single op.

To me little endian just means "little end comes first". So if
you think of things as just a stream of bytes CPU word size
etc. doesn't matter. And I guess if you really wanted to you
could even make a CPU with 24bit word size. 

Anyways, to make things more clear drm_fourcc.h could probably
document things better by showing explicitly which bits go into
which byte.

I don't know who did what patches for whatever project, but
clearly something has been lost in translation at some point.

> Just saying since I would not know which one of the two
> mapping here is right. I would probably have to go testing what DRM
> drivers do, which may not mean anything. I would also ask and get
> contradictory answers.
> 
> I have never myself tested these on big endian drivers though, as you
> say, nobody seems to care about graphics on those anymore. So the easy
> statement is to say they are little endian, like you just did, and
> ignore the legacy, but there is some catch. YUV formats has been added
> without applying this rules.

All drm formats follow the same rule (ignoring the recently added
non-byte aligned stuff which I guess don't really follow any rules).

> So V4L2 YUYV match YUYV in DRM format name
> instead of little endian UYVY. (at least 4 tested drivers implements it
> this way). Same for NV12, for which little endian CPU representation
> would swap the UV paid on a 16bit word.

DRM NV12 and YUYV (YUY2) match the NV12 and YUY2 defined here
https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering

> 
> To me, all the YUV stuff is the right way, because this is what the
> rest of the world is doing, it's not ambiguous.
> 
> [0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/ext/wayland/wlvideoformat.c#L86
> 
> 
> 
> Nicolas

Ville Syrjälä March 22, 2019, 2:42 p.m. UTC | #13

On Thu, Mar 21, 2019 at 03:14:06PM -0400, Nicolas Dufresne wrote:
> Le jeudi 21 mars 2019 à 18:35 +0200, Ville Syrjälä a écrit :
> > > I'm not sure what it's worth, but there is a "pixel format guide"
> > > project that is all about matching formats from one API to another: 
> > > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > > tool too).
> > > 
> > > On the page about DRM, it seems that they got the word that DRM formats
> > > are the native pixel order in little-endian systems:
> > > https://afrantzis.com/pixel-format-guide/drm.html
> > 
> > Looks consistent with the official word in drm_fourcc.h.
> > 
> > $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> > Format: V4L2_PIX_FMT_XBGR32
> > Is compatible on all systems with:
> >         DRM_FORMAT_XRGB8888
> > Is compatible on little-endian systems with:
> > Is compatible on big-endian systems with:
> > 
> > $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> > Format: DRM_FORMAT_XRGB8888
> > Is compatible on all systems with:
> >         V4L2_PIX_FMT_XBGR32
> > Is compatible on little-endian systems with:
> > Is compatible on big-endian systems with:
> > 
> > Even works both ways :)
> > 
> > > Perhaps some drivers have been abusing the format definitions, leading
> > > to the inconsistencies that Nicolas could witness?
> > 
> > That is quite possible, perhaps even likely. No one really
> > seems interested in making sure big endian systems actually
> > work properly. I believe the usual approach is to hack
> > around semi-rnadomly until the correct colors accidentally
> > appear on the screen.
> 
> We did not hack around randomly.

BTW I didn't mean to imply it was you who hacked around randomly.
Sorry if you got that impression.

What I was trying to convey is the following sequence of events:
1. random person X gets their hand on a big endian machine for
   a while
2. colors are wrong
3. they hack stuff until the colors are correct in their
   current use case
4. they move on to more interesting things

Nicolas Dufresne March 22, 2019, 6:11 p.m. UTC | #14

Le vendredi 22 mars 2019 à 16:42 +0200, Ville Syrjälä a écrit :
> On Thu, Mar 21, 2019 at 03:14:06PM -0400, Nicolas Dufresne wrote:
> > Le jeudi 21 mars 2019 à 18:35 +0200, Ville Syrjälä a écrit :
> > > > I'm not sure what it's worth, but there is a "pixel format guide"
> > > > project that is all about matching formats from one API to another: 
> > > > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > > > tool too).
> > > > 
> > > > On the page about DRM, it seems that they got the word that DRM formats
> > > > are the native pixel order in little-endian systems:
> > > > https://afrantzis.com/pixel-format-guide/drm.html
> > > 
> > > Looks consistent with the official word in drm_fourcc.h.
> > > 
> > > $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> > > Format: V4L2_PIX_FMT_XBGR32
> > > Is compatible on all systems with:
> > >         DRM_FORMAT_XRGB8888
> > > Is compatible on little-endian systems with:
> > > Is compatible on big-endian systems with:
> > > 
> > > $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> > > Format: DRM_FORMAT_XRGB8888
> > > Is compatible on all systems with:
> > >         V4L2_PIX_FMT_XBGR32
> > > Is compatible on little-endian systems with:
> > > Is compatible on big-endian systems with:
> > > 
> > > Even works both ways :)
> > > 
> > > > Perhaps some drivers have been abusing the format definitions, leading
> > > > to the inconsistencies that Nicolas could witness?
> > > 
> > > That is quite possible, perhaps even likely. No one really
> > > seems interested in making sure big endian systems actually
> > > work properly. I believe the usual approach is to hack
> > > around semi-rnadomly until the correct colors accidentally
> > > appear on the screen.
> > 
> > We did not hack around randomly.
> 
> BTW I didn't mean to imply it was you who hacked around randomly.
> Sorry if you got that impression.
> 
> What I was trying to convey is the following sequence of events:
> 1. random person X gets their hand on a big endian machine for
>    a while
> 2. colors are wrong
> 3. they hack stuff until the colors are correct in their
>    current use case
> 4. they move on to more interesting things

Thanks for clarification.

Nicolas

Nicolas Dufresne March 22, 2019, 6:24 p.m. UTC | #15

Le jeudi 21 mars 2019 à 23:44 +0200, Ville Syrjälä a écrit :
> On Thu, Mar 21, 2019 at 03:14:06PM -0400, Nicolas Dufresne wrote:
> > Le jeudi 21 mars 2019 à 18:35 +0200, Ville Syrjälä a écrit :
> > > > I'm not sure what it's worth, but there is a "pixel format guide"
> > > > project that is all about matching formats from one API to another: 
> > > > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > > > tool too).
> > > > 
> > > > On the page about DRM, it seems that they got the word that DRM formats
> > > > are the native pixel order in little-endian systems:
> > > > https://afrantzis.com/pixel-format-guide/drm.html
> > > 
> > > Looks consistent with the official word in drm_fourcc.h.
> > > 
> > > $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> > > Format: V4L2_PIX_FMT_XBGR32
> > > Is compatible on all systems with:
> > >         DRM_FORMAT_XRGB8888
> > > Is compatible on little-endian systems with:
> > > Is compatible on big-endian systems with:
> > > 
> > > $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> > > Format: DRM_FORMAT_XRGB8888
> > > Is compatible on all systems with:
> > >         V4L2_PIX_FMT_XBGR32
> > > Is compatible on little-endian systems with:
> > > Is compatible on big-endian systems with:
> > > 
> > > Even works both ways :)
> > > 
> > > > Perhaps some drivers have been abusing the format definitions, leading
> > > > to the inconsistencies that Nicolas could witness?
> > > 
> > > That is quite possible, perhaps even likely. No one really
> > > seems interested in making sure big endian systems actually
> > > work properly. I believe the usual approach is to hack
> > > around semi-rnadomly until the correct colors accidentally
> > > appear on the screen.
> > 
> > We did not hack around randomly. The code in GStreamer is exactly what
> > the DRM and Wayland dev told us to do (they provided the initial
> > patches to make it work). These are initially patches from Intel for
> > what it's worth (see the wlvideoformat.c header [0]). Even though I
> > just notice that in this file, it seems that we ended up with a
> > different mapping order for WL and DRM format in 24bit RGB (no
> > padding), clearly is a bug. That being said, there is no logical
> > meaning for little endian 24bit RGB, you can't load a pixel on CPU in a
> > single op.
> 
> To me little endian just means "little end comes first". So if
> you think of things as just a stream of bytes CPU word size
> etc. doesn't matter. And I guess if you really wanted to you
> could even make a CPU with 24bit word size. 
> 
> Anyways, to make things more clear drm_fourcc.h could probably
> document things better by showing explicitly which bits go into
> which byte.
> 
> I don't know who did what patches for whatever project, but
> clearly something has been lost in translation at some point.
> 
> > Just saying since I would not know which one of the two
> > mapping here is right. I would probably have to go testing what DRM
> > drivers do, which may not mean anything. I would also ask and get
> > contradictory answers.
> > 
> > I have never myself tested these on big endian drivers though, as you
> > say, nobody seems to care about graphics on those anymore. So the easy
> > statement is to say they are little endian, like you just did, and
> > ignore the legacy, but there is some catch. YUV formats has been added
> > without applying this rules.
> 
> All drm formats follow the same rule (ignoring the recently added
> non-byte aligned stuff which I guess don't really follow any rules).
> 
> > So V4L2 YUYV match YUYV in DRM format name
> > instead of little endian UYVY. (at least 4 tested drivers implements it
> > this way). Same for NV12, for which little endian CPU representation
> > would swap the UV paid on a 16bit word.
> 
> DRM NV12 and YUYV (YUY2) match the NV12 and YUY2 defined here
> https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering

Problem is that these are all expressed MSB first like (the way V4L2
and GStreamer do appart for the padding X, which is usually first in
V4L2). Let's say you have 2 pixels of YUYV in a 32bit buffer.

   buf[0] = Y
   buf[1] = U
   buf[2] = Y
   buf[3] = V

While with LSB first (was this what you wanted to say ?), this format
would be VYUY as:

  buf[0] = V
  buf[1] = Y
  buf[2] = U
  buf[3] = Y

But I tested this format on i915, xlnx, msm and imx drm drivers, and
they are all mapped as MSB. The LSB version of NV12 is called NV21 in
MS pixel formats. It would be really confusing if the LSB rule was to
be applied to these format in my opinion, because the names don't
explicitly express the placement for the components.

Anyway, to cut short, as per currently tested implementation of DRM
driver, we can keep the RGB mapping static (reversed) for now, but for
Maxime, who probably have no clue about all this, the YUYV mapping is
correct in this patch (in as of currently tested drivers).

> 
> > To me, all the YUV stuff is the right way, because this is what the
> > rest of the world is doing, it's not ambiguous.
> > 
> > [0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/ext/wayland/wlvideoformat.c#L86
> > 
> > 
> > 
> > Nicolas

Ville Syrjälä March 22, 2019, 6:44 p.m. UTC | #16

On Fri, Mar 22, 2019 at 02:24:29PM -0400, Nicolas Dufresne wrote:
> Le jeudi 21 mars 2019 à 23:44 +0200, Ville Syrjälä a écrit :
> > On Thu, Mar 21, 2019 at 03:14:06PM -0400, Nicolas Dufresne wrote:
> > > Le jeudi 21 mars 2019 à 18:35 +0200, Ville Syrjälä a écrit :
> > > > > I'm not sure what it's worth, but there is a "pixel format guide"
> > > > > project that is all about matching formats from one API to another: 
> > > > > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > > > > tool too).
> > > > > 
> > > > > On the page about DRM, it seems that they got the word that DRM formats
> > > > > are the native pixel order in little-endian systems:
> > > > > https://afrantzis.com/pixel-format-guide/drm.html
> > > > 
> > > > Looks consistent with the official word in drm_fourcc.h.
> > > > 
> > > > $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> > > > Format: V4L2_PIX_FMT_XBGR32
> > > > Is compatible on all systems with:
> > > >         DRM_FORMAT_XRGB8888
> > > > Is compatible on little-endian systems with:
> > > > Is compatible on big-endian systems with:
> > > > 
> > > > $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> > > > Format: DRM_FORMAT_XRGB8888
> > > > Is compatible on all systems with:
> > > >         V4L2_PIX_FMT_XBGR32
> > > > Is compatible on little-endian systems with:
> > > > Is compatible on big-endian systems with:
> > > > 
> > > > Even works both ways :)
> > > > 
> > > > > Perhaps some drivers have been abusing the format definitions, leading
> > > > > to the inconsistencies that Nicolas could witness?
> > > > 
> > > > That is quite possible, perhaps even likely. No one really
> > > > seems interested in making sure big endian systems actually
> > > > work properly. I believe the usual approach is to hack
> > > > around semi-rnadomly until the correct colors accidentally
> > > > appear on the screen.
> > > 
> > > We did not hack around randomly. The code in GStreamer is exactly what
> > > the DRM and Wayland dev told us to do (they provided the initial
> > > patches to make it work). These are initially patches from Intel for
> > > what it's worth (see the wlvideoformat.c header [0]). Even though I
> > > just notice that in this file, it seems that we ended up with a
> > > different mapping order for WL and DRM format in 24bit RGB (no
> > > padding), clearly is a bug. That being said, there is no logical
> > > meaning for little endian 24bit RGB, you can't load a pixel on CPU in a
> > > single op.
> > 
> > To me little endian just means "little end comes first". So if
> > you think of things as just a stream of bytes CPU word size
> > etc. doesn't matter. And I guess if you really wanted to you
> > could even make a CPU with 24bit word size. 
> > 
> > Anyways, to make things more clear drm_fourcc.h could probably
> > document things better by showing explicitly which bits go into
> > which byte.
> > 
> > I don't know who did what patches for whatever project, but
> > clearly something has been lost in translation at some point.
> > 
> > > Just saying since I would not know which one of the two
> > > mapping here is right. I would probably have to go testing what DRM
> > > drivers do, which may not mean anything. I would also ask and get
> > > contradictory answers.
> > > 
> > > I have never myself tested these on big endian drivers though, as you
> > > say, nobody seems to care about graphics on those anymore. So the easy
> > > statement is to say they are little endian, like you just did, and
> > > ignore the legacy, but there is some catch. YUV formats has been added
> > > without applying this rules.
> > 
> > All drm formats follow the same rule (ignoring the recently added
> > non-byte aligned stuff which I guess don't really follow any rules).
> > 
> > > So V4L2 YUYV match YUYV in DRM format name
> > > instead of little endian UYVY. (at least 4 tested drivers implements it
> > > this way). Same for NV12, for which little endian CPU representation
> > > would swap the UV paid on a 16bit word.
> > 
> > DRM NV12 and YUYV (YUY2) match the NV12 and YUY2 defined here
> > https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering
> 
> Problem is that these are all expressed MSB first like (the way V4L2
> and GStreamer do appart for the padding X, which is usually first in
> V4L2). Let's say you have 2 pixels of YUYV in a 32bit buffer.
> 
>    buf[0] = Y
>    buf[1] = U
>    buf[2] = Y
>    buf[3] = V

This is drm YUYV (aka. YUY2). Well, assuming buf[] is made up of bytes.

> 
> While with LSB first (was this what you wanted to say ?), this format
> would be VYUY as:
> 
>   buf[0] = V
>   buf[1] = Y
>   buf[2] = U
>   buf[3] = Y

This is VYUY as far as drm is concerned.

> 
> But I tested this format on i915, xlnx, msm and imx drm drivers, and
> they are all mapped as MSB. The LSB version of NV12 is called NV21 in
> MS pixel formats. It would be really confusing if the LSB rule was to
> be applied to these format in my opinion, because the names don't
> explicitly express the placement for the components.

The names don't really mean anything. Don't try to give any any special
meaning. They're just that, names. The fact that we used fourccs for
them was a mistake IMO. IIRC I objected but ended up going with them
to get the bikeshed painted at all.

And yes, the fact that we used a different naming scheme for packed YUV
vs. RGB was probably a mistake by me. But it was done long ago and we
have to live with it. Fortunately the mess has gotten much worse since
then and we are even more inconsistent now. We now YUV formats where
the A vs. X variants use totally different naming schemes.

> 
> Anyway, to cut short, as per currently tested implementation of DRM
> driver, we can keep the RGB mapping static (reversed) for now, but for
> Maxime, who probably have no clue about all this, the YUYV mapping is
> correct in this patch (in as of currently tested drivers).
> 
> > 
> > > To me, all the YUV stuff is the right way, because this is what the
> > > rest of the world is doing, it's not ambiguous.
> > > 
> > > [0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/ext/wayland/wlvideoformat.c#L86
> > > 
> > > 
> > > 
> > > Nicolas

Nicolas Dufresne March 22, 2019, 7:25 p.m. UTC | #17

Le vendredi 22 mars 2019 à 20:44 +0200, Ville Syrjälä a écrit :
> On Fri, Mar 22, 2019 at 02:24:29PM -0400, Nicolas Dufresne wrote:
> > Le jeudi 21 mars 2019 à 23:44 +0200, Ville Syrjälä a écrit :
> > > On Thu, Mar 21, 2019 at 03:14:06PM -0400, Nicolas Dufresne wrote:
> > > > Le jeudi 21 mars 2019 à 18:35 +0200, Ville Syrjälä a écrit :
> > > > > > I'm not sure what it's worth, but there is a "pixel format guide"
> > > > > > project that is all about matching formats from one API to another: 
> > > > > > https://afrantzis.com/pixel-format-guide/ (and it has an associated
> > > > > > tool too).
> > > > > > 
> > > > > > On the page about DRM, it seems that they got the word that DRM formats
> > > > > > are the native pixel order in little-endian systems:
> > > > > > https://afrantzis.com/pixel-format-guide/drm.html
> > > > > 
> > > > > Looks consistent with the official word in drm_fourcc.h.
> > > > > 
> > > > > $ python3 -m pfg find-compatible V4L2_PIX_FMT_XBGR32 drm
> > > > > Format: V4L2_PIX_FMT_XBGR32
> > > > > Is compatible on all systems with:
> > > > >         DRM_FORMAT_XRGB8888
> > > > > Is compatible on little-endian systems with:
> > > > > Is compatible on big-endian systems with:
> > > > > 
> > > > > $ python3 -m pfg find-compatible DRM_FORMAT_XRGB8888 v4l2
> > > > > Format: DRM_FORMAT_XRGB8888
> > > > > Is compatible on all systems with:
> > > > >         V4L2_PIX_FMT_XBGR32
> > > > > Is compatible on little-endian systems with:
> > > > > Is compatible on big-endian systems with:
> > > > > 
> > > > > Even works both ways :)
> > > > > 
> > > > > > Perhaps some drivers have been abusing the format definitions, leading
> > > > > > to the inconsistencies that Nicolas could witness?
> > > > > 
> > > > > That is quite possible, perhaps even likely. No one really
> > > > > seems interested in making sure big endian systems actually
> > > > > work properly. I believe the usual approach is to hack
> > > > > around semi-rnadomly until the correct colors accidentally
> > > > > appear on the screen.
> > > > 
> > > > We did not hack around randomly. The code in GStreamer is exactly what
> > > > the DRM and Wayland dev told us to do (they provided the initial
> > > > patches to make it work). These are initially patches from Intel for
> > > > what it's worth (see the wlvideoformat.c header [0]). Even though I
> > > > just notice that in this file, it seems that we ended up with a
> > > > different mapping order for WL and DRM format in 24bit RGB (no
> > > > padding), clearly is a bug. That being said, there is no logical
> > > > meaning for little endian 24bit RGB, you can't load a pixel on CPU in a
> > > > single op.
> > > 
> > > To me little endian just means "little end comes first". So if
> > > you think of things as just a stream of bytes CPU word size
> > > etc. doesn't matter. And I guess if you really wanted to you
> > > could even make a CPU with 24bit word size. 
> > > 
> > > Anyways, to make things more clear drm_fourcc.h could probably
> > > document things better by showing explicitly which bits go into
> > > which byte.
> > > 
> > > I don't know who did what patches for whatever project, but
> > > clearly something has been lost in translation at some point.
> > > 
> > > > Just saying since I would not know which one of the two
> > > > mapping here is right. I would probably have to go testing what DRM
> > > > drivers do, which may not mean anything. I would also ask and get
> > > > contradictory answers.
> > > > 
> > > > I have never myself tested these on big endian drivers though, as you
> > > > say, nobody seems to care about graphics on those anymore. So the easy
> > > > statement is to say they are little endian, like you just did, and
> > > > ignore the legacy, but there is some catch. YUV formats has been added
> > > > without applying this rules.
> > > 
> > > All drm formats follow the same rule (ignoring the recently added
> > > non-byte aligned stuff which I guess don't really follow any rules).
> > > 
> > > > So V4L2 YUYV match YUYV in DRM format name
> > > > instead of little endian UYVY. (at least 4 tested drivers implements it
> > > > this way). Same for NV12, for which little endian CPU representation
> > > > would swap the UV paid on a 16bit word.
> > > 
> > > DRM NV12 and YUYV (YUY2) match the NV12 and YUY2 defined here
> > > https://docs.microsoft.com/en-us/windows/desktop/medfound/recommended-8-bit-yuv-formats-for-video-rendering
> > 
> > Problem is that these are all expressed MSB first like (the way V4L2
> > and GStreamer do appart for the padding X, which is usually first in
> > V4L2). Let's say you have 2 pixels of YUYV in a 32bit buffer.
> > 
> >    buf[0] = Y
> >    buf[1] = U
> >    buf[2] = Y
> >    buf[3] = V
> 
> This is drm YUYV (aka. YUY2). Well, assuming buf[] is made up of bytes.
> 
> > While with LSB first (was this what you wanted to say ?), this format
> > would be VYUY as:
> > 
> >   buf[0] = V
> >   buf[1] = Y
> >   buf[2] = U
> >   buf[3] = Y
> 
> This is VYUY as far as drm is concerned.
> 
> > But I tested this format on i915, xlnx, msm and imx drm drivers, and
> > they are all mapped as MSB. The LSB version of NV12 is called NV21 in
> > MS pixel formats. It would be really confusing if the LSB rule was to
> > be applied to these format in my opinion, because the names don't
> > explicitly express the placement for the components.
> 
> The names don't really mean anything. Don't try to give any any special
> meaning. They're just that, names. The fact that we used fourccs for
> them was a mistake IMO. IIRC I objected but ended up going with them
> to get the bikeshed painted at all.

I can only agree with you. Note, it was not obvious to me that when you
said the format are little endian you refered to the description next
to the format name inside drm_fourcc.h.

> 
> And yes, the fact that we used a different naming scheme for packed YUV
> vs. RGB was probably a mistake by me. But it was done long ago and we
> have to live with it. Fortunately the mess has gotten much worse since
> then and we are even more inconsistent now. We now YUV formats where
> the A vs. X variants use totally different naming schemes.

Ok, so now that we are set, I'll retake this patch and simply comment
next to each badly mapped format.

> 
> > Anyway, to cut short, as per currently tested implementation of DRM
> > driver, we can keep the RGB mapping static (reversed) for now, but for
> > Maxime, who probably have no clue about all this, the YUYV mapping is
> > correct in this patch (in as of currently tested drivers).
> > 
> > > > To me, all the YUV stuff is the right way, because this is what the
> > > > rest of the world is doing, it's not ambiguous.
> > > > 
> > > > [0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/blob/master/ext/wayland/wlvideoformat.c#L86
> > > > 
> > > > 
> > > > 
> > > > Nicolas
> 
>

Nicolas Dufresne March 22, 2019, 7:55 p.m. UTC | #18

Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> V4L2 uses different fourcc's than DRM, and has a different set of formats.
> For now, let's add the v4l2 fourcc's for the already existing formats.

Hopefully I get the fixup right this time, see inline.

> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  include/linux/image-formats.h |  9 +++++-
>  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 76 insertions(+)
> 
> diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> index 53fd73a71b3d..fbc3a4501ebd 100644
> --- a/include/linux/image-formats.h
> +++ b/include/linux/image-formats.h
> @@ -26,6 +26,13 @@ struct image_format_info {
>  	};
>  
>  	/**
> +	 * @v4l2_fmt:
> +	 *
> +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> +	 */
> +	u32 v4l2_fmt;
> +
> +	/**
>  	 * @depth:
>  	 *
>  	 * Color depth (number of bits per pixel excluding padding bits),
> @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
>  
>  const struct image_format_info *__image_format_drm_lookup(u32 drm);
>  const struct image_format_info *image_format_drm_lookup(u32 drm);
> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
>  unsigned int image_format_plane_cpp(const struct image_format_info *format,
>  				    int plane);
>  unsigned int image_format_plane_width(int width,
> diff --git a/lib/image-formats.c b/lib/image-formats.c
> index 9b9a73220c5d..39f1d38ae861 100644
> --- a/lib/image-formats.c
> +++ b/lib/image-formats.c
> @@ -8,6 +8,7 @@
>  static const struct image_format_info formats[] = {
>  	{
>  		.drm_fmt = DRM_FORMAT_C8,
> +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
>  		.depth = 8,
>  		.num_planes = 1,
>  		.cpp = { 1, 0, 0 },
> @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB332,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
>  		.depth = 8,
>  		.num_planes = 1,
>  		.cpp = { 1, 0, 0 },
> @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB4444,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,

Not completely sure, looks like in the V4L2 variant, the padding is on
the 4 MSB of the second byte, which makes it incompatible. There is no
other equivalent.

>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB4444,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,

Similarly.

>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB1555,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,

Same swapping, can't find a match.

>  		.depth = 15,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB1555,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,

Same.

>  		.depth = 15,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB565,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,

-> V4L2_PIX_FMT_RGB565X

Was added later, as what you expect is not compatible.

>  		.depth = 16,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB888,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,

-> V4L2_PIX_FMT_BGR24

>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 3, 0, 0 },
> @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_BGR888,
> +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,

-> V4L2_PIX_FMT_RGB24

>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 3, 0, 0 },
> @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB8888,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,

-> V4L2_PIX_FMT_XBGR32

>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 4, 0, 0 },
> @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB8888,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,

-> V4L2_PIX_FMT_ABGR32

>  		.depth = 32,
>  		.num_planes = 1,
>  		.cpp = { 4, 0, 0 },
> @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV410,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU410,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV420,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,

There is two valid matches in V4L2 for this format, not sure how this
will be handled. The M variant is to be used with MPLANE v4l2_buffer
when num_planes is bigger then 1.

>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU420,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,

Same.

>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV422,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,

Same.

>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU422,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,

Same. 

>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV444,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,

Same.

>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU444,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,

Same.

>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV12,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV12,

V4L2_PIX_FMT_NV12M is the mplane variant. Depending on how you handle,
which should be concistant picking one up.

>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV21,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV21,

Same, NV12M for mplane.

>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV16,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV16,

Same, NV16M.

>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV61,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV61,

Same, NV61M.

>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV24,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV24,

For extra fun, the M variant has not been added yet.

>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV42,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUYV,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVYU,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_UYVY,
> +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_VYUY,
> +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
>  EXPORT_SYMBOL(image_format_drm_lookup);
>  
>  /**
> + * __image_format_v4l2_lookup - query information for a given format
> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> +{
> +	return __image_format_lookup(v4l2_fmt, v4l2);
> +}
> +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> +
> +/**
> + * image_format_v4l2_lookup - query information for a given format
> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + * Unsupported pixel formats will generate a warning in the kernel log.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> +{
> +	const struct image_format_info *format;
> +
> +	format = __image_format_v4l2_lookup(v4l2);
> +
> +	WARN_ON(!format);
> +	return format;
> +}
> +EXPORT_SYMBOL(image_format_v4l2_lookup);
> +
> +/**
>   * image_format_plane_cpp - determine the bytes per pixel value
>   * @format: pointer to the image_format
>   * @plane: plane index

Maxime Ripard April 1, 2019, 2:44 p.m. UTC | #19

Hi Nicolas,

On Fri, Mar 22, 2019 at 03:55:19PM -0400, Nicolas Dufresne wrote:
> Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
> > V4L2 uses different fourcc's than DRM, and has a different set of formats.
> > For now, let's add the v4l2 fourcc's for the already existing formats.
>
> Hopefully I get the fixup right this time, see inline.

Thanks a lot for that extensive review.

About the single vs multi-planar issue, I'd be inclined with just
supporting single-planar formats for now, and extend it later to deal
with multiplanar formats.

Would that work for everyone?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Hans Verkuil April 11, 2019, 7:12 a.m. UTC | #20

Hi Maxime,

Some comments below...

On 3/19/19 10:57 PM, Maxime Ripard wrote:
> V4L2 uses different fourcc's than DRM, and has a different set of formats.
> For now, let's add the v4l2 fourcc's for the already existing formats.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  include/linux/image-formats.h |  9 +++++-
>  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 76 insertions(+)
> 
> diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> index 53fd73a71b3d..fbc3a4501ebd 100644
> --- a/include/linux/image-formats.h
> +++ b/include/linux/image-formats.h
> @@ -26,6 +26,13 @@ struct image_format_info {
>  	};
>  
>  	/**
> +	 * @v4l2_fmt:
> +	 *
> +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
> +	 */
> +	u32 v4l2_fmt;
> +
> +	/**
>  	 * @depth:
>  	 *
>  	 * Color depth (number of bits per pixel excluding padding bits),
> @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
>  
>  const struct image_format_info *__image_format_drm_lookup(u32 drm);
>  const struct image_format_info *image_format_drm_lookup(u32 drm);
> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
>  unsigned int image_format_plane_cpp(const struct image_format_info *format,
>  				    int plane);
>  unsigned int image_format_plane_width(int width,
> diff --git a/lib/image-formats.c b/lib/image-formats.c
> index 9b9a73220c5d..39f1d38ae861 100644
> --- a/lib/image-formats.c
> +++ b/lib/image-formats.c
> @@ -8,6 +8,7 @@
>  static const struct image_format_info formats[] = {
>  	{
>  		.drm_fmt = DRM_FORMAT_C8,
> +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
>  		.depth = 8,
>  		.num_planes = 1,
>  		.cpp = { 1, 0, 0 },
> @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB332,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
>  		.depth = 8,
>  		.num_planes = 1,
>  		.cpp = { 1, 0, 0 },
> @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB4444,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB4444,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB1555,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
>  		.depth = 15,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB1555,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
>  		.depth = 15,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB565,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
>  		.depth = 16,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_RGB888,
> +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,

The V4L2 pixelformats describe the order of the components in memory, and as
such are independent of the CPU endianness. How is that for the DRM formats?

Will the order of DRM_FORMAT_RGB888 in memory differ between little and big
endian systems?

Mind you, V4L2 drivers are frankly never tested on big endian systems and I
suspect many if not all will fail miserably on details like this.

>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 3, 0, 0 },
> @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_BGR888,
> +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 3, 0, 0 },
> @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
>  		.vsub = 1,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_XRGB8888,
> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
>  		.depth = 24,
>  		.num_planes = 1,
>  		.cpp = { 4, 0, 0 },
> @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_ARGB8888,
> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
>  		.depth = 32,
>  		.num_planes = 1,
>  		.cpp = { 4, 0, 0 },
> @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
>  		.has_alpha = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV410,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU410,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV420,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU420,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV422,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU422,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUV444,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVU444,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
>  		.depth = 0,
>  		.num_planes = 3,
>  		.cpp = { 1, 1, 1 },
> @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV12,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV21,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV16,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV61,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV24,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_NV42,
> +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
>  		.depth = 0,
>  		.num_planes = 2,
>  		.cpp = { 1, 2, 0 },
> @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YUYV,
> +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_YVYU,
> +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_UYVY,
> +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
>  		.is_yuv = true,
>  	}, {
>  		.drm_fmt = DRM_FORMAT_VYUY,
> +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
>  		.depth = 0,
>  		.num_planes = 1,
>  		.cpp = { 2, 0, 0 },
> @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
>  EXPORT_SYMBOL(image_format_drm_lookup);
>  
>  /**
> + * __image_format_v4l2_lookup - query information for a given format
> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
> +{
> +	return __image_format_lookup(v4l2_fmt, v4l2);
> +}
> +EXPORT_SYMBOL(__image_format_v4l2_lookup);
> +
> +/**
> + * image_format_v4l2_lookup - query information for a given format
> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
> + *
> + * The caller should only pass a supported pixel format to this function.
> + * Unsupported pixel formats will generate a warning in the kernel log.
> + *
> + * Returns:
> + * The instance of struct image_format_info that describes the pixel format, or
> + * NULL if the format is unsupported.
> + */
> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
> +{
> +	const struct image_format_info *format;
> +
> +	format = __image_format_v4l2_lookup(v4l2);
> +
> +	WARN_ON(!format);
> +	return format;
> +}
> +EXPORT_SYMBOL(image_format_v4l2_lookup);
> +
> +/**
>   * image_format_plane_cpp - determine the bytes per pixel value
>   * @format: pointer to the image_format
>   * @plane: plane index
> 

Anyway, I think this is a great improvement!

Regards,

	Hans

Hans Verkuil April 11, 2019, 7:15 a.m. UTC | #21

On 4/11/19 9:12 AM, Hans Verkuil wrote:
> Hi Maxime,
> 
> Some comments below...
> 
> On 3/19/19 10:57 PM, Maxime Ripard wrote:
>> V4L2 uses different fourcc's than DRM, and has a different set of formats.
>> For now, let's add the v4l2 fourcc's for the already existing formats.
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  include/linux/image-formats.h |  9 +++++-
>>  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 76 insertions(+)
>>
>> diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
>> index 53fd73a71b3d..fbc3a4501ebd 100644
>> --- a/include/linux/image-formats.h
>> +++ b/include/linux/image-formats.h
>> @@ -26,6 +26,13 @@ struct image_format_info {
>>  	};
>>  
>>  	/**
>> +	 * @v4l2_fmt:
>> +	 *
>> +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
>> +	 */
>> +	u32 v4l2_fmt;
>> +
>> +	/**
>>  	 * @depth:
>>  	 *
>>  	 * Color depth (number of bits per pixel excluding padding bits),
>> @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
>>  
>>  const struct image_format_info *__image_format_drm_lookup(u32 drm);
>>  const struct image_format_info *image_format_drm_lookup(u32 drm);
>> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
>> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
>>  unsigned int image_format_plane_cpp(const struct image_format_info *format,
>>  				    int plane);
>>  unsigned int image_format_plane_width(int width,
>> diff --git a/lib/image-formats.c b/lib/image-formats.c
>> index 9b9a73220c5d..39f1d38ae861 100644
>> --- a/lib/image-formats.c
>> +++ b/lib/image-formats.c
>> @@ -8,6 +8,7 @@
>>  static const struct image_format_info formats[] = {
>>  	{
>>  		.drm_fmt = DRM_FORMAT_C8,
>> +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
>>  		.depth = 8,
>>  		.num_planes = 1,
>>  		.cpp = { 1, 0, 0 },
>> @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_RGB332,
>> +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
>>  		.depth = 8,
>>  		.num_planes = 1,
>>  		.cpp = { 1, 0, 0 },
>> @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_XRGB4444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_ARGB4444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_XRGB1555,
>> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
>>  		.depth = 15,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_ARGB1555,
>> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
>>  		.depth = 15,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_RGB565,
>> +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
>>  		.depth = 16,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_RGB888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> 
> The V4L2 pixelformats describe the order of the components in memory, and as
> such are independent of the CPU endianness. How is that for the DRM formats?
> 
> Will the order of DRM_FORMAT_RGB888 in memory differ between little and big
> endian systems?
> 
> Mind you, V4L2 drivers are frankly never tested on big endian systems and I
> suspect many if not all will fail miserably on details like this.

Never mind, I now see that there was a long discussion about this same topic.

	Hans

> 
>>  		.depth = 24,
>>  		.num_planes = 1,
>>  		.cpp = { 3, 0, 0 },
>> @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_BGR888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
>>  		.depth = 24,
>>  		.num_planes = 1,
>>  		.cpp = { 3, 0, 0 },
>> @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_XRGB8888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
>>  		.depth = 24,
>>  		.num_planes = 1,
>>  		.cpp = { 4, 0, 0 },
>> @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_ARGB8888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
>>  		.depth = 32,
>>  		.num_planes = 1,
>>  		.cpp = { 4, 0, 0 },
>> @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV410,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU410,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV420,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU420,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV422,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU422,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV12,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV21,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV16,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV61,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV24,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV42,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUYV,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVYU,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_UYVY,
>> +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_VYUY,
>> +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
>>  EXPORT_SYMBOL(image_format_drm_lookup);
>>  
>>  /**
>> + * __image_format_v4l2_lookup - query information for a given format
>> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
>> + *
>> + * The caller should only pass a supported pixel format to this function.
>> + *
>> + * Returns:
>> + * The instance of struct image_format_info that describes the pixel format, or
>> + * NULL if the format is unsupported.
>> + */
>> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
>> +{
>> +	return __image_format_lookup(v4l2_fmt, v4l2);
>> +}
>> +EXPORT_SYMBOL(__image_format_v4l2_lookup);
>> +
>> +/**
>> + * image_format_v4l2_lookup - query information for a given format
>> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
>> + *
>> + * The caller should only pass a supported pixel format to this function.
>> + * Unsupported pixel formats will generate a warning in the kernel log.
>> + *
>> + * Returns:
>> + * The instance of struct image_format_info that describes the pixel format, or
>> + * NULL if the format is unsupported.
>> + */
>> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
>> +{
>> +	const struct image_format_info *format;
>> +
>> +	format = __image_format_v4l2_lookup(v4l2);
>> +
>> +	WARN_ON(!format);
>> +	return format;
>> +}
>> +EXPORT_SYMBOL(image_format_v4l2_lookup);
>> +
>> +/**
>>   * image_format_plane_cpp - determine the bytes per pixel value
>>   * @format: pointer to the image_format
>>   * @plane: plane index
>>
> 
> Anyway, I think this is a great improvement!
> 
> Regards,
> 
> 	Hans
>

Hans Verkuil April 11, 2019, 7:24 a.m. UTC | #22

On 4/1/19 4:44 PM, Maxime Ripard wrote:
> Hi Nicolas,
> 
> On Fri, Mar 22, 2019 at 03:55:19PM -0400, Nicolas Dufresne wrote:
>> Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
>>> V4L2 uses different fourcc's than DRM, and has a different set of formats.
>>> For now, let's add the v4l2 fourcc's for the already existing formats.
>>
>> Hopefully I get the fixup right this time, see inline.
> 
> Thanks a lot for that extensive review.
> 
> About the single vs multi-planar issue, I'd be inclined with just
> supporting single-planar formats for now, and extend it later to deal
> with multiplanar formats.
> 
> Would that work for everyone?

I'd say that is the right approach.

Regards,

	Hans

> 
> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>

Hans Verkuil April 11, 2019, 7:38 a.m. UTC | #23

On 3/22/19 8:55 PM, Nicolas Dufresne wrote:
> Le mardi 19 mars 2019 à 22:57 +0100, Maxime Ripard a écrit :
>> V4L2 uses different fourcc's than DRM, and has a different set of formats.
>> For now, let's add the v4l2 fourcc's for the already existing formats.
> 
> Hopefully I get the fixup right this time, see inline.
> 
>>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  include/linux/image-formats.h |  9 +++++-
>>  lib/image-formats.c           | 67 ++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 76 insertions(+)
>>
>> diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
>> index 53fd73a71b3d..fbc3a4501ebd 100644
>> --- a/include/linux/image-formats.h
>> +++ b/include/linux/image-formats.h
>> @@ -26,6 +26,13 @@ struct image_format_info {
>>  	};
>>  
>>  	/**
>> +	 * @v4l2_fmt:
>> +	 *
>> +	 * V4L2 4CC format identifier (V4L2_PIX_FMT_*)
>> +	 */
>> +	u32 v4l2_fmt;
>> +
>> +	/**
>>  	 * @depth:
>>  	 *
>>  	 * Color depth (number of bits per pixel excluding padding bits),
>> @@ -222,6 +229,8 @@ image_format_info_is_yuv_sampling_444(const struct image_format_info *info)
>>  
>>  const struct image_format_info *__image_format_drm_lookup(u32 drm);
>>  const struct image_format_info *image_format_drm_lookup(u32 drm);
>> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
>> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2);
>>  unsigned int image_format_plane_cpp(const struct image_format_info *format,
>>  				    int plane);
>>  unsigned int image_format_plane_width(int width,
>> diff --git a/lib/image-formats.c b/lib/image-formats.c
>> index 9b9a73220c5d..39f1d38ae861 100644
>> --- a/lib/image-formats.c
>> +++ b/lib/image-formats.c
>> @@ -8,6 +8,7 @@
>>  static const struct image_format_info formats[] = {
>>  	{
>>  		.drm_fmt = DRM_FORMAT_C8,
>> +		.v4l2_fmt = V4L2_PIX_FMT_GREY,
>>  		.depth = 8,
>>  		.num_planes = 1,
>>  		.cpp = { 1, 0, 0 },
>> @@ -15,6 +16,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_RGB332,
>> +		.v4l2_fmt = V4L2_PIX_FMT_RGB332,
>>  		.depth = 8,
>>  		.num_planes = 1,
>>  		.cpp = { 1, 0, 0 },
>> @@ -29,6 +31,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_XRGB4444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB444,
> 
> Not completely sure, looks like in the V4L2 variant, the padding is on
> the 4 MSB of the second byte, which makes it incompatible. There is no
> other equivalent.
> 
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -57,6 +60,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_ARGB4444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB444,
> 
> Similarly.
> 
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -89,6 +93,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_XRGB1555,
>> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB555,
> 
> Same swapping, can't find a match.
> 
>>  		.depth = 15,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -117,6 +122,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_ARGB1555,
>> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB555,
> 
> Same.
> 
>>  		.depth = 15,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_RGB565,
>> +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> 
> -> V4L2_PIX_FMT_RGB565X
> 
> Was added later, as what you expect is not compatible.

All these 16-bit V4L2 pixelformats are ancient, but they are (to my knowledge)
correct w.r.t. the documented layout of the bits in memory. I.e., that's what
the hardware will give you.

I think if there is no equivalence between the drm and v4l2 fourcc's, then
you need two entries in this table, one for drm (v4l2_fmt == 0), one for v4l2
(drm_fmt == 0).

> 
>>  		.depth = 16,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -163,6 +170,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_RGB888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_RGB24,
> 
> -> V4L2_PIX_FMT_BGR24
> 
>>  		.depth = 24,
>>  		.num_planes = 1,
>>  		.cpp = { 3, 0, 0 },
>> @@ -170,6 +178,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_BGR888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_BGR24,
> 
> -> V4L2_PIX_FMT_RGB24
> 
>>  		.depth = 24,
>>  		.num_planes = 1,
>>  		.cpp = { 3, 0, 0 },
>> @@ -177,6 +186,7 @@ static const struct image_format_info formats[] = {
>>  		.vsub = 1,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_XRGB8888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_XRGB32,
> 
> -> V4L2_PIX_FMT_XBGR32
> 
>>  		.depth = 24,
>>  		.num_planes = 1,
>>  		.cpp = { 4, 0, 0 },
>> @@ -281,6 +291,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_ARGB8888,
>> +		.v4l2_fmt = V4L2_PIX_FMT_ARGB32,
> 
> -> V4L2_PIX_FMT_ABGR32
> 
>>  		.depth = 32,
>>  		.num_planes = 1,
>>  		.cpp = { 4, 0, 0 },
>> @@ -361,6 +372,7 @@ static const struct image_format_info formats[] = {
>>  		.has_alpha = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV410,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV410,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -369,6 +381,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU410,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU410,
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -393,6 +406,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV420,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV420M,
> 
> There is two valid matches in V4L2 for this format, not sure how this
> will be handled. The M variant is to be used with MPLANE v4l2_buffer
> when num_planes is bigger then 1.

We should use the non-multiplanar variant (without the M). At least for now.

> 
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -401,6 +415,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU420,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU420M,
> 
> Same.
> 
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -409,6 +424,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV422,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV422M,
> 
> Same.
> 
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -417,6 +433,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU422,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU422M,
> 
> Same. 
> 
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -425,6 +442,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUV444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUV444M,
> 
> Same.
> 
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -433,6 +451,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVU444,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVU444M,
> 
> Same.
> 
>>  		.depth = 0,
>>  		.num_planes = 3,
>>  		.cpp = { 1, 1, 1 },
>> @@ -441,6 +460,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV12,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV12,
> 
> V4L2_PIX_FMT_NV12M is the mplane variant. Depending on how you handle,
> which should be concistant picking one up.
> 
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -449,6 +469,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV21,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV21,
> 
> Same, NV12M for mplane.
> 
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -457,6 +478,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV16,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV16,
> 
> Same, NV16M.
> 
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -465,6 +487,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV61,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV61,
> 
> Same, NV61M.
> 
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV24,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> 
> For extra fun, the M variant has not been added yet.

Note that it has been the practice in V4L2 to avoid adding pixelformats unless
they are in use by a V4L2 driver. So that's why some combinations do not exist.

If we are creating a common library then I think we should change that rule
to: "unless they are in use by a DRM or V4L2 driver". And when new formats are
added, and they exists already for DRM or V4L2, then we should use the same
fourcc for the other subsystem.

I.e. if pixelformat V4L2_PIX_FMT_FOO was already defined, then add a:

#define DRM_FORMAT_FOO V4L2_PIX_FMT_FOO

rather than creating a new fourcc.

We could even start looking at redoing the whole scheme in a unified way, but
that's something for the (far) future.

This is already a big step forward.

Regards,

	Hans

> 
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -481,6 +505,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_NV42,
>> +		.v4l2_fmt = V4L2_PIX_FMT_NV42,
>>  		.depth = 0,
>>  		.num_planes = 2,
>>  		.cpp = { 1, 2, 0 },
>> @@ -489,6 +514,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YUYV,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YUYV,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -497,6 +523,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_YVYU,
>> +		.v4l2_fmt = V4L2_PIX_FMT_YVYU,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -505,6 +532,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_UYVY,
>> +		.v4l2_fmt = V4L2_PIX_FMT_UYVY,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -513,6 +541,7 @@ static const struct image_format_info formats[] = {
>>  		.is_yuv = true,
>>  	}, {
>>  		.drm_fmt = DRM_FORMAT_VYUY,
>> +		.v4l2_fmt = V4L2_PIX_FMT_VYUY,
>>  		.depth = 0,
>>  		.num_planes = 1,
>>  		.cpp = { 2, 0, 0 },
>> @@ -632,6 +661,44 @@ const struct image_format_info *image_format_drm_lookup(u32 drm)
>>  EXPORT_SYMBOL(image_format_drm_lookup);
>>  
>>  /**
>> + * __image_format_v4l2_lookup - query information for a given format
>> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
>> + *
>> + * The caller should only pass a supported pixel format to this function.
>> + *
>> + * Returns:
>> + * The instance of struct image_format_info that describes the pixel format, or
>> + * NULL if the format is unsupported.
>> + */
>> +const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2)
>> +{
>> +	return __image_format_lookup(v4l2_fmt, v4l2);
>> +}
>> +EXPORT_SYMBOL(__image_format_v4l2_lookup);
>> +
>> +/**
>> + * image_format_v4l2_lookup - query information for a given format
>> + * @v4l2: V4L2 fourcc pixel format (V4L2_PIX_FMT_*)
>> + *
>> + * The caller should only pass a supported pixel format to this function.
>> + * Unsupported pixel formats will generate a warning in the kernel log.
>> + *
>> + * Returns:
>> + * The instance of struct image_format_info that describes the pixel format, or
>> + * NULL if the format is unsupported.
>> + */
>> +const struct image_format_info *image_format_v4l2_lookup(u32 v4l2)
>> +{
>> +	const struct image_format_info *format;
>> +
>> +	format = __image_format_v4l2_lookup(v4l2);
>> +
>> +	WARN_ON(!format);
>> +	return format;
>> +}
>> +EXPORT_SYMBOL(image_format_v4l2_lookup);
>> +
>> +/**
>>   * image_format_plane_cpp - determine the bytes per pixel value
>>   * @format: pointer to the image_format
>>   * @plane: plane index

Maxime Ripard April 11, 2019, 3:55 p.m. UTC | #24

Hi,

On Thu, Apr 11, 2019 at 09:38:06AM +0200, Hans Verkuil wrote:
> >> @@ -149,6 +155,7 @@ static const struct image_format_info formats[] = {
> >>  		.has_alpha = true,
> >>  	}, {
> >>  		.drm_fmt = DRM_FORMAT_RGB565,
> >> +		.v4l2_fmt = V4L2_PIX_FMT_RGB565,
> >
> > -> V4L2_PIX_FMT_RGB565X
> >
> > Was added later, as what you expect is not compatible.
>
> All these 16-bit V4L2 pixelformats are ancient, but they are (to my knowledge)
> correct w.r.t. the documented layout of the bits in memory. I.e., that's what
> the hardware will give you.
>
> I think if there is no equivalence between the drm and v4l2 fourcc's, then
> you need two entries in this table, one for drm (v4l2_fmt == 0), one for v4l2
> (drm_fmt == 0).

Yeah, that was my plan. I'd definitely expect some formats in V4L2
while not supported by DRM, or the other way around.

> >> @@ -473,6 +496,7 @@ static const struct image_format_info formats[] = {
> >>  		.is_yuv = true,
> >>  	}, {
> >>  		.drm_fmt = DRM_FORMAT_NV24,
> >> +		.v4l2_fmt = V4L2_PIX_FMT_NV24,
> >
> > For extra fun, the M variant has not been added yet.
>
> Note that it has been the practice in V4L2 to avoid adding pixelformats unless
> they are in use by a V4L2 driver. So that's why some combinations do not exist.
>
> If we are creating a common library then I think we should change that rule
> to: "unless they are in use by a DRM or V4L2 driver". And when new formats are
> added, and they exists already for DRM or V4L2, then we should use the same
> fourcc for the other subsystem.
>
> I.e. if pixelformat V4L2_PIX_FMT_FOO was already defined, then add a:
>
> #define DRM_FORMAT_FOO V4L2_PIX_FMT_FOO
>
> rather than creating a new fourcc.

That makes sense

> We could even start looking at redoing the whole scheme in a unified way, but
> that's something for the (far) future.

Yeah, my secret plan is to have eventually a single fourcc (or even
#define) for both DRM and v4l2 for any new format. But don't tell
anyone :)

> This is already a big step forward.

Great, I'll respin this then.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[RFC,18/20] lib: image-formats: Add v4l2 formats support

Commit Message

Comments

Patch