diff mbox series

[v6,3/5] fbdev: Disable sysfb device registration when removing conflicting FBs

Message ID 20220607182338.344270-4-javierm@redhat.com (mailing list archive)
State New, archived
Headers show
Series Fix some races between sysfb device registration and drivers probe | expand

Commit Message

Javier Martinez Canillas June 7, 2022, 6:23 p.m. UTC
The platform devices registered by sysfb match with firmware-based DRM or
fbdev drivers, that are used to have early graphics using a framebuffer
provided by the system firmware.

DRM or fbdev drivers later are probed and remove all conflicting framebuffers,
leading to these platform devices for generic drivers to be unregistered.

But the current solution has a race, since the sysfb_init() function could
be called after a DRM or fbdev driver is probed and request to unregister
the devices for drivers with conflicting framebuffes.

To prevent this, disable any future sysfb platform device registration by
calling sysfb_disable(), if a driver requests to remove the conflicting
framebuffers.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---

Changes in v6:
- Move the sysfb_disable() before the remove conflicting framebuffers
  loop (Daniel Vetter).

Changes in v5:
- Move the sysfb_disable() call at conflicting framebuffers again to
  avoid the need of a DRIVER_FIRMWARE capability flag.
- Add Daniel Vetter's Reviewed-by tag again since reverted to the old
  patch that he already reviewed in v2.

Changes in v3:
- Call sysfb_disable() when a DRM dev and a fbdev are registered rather
  than when conflicting framebuffers are removed (Thomas Zimmermann).
- Call sysfb_disable() when a fbdev framebuffer is registered rather
  than when conflicting framebuffers are removed (Thomas Zimmermann).
- Drop Daniel Vetter's Reviewed-by tag since patch changed a lot.

Changes in v2:
- Explain in the commit message that fbmem has to unregister the device
  as fallback if a driver registered the device itself (Daniel Vetter).
- Also explain that fallback in a comment in the code (Daniel Vetter).
- Don't encode in fbmem the assumption that sysfb will always register
  platform devices (Daniel Vetter).
- Add a FIXME comment about drivers registering devices (Daniel Vetter).

 drivers/video/fbdev/core/fbmem.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Zack Rusin June 16, 2022, 7:29 p.m. UTC | #1
On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
> The platform devices registered by sysfb match with firmware-based DRM or
> fbdev drivers, that are used to have early graphics using a framebuffer
> provided by the system firmware.
> 
> DRM or fbdev drivers later are probed and remove all conflicting framebuffers,
> leading to these platform devices for generic drivers to be unregistered.
> 
> But the current solution has a race, since the sysfb_init() function could
> be called after a DRM or fbdev driver is probed and request to unregister
> the devices for drivers with conflicting framebuffes.
> 
> To prevent this, disable any future sysfb platform device registration by
> calling sysfb_disable(), if a driver requests to remove the conflicting
> framebuffers.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> 
> Changes in v6:
> - Move the sysfb_disable() before the remove conflicting framebuffers
>   loop (Daniel Vetter).
> 
> Changes in v5:
> - Move the sysfb_disable() call at conflicting framebuffers again to
>   avoid the need of a DRIVER_FIRMWARE capability flag.
> - Add Daniel Vetter's Reviewed-by tag again since reverted to the old
>   patch that he already reviewed in v2.
> 
> Changes in v3:
> - Call sysfb_disable() when a DRM dev and a fbdev are registered rather
>   than when conflicting framebuffers are removed (Thomas Zimmermann).
> - Call sysfb_disable() when a fbdev framebuffer is registered rather
>   than when conflicting framebuffers are removed (Thomas Zimmermann).
> - Drop Daniel Vetter's Reviewed-by tag since patch changed a lot.
> 
> Changes in v2:
> - Explain in the commit message that fbmem has to unregister the device
>   as fallback if a driver registered the device itself (Daniel Vetter).
> - Also explain that fallback in a comment in the code (Daniel Vetter).
> - Don't encode in fbmem the assumption that sysfb will always register
>   platform devices (Daniel Vetter).
> - Add a FIXME comment about drivers registering devices (Daniel Vetter).
> 
>  drivers/video/fbdev/core/fbmem.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 2fda5917c212..e0720fef0ee6 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -19,6 +19,7 @@
>  #include <linux/kernel.h>
>  #include <linux/major.h>
>  #include <linux/slab.h>
> +#include <linux/sysfb.h>
>  #include <linux/mm.h>
>  #include <linux/mman.h>
>  #include <linux/vt.h>
> @@ -1764,6 +1765,17 @@ int remove_conflicting_framebuffers(struct apertures_struct *a,
>  		do_free = true;
>  	}
>  
> +	/*
> +	 * If a driver asked to unregister a platform device registered by
> +	 * sysfb, then can be assumed that this is a driver for a display
> +	 * that is set up by the system firmware and has a generic driver.
> +	 *
> +	 * Drivers for devices that don't have a generic driver will never
> +	 * ask for this, so let's assume that a real driver for the display
> +	 * was already probed and prevent sysfb to register devices later.
> +	 */
> +	sysfb_disable();
> +
>  	mutex_lock(&registration_lock);
>  	do_remove_conflicting_framebuffers(a, name, primary);
>  	mutex_unlock(&registration_lock);

