[PATCH 0/3] drm/simpledrm: Apple M1 / DT platform support fixes

[PATCH 0/3] drm/simpledrm: Apple M1 / DT platform support fixes

[Hector Martin]

Hi DRM folks,

This short series makes simpledrm work on Apple M1 (including Pro/Max)
platforms the way simplefb already does, by adding XRGB2101010 support
and making it bind to framebuffers in /chosen the same way simplefb
does.

This avoids breaking the bootloader-provided framebuffer console when
simpledrm is selected to replace simplefb, as these FBs always seem to
be 10-bit (at least when a real screen is attached).

Hector Martin (3):
  drm/simpledrm: Bind to OF framebuffers in /chosen
  drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()
  drm/simpledrm: Enable XRGB2101010 format

 drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/simpledrm.c    | 19 ++++++++-
 include/drm/drm_format_helper.h     |  4 ++
 3 files changed, 86 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Hector Martin]

This matches the simplefb behavior; these nodes are not matched by the
standard OF machinery. This fixes a regression when simpledrm replaces
simeplefb.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/gpu/drm/tiny/simpledrm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 481b48bde047..2c84f2ea1fa2 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -2,6 +2,7 @@
 
 #include <linux/clk.h>
 #include <linux/of_clk.h>
+#include <linux/of_platform.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
@@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = {
 
 module_platform_driver(simpledrm_platform_driver);
 
+static int __init simpledrm_init(void)
+{
+	struct device_node *np;
+
+	if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
+		for_each_child_of_node(of_chosen, np) {
+			if (of_device_is_compatible(np, "simple-framebuffer"))
+				of_platform_device_create(np, NULL, NULL);
+		}
+	}
+
+	return 0;
+}
+
+fs_initcall(simpledrm_init);
+
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL v2");
-- 
2.33.0

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1877 bytes --]

Hi

