All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Nicolas Dufresne <nicolas@ndufresne.ca>
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	David Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Sean Paul <seanpaul@chromium.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	linux-media@vger.kernel.org, Daniel Stone <daniels@collabora.com>
Subject: Re: [RFC PATCH 18/20] lib: image-formats: Add v4l2 formats support
Date: Fri, 22 Mar 2019 20:44:35 +0200	[thread overview]
Message-ID: <20190322184435.GJ3888@intel.com> (raw)
In-Reply-To: <3e804c622fdc0ce43581982d81d2db67597508ec.camel@ndufresne.ca>

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



-- 
Ville Syrjälä
Intel

  reply	other threads:[~2019-03-22 18:44 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-19 21:57 [RFC PATCH 00/20] drm: Split out the formats API and move it to a common place Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 01/20] drm: Remove users of drm_format_num_planes Maxime Ripard
2019-03-20 14:16   ` Paul Kocialkowski
2019-04-02  9:43   ` Emil Velikov
2019-04-02 14:51     ` Maxime Ripard
2019-04-04 16:24       ` Emil Velikov
2019-03-19 21:57 ` [RFC PATCH 02/20] drm: Remove users of drm_format_(horz|vert)_chroma_subsampling Maxime Ripard
2019-03-20 14:19   ` Paul Kocialkowski
2019-03-20 14:19     ` Paul Kocialkowski
2019-03-19 21:57 ` [RFC PATCH 03/20] drm/fourcc: Pass the format_info pointer to drm_format_plane_cpp Maxime Ripard
2019-03-19 21:57   ` Maxime Ripard
2019-03-20 14:24   ` Paul Kocialkowski
2019-03-21 10:13     ` Maxime Ripard
2019-03-21 10:13       ` Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 04/20] drm/fourcc: Pass the format_info pointer to drm_format_plane_width/height Maxime Ripard
2019-03-20 14:26   ` Paul Kocialkowski
2019-03-19 21:57 ` [RFC PATCH 05/20] drm: Replace instances of drm_format_info by drm_get_format_info Maxime Ripard
2019-03-20 14:27   ` Paul Kocialkowski
2019-03-19 21:57 ` [RFC PATCH 06/20] lib: Add video format information library Maxime Ripard
2019-03-19 21:57   ` Maxime Ripard
2019-03-20 13:39   ` Boris Brezillon
2019-03-21  8:20     ` Maxime Ripard
2019-03-21  8:20       ` Maxime Ripard
2019-03-21  8:40       ` Boris Brezillon
2019-03-19 21:57 ` [RFC PATCH 07/20] drm/fb: Move from drm_format_info to image_format_info Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 08/20] drm/malidp: Convert to generic image format library Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 09/20] drm/client: " Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 10/20] drm/exynos: " Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 11/20] drm/i915: " Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 12/20] drm/ipuv3: " Maxime Ripard
2019-03-19 21:57   ` Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 13/20] drm/msm: " Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 14/20] drm/omap: " Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 15/20] drm/rockchip: " Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 16/20] drm/tegra: " Maxime Ripard
2019-03-19 21:57   ` Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 17/20] drm/fourcc: Remove old DRM format API Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 18/20] lib: image-formats: Add v4l2 formats support Maxime Ripard
2019-03-19 23:29   ` Nicolas Dufresne
2019-03-20 14:27     ` Ville Syrjälä
2019-03-20 15:51       ` Nicolas Dufresne
2019-03-20 16:09         ` Ville Syrjälä
2019-03-20 16:30           ` Nicolas Dufresne
2019-03-20 16:41             ` Ville Syrjälä
2019-03-20 18:27               ` Nicolas Dufresne
2019-03-20 18:39                 ` Ville Syrjälä
2019-03-21 16:04                   ` Paul Kocialkowski
2019-03-21 16:04                     ` Paul Kocialkowski
2019-03-21 16:35                     ` Ville Syrjälä
2019-03-21 19:14                       ` Nicolas Dufresne
2019-03-21 21:44                         ` Ville Syrjälä
2019-03-22 18:24                           ` Nicolas Dufresne
2019-03-22 18:44                             ` Ville Syrjälä [this message]
2019-03-22 19:25                               ` Nicolas Dufresne
2019-03-22 14:42                         ` Ville Syrjälä
2019-03-22 18:11                           ` Nicolas Dufresne
2019-03-20 18:15     ` Brian Starkey
2019-03-21 15:47       ` Maxime Ripard
2019-03-22 19:55   ` Nicolas Dufresne
2019-04-01 14:44     ` Maxime Ripard
2019-04-11  7:24       ` Hans Verkuil
2019-04-11  7:38     ` Hans Verkuil
2019-04-11 15:55       ` Maxime Ripard
2019-04-11  7:12   ` Hans Verkuil
2019-04-11  7:15     ` Hans Verkuil
2019-04-11  7:15       ` Hans Verkuil
2019-03-19 21:57 ` [RFC PATCH 19/20] lib: image-formats: Add more functions Maxime Ripard
2019-03-19 21:57   ` Maxime Ripard
2019-03-19 21:57 ` [RFC PATCH 20/20] media: sun6i: Convert to the image format API Maxime Ripard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190322184435.GJ3888@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=airlied@linux.ie \
    --cc=daniel.vetter@intel.com \
    --cc=daniels@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hans.verkuil@cisco.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=maxime.ripard@bootlin.com \
    --cc=mchehab@kernel.org \
    --cc=nicolas@ndufresne.ca \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=sakari.ailus@linux.intel.com \
    --cc=seanpaul@chromium.org \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.