[RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Some DT platforms use EFI to boot and in this case the EFI Boot Services
may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
queried by the Linux EFI stub to fill the global struct screen_info data.

The data is used by the Generic System Framebuffers (sysfb) framework to
add a platform device with platform data about the system framebuffer.

But if there is a "simple-framebuffer" node in the DT, the OF core will
also do the same and add another device for the system framebuffer.

This could lead for example, to two platform devices ("simple-framebuffer"
and "efi-framebuffer") to be added and matched with their corresponding
drivers. So both efifb and simpledrm will be probed, leading to following:

[    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, total 16000k
[    0.055755] efifb: mode is 2560x1600x32, linelength=10240, pages=1
[    0.055758] efifb: scrolling: redraw
[    0.055759] efifb: Truecolor: size=2:10:10:10, shift=30:20:10:0
...
[    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERROR*
could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
flags 0x0]: -16
[    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
failed with error -16

To prevent the issue, make the OF core to disable sysfb if there is a node
with a "simple-framebuffer" compatible. That way only this device will be
registered and sysfb would not attempt to register another one using the
screen_info data even if this has been filled.

This seems the correct thing to do in this case because:

a) On a DT platform, the DTB is the single source of truth since is what
   describes the hardware topology. Even if EFI Boot Services are used to
   boot the machine.

b) The of_platform_default_populate_init() function is called in the
   arch_initcall_sync() initcall level while the sysfb_init() function
   is called later in the subsys_initcall() initcall level.

Reported-by: Andrew Worsley <amworsley@gmail.com>
Closes: https://lore.kernel.org/all/20231111042926.52990-2-amworsley@gmail.com
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/of/platform.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index f235ab55b91e..0a692fdfef59 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/sysfb.h>
 
 #include "of_private.h"
 
@@ -621,8 +622,21 @@ static int __init of_platform_default_populate_init(void)
 		}
 
 		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-		of_platform_device_create(node, NULL, NULL);
-		of_node_put(node);
+		if (node) {
+			/*
+			 * Since a "simple-framebuffer" device is already added
+			 * here, disable the Generic System Framebuffers (sysfb)
+			 * to prevent it from registering another device for the
+			 * system framebuffer later (e.g: using the screen_info
+			 * data that may had been filled as well).
+			 *
+			 * This can happen for example on DT systems that do EFI
+			 * booting and may provide a GOP handle to the EFI stub.
+			 */
+			sysfb_disable();
+			of_platform_device_create(node, NULL, NULL);
+			of_node_put(node);
+		}
 
 		/* Populate everything else. */
 		of_platform_default_populate(NULL, NULL, NULL);
-- 
2.41.0

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Thomas Zimmermann]

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



Am 13.11.23 um 09:51 schrieb Javier Martinez Canillas:
> Some DT platforms use EFI to boot and in this case the EFI Boot Services
> may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
> queried by the Linux EFI stub to fill the global struct screen_info data.
> 
> The data is used by the Generic System Framebuffers (sysfb) framework to
> add a platform device with platform data about the system framebuffer.
> 
> But if there is a "simple-framebuffer" node in the DT, the OF core will
> also do the same and add another device for the system framebuffer.
> 
> This could lead for example, to two platform devices ("simple-framebuffer"
> and "efi-framebuffer") to be added and matched with their corresponding
> drivers. So both efifb and simpledrm will be probed, leading to following:
> 
> [    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, total 16000k
> [    0.055755] efifb: mode is 2560x1600x32, linelength=10240, pages=1
> [    0.055758] efifb: scrolling: redraw
> [    0.055759] efifb: Truecolor: size=2:10:10:10, shift=30:20:10:0
> ...
> [    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERROR*
> could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
> flags 0x0]: -16
> [    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
> failed with error -16
> 
> To prevent the issue, make the OF core to disable sysfb if there is a node
> with a "simple-framebuffer" compatible. That way only this device will be
> registered and sysfb would not attempt to register another one using the
> screen_info data even if this has been filled.
> 
> This seems the correct thing to do in this case because:
> 
> a) On a DT platform, the DTB is the single source of truth since is what
>     describes the hardware topology. Even if EFI Boot Services are used to
>     boot the machine.
> 
> b) The of_platform_default_populate_init() function is called in the
>     arch_initcall_sync() initcall level while the sysfb_init() function
>     is called later in the subsys_initcall() initcall level.
> 
> Reported-by: Andrew Worsley <amworsley@gmail.com>
> Closes: https://lore.kernel.org/all/20231111042926.52990-2-amworsley@gmail.com
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

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

> ---
> 
>   drivers/of/platform.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index f235ab55b91e..0a692fdfef59 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -20,6 +20,7 @@
>   #include <linux/of_irq.h>
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
> +#include <linux/sysfb.h>
>   
>   #include "of_private.h"
>   
> @@ -621,8 +622,21 @@ static int __init of_platform_default_populate_init(void)
>   		}
>   
>   		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> -		of_platform_device_create(node, NULL, NULL);
> -		of_node_put(node);
> +		if (node) {
> +			/*
> +			 * Since a "simple-framebuffer" device is already added
> +			 * here, disable the Generic System Framebuffers (sysfb)
> +			 * to prevent it from registering another device for the
> +			 * system framebuffer later (e.g: using the screen_info
> +			 * data that may had been filled as well).
> +			 *
> +			 * This can happen for example on DT systems that do EFI
> +			 * booting and may provide a GOP handle to the EFI stub.
> +			 */
> +			sysfb_disable();
> +			of_platform_device_create(node, NULL, NULL);
> +			of_node_put(node);
> +		}
>   
>   		/* Populate everything else. */
>   		of_platform_default_populate(NULL, NULL, NULL);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Andrew Worsley]