Am 17.11.21 um 15:58 schrieb Hector Martin:
> This matches the simplefb behavior; these nodes are not matched by the
> standard OF machinery. This fixes a regression when simpledrm replaces
> simeplefb.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 481b48bde047..2c84f2ea1fa2 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -2,6 +2,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/of_clk.h>
> +#include <linux/of_platform.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
>   #include <linux/regulator/consumer.h>
> @@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = {
>   
>   module_platform_driver(simpledrm_platform_driver);
>   
> +static int __init simpledrm_init(void)
> +{
> +	struct device_node *np;
> +
> +	if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
> +		for_each_child_of_node(of_chosen, np) {
> +			if (of_device_is_compatible(np, "simple-framebuffer"))
> +				of_platform_device_create(np, NULL, NULL);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +fs_initcall(simpledrm_init);
> +

Simpledrm is just a driver, but this is platform setup code. Why is this 
code located here and not under arch/ or drivers/firmware/?

I know that other drivers do similar things, it doesn't seem to belong here.

Best regards
Thomas

>   MODULE_DESCRIPTION(DRIVER_DESC);
>   MODULE_LICENSE("GPL v2");
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Hector Martin]

On 18/11/2021 18.19, Thomas Zimmermann wrote:
> Hi
> 
> Am 17.11.21 um 15:58 schrieb Hector Martin:
>> @@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = {
>>    
>>    module_platform_driver(simpledrm_platform_driver);
>>    
>> +static int __init simpledrm_init(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
>> +		for_each_child_of_node(of_chosen, np) {
>> +			if (of_device_is_compatible(np, "simple-framebuffer"))
>> +				of_platform_device_create(np, NULL, NULL);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +fs_initcall(simpledrm_init);
>> +
> 
> Simpledrm is just a driver, but this is platform setup code. Why is this
> code located here and not under arch/ or drivers/firmware/?
> 
> I know that other drivers do similar things, it doesn't seem to belong here.

This definitely doesn't belong in either of those, since it is not arch- 
or firmware-specific. It is implementing support for the standard 
simple-framebuffer OF binding, which specifies that it must be located 
within the /chosen node (and thus the default OF setup code won't do the 
matching for you); this applies to all OF platforms [1]

Adding Rob; do you think this should move from simplefb/simpledrm to 
common OF code? (where?)

[1] Documentation/devicetree/bindings/display/simple-framebuffer.yaml

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1805 bytes --]

Hi

Am 20.11.21 um 04:23 schrieb Hector Martin:
> On 18/11/2021 18.19, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 17.11.21 um 15:58 schrieb Hector Martin:
>>> @@ -897,5 +898,21 @@ static struct platform_driver 
>>> simpledrm_platform_driver = {
>>>    module_platform_driver(simpledrm_platform_driver);
>>> +static int __init simpledrm_init(void)
>>> +{
>>> +    struct device_node *np;
>>> +
>>> +    if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
>>> +        for_each_child_of_node(of_chosen, np) {
>>> +            if (of_device_is_compatible(np, "simple-framebuffer"))
>>> +                of_platform_device_create(np, NULL, NULL);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +fs_initcall(simpledrm_init);
>>> +
>>
>> Simpledrm is just a driver, but this is platform setup code. Why is this
>> code located here and not under arch/ or drivers/firmware/?
>>
>> I know that other drivers do similar things, it doesn't seem to belong 
>> here.
> 
> This definitely doesn't belong in either of those, since it is not arch- 
> or firmware-specific. It is implementing support for the standard 
> simple-framebuffer OF binding, which specifies that it must be located 
> within the /chosen node (and thus the default OF setup code won't do the 
> matching for you); this applies to all OF platforms [1]
> 
> Adding Rob; do you think this should move from simplefb/simpledrm to 
> common OF code? (where?)

ping!

> 
> [1] Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Rob Herring]

On Fri, Nov 19, 2021 at 9:24 PM Hector Martin <marcan@marcan.st> wrote:
>
> On 18/11/2021 18.19, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 17.11.21 um 15:58 schrieb Hector Martin:
> >> @@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = {
> >>
> >>    module_platform_driver(simpledrm_platform_driver);
> >>
> >> +static int __init simpledrm_init(void)
> >> +{
> >> +    struct device_node *np;
> >> +
> >> +    if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
> >> +            for_each_child_of_node(of_chosen, np) {
> >> +                    if (of_device_is_compatible(np, "simple-framebuffer"))
> >> +                            of_platform_device_create(np, NULL, NULL);
> >> +            }
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +fs_initcall(simpledrm_init);
> >> +
> >
> > Simpledrm is just a driver, but this is platform setup code. Why is this
> > code located here and not under arch/ or drivers/firmware/?
> >
> > I know that other drivers do similar things, it doesn't seem to belong here.
>
> This definitely doesn't belong in either of those, since it is not arch-
> or firmware-specific. It is implementing support for the standard
> simple-framebuffer OF binding, which specifies that it must be located
> within the /chosen node (and thus the default OF setup code won't do the
> matching for you); this applies to all OF platforms [1]
>
> Adding Rob; do you think this should move from simplefb/simpledrm to
> common OF code? (where?)

of_platform_default_populate_init() should work.

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Javier Martinez Canillas]

> > >
> > > Simpledrm is just a driver, but this is platform setup code. Why is this
> > > code located here and not under arch/ or drivers/firmware/?
> > >

Agreed. Creating platform devices is something for platform code and
not really a DRM driver.

> > > I know that other drivers do similar things, it doesn't seem to belong here.
> >

Yeah, the simplefb driver does this but that seems like something that
should be changed.

> > This definitely doesn't belong in either of those, since it is not arch-
> > or firmware-specific. It is implementing support for the standard
> > simple-framebuffer OF binding, which specifies that it must be located
> > within the /chosen node (and thus the default OF setup code won't do the
> > matching for you); this applies to all OF platforms [1]
> >
> > Adding Rob; do you think this should move from simplefb/simpledrm to
> > common OF code? (where?)
>
> of_platform_default_populate_init() should work.

That should work but I still wonder if it is the correct place to add
this logic.

I think that instead it could be done in the sysfb_create_simplefb()
function [0], which already creates the "simple-framebuffer" device
for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do
the same for OF. That way the simplefb platform device registration
code could also be dropped from the driver and users would just need
to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same.

I have a couple of boards with a bootloader that populates a
"simple-framebuffer" in the /chosen node so I could attempt to write
the patches. But probably won't happen until next week.

[0]: https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/firmware/sysfb_simplefb.c#L60

Best regards,
Javier

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2584 bytes --]

Hi

Am 30.11.21 um 07:44 schrieb Javier Martinez Canillas:
>>>>
>>>> Simpledrm is just a driver, but this is platform setup code. Why is this
>>>> code located here and not under arch/ or drivers/firmware/?
>>>>
> 
> Agreed. Creating platform devices is something for platform code and
> not really a DRM driver.
> 
>>>> I know that other drivers do similar things, it doesn't seem to belong here.
>>>
> 
> Yeah, the simplefb driver does this but that seems like something that
> should be changed.
> 
>>> This definitely doesn't belong in either of those, since it is not arch-
>>> or firmware-specific. It is implementing support for the standard
>>> simple-framebuffer OF binding, which specifies that it must be located
>>> within the /chosen node (and thus the default OF setup code won't do the
>>> matching for you); this applies to all OF platforms [1]
>>>
>>> Adding Rob; do you think this should move from simplefb/simpledrm to
>>> common OF code? (where?)
>>
>> of_platform_default_populate_init() should work.
> 
> That should work but I still wonder if it is the correct place to add
> this logic.
> 
> I think that instead it could be done in the sysfb_create_simplefb()
> function [0], which already creates the "simple-framebuffer" device
> for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do
> the same for OF. That way the simplefb platform device registration
> code could also be dropped from the driver and users would just need
> to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same.
> 
> I have a couple of boards with a bootloader that populates a
> "simple-framebuffer" in the /chosen node so I could attempt to write
> the patches. But probably won't happen until next week.

IMHO it's better to keep the OF-related setup in the OF code. The sysfb 
code is for screen_info. We can try to find common code for OF and sysfb 
that then lives in a shared location.

Using a single global screen_info variable is somewhat awkward these 
days. In the long term, I can think of pushing sysfb code into 
architectures. Each architecture would then setup the platform devices 
that it supports. But that's not really important right now.

Best regards
Thomas

> 
> [0]: https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/firmware/sysfb_simplefb.c#L60
> 
> Best regards,
> Javier
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Javier Martinez Canillas]

Hello Thomas,

On Tue, Nov 30, 2021 at 9:31 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 30.11.21 um 07:44 schrieb Javier Martinez Canillas:

[snip]

> >
> > I think that instead it could be done in the sysfb_create_simplefb()
> > function [0], which already creates the "simple-framebuffer" device
> > for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do
> > the same for OF. That way the simplefb platform device registration
> > code could also be dropped from the driver and users would just need
> > to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same.
> >
> > I have a couple of boards with a bootloader that populates a
> > "simple-framebuffer" in the /chosen node so I could attempt to write
> > the patches. But probably won't happen until next week.
>
> IMHO it's better to keep the OF-related setup in the OF code. The sysfb
> code is for screen_info. We can try to find common code for OF and sysfb
> that then lives in a shared location.
>

Ok. As long as we don't end with code duplication then that works for me too.

> Using a single global screen_info variable is somewhat awkward these
> days. In the long term, I can think of pushing sysfb code into
> architectures. Each architecture would then setup the platform devices
> that it supports. But that's not really important right now.
>

That makes sense. And provide a set of helpers as you mentioned that could
be shared across the different architectures and firmware interfaces.

Best regards,
Javier

Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen

[Rob Herring]

On Tue, Nov 30, 2021 at 12:45 AM Javier Martinez Canillas
<javier@dowhile0.org> wrote:
>
> > > >
> > > > Simpledrm is just a driver, but this is platform setup code. Why is this
> > > > code located here and not under arch/ or drivers/firmware/?
> > > >
>
> Agreed. Creating platform devices is something for platform code and
> not really a DRM driver.
>
> > > > I know that other drivers do similar things, it doesn't seem to belong here.
> > >
>
> Yeah, the simplefb driver does this but that seems like something that
> should be changed.
>
> > > This definitely doesn't belong in either of those, since it is not arch-
> > > or firmware-specific. It is implementing support for the standard
> > > simple-framebuffer OF binding, which specifies that it must be located
> > > within the /chosen node (and thus the default OF setup code won't do the
> > > matching for you); this applies to all OF platforms [1]
> > >
> > > Adding Rob; do you think this should move from simplefb/simpledrm to
> > > common OF code? (where?)
> >
> > of_platform_default_populate_init() should work.
>
> That should work but I still wonder if it is the correct place to add
> this logic.

It is because that is where most of the other devices are created
unless the bus handles it.

> I think that instead it could be done in the sysfb_create_simplefb()
> function [0], which already creates the "simple-framebuffer" device
> for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do
> the same for OF. That way the simplefb platform device registration
> code could also be dropped from the driver and users would just need
> to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same.

Doesn't look like that would share anything with anything else (BIOS/EFI/ACPI).

Rob

[PATCH 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()

[Hector Martin]

Add XRGB8888 emulation support for devices that can only do XRGB2101010.

This is chiefly useful for simpledrm on Apple devices where the
bootloader-provided framebuffer is 10-bit, which already works fine with
simplefb. This is required to make simpledrm support this too.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |  4 ++
 2 files changed, 68 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 69fde60e36b3..5998e57d6ff2 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -378,6 +378,60 @@ void drm_fb_xrgb8888_to_rgb888_dstclip(void __iomem *dst, unsigned int dst_pitch
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_dstclip);
 
+static void drm_fb_xrgb8888_to_xrgb2101010_line(u32 *dbuf, u32 *sbuf,
+						unsigned int pixels)
+{
+	unsigned int x;
+
+	for (x = 0; x < pixels; x++) {
+		*dbuf++ = ((sbuf[x] & 0x000000FF) << 2) |
+			  ((sbuf[x] & 0x0000FF00) << 4) |
+			  ((sbuf[x] & 0x00FF0000) << 6);
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_xrgb2101010_dstclip - Convert XRGB8888 to XRGB2101010 clip
+ * buffer
+ * @dst: XRGB2101010 destination buffer (iomem)
+ * @dst_pitch: destination buffer pitch
+ * @vaddr: XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * Drivers can use this function for XRGB2101010 devices that don't natively
+ * support XRGB8888.
+ *
+ * This function applies clipping on dst, i.e. the destination is a
+ * full (iomem) framebuffer but only the clip rect content is copied over.
+ */
+void drm_fb_xrgb8888_to_xrgb2101010_dstclip(void __iomem *dst,
+					    unsigned int dst_pitch, void *vaddr,
+					    struct drm_framebuffer *fb,
+					    struct drm_rect *clip)
+{
+	size_t linepixels = clip->x2 - clip->x1;
+	size_t dst_len = linepixels * 4;
+	unsigned int y, lines = clip->y2 - clip->y1;
+	void *dbuf;
+
+	dbuf = kmalloc(dst_len, GFP_KERNEL);
+	if (!dbuf)
+		return;
+
+	vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
+	dst += clip_offset(clip, dst_pitch, sizeof(u32));
+	for (y = 0; y < lines; y++) {
+		drm_fb_xrgb8888_to_xrgb2101010_line(dbuf, vaddr, linepixels);
+		memcpy_toio(dst, dbuf, dst_len);
+		vaddr += fb->pitches[0];
+		dst += dst_pitch;
+	}
+
+	kfree(dbuf);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_dstclip);
+
 /**
  * drm_fb_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
  * @dst: 8-bit grayscale destination buffer
@@ -464,6 +518,10 @@ int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
 		fb_format = DRM_FORMAT_XRGB8888;
 	if (dst_format == DRM_FORMAT_ARGB8888)
 		dst_format = DRM_FORMAT_XRGB8888;
+	if (fb_format == DRM_FORMAT_ARGB2101010)
+		fb_format = DRM_FORMAT_XRGB2101010;
+	if (dst_format == DRM_FORMAT_ARGB2101010)
+		dst_format = DRM_FORMAT_XRGB2101010;
 
 	if (dst_format == fb_format) {
 		drm_fb_memcpy_dstclip(dst, dst_pitch, vmap, fb, clip);
@@ -482,6 +540,12 @@ int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
 							  vmap, fb, clip);
 			return 0;
 		}
+	} else if (dst_format == DRM_FORMAT_XRGB2101010) {
+		if (fb_format == DRM_FORMAT_XRGB8888) {
+			drm_fb_xrgb8888_to_xrgb2101010_dstclip(dst, dst_pitch,
+							       vmap, fb, clip);
+			return 0;
+		}
 	}
 
 	return -EINVAL;
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index e86925cf07b9..a0faa710878b 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -29,6 +29,10 @@ void drm_fb_xrgb8888_to_rgb888(void *dst, void *src, struct drm_framebuffer *fb,
 void drm_fb_xrgb8888_to_rgb888_dstclip(void __iomem *dst, unsigned int dst_pitch,
 				       void *vaddr, struct drm_framebuffer *fb,
 				       struct drm_rect *clip);
+void drm_fb_xrgb8888_to_xrgb2101010_dstclip(void __iomem *dst,
+					    unsigned int dst_pitch, void *vaddr,
+					    struct drm_framebuffer *fb,
+					    struct drm_rect *clip);
 void drm_fb_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
 			      struct drm_rect *clip);
 
-- 
2.33.0

Re: [PATCH 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4910 bytes --]

Hi

Am 17.11.21 um 15:58 schrieb Hector Martin:
> Add XRGB8888 emulation support for devices that can only do XRGB2101010.
> 
> This is chiefly useful for simpledrm on Apple devices where the
> bootloader-provided framebuffer is 10-bit, which already works fine with
> simplefb. This is required to make simpledrm support this too.

As mentioned in the other review, this requires a rebase onto the latest 
DRM tree; preferably drm-tip.

Best regards
Thomas

> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
>   include/drm/drm_format_helper.h     |  4 ++
>   2 files changed, 68 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 69fde60e36b3..5998e57d6ff2 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -378,6 +378,60 @@ void drm_fb_xrgb8888_to_rgb888_dstclip(void __iomem *dst, unsigned int dst_pitch
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_dstclip);
>   
> +static void drm_fb_xrgb8888_to_xrgb2101010_line(u32 *dbuf, u32 *sbuf,
> +						unsigned int pixels)
> +{
> +	unsigned int x;
> +
> +	for (x = 0; x < pixels; x++) {
> +		*dbuf++ = ((sbuf[x] & 0x000000FF) << 2) |
> +			  ((sbuf[x] & 0x0000FF00) << 4) |
> +			  ((sbuf[x] & 0x00FF0000) << 6);
> +	}
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_xrgb2101010_dstclip - Convert XRGB8888 to XRGB2101010 clip
> + * buffer
> + * @dst: XRGB2101010 destination buffer (iomem)
> + * @dst_pitch: destination buffer pitch
> + * @vaddr: XRGB8888 source buffer
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
> + *
> + * Drivers can use this function for XRGB2101010 devices that don't natively
> + * support XRGB8888.
> + *
> + * This function applies clipping on dst, i.e. the destination is a
> + * full (iomem) framebuffer but only the clip rect content is copied over.
> + */
> +void drm_fb_xrgb8888_to_xrgb2101010_dstclip(void __iomem *dst,
> +					    unsigned int dst_pitch, void *vaddr,
> +					    struct drm_framebuffer *fb,
> +					    struct drm_rect *clip)
> +{
> +	size_t linepixels = clip->x2 - clip->x1;
> +	size_t dst_len = linepixels * 4;
> +	unsigned int y, lines = clip->y2 - clip->y1;
> +	void *dbuf;
> +
> +	dbuf = kmalloc(dst_len, GFP_KERNEL);
> +	if (!dbuf)
> +		return;
> +
> +	vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
> +	dst += clip_offset(clip, dst_pitch, sizeof(u32));
> +	for (y = 0; y < lines; y++) {
> +		drm_fb_xrgb8888_to_xrgb2101010_line(dbuf, vaddr, linepixels);
> +		memcpy_toio(dst, dbuf, dst_len);
> +		vaddr += fb->pitches[0];
> +		dst += dst_pitch;
> +	}
> +
> +	kfree(dbuf);
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_dstclip);
> +
>   /**
>    * drm_fb_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
>    * @dst: 8-bit grayscale destination buffer
> @@ -464,6 +518,10 @@ int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
>   		fb_format = DRM_FORMAT_XRGB8888;
>   	if (dst_format == DRM_FORMAT_ARGB8888)
>   		dst_format = DRM_FORMAT_XRGB8888;
> +	if (fb_format == DRM_FORMAT_ARGB2101010)
> +		fb_format = DRM_FORMAT_XRGB2101010;
> +	if (dst_format == DRM_FORMAT_ARGB2101010)
> +		dst_format = DRM_FORMAT_XRGB2101010;
>   
>   	if (dst_format == fb_format) {
>   		drm_fb_memcpy_dstclip(dst, dst_pitch, vmap, fb, clip);
> @@ -482,6 +540,12 @@ int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned int dst_pitch,
>   							  vmap, fb, clip);
>   			return 0;
>   		}
> +	} else if (dst_format == DRM_FORMAT_XRGB2101010) {
> +		if (fb_format == DRM_FORMAT_XRGB8888) {
> +			drm_fb_xrgb8888_to_xrgb2101010_dstclip(dst, dst_pitch,
> +							       vmap, fb, clip);
> +			return 0;
> +		}
>   	}
>   
>   	return -EINVAL;
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index e86925cf07b9..a0faa710878b 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -29,6 +29,10 @@ void drm_fb_xrgb8888_to_rgb888(void *dst, void *src, struct drm_framebuffer *fb,
>   void drm_fb_xrgb8888_to_rgb888_dstclip(void __iomem *dst, unsigned int dst_pitch,
>   				       void *vaddr, struct drm_framebuffer *fb,
>   				       struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_xrgb2101010_dstclip(void __iomem *dst,
> +					    unsigned int dst_pitch, void *vaddr,
> +					    struct drm_framebuffer *fb,
> +					    struct drm_rect *clip);
>   void drm_fb_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>   			      struct drm_rect *clip);
>   
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Wed, 17 Nov 2021 23:58:28 +0900
Hector Martin <marcan@marcan.st> wrote:

> Add XRGB8888 emulation support for devices that can only do XRGB2101010.
> 
> This is chiefly useful for simpledrm on Apple devices where the
> bootloader-provided framebuffer is 10-bit, which already works fine with
> simplefb. This is required to make simpledrm support this too.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
>  include/drm/drm_format_helper.h     |  4 ++
>  2 files changed, 68 insertions(+)

Hi Hector,

I'm curious, since the bootloader seems to always set up a 10-bit mode,
is there a reason for it that you can guess? Is the monitor in WCG or
even HDR mode?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()

[Hector Martin]

On 22/11/2021 18.52, Pekka Paalanen wrote:
> On Wed, 17 Nov 2021 23:58:28 +0900
> Hector Martin <marcan@marcan.st> wrote:
> 
>> Add XRGB8888 emulation support for devices that can only do XRGB2101010.
>>
>> This is chiefly useful for simpledrm on Apple devices where the
>> bootloader-provided framebuffer is 10-bit, which already works fine with
>> simplefb. This is required to make simpledrm support this too.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
>>   include/drm/drm_format_helper.h     |  4 ++
>>   2 files changed, 68 insertions(+)
> 
> Hi Hector,
> 
> I'm curious, since the bootloader seems to always set up a 10-bit mode,
> is there a reason for it that you can guess? Is the monitor in WCG or
> even HDR mode?

My guess is that Apple prefer to use 10-bit framebuffers for seamless 
handover with their graphics stack, which presumably uses 10-bit 
framebuffers these days. It seems to be unconditional; I've never seen 
anything but 10 bits across all Apple devices, both with the internal 
panels on laptops and with bog standard external displays on the Mac 
Mini via HDMI. HDR is not necessary, even very dumb capture cards and 
old screens get a 10-bit framebufer in memory.

The only time I see an 8-bit framebuffer is with *no* monitor connected 
on the Mini, in which case you get an 8-bit 640x1136 dummy framebuffer 
(that's the iPhone 5 screen resolution... :-) )

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

On Mon, 22 Nov 2021 19:05:16 +0900
Hector Martin <marcan@marcan.st> wrote:

> On 22/11/2021 18.52, Pekka Paalanen wrote:
> > On Wed, 17 Nov 2021 23:58:28 +0900
> > Hector Martin <marcan@marcan.st> wrote:
> >   
> >> Add XRGB8888 emulation support for devices that can only do XRGB2101010.
> >>
> >> This is chiefly useful for simpledrm on Apple devices where the
> >> bootloader-provided framebuffer is 10-bit, which already works fine with
> >> simplefb. This is required to make simpledrm support this too.
> >>
> >> Signed-off-by: Hector Martin <marcan@marcan.st>
> >> ---
> >>   drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
> >>   include/drm/drm_format_helper.h     |  4 ++
> >>   2 files changed, 68 insertions(+)  
> > 
> > Hi Hector,
> > 
> > I'm curious, since the bootloader seems to always set up a 10-bit mode,
> > is there a reason for it that you can guess? Is the monitor in WCG or
> > even HDR mode?  
> 
> My guess is that Apple prefer to use 10-bit framebuffers for seamless 
> handover with their graphics stack, which presumably uses 10-bit 
> framebuffers these days. It seems to be unconditional; I've never seen 
> anything but 10 bits across all Apple devices, both with the internal 
> panels on laptops and with bog standard external displays on the Mac 
> Mini via HDMI. HDR is not necessary, even very dumb capture cards and 
> old screens get a 10-bit framebufer in memory.

That makes perfect sense, thanks!

Switching between sRGB and WCG or HDR mode is not a modeset, it's just
HDMI/DP/whatever metadata/infoframe.

> The only time I see an 8-bit framebuffer is with *no* monitor connected 
> on the Mini, in which case you get an 8-bit 640x1136 dummy framebuffer 
> (that's the iPhone 5 screen resolution... :-) )
> 

Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] drm/simpledrm: Enable XRGB2101010 format

[Hector Martin]

This is the format used by the bootloader framebuffer on Apple ARM64
platforms, and is already supported by simplefb. This avoids regressing
on these platforms when simpledrm is enabled and replaces simplefb.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/gpu/drm/tiny/simpledrm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 2c84f2ea1fa2..b4b69f3a7e79 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -571,7 +571,7 @@ static const uint32_t simpledrm_default_formats[] = {
 	//DRM_FORMAT_XRGB1555,
 	//DRM_FORMAT_ARGB1555,
 	DRM_FORMAT_RGB888,
-	//DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_XRGB2101010,
 	//DRM_FORMAT_ARGB2101010,
 };
 
-- 
2.33.0

Re: [PATCH 3/3] drm/simpledrm: Enable XRGB2101010 format

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1159 bytes --]

Hi

Am 17.11.21 um 15:58 schrieb Hector Martin:
> This is the format used by the bootloader framebuffer on Apple ARM64
> platforms, and is already supported by simplefb. This avoids regressing
> on these platforms when simpledrm is enabled and replaces simplefb.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 2c84f2ea1fa2..b4b69f3a7e79 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -571,7 +571,7 @@ static const uint32_t simpledrm_default_formats[] = {
>   	//DRM_FORMAT_XRGB1555,
>   	//DRM_FORMAT_ARGB1555,
>   	DRM_FORMAT_RGB888,
> -	//DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_XRGB2101010,
>   	//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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]