Re: [PATCH v2 04/10] drm/fourcc: Add DRM_FORMAT_Y10_LE32

From: Tomi Valkeinen
Date: Wed Jan 15 2025 - 09:34:23 EST

On 15/01/2025 16:08, Geert Uytterhoeven wrote:
Hi Tomi,

On Wed, Jan 15, 2025 at 2:46 PM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
On 15/01/2025 14:52, Geert Uytterhoeven wrote:
On Wed, Jan 15, 2025 at 1:42 PM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
On 15/01/2025 14:33, Geert Uytterhoeven wrote:
On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
On 15/01/2025 12:33, Geert Uytterhoeven wrote:
On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
32-bit container.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>

Thanks for your patch!

--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -408,6 +408,7 @@ extern "C" {
/* Greyscale formats */

#define DRM_FORMAT_Y8 fourcc_code('G', 'R', 'E', 'Y') /* 8-bit Y-only */
+#define DRM_FORMAT_Y10_LE32 fourcc_code('Y', 'P', 'A', '4') /* [31:0] x:Y2:Y1:Y0 2:10:10:10 little endian */

R10_LE32? Or R10_PA4?

Can we discuss the "R" vs "Y" question under the cover letter? There's
some more context about it in there.

Sorry, hadn't read the cover letter. I got attracted by "Y8" and "Y10".

I took the "LE32" from Gstreamer's format. Maybe it's a bit pointless.

I don't know if it makes sense to add the fourcc to the DRM format name.
The fourcc is very limited. Rather, we could, say, have
DRM_FORMAT_Y10_PACKED_32 (or "R", if you insist =).

Does LE32 have a meaning? My first guess just reading the subject
was wrong ("little endian 32-bit" ;-)

I'm not sure I follow. It's little-endian. The pixel group/unit is a
32-bit number, where the leftmost pixel on the screen is in bits 9-0,
and the padding is in bits 31-30, and stored in memory as little-endian.

Ah, the "LE" applies to the pixels inside each word.

No, to the 32-bit container.

DRM formats stored in memory are always little-endian, unless the
DRM_FORMAT_BIG_ENDIAN bit is set, which is what I was hinting
at...

Indeed you're right. The "LE" is pointless. So back to the bike-shedding
about the name =).

As the order inside the container is Y2:Y1:Y0, it _is_ little endian.
Cfr.

#define DRM_FORMAT_YUYV fourcc_code('Y', 'U', 'Y', 'V') /* [31:0]
Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */

Hmm, I see. I hadn't thought LE in that context, but I think it makes
sense when there are multiple pixels in one container. So the "little
endian" above would refer to the order of Y1 and Y0. So is Y1 the
least-significant-pixel? =)

No, Y0 is the least-significant member of the container, which
corresponds to the first pixel ("little end first").

In the number 0x1234, 0x34 is the least-significant byte, and stored first in memory in little endian.

So, similarly, with Y0:Y1, Y1 is stored first in little-endian order, so it's the least-significant-pixel =).

But, say, in

#define DRM_FORMAT_RG88 fourcc_code('R', 'G', '8', '8') /* [15:0] R:G
8:8 little endian */

the "little endian" refers to the 16-bit value itself? Which is not
necessary, as the default assumption is little endian.

I think so.

In any case, when considering your latest point... "LE" in the name
makes sense? But with a quick look I didn't find any formats that would
have "big endian pixel order", so maybe we can just assume little endian
pixel order too.

[CDR][124] have. See the descriptions of the commits that introduced
them for the rationale behind this:

b92db7e4fe740daa drm/fourcc: Add DRM_FORMAT_D[1248]
d093100b425df6fe drm/fourcc: Add DRM_FORMAT_R[124]
e5bd7e3e4a68f0be drm/fourcc: Add DRM_FORMAT_C[124]

Ah, I see. But they don't call the pixel ordering "big endian".

Well, this is all somewhat beside the point. So is "_Y10_32" (or "_R10_32" if we use R) fine?

Tomi