On Mon, 13 Nov 2023 at 20:18, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>
>
> Am 13.11.23 um 09:51 schrieb Javier Martinez Canillas:
> > Some DT platforms use EFI to boot and in this case the EFI Boot Services
> > may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
> > queried by the Linux EFI stub to fill the global struct screen_info data.
> >
> > The data is used by the Generic System Framebuffers (sysfb) framework to
> > add a platform device with platform data about the system framebuffer.
> >
> > But if there is a "simple-framebuffer" node in the DT, the OF core will
> > also do the same and add another device for the system framebuffer.
> >
> > This could lead for example, to two platform devices ("simple-framebuffer"
> > and "efi-framebuffer") to be added and matched with their corresponding
> > drivers. So both efifb and simpledrm will be probed, leading to following:
> >
> > [    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, total 16000k
> > [    0.055755] efifb: mode is 2560x1600x32, linelength=10240, pages=1
> > [    0.055758] efifb: scrolling: redraw
> > [    0.055759] efifb: Truecolor: size=2:10:10:10, shift=30:20:10:0
> > ...
> > [    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERROR*
> > could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
> > flags 0x0]: -16
> > [    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
> > failed with error -16
> >
> > To prevent the issue, make the OF core to disable sysfb if there is a node
> > with a "simple-framebuffer" compatible. That way only this device will be
> > registered and sysfb would not attempt to register another one using the
> > screen_info data even if this has been filled.
> >
> > This seems the correct thing to do in this case because:
> >
> > a) On a DT platform, the DTB is the single source of truth since is what
> >     describes the hardware topology. Even if EFI Boot Services are used to
> >     boot the machine.
> >
> > b) The of_platform_default_populate_init() function is called in the
> >     arch_initcall_sync() initcall level while the sysfb_init() function
> >     is called later in the subsys_initcall() initcall level.
> >
> > Reported-by: Andrew Worsley <amworsley@gmail.com>
> > Closes: https://lore.kernel.org/all/20231111042926.52990-2-amworsley@gmail.com
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>
> > ---
> >
> >   drivers/of/platform.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index f235ab55b91e..0a692fdfef59 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/of_irq.h>
> >   #include <linux/of_platform.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/sysfb.h>
> >
> >   #include "of_private.h"
> >
> > @@ -621,8 +622,21 @@ static int __init of_platform_default_populate_init(void)
> >               }
> >
> >               node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> > -             of_platform_device_create(node, NULL, NULL);
> > -             of_node_put(node);
> > +             if (node) {
> > +                     /*
> > +                      * Since a "simple-framebuffer" device is already added
> > +                      * here, disable the Generic System Framebuffers (sysfb)
> > +                      * to prevent it from registering another device for the
> > +                      * system framebuffer later (e.g: using the screen_info
> > +                      * data that may had been filled as well).
> > +                      *
> > +                      * This can happen for example on DT systems that do EFI
> > +                      * booting and may provide a GOP handle to the EFI stub.
> > +                      */
> > +                     sysfb_disable();
> > +                     of_platform_device_create(node, NULL, NULL);
> > +                     of_node_put(node);
> > +             }
> >
> >               /* Populate everything else. */
> >               of_platform_default_populate(NULL, NULL, NULL);
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)

I applied the patch and just the simpledrm driver is probed (the efifb is not):

grep -i -E 'drm|efifb' --color -C3 dmesg-6.5.0-asahi-00780-gf5aadc85a34d.txt
[    2.621433] systemd-journald[276]: varlink-21: Changing state
idle-server \xe2\x86\x92 pending-disconnect
[    2.621437] systemd-journald[276]: varlink-21: Changing state
pending-disconnect \xe2\x86\x92 processing-disconnect
[    2.621439] systemd-journald[276]: varlink-21: Changing state
processing-disconnect \xe2\x86\x92 disconnected
[    2.878828] [drm] Initialized simpledrm 1.0.0 20200625 for
bd58dc000.framebuffer on minor 0
[    2.909764] Console: switching to colour frame buffer device 160x50
[    2.915628] tas2770 1-0031: Property ti,imon-slot-no is missing
setting default slot
[    2.915631] tas2770 1-0031: Property ti,vmon-slot-no is missing
setting default slot
--
[    2.921407] cs42l42 2-0048: supply VCP not found, using dummy regulator
[    2.921412] cs42l42 2-0048: supply VD_FILT not found, using dummy regulator
[    2.921416] cs42l42 2-0048: supply VL not found, using dummy regulator
[    2.934530] simple-framebuffer bd58dc000.framebuffer: [drm] fb0:
simpledrmdrmfb frame buffer device
[    2.938437] cs42l42 2-0048: CS42L42 Device ID (42A83). Expected 42A42
[    2.944183] cs42l83 2-0048: supply VA not found, using dummy regulator
[    2.944769] cs42l83 2-0048: supply VP not found, using dummy regulator

I am wondering if the drm_aperture_remove_framebuffers() shouldn't be
called in the probe function anyway
as it ends up overriding the efifb one as wanted and handles the case
the simpledrm (CONFIG_DRM_SIMPLEDRM)
is not present.
Perhaps there is an accepted principle that such kernels *should* fail
to set up a FB?

Andrew

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Andrew Worsley <amworsley@gmail.com> writes:

Hello Andrew,

> On Mon, 13 Nov 2023 at 20:18, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 13.11.23 um 09:51 schrieb Javier Martinez Canillas:
>> > Some DT platforms use EFI to boot and in this case the EFI Boot Services
>> > may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
>> > queried by the Linux EFI stub to fill the global struct screen_info data.
>> >

[...]

>
> I applied the patch and just the simpledrm driver is probed (the efifb is not):
>
> grep -i -E 'drm|efifb' --color -C3 dmesg-6.5.0-asahi-00780-gf5aadc85a34d.txt
> [    2.621433] systemd-journald[276]: varlink-21: Changing state
> idle-server \xe2\x86\x92 pending-disconnect
> [    2.621437] systemd-journald[276]: varlink-21: Changing state
> pending-disconnect \xe2\x86\x92 processing-disconnect
> [    2.621439] systemd-journald[276]: varlink-21: Changing state
> processing-disconnect \xe2\x86\x92 disconnected
> [    2.878828] [drm] Initialized simpledrm 1.0.0 20200625 for
> bd58dc000.framebuffer on minor 0
> [    2.909764] Console: switching to colour frame buffer device 160x50

Great, thanks for testing. The patch works then as expected. Can I get
your Tested-by then ?

>
> I am wondering if the drm_aperture_remove_framebuffers() shouldn't be
> called in the probe function anyway
> as it ends up overriding the efifb one as wanted and handles the case
> the simpledrm (CONFIG_DRM_SIMPLEDRM)
> is not present.
> Perhaps there is an accepted principle that such kernels *should* fail
> to set up a FB?
>

We were talking with Thomas that the sysfb design seems to be reaching its
limits and need some rework but currently you either need some driver that
matches the "simple-framebuffer" device that is registered by OF or won't
get an early framebuffer in the system.

That could be either simpledrm or simplefb. But if a DT has a device node
for "simple-framebuffer", how can the OF core know if there is a driver to
match that device? And same for any other device defined in the DTB.

It's similar on platforms that use sysfb to register the device (e.g: x86)
since either "simple-framebuffer" is registered (if CONFIG_SYSFB_SIMPLEFB
is enabled) or "efi-framebuffer" (if CONFIG_SYSFB_SIMPLEFB is disabled).

