diff mbox series

[v2,04/10] drm/fourcc: Add DRM_FORMAT_Y10_LE32

Message ID 20250115-xilinx-formats-v2-4-160327ca652a@ideasonboard.com (mailing list archive)
State New
Headers show
Series drm: Add new pixel formats for Xilinx Zynqmp | expand

Commit Message

Tomi Valkeinen Jan. 15, 2025, 9:03 a.m. UTC
Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
32-bit container.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/drm_fourcc.c  | 4 ++++
 include/uapi/drm/drm_fourcc.h | 1 +
 2 files changed, 5 insertions(+)

Comments

Geert Uytterhoeven Jan. 15, 2025, 10:33 a.m. UTC | #1
Hi Tomi,

On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
> 32-bit container.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

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?

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

Gr{oetje,eeting}s,

                        Geert
Tomi Valkeinen Jan. 15, 2025, 11:11 a.m. UTC | #2
Hi,

On 15/01/2025 12:33, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
>> 32-bit container.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> 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.

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.

  Tomi
Geert Uytterhoeven Jan. 15, 2025, 12:33 p.m. UTC | #3
Hi Tomi,

On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> On 15/01/2025 12:33, Geert Uytterhoeven wrote:
> > On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
> >> 32-bit container.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >
> > 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.

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...

Gr{oetje,eeting}s,

                        Geert
Tomi Valkeinen Jan. 15, 2025, 12:42 p.m. UTC | #4
Hi,

On 15/01/2025 14:33, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> On 15/01/2025 12:33, Geert Uytterhoeven wrote:
>>> On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
>>>> 32-bit container.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>
>>> 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 =).

  Tomi
Geert Uytterhoeven Jan. 15, 2025, 12:52 p.m. UTC | #5
Hi Tomi,

On Wed, Jan 15, 2025 at 1:42 PM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> On 15/01/2025 14:33, Geert Uytterhoeven wrote:
> > On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >> On 15/01/2025 12:33, Geert Uytterhoeven wrote:
> >>> On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
> >>> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
> >>>> 32-bit container.
> >>>>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>
> >>> 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 */

Gr{oetje,eeting}s,

                        Geert
Tomi Valkeinen Jan. 15, 2025, 1:46 p.m. UTC | #6
Hi,

On 15/01/2025 14:52, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Wed, Jan 15, 2025 at 1:42 PM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> On 15/01/2025 14:33, Geert Uytterhoeven wrote:
>>> On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>> On 15/01/2025 12:33, Geert Uytterhoeven wrote:
>>>>> On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
>>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
>>>>>> 32-bit container.
>>>>>>
>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>
>>>>> 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? =)

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.

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.

  Tomi
Geert Uytterhoeven Jan. 15, 2025, 2:08 p.m. UTC | #7
Hi Tomi,

On Wed, Jan 15, 2025 at 2:46 PM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> On 15/01/2025 14:52, Geert Uytterhoeven wrote:
> > On Wed, Jan 15, 2025 at 1:42 PM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >> On 15/01/2025 14:33, Geert Uytterhoeven wrote:
> >>> On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
> >>> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>> On 15/01/2025 12:33, Geert Uytterhoeven wrote:
> >>>>> On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
> >>>>> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>>>> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
> >>>>>> 32-bit container.
> >>>>>>
> >>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>>
> >>>>> 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").

> 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]

Gr{oetje,eeting}s,

                        Geert
Tomi Valkeinen Jan. 15, 2025, 2:34 p.m. UTC | #8
On 15/01/2025 16:08, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Wed, Jan 15, 2025 at 2:46 PM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> On 15/01/2025 14:52, Geert Uytterhoeven wrote:
>>> On Wed, Jan 15, 2025 at 1:42 PM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>> On 15/01/2025 14:33, Geert Uytterhoeven wrote:
>>>>> On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
>>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>> On 15/01/2025 12:33, Geert Uytterhoeven wrote:
>>>>>>> On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
>>>>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>>>> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
>>>>>>>> 32-bit container.
>>>>>>>>
>>>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>>
>>>>>>> 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
Geert Uytterhoeven Jan. 15, 2025, 2:49 p.m. UTC | #9
Hi Tomi,

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

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

IAARDD[*], but "10_32" makes me think it stores one 10-bit pixel in a
32-bit word (i.e. a 32-bit variant of R10). So [YR]PA4 would be better.

[*] I am a rookie DRM developer...


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Tomi Valkeinen Jan. 15, 2025, 4:07 p.m. UTC | #10
On 15/01/2025 16:49, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> On Wed, Jan 15, 2025 at 3:34 PM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> On 15/01/2025 16:08, Geert Uytterhoeven wrote:
>>> On Wed, Jan 15, 2025 at 2:46 PM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>> On 15/01/2025 14:52, Geert Uytterhoeven wrote:
>>>>> On Wed, Jan 15, 2025 at 1:42 PM Tomi Valkeinen
>>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>> On 15/01/2025 14:33, Geert Uytterhoeven wrote:
>>>>>>> On Wed, Jan 15, 2025 at 12:11 PM Tomi Valkeinen
>>>>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>>>> On 15/01/2025 12:33, Geert Uytterhoeven wrote:
>>>>>>>>> On Wed, Jan 15, 2025 at 10:04 AM Tomi Valkeinen
>>>>>>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>>>>>> Add Y10_LE32, a 10 bit greyscale format, with 3 pixels packed into
>>>>>>>>>> 32-bit container.
> 
>> Well, this is all somewhat beside the point. So is "_Y10_32" (or
>> "_R10_32" if we use R) fine?
> 
> IAARDD[*], but "10_32" makes me think it stores one 10-bit pixel in a
> 32-bit word (i.e. a 32-bit variant of R10). So [YR]PA4 would be better.

That would be a very silly format. So I think some HW guy is already 
designing chips that have it.

[YR]10_PACKED_32? We can have longer names, we don't need to do encoding 
like [YR]PA4.

> [*] I am a rookie DRM developer...

I haven't heard that before...

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index d721d9fdbe98..6048e0a191dc 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -327,6 +327,10 @@  const struct drm_format_info *__drm_format_info(u32 format)
 		  .num_planes = 2, .char_per_block = { 4, 8, 0 },
 		  .block_w = { 3, 3, 0 }, .block_h = { 1, 1, 0 }, .hsub = 2,
 		  .vsub = 1, .is_yuv = true },
+		{ .format = DRM_FORMAT_Y10_LE32,        .depth = 0,
+		  .num_planes = 1, .char_per_block =  { 4, 0, 0 },
+		  .block_w = { 3, 0, 0 }, .block_h = { 1, 0, 0 }, .hsub = 1,
+		  .vsub = 1, .is_yuv = true },
 	};
 
 	unsigned int i;
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index f79ee3b93f09..03be2aa40265 100644
--- 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 */
 
 /*
  * Format Modifiers: