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

[PATCH v3 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).

Changes since v2:
- Made 10-bit conversion code fill the LSBs
- Added ARGB2101010 to supported formats list
- Simplified OF core code per review feedback
Hector Martin (3):
  of: Move simple-framebuffer device handling from simplefb to of
  drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio()
  drm/simpledrm: Add [AX]RGB2101010 formats

 drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
 drivers/gpu/drm/tiny/simpledrm.c    |  4 +-
 drivers/of/platform.c               |  4 ++
 drivers/video/fbdev/simplefb.c      | 21 +---------
 include/drm/drm_format_helper.h     |  3 ++
 5 files changed, 74 insertions(+), 22 deletions(-)

-- 
2.33.0

[PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Hector Martin]

This code is required for both simplefb and simpledrm, so let's move it
into the OF core instead of having it as an ad-hoc initcall in the
drivers.

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/of/platform.c          |  4 ++++
 drivers/video/fbdev/simplefb.c | 21 +--------------------
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b3faf89744aa..793350028906 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
 		of_node_put(node);
 	}
 
+	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+	of_platform_device_create(node, NULL, NULL);
+	of_node_put(node);
+
 	/* Populate everything else. */
 	of_platform_default_populate(NULL, NULL, NULL);
 
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index b63074fd892e..57541887188b 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -541,26 +541,7 @@ static struct platform_driver simplefb_driver = {
 	.remove = simplefb_remove,
 };
 
-static int __init simplefb_init(void)
-{
-	int ret;
-	struct device_node *np;
-
-	ret = platform_driver_register(&simplefb_driver);
-	if (ret)
-		return ret;
-
-	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(simplefb_init);
+module_platform_driver(simplefb_driver);
 
 MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
 MODULE_DESCRIPTION("Simple framebuffer driver");
-- 
2.33.0

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

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

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

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index dbe3e830096e..0f28dd2bdd72 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -409,6 +409,61 @@ void drm_fb_xrgb8888_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch,
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_toio);
 