That means CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_SIMPLEDRM disabled won't
work either, even if CONFIG_FB_EFI=y which is the case you are mentioning.

What I think that doesn't make sense is to remove conflicting framebuffers
from drivers that can only handle firmware provided framebuffers. As said
in the other thread, drm_aperture_remove_framebuffers() is only meant for
native DRM drivers.

> Andrew
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Andrew Worsley]

On Mon, 13 Nov 2023 at 23:57, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Andrew Worsley <amworsley@gmail.com> writes:
>
> Hello Andrew,
>
> > On Mon, 13 Nov 2023 at 20:18, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Am 13.11.23 um 09:51 schrieb Javier Martinez Canillas:
> >> > Some DT platforms use EFI to boot and in this case the EFI Boot Services
> >> > may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
> >> > queried by the Linux EFI stub to fill the global struct screen_info data.
> >> >
>
> [...]
>
> >
> > I applied the patch and just the simpledrm driver is probed (the efifb is not):
> >
>[...]
>
> Great, thanks for testing. The patch works then as expected. Can I get
> your Tested-by then ?

Sure absolutely.
>
> >
[...]
> We were talking with Thomas that the sysfb design seems to be reaching its
> limits and need some rework but currently you either need some driver that
> matches the "simple-framebuffer" device that is registered by OF or won't
> get an early framebuffer in the system.
>
> That could be either simpledrm or simplefb. But if a DT has a device node
> for "simple-framebuffer", how can the OF core know if there is a driver to
> match that device? And same for any other device defined in the DTB.
>
> It's similar on platforms that use sysfb to register the device (e.g: x86)
> since either "simple-framebuffer" is registered (if CONFIG_SYSFB_SIMPLEFB
> is enabled) or "efi-framebuffer" (if CONFIG_SYSFB_SIMPLEFB is disabled).
>
> That means CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_SIMPLEDRM disabled won't
> work either, even if CONFIG_FB_EFI=y which is the case you are mentioning.
>
> What I think that doesn't make sense is to remove conflicting framebuffers
> from drivers that can only handle firmware provided framebuffers. As said
> in the other thread, drm_aperture_remove_framebuffers() is only meant for
> native DRM drivers.

Ok - I'm taking it that conflicts between EFI and DT didn't happen in the past
but when they do DT wins. I guess there may be more such conflicts in
the future so
would be resolved in a similar way as more drivers are updated to
support DT settings.
Perhaps one day all drivers would have DT settings and this could be
standardised in some way.


> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
Thanks

Andrew

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Rob Herring]

