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

[Andrew Worsley <amworsley@gmail.com>]

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