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
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 <[email protected]>
Signed-off-by: Hector Martin <[email protected]>
---
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 <[email protected]>");
MODULE_DESCRIPTION("Simple framebuffer driver");
--
2.33.0
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 <[email protected]>
---
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
This is the format used by the bootloader framebuffer on Apple ARM64
platforms.
Reviewed-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Hector Martin <[email protected]>
---
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
On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <[email protected]> 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 <[email protected]>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> drivers/of/platform.c | 4 ++++
> drivers/video/fbdev/simplefb.c | 21 +--------------------
> 2 files changed, 5 insertions(+), 20 deletions(-)
Reviewed-by: Rob Herring <[email protected]>
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 <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
> ---
> 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
Hi
Am 12.12.21 um 22:29 schrieb Rob Herring:
> On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <[email protected]> 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 <[email protected]>
>> Signed-off-by: Hector Martin <[email protected]>
>> ---
>> drivers/of/platform.c | 4 ++++
>> drivers/video/fbdev/simplefb.c | 21 +--------------------
>> 2 files changed, 5 insertions(+), 20 deletions(-)
>
> Reviewed-by: Rob Herring <[email protected]>
>
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
Hello Hector,
On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <[email protected]> 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 <[email protected]>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> 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 <[email protected]>
Best regards,
Javier
On 13/12/2021 17.44, Javier Martinez Canillas wrote:
> Hello Hector,
>
> On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <[email protected]> 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 <[email protected]>
>> Signed-off-by: Hector Martin <[email protected]>
>> ---
>> 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 ([email protected])
Public Key: https://mrcn.st/pub
On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <[email protected]> wrote:
>
> On 13/12/2021 17.44, Javier Martinez Canillas wrote:
> > Hello Hector,
> >
> > On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Hector Martin <[email protected]>
> >> ---
> >> 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
On 13/12/2021 20.30, Javier Martinez Canillas wrote:
> On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <[email protected]> wrote:
>>
>> On 13/12/2021 17.44, Javier Martinez Canillas wrote:
>>> Hello Hector,
>>>
>>> On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Hector Martin <[email protected]>
>>>> ---
>>>> 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 ([email protected])
Public Key: https://mrcn.st/pub
On Mon, Dec 13, 2021 at 5:30 AM Javier Martinez Canillas
<[email protected]> wrote:
>
> On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <[email protected]> wrote:
> >
> > On 13/12/2021 17.44, Javier Martinez Canillas wrote:
> > > Hello Hector,
> > >
> > > On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <[email protected]> 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 <[email protected]>
> > >> Signed-off-by: Hector Martin <[email protected]>
> > >> ---
> > >> 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
On Mon, Dec 13, 2021 at 2:16 AM Thomas Zimmermann <[email protected]> wrote:
>
> Hi
>
> Am 12.12.21 um 22:29 schrieb Rob Herring:
> > On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Hector Martin <[email protected]>
> >> ---
> >> drivers/of/platform.c | 4 ++++
> >> drivers/video/fbdev/simplefb.c | 21 +--------------------
> >> 2 files changed, 5 insertions(+), 20 deletions(-)
> >
> > Reviewed-by: Rob Herring <[email protected]>
> >
>
> Can I merge this patch through DRM trees?
Yes.
Rob
On Mon, Dec 13, 2021 at 3:50 PM Rob Herring <[email protected]> wrote:
>
> On Mon, Dec 13, 2021 at 5:30 AM Javier Martinez Canillas
> <[email protected]> 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
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
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