On Mon, Nov 13, 2023 at 2:53 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Some DT platforms use EFI to boot and in this case the EFI Boot Services
> may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
> queried by the Linux EFI stub to fill the global struct screen_info data.
>
> The data is used by the Generic System Framebuffers (sysfb) framework to
> add a platform device with platform data about the system framebuffer.
>
> But if there is a "simple-framebuffer" node in the DT, the OF core will
> also do the same and add another device for the system framebuffer.
>
> This could lead for example, to two platform devices ("simple-framebuffer"
> and "efi-framebuffer") to be added and matched with their corresponding
> drivers. So both efifb and simpledrm will be probed, leading to following:
>
> [    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, total 16000k
> [    0.055755] efifb: mode is 2560x1600x32, linelength=10240, pages=1
> [    0.055758] efifb: scrolling: redraw
> [    0.055759] efifb: Truecolor: size=2:10:10:10, shift=30:20:10:0
> ...
> [    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERROR*
> could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
> flags 0x0]: -16
> [    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
> failed with error -16
>
> To prevent the issue, make the OF core to disable sysfb if there is a node
> with a "simple-framebuffer" compatible. That way only this device will be
> registered and sysfb would not attempt to register another one using the
> screen_info data even if this has been filled.
>
> This seems the correct thing to do in this case because:
>
> a) On a DT platform, the DTB is the single source of truth since is what
>    describes the hardware topology. Even if EFI Boot Services are used to
>    boot the machine.

This is the opposite of what we do for memory and memory reservations.
EFI is the source of truth for those.

This could also lead to an interesting scenario. As simple-framebuffer
can define its memory in a /reserved-memory node, but that is ignored
in EFI boot. Probably would work, but only because EFI probably
generates its memory map table from the /reserved-memory nodes.

Rob

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Rob Herring <robh@kernel.org> writes:

Hello Rob,

> On Mon, Nov 13, 2023 at 2:53 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>>
>> Some DT platforms use EFI to boot and in this case the EFI Boot Services
>> may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
>> queried by the Linux EFI stub to fill the global struct screen_info data.
>>
>> The data is used by the Generic System Framebuffers (sysfb) framework to
>> add a platform device with platform data about the system framebuffer.
>>
>> But if there is a "simple-framebuffer" node in the DT, the OF core will
>> also do the same and add another device for the system framebuffer.
>>
>> This could lead for example, to two platform devices ("simple-framebuffer"
>> and "efi-framebuffer") to be added and matched with their corresponding
>> drivers. So both efifb and simpledrm will be probed, leading to following:
>>
>> [    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, total 16000k
>> [    0.055755] efifb: mode is 2560x1600x32, linelength=10240, pages=1
>> [    0.055758] efifb: scrolling: redraw
>> [    0.055759] efifb: Truecolor: size=2:10:10:10, shift=30:20:10:0
>> ...
>> [    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERROR*
>> could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
>> flags 0x0]: -16
>> [    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
>> failed with error -16
>>
>> To prevent the issue, make the OF core to disable sysfb if there is a node
>> with a "simple-framebuffer" compatible. That way only this device will be
>> registered and sysfb would not attempt to register another one using the
>> screen_info data even if this has been filled.
>>
>> This seems the correct thing to do in this case because:
>>
>> a) On a DT platform, the DTB is the single source of truth since is what
>>    describes the hardware topology. Even if EFI Boot Services are used to
>>    boot the machine.
>
> This is the opposite of what we do for memory and memory reservations.
> EFI is the source of truth for those.
>
> This could also lead to an interesting scenario. As simple-framebuffer
> can define its memory in a /reserved-memory node, but that is ignored
> in EFI boot. Probably would work, but only because EFI probably
> generates its memory map table from the /reserved-memory nodes.
>

I see. So what would be the solution then? Ignoring creating a platform
device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?

> Rob
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Rob Herring]

On Thu, Nov 16, 2023 at 3:36 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Rob Herring <robh@kernel.org> writes:
>
> Hello Rob,
>
> > On Mon, Nov 13, 2023 at 2:53 AM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >>
> >> Some DT platforms use EFI to boot and in this case the EFI Boot Services
> >> may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
> >> queried by the Linux EFI stub to fill the global struct screen_info data.
> >>
> >> The data is used by the Generic System Framebuffers (sysfb) framework to
> >> add a platform device with platform data about the system framebuffer.
> >>
> >> But if there is a "simple-framebuffer" node in the DT, the OF core will
> >> also do the same and add another device for the system framebuffer.
> >>
> >> This could lead for example, to two platform devices ("simple-framebuffer"
> >> and "efi-framebuffer") to be added and matched with their corresponding
> >> drivers. So both efifb and simpledrm will be probed, leading to following:
> >>
> >> [    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, total 16000k
> >> [    0.055755] efifb: mode is 2560x1600x32, linelength=10240, pages=1
> >> [    0.055758] efifb: scrolling: redraw
> >> [    0.055759] efifb: Truecolor: size=2:10:10:10, shift=30:20:10:0
> >> ...
> >> [    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERROR*
> >> could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
> >> flags 0x0]: -16
> >> [    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
> >> failed with error -16
> >>
> >> To prevent the issue, make the OF core to disable sysfb if there is a node
> >> with a "simple-framebuffer" compatible. That way only this device will be
> >> registered and sysfb would not attempt to register another one using the
> >> screen_info data even if this has been filled.
> >>
> >> This seems the correct thing to do in this case because:
> >>
> >> a) On a DT platform, the DTB is the single source of truth since is what
> >>    describes the hardware topology. Even if EFI Boot Services are used to
> >>    boot the machine.
> >
> > This is the opposite of what we do for memory and memory reservations.
> > EFI is the source of truth for those.
> >
> > This could also lead to an interesting scenario. As simple-framebuffer
> > can define its memory in a /reserved-memory node, but that is ignored
> > in EFI boot. Probably would work, but only because EFI probably
> > generates its memory map table from the /reserved-memory nodes.
> >
>
> I see. So what would be the solution then? Ignoring creating a platform
> device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?

Shrug. I don't really know anything more about EFI FB, but I would
guess it can't support handling resources like clocks, power domains,
regulators, etc. that simple-fb can. So if a platform needs those, do
we say they should not setup EFI-GOP? Or is there a use case for
having both? Clients that don't muck with resources can use EFI-GOP
and those that do use simple-fb. For example, does/can grub use
EFI-GOP, but not simple-fb?

Rob

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Ard Biesheuvel]

On Fri, 17 Nov 2023 at 00:09, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Nov 16, 2023 at 3:36 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
> >
> > Rob Herring <robh@kernel.org> writes:
> >
> > Hello Rob,
> >
> > > On Mon, Nov 13, 2023 at 2:53 AM Javier Martinez Canillas
> > > <javierm@redhat.com> wrote:
> > >>
> > >> Some DT platforms use EFI to boot and in this case the EFI Boot Services
> > >> may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later be
> > >> queried by the Linux EFI stub to fill the global struct screen_info data.
> > >>
> > >> The data is used by the Generic System Framebuffers (sysfb) framework to
> > >> add a platform device with platform data about the system framebuffer.
> > >>
> > >> But if there is a "simple-framebuffer" node in the DT, the OF core will
> > >> also do the same and add another device for the system framebuffer.
> > >>
> > >> This could lead for example, to two platform devices ("simple-framebuffer"
> > >> and "efi-framebuffer") to be added and matched with their corresponding
> > >> drivers. So both efifb and simpledrm will be probed, leading to following:
> > >>
> > >> [    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, total 16000k
> > >> [    0.055755] efifb: mode is 2560x1600x32, linelength=10240, pages=1
> > >> [    0.055758] efifb: scrolling: redraw
> > >> [    0.055759] efifb: Truecolor: size=2:10:10:10, shift=30:20:10:0
> > >> ...
> > >> [    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERROR*
> > >> could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
> > >> flags 0x0]: -16
> > >> [    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
> > >> failed with error -16
> > >>
> > >> To prevent the issue, make the OF core to disable sysfb if there is a node
> > >> with a "simple-framebuffer" compatible. That way only this device will be
> > >> registered and sysfb would not attempt to register another one using the
> > >> screen_info data even if this has been filled.
> > >>
> > >> This seems the correct thing to do in this case because:
> > >>
> > >> a) On a DT platform, the DTB is the single source of truth since is what
> > >>    describes the hardware topology. Even if EFI Boot Services are used to
> > >>    boot the machine.
> > >
> > > This is the opposite of what we do for memory and memory reservations.
> > > EFI is the source of truth for those.
> > >
> > > This could also lead to an interesting scenario. As simple-framebuffer
> > > can define its memory in a /reserved-memory node, but that is ignored
> > > in EFI boot. Probably would work, but only because EFI probably
> > > generates its memory map table from the /reserved-memory nodes.
> > >
> >
> > I see. So what would be the solution then? Ignoring creating a platform
> > device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?
>
> Shrug. I don't really know anything more about EFI FB, but I would
> guess it can't support handling resources like clocks, power domains,
> regulators, etc. that simple-fb can. So if a platform needs those, do
> we say they should not setup EFI-GOP? Or is there a use case for
> having both? Clients that don't muck with resources can use EFI-GOP
> and those that do use simple-fb. For example, does/can grub use
> EFI-GOP, but not simple-fb?
>

The EFI GOP is just a dumb framebuffer, and it is not even generally
possible to cross reference the GOP with a particular device in the
device hierarchy unless you e.g., compare the BARs of each device with
the region described by the GOP protocol.

GRUB for EFI will use the GOP and nothing else, but only at boot time
(the GOP protocol is more than a magic linear memory region, it also
implements a Blt() abstraction that permits the use of framebuffers
that are not mapped linearly into the address space at all, and GRUB
makes use of this)

The EFI stub will only expose GOPs to the kernel if they are in fact
linear framebuffers, but has zero insight into whether the hardware
needs clocks and regulators, and whether or not the framebuffer needs
IOMMU pass through (which might be the case if the scanout is using
DMA into system memory)

So calling EFI GOP 'source of truth' is rather generous, and I think
it makes sense to prioritize more accurate descriptions of the
underlying framebuffer over EFI GOP.

However, making 'simple-framebuffer' magic in this regard doesn't seem
like a great approach to me. Is there a better way we could get the
resource conflict to be decided in a way where the EFI GOP gets
superseded if its resources are claimed by another device?

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Ard Biesheuvel <ardb@kernel.org> writes:

Hello Ard,

> On Fri, 17 Nov 2023 at 00:09, Rob Herring <robh@kernel.org> wrote:

[...]

>> > >
>> > > This could also lead to an interesting scenario. As simple-framebuffer
>> > > can define its memory in a /reserved-memory node, but that is ignored
>> > > in EFI boot. Probably would work, but only because EFI probably
>> > > generates its memory map table from the /reserved-memory nodes.
>> > >
>> >
>> > I see. So what would be the solution then? Ignoring creating a platform
>> > device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?
>>
>> Shrug. I don't really know anything more about EFI FB, but I would
>> guess it can't support handling resources like clocks, power domains,
>> regulators, etc. that simple-fb can. So if a platform needs those, do
>> we say they should not setup EFI-GOP? Or is there a use case for
>> having both? Clients that don't muck with resources can use EFI-GOP
>> and those that do use simple-fb. For example, does/can grub use
>> EFI-GOP, but not simple-fb?
>>
>
> The EFI GOP is just a dumb framebuffer, and it is not even generally
> possible to cross reference the GOP with a particular device in the
> device hierarchy unless you e.g., compare the BARs of each device with
> the region described by the GOP protocol.
>
> GRUB for EFI will use the GOP and nothing else, but only at boot time
> (the GOP protocol is more than a magic linear memory region, it also
> implements a Blt() abstraction that permits the use of framebuffers
> that are not mapped linearly into the address space at all, and GRUB
> makes use of this)
>
> The EFI stub will only expose GOPs to the kernel if they are in fact
> linear framebuffers, but has zero insight into whether the hardware
> needs clocks and regulators, and whether or not the framebuffer needs
> IOMMU pass through (which might be the case if the scanout is using
> DMA into system memory)
>
> So calling EFI GOP 'source of truth' is rather generous, and I think
> it makes sense to prioritize more accurate descriptions of the
> underlying framebuffer over EFI GOP.
>

That was my opinion as well and the reason why I called the DTB the
single source of truth.

> However, making 'simple-framebuffer' magic in this regard doesn't seem
> like a great approach to me. Is there a better way we could get the
> resource conflict to be decided in a way where the EFI GOP gets
> superseded if its resources are claimed by another device?
>

There is an aperture [0] framework that is used by the fbdev and DRM
subsystems to allow native drivers to remove any conflicting devices
that share the same framebuffer aperture.

But it only makes sense for native drivers to use that I think, but
in this case is about two drivers that attempt to use the same frame
buffer provided by the firmware but getting it from different places.

I don't have a better idea than this patch but maybe Thomas or Sima do?

[0]: https://elixir.bootlin.com/linux/latest/source/drivers/video/aperture.c

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Thomas Zimmermann]

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

Hi

Am 18.11.23 um 12:10 schrieb Javier Martinez Canillas:
> Ard Biesheuvel <ardb@kernel.org> writes:
> 
> Hello Ard,
> 
>> On Fri, 17 Nov 2023 at 00:09, Rob Herring <robh@kernel.org> wrote:
> 
> [...]
> 
>>>>>
>>>>> This could also lead to an interesting scenario. As simple-framebuffer
>>>>> can define its memory in a /reserved-memory node, but that is ignored
>>>>> in EFI boot. Probably would work, but only because EFI probably
>>>>> generates its memory map table from the /reserved-memory nodes.
>>>>>
>>>>
>>>> I see. So what would be the solution then? Ignoring creating a platform
>>>> device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?
>>>
>>> Shrug. I don't really know anything more about EFI FB, but I would
>>> guess it can't support handling resources like clocks, power domains,
>>> regulators, etc. that simple-fb can. So if a platform needs those, do
>>> we say they should not setup EFI-GOP? Or is there a use case for
>>> having both? Clients that don't muck with resources can use EFI-GOP
>>> and those that do use simple-fb. For example, does/can grub use
>>> EFI-GOP, but not simple-fb?
>>>
>>
>> The EFI GOP is just a dumb framebuffer, and it is not even generally
>> possible to cross reference the GOP with a particular device in the
>> device hierarchy unless you e.g., compare the BARs of each device with
>> the region described by the GOP protocol.
>>
>> GRUB for EFI will use the GOP and nothing else, but only at boot time
>> (the GOP protocol is more than a magic linear memory region, it also
>> implements a Blt() abstraction that permits the use of framebuffers
>> that are not mapped linearly into the address space at all, and GRUB
>> makes use of this)
>>
>> The EFI stub will only expose GOPs to the kernel if they are in fact
>> linear framebuffers, but has zero insight into whether the hardware
>> needs clocks and regulators, and whether or not the framebuffer needs
>> IOMMU pass through (which might be the case if the scanout is using
>> DMA into system memory)
>>
>> So calling EFI GOP 'source of truth' is rather generous, and I think
>> it makes sense to prioritize more accurate descriptions of the
>> underlying framebuffer over EFI GOP.
>>
> 
> That was my opinion as well and the reason why I called the DTB the
> single source of truth.
> 
>> However, making 'simple-framebuffer' magic in this regard doesn't seem
>> like a great approach to me. Is there a better way we could get the
>> resource conflict to be decided in a way where the EFI GOP gets
>> superseded if its resources are claimed by another device?
>>
> 
> There is an aperture [0] framework that is used by the fbdev and DRM
> subsystems to allow native drivers to remove any conflicting devices
> that share the same framebuffer aperture.
> 
> But it only makes sense for native drivers to use that I think, but
> in this case is about two drivers that attempt to use the same frame
> buffer provided by the firmware but getting it from different places.
> 
> I don't have a better idea than this patch but maybe Thomas or Sima do?

At SUSE, we've carried such a patch in our repos for some time. It works 
around the double-framebuffer problem in a similar way. [1]

As Javier mentioned, we do track the framebuffer ranges for EFI/VESA/OF 
framebuffers in the graphics aperture helpers. Fbdev has done this for 
decades, and the current codebase extends and harmonizes this 
functionality among fbdev and DRM drivers.

WRT DT vs EFI: AFAIK the EFI support on affected platforms looks at the 
DT to set up the EFI framebuffer. So IMHO the DT is the authoritative 
source on the framebuffer.

Best regards
Thomas

[1] https://bugzilla.suse.com/show_bug.cgi?id=1204315

> 
> [0]: https://elixir.bootlin.com/linux/latest/source/drivers/video/aperture.c
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 18.11.23 um 12:10 schrieb Javier Martinez Canillas:
>> Ard Biesheuvel <ardb@kernel.org> writes:
>> 
>> Hello Ard,
>> 
>>> On Fri, 17 Nov 2023 at 00:09, Rob Herring <robh@kernel.org> wrote:
>> 
>> [...]
>> 
>>>>>>
>>>>>> This could also lead to an interesting scenario. As simple-framebuffer
>>>>>> can define its memory in a /reserved-memory node, but that is ignored
>>>>>> in EFI boot. Probably would work, but only because EFI probably
>>>>>> generates its memory map table from the /reserved-memory nodes.
>>>>>>
>>>>>
>>>>> I see. So what would be the solution then? Ignoring creating a platform
>>>>> device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?
>>>>
>>>> Shrug. I don't really know anything more about EFI FB, but I would
>>>> guess it can't support handling resources like clocks, power domains,
>>>> regulators, etc. that simple-fb can. So if a platform needs those, do
>>>> we say they should not setup EFI-GOP? Or is there a use case for
>>>> having both? Clients that don't muck with resources can use EFI-GOP
>>>> and those that do use simple-fb. For example, does/can grub use
>>>> EFI-GOP, but not simple-fb?
>>>>
>>>
>>> The EFI GOP is just a dumb framebuffer, and it is not even generally
>>> possible to cross reference the GOP with a particular device in the
>>> device hierarchy unless you e.g., compare the BARs of each device with
>>> the region described by the GOP protocol.
>>>
>>> GRUB for EFI will use the GOP and nothing else, but only at boot time
>>> (the GOP protocol is more than a magic linear memory region, it also
>>> implements a Blt() abstraction that permits the use of framebuffers
>>> that are not mapped linearly into the address space at all, and GRUB
>>> makes use of this)
>>>
>>> The EFI stub will only expose GOPs to the kernel if they are in fact
>>> linear framebuffers, but has zero insight into whether the hardware
>>> needs clocks and regulators, and whether or not the framebuffer needs
>>> IOMMU pass through (which might be the case if the scanout is using
>>> DMA into system memory)
>>>
>>> So calling EFI GOP 'source of truth' is rather generous, and I think
>>> it makes sense to prioritize more accurate descriptions of the
>>> underlying framebuffer over EFI GOP.
>>>
>> 
>> That was my opinion as well and the reason why I called the DTB the
>> single source of truth.
>> 
>>> However, making 'simple-framebuffer' magic in this regard doesn't seem
>>> like a great approach to me. Is there a better way we could get the
>>> resource conflict to be decided in a way where the EFI GOP gets
>>> superseded if its resources are claimed by another device?
>>>
>> 
>> There is an aperture [0] framework that is used by the fbdev and DRM
>> subsystems to allow native drivers to remove any conflicting devices
>> that share the same framebuffer aperture.
>> 
>> But it only makes sense for native drivers to use that I think, but
>> in this case is about two drivers that attempt to use the same frame
>> buffer provided by the firmware but getting it from different places.
>> 
>> I don't have a better idea than this patch but maybe Thomas or Sima do?
>
> At SUSE, we've carried such a patch in our repos for some time. It works 
> around the double-framebuffer problem in a similar way. [1]
>

Thanks for the information. I see that your patch is basically mine but
doing it unconditionally while this one only does the sysfb_disable() if
a "simple-framebuffer" DT node has been found.

> As Javier mentioned, we do track the framebuffer ranges for EFI/VESA/OF 
> framebuffers in the graphics aperture helpers. Fbdev has done this for 
> decades, and the current codebase extends and harmonizes this 
> functionality among fbdev and DRM drivers.
>
> WRT DT vs EFI: AFAIK the EFI support on affected platforms looks at the 
> DT to set up the EFI framebuffer. So IMHO the DT is the authoritative 
> source on the framebuffer.
>

Agreed. Sima Vetter also mentioned on IRC that they think this patch is
the least bad option. Rob, Ard any thoughts? Maybe we can land this as
a fix and then figure how this could be better handled in the long term?

> Best regards
> Thomas
>
> [1] https://bugzilla.suse.com/show_bug.cgi?id=1204315
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Rob Herring]