+static void drm_fb_xrgb8888_to_xrgb2101010_line(u32 *dbuf, const u32 *sbuf,
+						unsigned int pixels)
+{
+	unsigned int x;
+	u32 val32;
+
+	for (x = 0; x < pixels; x++) {
+		val32 = ((sbuf[x] & 0x000000FF) << 2) |
+			((sbuf[x] & 0x0000FF00) << 4) |
+			((sbuf[x] & 0x00FF0000) << 6);
+		*dbuf++ = val32 | ((val32 >> 8) & 0x00300C03);
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_xrgb2101010_toio - Convert XRGB8888 to XRGB2101010 clip
+ * buffer
+ * @dst: XRGB2101010 destination buffer (iomem)
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @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.
+ */
+void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst,
+					 unsigned int dst_pitch, const void *vaddr,
+					 const struct drm_framebuffer *fb,
+					 const struct drm_rect *clip)
+{
+	size_t linepixels = clip->x2 - clip->x1;
+	size_t dst_len = linepixels * sizeof(u32);
+	unsigned int y, lines = clip->y2 - clip->y1;
+	void *dbuf;
+
+	if (!dst_pitch)
+		dst_pitch = dst_len;
+
+	dbuf = kmalloc(dst_len, GFP_KERNEL);
+	if (!dbuf)
+		return;
+
+	vaddr += clip_offset(clip, fb->pitches[0], 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_toio);
+
 /**
  * drm_fb_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
  * @dst: 8-bit grayscale destination buffer
@@ -500,6 +555,10 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 		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_toio(dst, dst_pitch, vmap, fb, clip);
@@ -515,6 +574,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 			drm_fb_xrgb8888_to_rgb888_toio(dst, 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_toio(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 97e4c3223af3..b30ed5de0a33 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -33,6 +33,9 @@ void drm_fb_xrgb8888_to_rgb888(void *dst, unsigned int dst_pitch, const void *sr
 void drm_fb_xrgb8888_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch,
 				    const void *vaddr, const struct drm_framebuffer *fb,
 				    const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst, unsigned int dst_pitch,
+					 const void *vaddr, const struct drm_framebuffer *fb,
+					 const struct drm_rect *clip);
 void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vaddr,
 			      const struct drm_framebuffer *fb, const struct drm_rect *clip);
 
-- 
2.33.0

[PATCH v3 3/3] drm/simpledrm: Add [AX]RGB2101010 formats

[Hector Martin]

This is the format used by the bootloader framebuffer on Apple ARM64
platforms.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/gpu/drm/tiny/simpledrm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Rob Herring]

On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <marcan@marcan.st> wrote:
>
> This code is required for both simplefb and simpledrm, so let's move it
> into the OF core instead of having it as an ad-hoc initcall in the
> drivers.
>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/of/platform.c          |  4 ++++
>  drivers/video/fbdev/simplefb.c | 21 +--------------------
>  2 files changed, 5 insertions(+), 20 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Thomas Zimmermann]

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



Am 12.12.21 um 07:24 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.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

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

> ---
>   drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
>   include/drm/drm_format_helper.h     |  3 ++
>   2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index dbe3e830096e..0f28dd2bdd72 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -409,6 +409,61 @@ void drm_fb_xrgb8888_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch,
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_toio);
>   
> +static void drm_fb_xrgb8888_to_xrgb2101010_line(u32 *dbuf, const u32 *sbuf,
> +						unsigned int pixels)
> +{
> +	unsigned int x;
> +	u32 val32;
> +
> +	for (x = 0; x < pixels; x++) {
> +		val32 = ((sbuf[x] & 0x000000FF) << 2) |
> +			((sbuf[x] & 0x0000FF00) << 4) |
> +			((sbuf[x] & 0x00FF0000) << 6);
> +		*dbuf++ = val32 | ((val32 >> 8) & 0x00300C03);
> +	}
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_xrgb2101010_toio - Convert XRGB8888 to XRGB2101010 clip
> + * buffer
> + * @dst: XRGB2101010 destination buffer (iomem)
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @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.
> + */
> +void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst,
> +					 unsigned int dst_pitch, const void *vaddr,
> +					 const struct drm_framebuffer *fb,
> +					 const struct drm_rect *clip)
> +{
> +	size_t linepixels = clip->x2 - clip->x1;
> +	size_t dst_len = linepixels * sizeof(u32);
> +	unsigned int y, lines = clip->y2 - clip->y1;
> +	void *dbuf;
> +
> +	if (!dst_pitch)
> +		dst_pitch = dst_len;
> +
> +	dbuf = kmalloc(dst_len, GFP_KERNEL);
> +	if (!dbuf)
> +		return;
> +
> +	vaddr += clip_offset(clip, fb->pitches[0], 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_toio);
> +
>   /**
>    * drm_fb_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
>    * @dst: 8-bit grayscale destination buffer
> @@ -500,6 +555,10 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   		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_toio(dst, dst_pitch, vmap, fb, clip);
> @@ -515,6 +574,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   			drm_fb_xrgb8888_to_rgb888_toio(dst, 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_toio(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 97e4c3223af3..b30ed5de0a33 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -33,6 +33,9 @@ void drm_fb_xrgb8888_to_rgb888(void *dst, unsigned int dst_pitch, const void *sr
>   void drm_fb_xrgb8888_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch,
>   				    const void *vaddr, const struct drm_framebuffer *fb,
>   				    const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst, unsigned int dst_pitch,
> +					 const void *vaddr, const struct drm_framebuffer *fb,
> +					 const struct drm_rect *clip);
>   void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vaddr,
>   			      const struct drm_framebuffer *fb, const 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 v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Thomas Zimmermann]

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

Hi

Am 12.12.21 um 22:29 schrieb Rob Herring:
> On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> This code is required for both simplefb and simpledrm, so let's move it
>> into the OF core instead of having it as an ad-hoc initcall in the
>> drivers.
>>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>   drivers/of/platform.c          |  4 ++++
>>   drivers/video/fbdev/simplefb.c | 21 +--------------------
>>   2 files changed, 5 insertions(+), 20 deletions(-)
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> 

Can I merge this patch through DRM trees?

Best regards
Thomas

-- 
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 v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Javier Martinez Canillas]

Hello Hector,

On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote:
>
> This code is required for both simplefb and simpledrm, so let's move it
> into the OF core instead of having it as an ad-hoc initcall in the
> drivers.
>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/of/platform.c          |  4 ++++
>  drivers/video/fbdev/simplefb.c | 21 +--------------------
>  2 files changed, 5 insertions(+), 20 deletions(-)
>

This is indeed a much better approach than what I suggested. I just
have one comment.

> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b3faf89744aa..793350028906 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
>                 of_node_put(node);
>         }
>
> +       node = of_get_compatible_child(of_chosen, "simple-framebuffer");

You have to check if the node variable is NULL here.

> +       of_platform_device_create(node, NULL, NULL);

Otherwise this could lead to a NULL pointer dereference if debug
output is enabled (the node->full_name is printed).

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier

Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Hector Martin]

On 13/12/2021 17.44, Javier Martinez Canillas wrote:
> Hello Hector,
> 
> On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> This code is required for both simplefb and simpledrm, so let's move it
>> into the OF core instead of having it as an ad-hoc initcall in the
>> drivers.
>>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>   drivers/of/platform.c          |  4 ++++
>>   drivers/video/fbdev/simplefb.c | 21 +--------------------
>>   2 files changed, 5 insertions(+), 20 deletions(-)
>>
> 
> This is indeed a much better approach than what I suggested. I just
> have one comment.
> 
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index b3faf89744aa..793350028906 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
>>                  of_node_put(node);
>>          }
>>
>> +       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> 
> You have to check if the node variable is NULL here.
> 
>> +       of_platform_device_create(node, NULL, NULL);
> 
> Otherwise this could lead to a NULL pointer dereference if debug
> output is enabled (the node->full_name is printed).

Where is it printed? I thought I might need a NULL check, but this code 
was suggested verbatim by Rob in v2 without the NULL check and digging 
through I found that the NULL codepath is safe.

of_platform_device_create calls of_platform_device_create_pdata 
directly, and:

static struct platform_device *of_platform_device_create_pdata(
                                         struct device_node *np,
                                         const char *bus_id,
                                         void *platform_data,
                                         struct device *parent)
{
         struct platform_device *dev;

         if (!of_device_is_available(np) ||
             of_node_test_and_set_flag(np, OF_POPULATED))
                 return NULL;

of_device_is_available takes a global spinlock and then calls 
__of_device_is_available, and that does:

static bool __of_device_is_available(const struct device_node *device)
{
         const char *status;
         int statlen;

         if (!device)
                 return false;

... so I don't see how this can do anything but immediately return false 
if node is NULL.

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

Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Javier Martinez Canillas]

On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <marcan@marcan.st> wrote:
>
> On 13/12/2021 17.44, Javier Martinez Canillas wrote:
> > Hello Hector,
> >
> > On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote:
> >>
> >> This code is required for both simplefb and simpledrm, so let's move it
> >> into the OF core instead of having it as an ad-hoc initcall in the
> >> drivers.
> >>
> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Signed-off-by: Hector Martin <marcan@marcan.st>
> >> ---
> >>   drivers/of/platform.c          |  4 ++++
> >>   drivers/video/fbdev/simplefb.c | 21 +--------------------
> >>   2 files changed, 5 insertions(+), 20 deletions(-)
> >>
> >
> > This is indeed a much better approach than what I suggested. I just
> > have one comment.
> >
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index b3faf89744aa..793350028906 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
> >>                  of_node_put(node);
> >>          }
> >>
> >> +       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> >
> > You have to check if the node variable is NULL here.
> >
> >> +       of_platform_device_create(node, NULL, NULL);
> >
> > Otherwise this could lead to a NULL pointer dereference if debug
> > output is enabled (the node->full_name is printed).
>
> Where is it printed? I thought I might need a NULL check, but this code

Sorry, I misread of_amba_device_create() as
of_platform_device_create(), which uses the "%pOF" printk format
specifier [0] to print the node's full name as a debug output [1].

[0]: https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/core-api/printk-formats.rst#L462
[1]: https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/of/platform.c#L233

> was suggested verbatim by Rob in v2 without the NULL check and digging
> through I found that the NULL codepath is safe.
>

You are right that passing NULL is a safe code path for now due the
of_device_is_available(node) check, but that seems fragile to me since
just adding a similar debug output to of_platform_device_create()
could trigger the NULL pointer dereference.

Best regards,
Javier

Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Hector Martin]

On 13/12/2021 20.30, Javier Martinez Canillas wrote:
> On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> On 13/12/2021 17.44, Javier Martinez Canillas wrote:
>>> Hello Hector,
>>>
>>> On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote:
>>>>
>>>> This code is required for both simplefb and simpledrm, so let's move it
>>>> into the OF core instead of having it as an ad-hoc initcall in the
>>>> drivers.
>>>>
>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Signed-off-by: Hector Martin <marcan@marcan.st>
>>>> ---
>>>>    drivers/of/platform.c          |  4 ++++
>>>>    drivers/video/fbdev/simplefb.c | 21 +--------------------
>>>>    2 files changed, 5 insertions(+), 20 deletions(-)
>>>>
>>>
>>> This is indeed a much better approach than what I suggested. I just
>>> have one comment.
>>>
>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>> index b3faf89744aa..793350028906 100644
>>>> --- a/drivers/of/platform.c
>>>> +++ b/drivers/of/platform.c
>>>> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
>>>>                   of_node_put(node);
>>>>           }
>>>>
>>>> +       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
>>>
>>> You have to check if the node variable is NULL here.
>>>
>>>> +       of_platform_device_create(node, NULL, NULL);
>>>
>>> Otherwise this could lead to a NULL pointer dereference if debug
>>> output is enabled (the node->full_name is printed).
>>
>> Where is it printed? I thought I might need a NULL check, but this code
> 
> Sorry, I misread of_amba_device_create() as
> of_platform_device_create(), which uses the "%pOF" printk format
> specifier [0] to print the node's full name as a debug output [1].
> 
> [0]: https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/core-api/printk-formats.rst#L462
> [1]: https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/of/platform.c#L233
> 
>> was suggested verbatim by Rob in v2 without the NULL check and digging
>> through I found that the NULL codepath is safe.
>>
> 
> You are right that passing NULL is a safe code path for now due the
> of_device_is_available(node) check, but that seems fragile to me since
> just adding a similar debug output to of_platform_device_create()
> could trigger the NULL pointer dereference.

Since Rob is the DT maintainer, I'm going to defer to his opinion on 
this one :-)

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

Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Rob Herring]

On Mon, Dec 13, 2021 at 5:30 AM Javier Martinez Canillas
<javier@dowhile0.org> wrote:
>
> On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <marcan@marcan.st> wrote:
> >
> > On 13/12/2021 17.44, Javier Martinez Canillas wrote:
> > > Hello Hector,
> > >
> > > On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote:
> > >>
> > >> This code is required for both simplefb and simpledrm, so let's move it
> > >> into the OF core instead of having it as an ad-hoc initcall in the
> > >> drivers.
> > >>
> > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > >> Signed-off-by: Hector Martin <marcan@marcan.st>
> > >> ---
> > >>   drivers/of/platform.c          |  4 ++++
> > >>   drivers/video/fbdev/simplefb.c | 21 +--------------------
> > >>   2 files changed, 5 insertions(+), 20 deletions(-)
> > >>
> > >
> > > This is indeed a much better approach than what I suggested. I just
> > > have one comment.
> > >
> > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > >> index b3faf89744aa..793350028906 100644
> > >> --- a/drivers/of/platform.c
> > >> +++ b/drivers/of/platform.c
> > >> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
> > >>                  of_node_put(node);
> > >>          }
> > >>
> > >> +       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> > >
> > > You have to check if the node variable is NULL here.
> > >
> > >> +       of_platform_device_create(node, NULL, NULL);
> > >
> > > Otherwise this could lead to a NULL pointer dereference if debug
> > > output is enabled (the node->full_name is printed).
> >
> > Where is it printed? I thought I might need a NULL check, but this code
>
> Sorry, I misread of_amba_device_create() as
> of_platform_device_create(), which uses the "%pOF" printk format
> specifier [0] to print the node's full name as a debug output [1].
>
> [0]: https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/core-api/printk-formats.rst#L462
> [1]: https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/of/platform.c#L233
>
> > was suggested verbatim by Rob in v2 without the NULL check and digging
> > through I found that the NULL codepath is safe.
> >
>
> You are right that passing NULL is a safe code path for now due the
> of_device_is_available(node) check, but that seems fragile to me since
> just adding a similar debug output to of_platform_device_create()
> could trigger the NULL pointer dereference.

All/most DT functions work with a NULL node ptr, so why should this
one be different?

Rob

Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Rob Herring]

On Mon, Dec 13, 2021 at 2:16 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 12.12.21 um 22:29 schrieb Rob Herring:
> > On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <marcan@marcan.st> wrote:
> >>
> >> This code is required for both simplefb and simpledrm, so let's move it
> >> into the OF core instead of having it as an ad-hoc initcall in the
> >> drivers.
> >>
> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Signed-off-by: Hector Martin <marcan@marcan.st>
> >> ---
> >>   drivers/of/platform.c          |  4 ++++
> >>   drivers/video/fbdev/simplefb.c | 21 +--------------------
> >>   2 files changed, 5 insertions(+), 20 deletions(-)
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> >
>
> Can I merge this patch through DRM trees?

Yes.

Rob

Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of

[Javier Martinez Canillas]

On Mon, Dec 13, 2021 at 3:50 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Dec 13, 2021 at 5:30 AM Javier Martinez Canillas
> <javier@dowhile0.org> wrote:

[snip]

> >
> > You are right that passing NULL is a safe code path for now due the
> > of_device_is_available(node) check, but that seems fragile to me since
> > just adding a similar debug output to of_platform_device_create()
> > could trigger the NULL pointer dereference.
>
> All/most DT functions work with a NULL node ptr, so why should this
> one be different?
>

If you are OK with the patch as is, then I won't object :)

> Rob

Best regards,
Javier

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

[Thomas Zimmermann]

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

Hi

Am 12.12.21 um 07:24 schrieb 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).

If there are no further comments, I'm going to apply the series to 
drm-misc-next.

Best regards
Thomas

> 
> Changes since v2:
> - Made 10-bit conversion code fill the LSBs
> - Added ARGB2101010 to supported formats list
> - Simplified OF core code per review feedback
> Hector Martin (3):
>    of: Move simple-framebuffer device handling from simplefb to of
>    drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio()
>    drm/simpledrm: Add [AX]RGB2101010 formats
> 
>   drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
>   drivers/gpu/drm/tiny/simpledrm.c    |  4 +-
>   drivers/of/platform.c               |  4 ++
>   drivers/video/fbdev/simplefb.c      | 21 +---------
>   include/drm/drm_format_helper.h     |  3 ++
>   5 files changed, 74 insertions(+), 22 deletions(-)
> 

-- 
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 v3 0/3] drm/simpledrm: Apple M1 / DT platform support fixes

[Thomas Zimmermann]

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

Hi

Am 15.12.21 um 14:29 schrieb Thomas Zimmermann:
> Hi
> 
> Am 12.12.21 um 07:24 schrieb 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).
> 
> If there are no further comments, I'm going to apply the series to 
> drm-misc-next.

I've added the series.

Best regards
Thomas

> 
> Best regards
> Thomas
> 
>>
>> Changes since v2:
>> - Made 10-bit conversion code fill the LSBs
>> - Added ARGB2101010 to supported formats list
>> - Simplified OF core code per review feedback
>> Hector Martin (3):
>>    of: Move simple-framebuffer device handling from simplefb to of
>>    drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio()
>>    drm/simpledrm: Add [AX]RGB2101010 formats
>>
>>   drivers/gpu/drm/drm_format_helper.c | 64 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/tiny/simpledrm.c    |  4 +-
>>   drivers/of/platform.c               |  4 ++
>>   drivers/video/fbdev/simplefb.c      | 21 +---------
>>   include/drm/drm_format_helper.h     |  3 ++
>>   5 files changed, 74 insertions(+), 22 deletions(-)
>>
> 

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