| Message ID | 20231113085305.1823455-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [RFC] of/platform: Disable sysfb if a simple-framebuffer node is found | expand |
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);
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
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 >
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
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
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 >
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
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?
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 >
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
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 <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 >
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
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 >
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
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 >
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
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 >
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);
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(-)