On Fri, Dec 1, 2023 at 4:21 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> > Hi
> >
> > Am 18.11.23 um 12:10 schrieb Javier Martinez Canillas:
> >> Ard Biesheuvel <ardb@kernel.org> writes:
> >>
> >> Hello Ard,
> >>
> >>> On Fri, 17 Nov 2023 at 00:09, Rob Herring <robh@kernel.org> wrote:
> >>
> >> [...]
> >>
> >>>>>>
> >>>>>> This could also lead to an interesting scenario. As simple-framebuffer
> >>>>>> can define its memory in a /reserved-memory node, but that is ignored
> >>>>>> in EFI boot. Probably would work, but only because EFI probably
> >>>>>> generates its memory map table from the /reserved-memory nodes.
> >>>>>>
> >>>>>
> >>>>> I see. So what would be the solution then? Ignoring creating a platform
> >>>>> device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?
> >>>>
> >>>> Shrug. I don't really know anything more about EFI FB, but I would
> >>>> guess it can't support handling resources like clocks, power domains,
> >>>> regulators, etc. that simple-fb can. So if a platform needs those, do
> >>>> we say they should not setup EFI-GOP? Or is there a use case for
> >>>> having both? Clients that don't muck with resources can use EFI-GOP
> >>>> and those that do use simple-fb. For example, does/can grub use
> >>>> EFI-GOP, but not simple-fb?
> >>>>
> >>>
> >>> The EFI GOP is just a dumb framebuffer, and it is not even generally
> >>> possible to cross reference the GOP with a particular device in the
> >>> device hierarchy unless you e.g., compare the BARs of each device with
> >>> the region described by the GOP protocol.
> >>>
> >>> GRUB for EFI will use the GOP and nothing else, but only at boot time
> >>> (the GOP protocol is more than a magic linear memory region, it also
> >>> implements a Blt() abstraction that permits the use of framebuffers
> >>> that are not mapped linearly into the address space at all, and GRUB
> >>> makes use of this)
> >>>
> >>> The EFI stub will only expose GOPs to the kernel if they are in fact
> >>> linear framebuffers, but has zero insight into whether the hardware
> >>> needs clocks and regulators, and whether or not the framebuffer needs
> >>> IOMMU pass through (which might be the case if the scanout is using
> >>> DMA into system memory)
> >>>
> >>> So calling EFI GOP 'source of truth' is rather generous, and I think
> >>> it makes sense to prioritize more accurate descriptions of the
> >>> underlying framebuffer over EFI GOP.
> >>>
> >>
> >> That was my opinion as well and the reason why I called the DTB the
> >> single source of truth.
> >>
> >>> However, making 'simple-framebuffer' magic in this regard doesn't seem
> >>> like a great approach to me. Is there a better way we could get the
> >>> resource conflict to be decided in a way where the EFI GOP gets
> >>> superseded if its resources are claimed by another device?
> >>>
> >>
> >> There is an aperture [0] framework that is used by the fbdev and DRM
> >> subsystems to allow native drivers to remove any conflicting devices
> >> that share the same framebuffer aperture.
> >>
> >> But it only makes sense for native drivers to use that I think, but
> >> in this case is about two drivers that attempt to use the same frame
> >> buffer provided by the firmware but getting it from different places.
> >>
> >> I don't have a better idea than this patch but maybe Thomas or Sima do?
> >
> > At SUSE, we've carried such a patch in our repos for some time. It works
> > around the double-framebuffer problem in a similar way. [1]
> >
>
> Thanks for the information. I see that your patch is basically mine but
> doing it unconditionally while this one only does the sysfb_disable() if
> a "simple-framebuffer" DT node has been found.
>
> > As Javier mentioned, we do track the framebuffer ranges for EFI/VESA/OF
> > framebuffers in the graphics aperture helpers. Fbdev has done this for
> > decades, and the current codebase extends and harmonizes this
> > functionality among fbdev and DRM drivers.
> >
> > WRT DT vs EFI: AFAIK the EFI support on affected platforms looks at the
> > DT to set up the EFI framebuffer. So IMHO the DT is the authoritative
> > source on the framebuffer.
> >
>
> Agreed. Sima Vetter also mentioned on IRC that they think this patch is
> the least bad option. Rob, Ard any thoughts? Maybe we can land this as
> a fix and then figure how this could be better handled in the long term?

