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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 ([email protected])
Public Key: https://mrcn.st/pub
On Wed, 17 Nov 2021 23:58:28 +0900
Hector Martin <[email protected]> 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 <[email protected]>
> ---
> 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
On 22/11/2021 18.52, Pekka Paalanen wrote:
> On Wed, 17 Nov 2021 23:58:28 +0900
> Hector Martin <[email protected]> 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 <[email protected]>
>> ---
>> 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 ([email protected])
Public Key: https://mrcn.st/pub
On Mon, 22 Nov 2021 19:05:16 +0900
Hector Martin <[email protected]> wrote:
> On 22/11/2021 18.52, Pekka Paalanen wrote:
> > On Wed, 17 Nov 2021 23:58:28 +0900
> > Hector Martin <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
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
On Fri, Nov 19, 2021 at 9:24 PM Hector Martin <[email protected]> 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.
> > >
> > > 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
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
Hello Thomas,
On Tue, Nov 30, 2021 at 9:31 AM Thomas Zimmermann <[email protected]> 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
On Tue, Nov 30, 2021 at 12:45 AM Javier Martinez Canillas
<[email protected]> 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