[v3,7/8] drm/simpledrm: Support the XB24/AB24 format

From: Thierry Reding <treding@nvidia.com>

Add XB24 and AB24 to the list of supported formats. The format helpers
support conversion to these formats and they are documented in the
simple-framebuffer device tree bindings.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- treat AB24 as XB24 and support both at the same time

 drivers/gpu/drm/tiny/simpledrm.c       | 2 ++
 include/linux/platform_data/simplefb.h | 1 +
 2 files changed, 3 insertions(+)

Hi

Am 17.11.22 um 19:40 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Add XB24 and AB24 to the list of supported formats. The format helpers
> support conversion to these formats and they are documented in the
> simple-framebuffer device tree bindings.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - treat AB24 as XB24 and support both at the same time
> 
>   drivers/gpu/drm/tiny/simpledrm.c       | 2 ++
>   include/linux/platform_data/simplefb.h | 1 +
>   2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 7f39bc58da52..ba1c2057fc65 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -483,6 +483,8 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>   static const uint32_t simpledrm_primary_plane_formats[] = {
>   	DRM_FORMAT_XRGB8888,
>   	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,

Does the hardware *really* support AB42 on its primary plane?

We recently had a discussion about the exported formats and the 
consensus is that we only want the hardware's native formats plus 
XRGB888. That's not implemented yet in simpledrm, but this format list 
will soon see a larger cleanup.

So I think ARGB8888 likely shouldn't be on the list here.

Best regards
Thomas

>   	DRM_FORMAT_RGB565,
>   	//DRM_FORMAT_XRGB1555,
>   	//DRM_FORMAT_ARGB1555,
> diff --git a/include/linux/platform_data/simplefb.h b/include/linux/platform_data/simplefb.h
> index 27ea99af6e1d..4f94d52ac99f 100644
> --- a/include/linux/platform_data/simplefb.h
> +++ b/include/linux/platform_data/simplefb.h
> @@ -22,6 +22,7 @@
>   	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, \
>   	{ "x8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_XRGB8888 }, \
>   	{ "a8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {24, 8}, DRM_FORMAT_ARGB8888 }, \
> +	{ "x8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {0, 0}, DRM_FORMAT_XBGR8888 }, \
>   	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {24, 8}, DRM_FORMAT_ABGR8888 }, \
>   	{ "x2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {0, 0}, DRM_FORMAT_XRGB2101010 }, \
>   	{ "a2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {30, 2}, DRM_FORMAT_ARGB2101010 }, \

On Fri, Nov 18, 2022 at 04:08:23PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 17.11.22 um 19:40 schrieb Thierry Reding:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Add XB24 and AB24 to the list of supported formats. The format helpers
> > support conversion to these formats and they are documented in the
> > simple-framebuffer device tree bindings.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - treat AB24 as XB24 and support both at the same time
> > 
> >   drivers/gpu/drm/tiny/simpledrm.c       | 2 ++
> >   include/linux/platform_data/simplefb.h | 1 +
> >   2 files changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index 7f39bc58da52..ba1c2057fc65 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -483,6 +483,8 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> >   static const uint32_t simpledrm_primary_plane_formats[] = {
> >   	DRM_FORMAT_XRGB8888,
> >   	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_ABGR8888,
> 
> Does the hardware *really* support AB42 on its primary plane?

Yes, Tegra display hardware supports this format on the primary plane.

> We recently had a discussion about the exported formats and the consensus is
> that we only want the hardware's native formats plus XRGB888. That's not
> implemented yet in simpledrm, but this format list will soon see a larger
> cleanup.
> 
> So I think ARGB8888 likely shouldn't be on the list here.

This is for consistency with the list below. If a device tree claims
that the framebuffer is ABGR8888 using the "a8b8g8r8" string, then
shouldn't we support it?

Thierry

> 
> Best regards
> Thomas
> 
> >   	DRM_FORMAT_RGB565,
> >   	//DRM_FORMAT_XRGB1555,
> >   	//DRM_FORMAT_ARGB1555,
> > diff --git a/include/linux/platform_data/simplefb.h b/include/linux/platform_data/simplefb.h
> > index 27ea99af6e1d..4f94d52ac99f 100644
> > --- a/include/linux/platform_data/simplefb.h
> > +++ b/include/linux/platform_data/simplefb.h
> > @@ -22,6 +22,7 @@
> >   	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, \
> >   	{ "x8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_XRGB8888 }, \
> >   	{ "a8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {24, 8}, DRM_FORMAT_ARGB8888 }, \
> > +	{ "x8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {0, 0}, DRM_FORMAT_XBGR8888 }, \
> >   	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {24, 8}, DRM_FORMAT_ABGR8888 }, \
> >   	{ "x2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {0, 0}, DRM_FORMAT_XRGB2101010 }, \
> >   	{ "a2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {30, 2}, DRM_FORMAT_ARGB2101010 }, \
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev

Hi

Am 18.11.22 um 16:44 schrieb Thierry Reding:
> On Fri, Nov 18, 2022 at 04:08:23PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 17.11.22 um 19:40 schrieb Thierry Reding:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Add XB24 and AB24 to the list of supported formats. The format helpers
>>> support conversion to these formats and they are documented in the
>>> simple-framebuffer device tree bindings.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> Changes in v2:
>>> - treat AB24 as XB24 and support both at the same time
>>>
>>>    drivers/gpu/drm/tiny/simpledrm.c       | 2 ++
>>>    include/linux/platform_data/simplefb.h | 1 +
>>>    2 files changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>>> index 7f39bc58da52..ba1c2057fc65 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -483,6 +483,8 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>>>    static const uint32_t simpledrm_primary_plane_formats[] = {
>>>    	DRM_FORMAT_XRGB8888,
>>>    	DRM_FORMAT_ARGB8888,
>>> +	DRM_FORMAT_XBGR8888,
>>> +	DRM_FORMAT_ABGR8888,
>>
>> Does the hardware *really* support AB42 on its primary plane?
> 
> Yes, Tegra display hardware supports this format on the primary plane.
> 
>> We recently had a discussion about the exported formats and the consensus is
>> that we only want the hardware's native formats plus XRGB888. That's not
>> implemented yet in simpledrm, but this format list will soon see a larger
>> cleanup.
>>
>> So I think ARGB8888 likely shouldn't be on the list here.
> 
> This is for consistency with the list below. If a device tree claims
> that the framebuffer is ABGR8888 using the "a8b8g8r8" string, then
> shouldn't we support it?

The situation is complicated. Several DTs claim that their framebuffers 
support Alpha+RGB when they actually mean X+RGB. But for compatibility, 
we cannot change this now AFAIU. So we're stuck with X+RGB framebuffers 
that claim that they have an alpha channel. OTOH, other hardware might 
actually support the announced alpha channel. Trying to render into an 
alpha channel would therefore produce undefined output.

The consensus is that we only want to announce XRGB8888 plus the native 
format to userspace. But if the native format has an alpha channel, we'd 
announce the non-alpha format instead. Our format-conversion helpers 
would then fill the alpha channel automatically with 0xff during the 
pageflip.

(This hasn't yet been fully implemented because we first need to fix a 
few things in fbdev emulation to make it work.)

Therefore ABGR8888 shouldn't be on the list. Note that a native DRM 
driver for your display hardware would be free to export ABGR8888. We 
only have this rule for the hardware-agnostic drivers.

Best regards
Thomas

> 
> Thierry
> 
>>
>> Best regards
>> Thomas
>>
>>>    	DRM_FORMAT_RGB565,
>>>    	//DRM_FORMAT_XRGB1555,
>>>    	//DRM_FORMAT_ARGB1555,
>>> diff --git a/include/linux/platform_data/simplefb.h b/include/linux/platform_data/simplefb.h
>>> index 27ea99af6e1d..4f94d52ac99f 100644
>>> --- a/include/linux/platform_data/simplefb.h
>>> +++ b/include/linux/platform_data/simplefb.h
>>> @@ -22,6 +22,7 @@
>>>    	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, \
>>>    	{ "x8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_XRGB8888 }, \
>>>    	{ "a8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {24, 8}, DRM_FORMAT_ARGB8888 }, \
>>> +	{ "x8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {0, 0}, DRM_FORMAT_XBGR8888 }, \
>>>    	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {24, 8}, DRM_FORMAT_ABGR8888 }, \
>>>    	{ "x2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {0, 0}, DRM_FORMAT_XRGB2101010 }, \
>>>    	{ "a2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {30, 2}, DRM_FORMAT_ARGB2101010 }, \
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
>

On Fri, Nov 18, 2022 at 05:10:38PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 18.11.22 um 16:44 schrieb Thierry Reding:
> > On Fri, Nov 18, 2022 at 04:08:23PM +0100, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 17.11.22 um 19:40 schrieb Thierry Reding:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Add XB24 and AB24 to the list of supported formats. The format helpers
> > > > support conversion to these formats and they are documented in the
> > > > simple-framebuffer device tree bindings.
> > > > 
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > > Changes in v2:
> > > > - treat AB24 as XB24 and support both at the same time
> > > > 
> > > >    drivers/gpu/drm/tiny/simpledrm.c       | 2 ++
> > > >    include/linux/platform_data/simplefb.h | 1 +
> > > >    2 files changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > > > index 7f39bc58da52..ba1c2057fc65 100644
> > > > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > > > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > > > @@ -483,6 +483,8 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> > > >    static const uint32_t simpledrm_primary_plane_formats[] = {
> > > >    	DRM_FORMAT_XRGB8888,
> > > >    	DRM_FORMAT_ARGB8888,
> > > > +	DRM_FORMAT_XBGR8888,
> > > > +	DRM_FORMAT_ABGR8888,
> > > 
> > > Does the hardware *really* support AB42 on its primary plane?
> > 
> > Yes, Tegra display hardware supports this format on the primary plane.
> > 
> > > We recently had a discussion about the exported formats and the consensus is
> > > that we only want the hardware's native formats plus XRGB888. That's not
> > > implemented yet in simpledrm, but this format list will soon see a larger
> > > cleanup.
> > > 
> > > So I think ARGB8888 likely shouldn't be on the list here.
> > 
> > This is for consistency with the list below. If a device tree claims
> > that the framebuffer is ABGR8888 using the "a8b8g8r8" string, then
> > shouldn't we support it?
> 
> The situation is complicated. Several DTs claim that their framebuffers
> support Alpha+RGB when they actually mean X+RGB. But for compatibility, we
> cannot change this now AFAIU. So we're stuck with X+RGB framebuffers that
> claim that they have an alpha channel. OTOH, other hardware might actually
> support the announced alpha channel. Trying to render into an alpha channel
> would therefore produce undefined output.

As long as we output 0xff into the alpha channel we should be able to
support those modes as well, shouldn't we? This would effectively be the
same as XRGB variants, except that the native mode could still be
reflected. It probably doesn't make sense to have an alpha channel for
these use-cases (what would we blend with), but I don't see how it would
hurt.

> The consensus is that we only want to announce XRGB8888 plus the native
> format to userspace. But if the native format has an alpha channel, we'd
> announce the non-alpha format instead. Our format-conversion helpers would
> then fill the alpha channel automatically with 0xff during the pageflip.
> 
> (This hasn't yet been fully implemented because we first need to fix a few
> things in fbdev emulation to make it work.)
> 
> Therefore ABGR8888 shouldn't be on the list. Note that a native DRM driver
> for your display hardware would be free to export ABGR8888. We only have
> this rule for the hardware-agnostic drivers.

I don't feel strongly about it, so I can also drop that format. Do we
also want to drop ARGB8888 and ARGB2101010 while we're at it? In a
separate patch of course.

Thierry