What about moving the DT setup code here to sysfb? Then we just setup
the right thing up front.

However, there might be one other issue with that and this fix. The DT
simplefb can have resources such as clocks and regulators. With
fw_devlink, the driver won't probe until those dependencies are met.
So if you want the framebuffer console up early, then you may want to
register the EFI framebuffer first and then handoff to the DT simplefb
when it probes (rather than registering the device).

But I agree, probably better to take this patch now and have those
quirks instead of flat out not working.

Rob

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Rob Herring <robh@kernel.org> writes:

Hello Rob,

> On Fri, Dec 1, 2023 at 4:21 AM Javier Martinez Canillas

[...]

>> >> I don't have a better idea than this patch but maybe Thomas or Sima do?
>> >
>> > At SUSE, we've carried such a patch in our repos for some time. It works
>> > around the double-framebuffer problem in a similar way. [1]
>> >
>>
>> Thanks for the information. I see that your patch is basically mine but
>> doing it unconditionally while this one only does the sysfb_disable() if
>> a "simple-framebuffer" DT node has been found.
>>
>> > As Javier mentioned, we do track the framebuffer ranges for EFI/VESA/OF
>> > framebuffers in the graphics aperture helpers. Fbdev has done this for
>> > decades, and the current codebase extends and harmonizes this
>> > functionality among fbdev and DRM drivers.
>> >
>> > WRT DT vs EFI: AFAIK the EFI support on affected platforms looks at the
>> > DT to set up the EFI framebuffer. So IMHO the DT is the authoritative
>> > source on the framebuffer.
>> >
>>
>> Agreed. Sima Vetter also mentioned on IRC that they think this patch is
>> the least bad option. Rob, Ard any thoughts? Maybe we can land this as
>> a fix and then figure how this could be better handled in the long term?
>
> What about moving the DT setup code here to sysfb? Then we just setup
> the right thing up front.
>