Hi, Javier.

This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
you'd like .config or just have us test something directly for you):


 Unable to handle kernel NULL pointer dereference at virtual address
0000000000000008
 Mem abort info:
   ESR = 0x96000004
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x04: level 0 translation fault
 Data abort info:
   ISV = 0, ISS = 0x00000004
   CM = 0, WnR = 0
 user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
 [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
 Internal error: Oops: 96000004 [#1] SMP
 Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
 CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
#12
 Hardware name: VMware, Inc. VBSA/VBSA, BIOS VEFI 12/31/2020
 pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : kernfs_find_and_get_ns+0x2c/0x80
 lr : sysfs_unmerge_group+0x30/0x80
 sp : ffff80000b78b3f0
 x29: ffff80000b78b3f0 x28: ffff0000f7970910 x27: 0000000000000002
 x26: ffff0000f7970900 x25: ffff80000abca358 x24: ffff80000936cfa0
 x23: ffff80000ac8f458 x22: 0000000000000000 x21: ffff800009431938
 x20: ffff800009431800 x19: 0000000000000000 x18: 0000000000000000
 x17: 42555300302e7265 x16: 66667562656d6172 x15: 4d006d726f667461
 x14: 6c703d4d45545359 x13: 595342555300302e x12: 726566667562656d
 x11: 00323137323d4d55 x10: 4e51455300726566 x9 : ffff8000085f7400
 x8 : ffff0000f7970980 x7 : 0000000000000000 x6 : 0000000077ffffff
 x5 : 0000000070000000 x4 : ffff80000b78b548 x3 : ffff8000088b0420
 x2 : 0000000000000000 x1 : ffff800009431938 x0 : 0000000000000000
 Call trace:
  kernfs_find_and_get_ns+0x2c/0x80
  sysfs_unmerge_group+0x30/0x80
  dpm_sysfs_remove+0x3c/0x17c
  device_del+0xb0/0x3a0
  platform_device_del.part.0+0x24/0xb0
  platform_device_unregister+0x30/0x50
  sysfb_disable+0x4c/0x80
  remove_conflicting_framebuffers+0x40/0x100
  remove_conflicting_pci_framebuffers+0x128/0x240
  drm_aperture_remove_conflicting_pci_framebuffers+0xb8/0x170
  vmw_probe+0x50/0xd30 [vmwgfx]
  local_pci_probe+0x4c/0xc0
  pci_device_probe+0x1e8/0x230
  really_probe+0x18c/0x3f0
  __driver_probe_device+0x124/0x1c0
  driver_probe_device+0x44/0x140
  __driver_attach+0xe0/0x234
  bus_for_each_dev+0x7c/0xe0
  driver_attach+0x30/0x40
  bus_add_driver+0x158/0x250
  driver_register+0x84/0x140
  __pci_register_driver+0x50/0x5c
  vmw_pci_driver_init+0x44/0x1000 [vmwgfx]
  do_one_initcall+0x50/0x250
  do_init_module+0x50/0x260
  load_module+0x23e4/0x27c0
  __do_sys_finit_module+0xac/0x12c
  __arm64_sys_finit_module+0x2c/0x40
  invoke_syscall+0x78/0x100
  el0_svc_common.constprop.0+0x54/0x184
  do_el0_svc+0x34/0x9c
  el0_svc+0x54/0x1e0
  el0t_64_sync_handler+0xa4/0x130
  el0t_64_sync+0x1a0/0x1a4
 Code: aa0003f3 a9025bf5 aa0103f5 aa0203f6 (f9400400)
 ---[ end trace 0000000000000000 ]---
Javier Martinez Canillas June 16, 2022, 7:55 p.m. UTC | #2
Hello Zack,

On 6/16/22 21:29, Zack Rusin wrote:
> On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
>> The platform devices registered by sysfb match with firmware-based DRM or
>> fbdev drivers, that are used to have early graphics using a framebuffer
>> provided by the system firmware.
>>

[snip]

> 
> Hi, Javier.
> 
> This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
> you'd like .config or just have us test something directly for you):
>

Yes please share your .config and I'll try to reproduce on an arm64 machine.

> 
>  Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000008
>  Mem abort info:
>    ESR = 0x96000004
>    EC = 0x25: DABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>    FSC = 0x04: level 0 translation fault
>  Data abort info:
>    ISV = 0, ISS = 0x00000004
>    CM = 0, WnR = 0
>  user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
>  [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
>  Internal error: Oops: 96000004 [#1] SMP
>  Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
> sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
>  CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
> #12

I'm confused, your kernel version seems to be 5.18.0-rc5 but this patch
is only in drm-misc-next now and will land in 5.20...

Did you backport it? Can you please try to reproduce with latest drm-tip ?
Zack Rusin June 16, 2022, 9:03 p.m. UTC | #3
On Thu, 2022-06-16 at 21:55 +0200, Javier Martinez Canillas wrote:
> Hello Zack,
> 
> On 6/16/22 21:29, Zack Rusin wrote:
> > On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
> > > The platform devices registered by sysfb match with firmware-based DRM or
> > > fbdev drivers, that are used to have early graphics using a framebuffer
> > > provided by the system firmware.
> > > 
> 
> [snip]
> 
> > 
> > Hi, Javier.
> > 
> > This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
> > you'd like .config or just have us test something directly for you):
> > 
> 
> Yes please share your .config and I'll try to reproduce on an arm64 machine.

Attached. It might be a little hard to reproduce unless you have an arm64 machine
with a dedicated gpu. You'll need a system that actually transitions from a generic
fb driver (e.g. efifb) to the dedicated one.

> > 
> >  Unable to handle kernel NULL pointer dereference at virtual address
> > 0000000000000008
> >  Mem abort info:
> >    ESR = 0x96000004
> >    EC = 0x25: DABT (current EL), IL = 32 bits
> >    SET = 0, FnV = 0
> >    EA = 0, S1PTW = 0
> >    FSC = 0x04: level 0 translation fault
> >  Data abort info:
> >    ISV = 0, ISS = 0x00000004
> >    CM = 0, WnR = 0
> >  user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
> >  [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
> >  Internal error: Oops: 96000004 [#1] SMP
> >  Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
> > sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
> >  CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
> > #12
> 
> I'm confused, your kernel version seems to be 5.18.0-rc5 but this patch
> is only in drm-misc-next now and will land in 5.20...
> 
> Did you backport it? Can you please try to reproduce with latest drm-tip ?

No, this is drm-misc-next as of yesterday. drm-misc-next was still on 5.18.0-rc5
yesterday.

z
Javier Martinez Canillas June 16, 2022, 10:18 p.m. UTC | #4
On 6/16/22 23:03, Zack Rusin wrote:
> On Thu, 2022-06-16 at 21:55 +0200, Javier Martinez Canillas wrote:
>> Hello Zack,
>>
>> On 6/16/22 21:29, Zack Rusin wrote:
>>> On Tue, 2022-06-07 at 20:23 +0200, Javier Martinez Canillas wrote:
>>>> The platform devices registered by sysfb match with firmware-based DRM or
>>>> fbdev drivers, that are used to have early graphics using a framebuffer
>>>> provided by the system firmware.
>>>>
>>
>> [snip]
>>
>>>
>>> Hi, Javier.
>>>
>>> This change broke arm64 with vmwgfx. We get a kernel oops at boot (let me know if
>>> you'd like .config or just have us test something directly for you):
>>>
>>
>> Yes please share your .config and I'll try to reproduce on an arm64 machine.
> 
> Attached. It might be a little hard to reproduce unless you have an arm64 machine
> with a dedicated gpu. You'll need a system that actually transitions from a generic
> fb driver (e.g. efifb) to the dedicated one.
>

Yes, all my testing for this was done with a rpi4 so I should be able to reproduce
that case. I'm confused though because I tested efifb -> vc4, simplefb -> vc4 and
simpledrm -> vc4.
 
>>>
>>>  Unable to handle kernel NULL pointer dereference at virtual address
>>> 0000000000000008
>>>  Mem abort info:
>>>    ESR = 0x96000004
>>>    EC = 0x25: DABT (current EL), IL = 32 bits
>>>    SET = 0, FnV = 0
>>>    EA = 0, S1PTW = 0
>>>    FSC = 0x04: level 0 translation fault
>>>  Data abort info:
>>>    ISV = 0, ISS = 0x00000004
>>>    CM = 0, WnR = 0
>>>  user pgtable: 4k pages, 48-bit VAs, pgdp=00000001787ee000
>>>  [0000000000000008] pgd=0000000000000000, p4d=0000000000000000
>>>  Internal error: Oops: 96000004 [#1] SMP
>>>  Modules linked in: vmwgfx(+) e1000e(+) nvme ahci(+) xhci_pci drm_ttm_helper ttm
>>> sha256_arm64 sha1_ce nvme_core xhci_pci_renesas aes_neon_bs aes_neon_blk aes>
>>>  CPU: 3 PID: 215 Comm: systemd-udevd Tainted: G     U            5.18.0-rc5-vmwgfx
>>> #12
>>
>> I'm confused, your kernel version seems to be 5.18.0-rc5 but this patch
>> is only in drm-misc-next now and will land in 5.20...
>>
>> Did you backport it? Can you please try to reproduce with latest drm-tip ?
> 
> No, this is drm-misc-next as of yesterday. drm-misc-next was still on 5.18.0-rc5
> yesterday.
> 

Right! I looked at the base for drm-tip but forgot that drm-misc was still on 5.18.

I'll look at this tomorrow but in the meantime, could you please look if the following
commits on top of drm-misc-next help ?

d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
Javier Martinez Canillas June 16, 2022, 11:21 p.m. UTC | #5
On 6/17/22 00:18, Javier Martinez Canillas wrote:
> On 6/16/22 23:03, Zack Rusin wrote:

[snip]

> 
> I'll look at this tomorrow but in the meantime, could you please look if the following
> commits on top of drm-misc-next help ?
> 
> d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
> 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
> 

Scratch that. I see in your config now that you are not using efifb but instead
simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.

Since you mentioned efifb I misunderstood that you are using it. Anyways, as
said I'll investigate this tomorrow.
Zack Rusin June 17, 2022, 1:35 a.m. UTC | #6
On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:
> On 6/17/22 00:18, Javier Martinez Canillas wrote:
> > On 6/16/22 23:03, Zack Rusin wrote:
> 
> [snip]
> 
> > 
> > I'll look at this tomorrow but in the meantime, could you please look if the following
> > commits on top of drm-misc-next help ?
> > 
> > d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
> > 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
> > 
> 
> Scratch that. I see in your config now that you are not using efifb but instead
> simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.
> 
> Since you mentioned efifb I misunderstood that you are using it. Anyways, as
> said I'll investigate this tomorrow.

Sounds good. Let me know if you'd like me to try it without SIMPLEFB.

z
Javier Martinez Canillas June 17, 2022, 6:46 a.m. UTC | #7
Hello Zack,

On 6/17/22 03:35, Zack Rusin wrote:
> On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:
>> On 6/17/22 00:18, Javier Martinez Canillas wrote:
>>> On 6/16/22 23:03, Zack Rusin wrote:
>>
>> [snip]
>>
>>>
>>> I'll look at this tomorrow but in the meantime, could you please look if the following
>>> commits on top of drm-misc-next help ?
>>>
>>> d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
>>> 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
>>>
>>
>> Scratch that. I see in your config now that you are not using efifb but instead
>> simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.
>>
>> Since you mentioned efifb I misunderstood that you are using it. Anyways, as
>> said I'll investigate this tomorrow.
> 
> Sounds good. Let me know if you'd like me to try it without SIMPLEFB.
>

Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
enabled (so that "efi-framebuffer" is registered and efifb probed) or with
CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
is used too but with simplefb instead of simpledrm).
 
I'm not able to reproduce, it would be useful to have another data point.
Xi Ruoyao July 4, 2022, 9:36 a.m. UTC | #8
On Fri, 2022-06-17 at 08:46 +0200, Javier Martinez Canillas wrote:
> Hello Zack,
> 
> On 6/17/22 03:35, Zack Rusin wrote:
> > On Fri, 2022-06-17 at 01:21 +0200, Javier Martinez Canillas wrote:
> > > On 6/17/22 00:18, Javier Martinez Canillas wrote:
> > > > On 6/16/22 23:03, Zack Rusin wrote:
> > > 
> > > [snip]
> > > 
> > > > 
> > > > I'll look at this tomorrow but in the meantime, could you please look if the following
> > > > commits on top of drm-misc-next help ?
> > > > 
> > > > d258d00fb9c7 fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove
> > > > 1b5853dfab7f fbdev: efifb: Fix a use-after-free due early fb_info cleanup
> > > > 
> > > 
> > > Scratch that. I see in your config now that you are not using efifb but instead
> > > simpledrm: CONFIG_DRM_SIMPLEDRM=y, CONFIG_SYSFB_SIMPLEFB=y and CONFIG_DRM_VMWGFX.
> > > 
> > > Since you mentioned efifb I misunderstood that you are using it. Anyways, as
> > > said I'll investigate this tomorrow.
> > 
> > Sounds good. Let me know if you'd like me to try it without SIMPLEFB.
> > 
> 
> Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
> enabled (so that "efi-framebuffer" is registered and efifb probed) or with
> CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
> is used too but with simplefb instead of simpledrm).
>  
> I'm not able to reproduce, it would be useful to have another data point.

Also happening for me with CONFIG_SYSFB_SIMPLEFB, on a Intel Core i7-
1065G7 (with iGPU).

Reverting this commit on top of 5.19-rc5 "fixes" the issue.
Xi Ruoyao July 4, 2022, 10:29 a.m. UTC | #9
On Mon, 2022-07-04 at 17:36 +0800, Xi Ruoyao wrote:

> > Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
> > enabled (so that "efi-framebuffer" is registered and efifb probed) or with
> > CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
> > is used too but with simplefb instead of simpledrm).
> >  
> > I'm not able to reproduce, it would be useful to have another data point.
> 
> Also happening for me with CONFIG_SYSFB_SIMPLEFB, on a Intel Core i7-
> 1065G7 (with iGPU).
> 
> Reverting this commit on top of 5.19-rc5 "fixes" the issue.

With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
issue.

I guess it's something going wrong on a "drm -> drm" pass over.  For now
I'll continue to use simpledrm with this commit reverted.
Javier Martinez Canillas July 4, 2022, 11:04 a.m. UTC | #10
Hello Xi,

On 7/4/22 12:29, Xi Ruoyao wrote:
> On Mon, 2022-07-04 at 17:36 +0800, Xi Ruoyao wrote:
> 
>>> Yes, please do. Either with CONFIG_SYSFB_SIMPLEFB disabled and CONFIG_FB_EFI
>>> enabled (so that "efi-framebuffer" is registered and efifb probed) or with
>>> CONFIG_SYSFB_SIMPLEFB but CONFIG_FB_SIMPLE enabled (so "simple-framebuffer
>>> is used too but with simplefb instead of simpledrm).
>>>  
>>> I'm not able to reproduce, it would be useful to have another data point.
>>
>> Also happening for me with CONFIG_SYSFB_SIMPLEFB, on a Intel Core i7-
>> 1065G7 (with iGPU).
>>
>> Reverting this commit on top of 5.19-rc5 "fixes" the issue.
> 
> With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
> issue.
> 
> I guess it's something going wrong on a "drm -> drm" pass over.  For now
> I'll continue to use simpledrm with this commit reverted.
> 

Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
removal before internal helpers") now that the sysfb_disable() patches
are in v5.19-rc5.
Xi Ruoyao July 4, 2022, 12:11 p.m. UTC | #11
On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:
> Hello Xi,
> > 
> > With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
> > issue.
> > 
> > I guess it's something going wrong on a "drm -> drm" pass over.  For now
> > I'll continue to use simpledrm with this commit reverted.
> > 
> 
> Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
> removal before internal helpers") now that the sysfb_disable() patches
> are in v5.19-rc5.

I confirm that cherry-picking b84efa28a48 fixes the issue for v5.19-rc5.
Javier Martinez Canillas July 4, 2022, 12:22 p.m. UTC | #12
On 7/4/22 14:11, Xi Ruoyao wrote:
> On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:
>> Hello Xi,
>>>
>>> With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
>>> issue.
>>>
>>> I guess it's something going wrong on a "drm -> drm" pass over.  For now
>>> I'll continue to use simpledrm with this commit reverted.
>>>
>>
>> Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
>> removal before internal helpers") now that the sysfb_disable() patches
>> are in v5.19-rc5.
> 
> I confirm that cherry-picking b84efa28a48 fixes the issue for v5.19-rc5.
> 

Thanks for testing it! Thomas is getting that patch through the drm-fixes
path, so it should make it to the v5.19-rc cycle at some point.
Thomas Zimmermann July 4, 2022, 12:22 p.m. UTC | #13
Hi

Am 04.07.22 um 14:11 schrieb Xi Ruoyao:
> On Mon, 2022-07-04 at 13:04 +0200, Javier Martinez Canillas wrote:
>> Hello Xi,
>>>
>>> With CONFIG_SYSFB_SIMPLEFB and CONFIG_FB_SIMPLE enabled, there is no
>>> issue.
>>>
>>> I guess it's something going wrong on a "drm -> drm" pass over.  For now
>>> I'll continue to use simpledrm with this commit reverted.
>>>
>>
>> Yes, we need to also cherry-pick b84efa28a48 ("drm/aperture: Run fbdev
>> removal before internal helpers") now that the sysfb_disable() patches
>> are in v5.19-rc5.
> 
> I confirm that cherry-picking b84efa28a48 fixes the issue for v5.19-rc5.

I've cherry-picked the commit into drm-misc-fixes. It will show up in 
mainline soon.

Best regards
Thomas
diff mbox series

Patch

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 2fda5917c212..e0720fef0ee6 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -19,6 +19,7 @@ 
 #include <linux/kernel.h>
 #include <linux/major.h>
 #include <linux/slab.h>
+#include <linux/sysfb.h>
 #include <linux/mm.h>
 #include <linux/mman.h>
 #include <linux/vt.h>
@@ -1764,6 +1765,17 @@  int remove_conflicting_framebuffers(struct apertures_struct *a,
 		do_free = true;
 	}
 
+	/*
+	 * If a driver asked to unregister a platform device registered by
+	 * sysfb, then can be assumed that this is a driver for a display
+	 * that is set up by the system firmware and has a generic driver.
+	 *
+	 * Drivers for devices that don't have a generic driver will never
+	 * ask for this, so let's assume that a real driver for the display
+	 * was already probed and prevent sysfb to register devices later.
+	 */
+	sysfb_disable();
+
 	mutex_lock(&registration_lock);
 	do_remove_conflicting_framebuffers(a, name, primary);
 	mutex_unlock(&registration_lock);