Right now sysfb is pretty platform agnostic, in the sense that just looks
at the screen_info global struct and uses it to add the platform data that
contains the firmware provided framebuffer.

How the screen_info was filled (e.g: by the Linux EFI stub using the GOP
or the x86 early boot code using VESA) is transparent to sysfb. For this
reason adding platform specific logic there, like finding OF nodes, is not
desirable.

I also suggested to Thomas to move the DT setup code to sysfb but he said
that would be the wrong approach for the reason mentioned above. Please
correct me Thomas if I'm misremembering here.

> However, there might be one other issue with that and this fix. The DT
> simplefb can have resources such as clocks and regulators. With
> fw_devlink, the driver won't probe until those dependencies are met.
> So if you want the framebuffer console up early, then you may want to
> register the EFI framebuffer first and then handoff to the DT simplefb
> when it probes (rather than registering the device).
>
> But I agree, probably better to take this patch now and have those
> quirks instead of flat out not working.
>

If we do that what's the plan? Are you thinking about merging this patch
through your OF tree or do you want to go through drm-misc with your ack?

> Rob
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Rob Herring]

On Mon, Dec 4, 2023 at 3:39 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Rob Herring <robh@kernel.org> writes:
>
> Hello Rob,
>
> > On Fri, Dec 1, 2023 at 4:21 AM Javier Martinez Canillas
>
> [...]
>
> >> >> I don't have a better idea than this patch but maybe Thomas or Sima do?
> >> >
> >> > At SUSE, we've carried such a patch in our repos for some time. It works
> >> > around the double-framebuffer problem in a similar way. [1]
> >> >
> >>
> >> Thanks for the information. I see that your patch is basically mine but
> >> doing it unconditionally while this one only does the sysfb_disable() if
> >> a "simple-framebuffer" DT node has been found.
> >>
> >> > As Javier mentioned, we do track the framebuffer ranges for EFI/VESA/OF
> >> > framebuffers in the graphics aperture helpers. Fbdev has done this for
> >> > decades, and the current codebase extends and harmonizes this
> >> > functionality among fbdev and DRM drivers.
> >> >
> >> > WRT DT vs EFI: AFAIK the EFI support on affected platforms looks at the
> >> > DT to set up the EFI framebuffer. So IMHO the DT is the authoritative
> >> > source on the framebuffer.
> >> >
> >>
> >> Agreed. Sima Vetter also mentioned on IRC that they think this patch is
> >> the least bad option. Rob, Ard any thoughts? Maybe we can land this as
> >> a fix and then figure how this could be better handled in the long term?
> >
> > What about moving the DT setup code here to sysfb? Then we just setup
> > the right thing up front.
> >
>
> Right now sysfb is pretty platform agnostic, in the sense that just looks
> at the screen_info global struct and uses it to add the platform data that
> contains the firmware provided framebuffer.
>
> How the screen_info was filled (e.g: by the Linux EFI stub using the GOP
> or the x86 early boot code using VESA) is transparent to sysfb. For this
> reason adding platform specific logic there, like finding OF nodes, is not
> desirable.
>
> I also suggested to Thomas to move the DT setup code to sysfb but he said
> that would be the wrong approach for the reason mentioned above. Please
> correct me Thomas if I'm misremembering here.
>
> > However, there might be one other issue with that and this fix. The DT
> > simplefb can have resources such as clocks and regulators. With
> > fw_devlink, the driver won't probe until those dependencies are met.
> > So if you want the framebuffer console up early, then you may want to
> > register the EFI framebuffer first and then handoff to the DT simplefb
> > when it probes (rather than registering the device).
> >
> > But I agree, probably better to take this patch now and have those
> > quirks instead of flat out not working.
> >
>
> If we do that what's the plan? Are you thinking about merging this patch
> through your OF tree or do you want to go through drm-misc with your ack?

I can take it. Do we need this in 6.7 and stable?

Rob

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Rob Herring <robh@kernel.org> writes:

> On Mon, Dec 4, 2023 at 3:39 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Rob Herring <robh@kernel.org> writes:
>> > On Fri, Dec 1, 2023 at 4:21 AM Javier Martinez Canillas

[...]

>>
>> > However, there might be one other issue with that and this fix. The DT
>> > simplefb can have resources such as clocks and regulators. With
>> > fw_devlink, the driver won't probe until those dependencies are met.
>> > So if you want the framebuffer console up early, then you may want to
>> > register the EFI framebuffer first and then handoff to the DT simplefb
>> > when it probes (rather than registering the device).
>> >
>> > But I agree, probably better to take this patch now and have those
>> > quirks instead of flat out not working.
>> >
>>
>> If we do that what's the plan? Are you thinking about merging this patch
>> through your OF tree or do you want to go through drm-misc with your ack?
>
> I can take it. Do we need this in 6.7 and stable?
>

IMO this can wait for v6.8 since is not a fix for a change introduced in
the v6.7 merge window and something that only happens on a very specific
setup (DT systems booting with u-boot EFI and providing an EFI-GOP table).

Also the -rc cycle is already in -rc5, so it seems risky to push a change
at this point. And distros can pick the patch if want to have it earlier.

> Rob
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Rob Herring]

On Mon, Dec 04, 2023 at 05:05:30PM +0100, Javier Martinez Canillas wrote:
> Rob Herring <robh@kernel.org> writes:
> 
> > On Mon, Dec 4, 2023 at 3:39 AM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> Rob Herring <robh@kernel.org> writes:
> >> > On Fri, Dec 1, 2023 at 4:21 AM Javier Martinez Canillas
> 
> [...]
> 
> >>
> >> > However, there might be one other issue with that and this fix. The DT
> >> > simplefb can have resources such as clocks and regulators. With
> >> > fw_devlink, the driver won't probe until those dependencies are met.
> >> > So if you want the framebuffer console up early, then you may want to
> >> > register the EFI framebuffer first and then handoff to the DT simplefb
> >> > when it probes (rather than registering the device).
> >> >
> >> > But I agree, probably better to take this patch now and have those
> >> > quirks instead of flat out not working.
> >> >
> >>
> >> If we do that what's the plan? Are you thinking about merging this patch
> >> through your OF tree or do you want to go through drm-misc with your ack?
> >
> > I can take it. Do we need this in 6.7 and stable?
> >
> 
> IMO this can wait for v6.8 since is not a fix for a change introduced in
> the v6.7 merge window and something that only happens on a very specific
> setup (DT systems booting with u-boot EFI and providing an EFI-GOP table).
> 
> Also the -rc cycle is already in -rc5, so it seems risky to push a change
> at this point. And distros can pick the patch if want to have it earlier.

Okay, I've applied it for 6.8.

Rob

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Rob Herring <robh@kernel.org> writes:

> On Mon, Dec 04, 2023 at 05:05:30PM +0100, Javier Martinez Canillas wrote:
>> Rob Herring <robh@kernel.org> writes:
>> 
>> > On Mon, Dec 4, 2023 at 3:39 AM Javier Martinez Canillas
>> > <javierm@redhat.com> wrote:
>> >> Rob Herring <robh@kernel.org> writes:
>> >> > On Fri, Dec 1, 2023 at 4:21 AM Javier Martinez Canillas
>> 
>> [...]
>> 
>> >>
>> >> > However, there might be one other issue with that and this fix. The DT
>> >> > simplefb can have resources such as clocks and regulators. With
>> >> > fw_devlink, the driver won't probe until those dependencies are met.
>> >> > So if you want the framebuffer console up early, then you may want to
>> >> > register the EFI framebuffer first and then handoff to the DT simplefb
>> >> > when it probes (rather than registering the device).
>> >> >
>> >> > But I agree, probably better to take this patch now and have those
>> >> > quirks instead of flat out not working.
>> >> >
>> >>
>> >> If we do that what's the plan? Are you thinking about merging this patch
>> >> through your OF tree or do you want to go through drm-misc with your ack?
>> >
>> > I can take it. Do we need this in 6.7 and stable?
>> >
>> 
>> IMO this can wait for v6.8 since is not a fix for a change introduced in
>> the v6.7 merge window and something that only happens on a very specific
>> setup (DT systems booting with u-boot EFI and providing an EFI-GOP table).
>> 
>> Also the -rc cycle is already in -rc5, so it seems risky to push a change
>> at this point. And distros can pick the patch if want to have it earlier.
>
> Okay, I've applied it for 6.8.
>

Great, thanks a lot.

> Rob
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer node is found

[Javier Martinez Canillas]

Rob Herring <robh@kernel.org> writes:

> On Thu, Nov 16, 2023 at 3:36 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:

[...]

>> >
>> > This is the opposite of what we do for memory and memory reservations.
>> > EFI is the source of truth for those.
>> >
>> > This could also lead to an interesting scenario. As simple-framebuffer
>> > can define its memory in a /reserved-memory node, but that is ignored
>> > in EFI boot. Probably would work, but only because EFI probably
>> > generates its memory map table from the /reserved-memory nodes.
>> >
>>
>> I see. So what would be the solution then? Ignoring creating a platform
>> device for "simple-framebuffer" if booted using EFI and have an EFI-GOP?
>
> Shrug. I don't really know anything more about EFI FB, but I would
> guess it can't support handling resources like clocks, power domains,
> regulators, etc. that simple-fb can. So if a platform needs those, do

That's correct, and the reason why I thought that the DTB would be the
single source of truth for the firmware provided framebuffer.

For example, in some arm platforms that u-boot does provide an EFI-GOP,
you need to boot with clk_ignore_unused or the system framebuffer just
goes away once the unused clocks are gated. Same for PD, regulators, etc.

> we say they should not setup EFI-GOP? Or is there a use case for
> having both? Clients that don't muck with resources can use EFI-GOP
> and those that do use simple-fb. For example, does/can grub use
> EFI-GOP, but not simple-fb?
>

I don't think grub can use the simple-fb, it can use the EFI-GOP if is
available though. And things work because of course grub won't try to
disable unused resources like Linux does.

> Rob
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat