We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Signed-off-by: Huacai Chen chenhuacai@loongson.cn --- drivers/gpu/vga/vgaarb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 5180c5687ee5..4b8a62af34cf 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -1564,4 +1564,4 @@ static int __init vga_arb_device_init(void) pr_info("loaded\n"); return rc; } -subsys_initcall(vga_arb_device_init); +subsys_initcall_sync(vga_arb_device_init);
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote:
We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Signed-off-by: Huacai Chen chenhuacai@loongson.cn
Is there a bug report for this? Should we mark this cc: stable?
Also adding Bjorn/Greg since I have no idea whether this is the right fix. It does do what it says on the tin, but that's maybe not the best approach here. -Daniel
drivers/gpu/vga/vgaarb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 5180c5687ee5..4b8a62af34cf 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -1564,4 +1564,4 @@ static int __init vga_arb_device_init(void) pr_info("loaded\n"); return rc; } -subsys_initcall(vga_arb_device_init);
+subsys_initcall_sync(vga_arb_device_init);
2.27.0
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote:
We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
thanks,
greg k-h
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote:
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote:
We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Thoughts on this? -Daniel
Hi, Daniel,
On Wed, Jun 2, 2021 at 2:03 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote:
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote:
We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Emmm, this patch cannot handle the hotplug case and module case, it just handles the case that vgaarb, drm driver and pci all built-in. But I think this is enough, because the original problem only happens on very few BMC-based VGA cards (BMC doesn't set the VGA Enable bit on the bridge, which breaks vgaarb).
Huacai
Thoughts on this?
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Jun 02, 2021 at 06:36:03PM +0800, Huacai Chen wrote:
Hi, Daniel,
On Wed, Jun 2, 2021 at 2:03 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote:
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote:
We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Emmm, this patch cannot handle the hotplug case and module case, it just handles the case that vgaarb, drm driver and pci all built-in. But I think this is enough, because the original problem only happens on very few BMC-based VGA cards (BMC doesn't set the VGA Enable bit on the bridge, which breaks vgaarb).
I'm not talking aout hotplug, just ordering the various pieces correctly. That vgaarb isn't really a driver and also can't really handle hotplug is my point. I guess that got lost a bit?
Anyway my proposal is essentially to do a
$ git move drivers/gpu/vga/vgaarb.c drivers/pci
But I just realized that vgaarb is a bool option, so module isn't possible anyway, and we could fix this by calling vgaarb from pcibios init (with an empty static inline in the header if vgaarb is disabled). That makes the dependency very explicit and guarantees it works correctly.
Whether we move vgaarb into drivers/pci or not is then kinda orthogonal.
Thoughts on that? -Daniel
[+cc linux-pci]
On Wed, Jun 2, 2021 at 11:22 AM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jun 02, 2021 at 06:36:03PM +0800, Huacai Chen wrote:
On Wed, Jun 2, 2021 at 2:03 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote:
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote:
We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Emmm, this patch cannot handle the hotplug case and module case, it just handles the case that vgaarb, drm driver and pci all built-in. But I think this is enough, because the original problem only happens on very few BMC-based VGA cards (BMC doesn't set the VGA Enable bit on the bridge, which breaks vgaarb).
I'm not talking aout hotplug, just ordering the various pieces correctly. That vgaarb isn't really a driver and also can't really handle hotplug is my point. I guess that got lost a bit?
Anyway my proposal is essentially to do a
$ git move drivers/gpu/vga/vgaarb.c drivers/pci
But I just realized that vgaarb is a bool option, so module isn't possible anyway, and we could fix this by calling vgaarb from pcibios init (with an empty static inline in the header if vgaarb is disabled). That makes the dependency very explicit and guarantees it works correctly.
pcibios_init() is also an initcall and is implemented by every arch. I agree that calling vga_arb_device_init() directly from pcibios_init() would probably fix this problem, and it would be really nice to have it not be an initcall. But it's also kind of a pain to have to update all those copies of pcibios_init(), and I would be looking for a way to unify it since it's not really an arch-specific thing.
I think the simplest solution, which I suggested earlier [1], would be to explicitly call vga_arbiter_add_pci_device() directly from the PCI core when it enumerates a VGA device. Then there's no initcall and no need for the BUS_NOTIFY_ADD/DEL_DEVICE stuff. vga_arbiter_add_pci_device() could set the default VGA device when it is enumerated, and change the default device if we enumerate a "better" one. And hotplug VGA devices would work automatically.
Whether we move vgaarb into drivers/pci or not is then kinda orthogonal.
I'm fine with moving it to drivers/pci if that makes anything easier. It definitely is PCI-related stuff, not GPU-related stuff.
[1] https://lore.kernel.org/r/20210526182940.GA1303599@bjorn-Precision-5520
On Wed, Jun 02, 2021 at 01:31:16PM -0500, Bjorn Helgaas wrote:
On Wed, Jun 2, 2021 at 11:22 AM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jun 02, 2021 at 06:36:03PM +0800, Huacai Chen wrote:
On Wed, Jun 2, 2021 at 2:03 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote:
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote: > We should call vga_arb_device_init() after PCI enumeration, otherwise it > may fail to select the default VGA device. Since vga_arb_device_init() > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are > both wrapped by subsys_initcall(), their sequence is not assured. So, we > use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ > device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Emmm, this patch cannot handle the hotplug case and module case, it just handles the case that vgaarb, drm driver and pci all built-in. But I think this is enough, because the original problem only happens on very few BMC-based VGA cards (BMC doesn't set the VGA Enable bit on the bridge, which breaks vgaarb).
I'm not talking aout hotplug, just ordering the various pieces correctly. That vgaarb isn't really a driver and also can't really handle hotplug is my point. I guess that got lost a bit?
Anyway my proposal is essentially to do a
$ git move drivers/gpu/vga/vgaarb.c drivers/pci
But I just realized that vgaarb is a bool option, so module isn't possible anyway, and we could fix this by calling vgaarb from pcibios init (with an empty static inline in the header if vgaarb is disabled). That makes the dependency very explicit and guarantees it works correctly.
pcibios_init() is also an initcall and is implemented by every arch. I agree that calling vga_arb_device_init() directly from pcibios_init() would probably fix this problem, and it would be really nice to have it not be an initcall. But it's also kind of a pain to have to update all those copies of pcibios_init(), and I would be looking for a way to unify it since it's not really an arch-specific thing.
I think the simplest solution, which I suggested earlier [1], would be to explicitly call vga_arbiter_add_pci_device() directly from the PCI core when it enumerates a VGA device. Then there's no initcall and no need for the BUS_NOTIFY_ADD/DEL_DEVICE stuff. vga_arbiter_add_pci_device() could set the default VGA device when it is enumerated, and change the default device if we enumerate a "better" one. And hotplug VGA devices would work automatically.
Hm yeah that sounds most reasonable, and if it doesn't work I guess unifying the pcibios_init() stuff a bit and adding it there.
And somehow I missed that mail thread, too much stuff going on.
Whether we move vgaarb into drivers/pci or not is then kinda orthogonal.
I'm fine with moving it to drivers/pci if that makes anything easier. It definitely is PCI-related stuff, not GPU-related stuff.
[1] https://lore.kernel.org/r/20210526182940.GA1303599@bjorn-Precision-5520
Yeah I think it'd fit in pci better tbh, but not strong opinion I guess. If we move it we probably want to keep the entry to Cc: dri-devel still since it's really just for gpus. -Daniel
Hi, Bjorn,
On Thu, Jun 3, 2021 at 2:31 AM Bjorn Helgaas bhelgaas@google.com wrote:
[+cc linux-pci]
On Wed, Jun 2, 2021 at 11:22 AM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jun 02, 2021 at 06:36:03PM +0800, Huacai Chen wrote:
On Wed, Jun 2, 2021 at 2:03 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote:
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote: > We should call vga_arb_device_init() after PCI enumeration, otherwise it > may fail to select the default VGA device. Since vga_arb_device_init() > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are > both wrapped by subsys_initcall(), their sequence is not assured. So, we > use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ > device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Emmm, this patch cannot handle the hotplug case and module case, it just handles the case that vgaarb, drm driver and pci all built-in. But I think this is enough, because the original problem only happens on very few BMC-based VGA cards (BMC doesn't set the VGA Enable bit on the bridge, which breaks vgaarb).
I'm not talking aout hotplug, just ordering the various pieces correctly. That vgaarb isn't really a driver and also can't really handle hotplug is my point. I guess that got lost a bit?
Anyway my proposal is essentially to do a
$ git move drivers/gpu/vga/vgaarb.c drivers/pci
But I just realized that vgaarb is a bool option, so module isn't possible anyway, and we could fix this by calling vgaarb from pcibios init (with an empty static inline in the header if vgaarb is disabled). That makes the dependency very explicit and guarantees it works correctly.
pcibios_init() is also an initcall and is implemented by every arch. I agree that calling vga_arb_device_init() directly from pcibios_init() would probably fix this problem, and it would be really nice to have it not be an initcall. But it's also kind of a pain to have to update all those copies of pcibios_init(), and I would be looking for a way to unify it since it's not really an arch-specific thing.
I think the simplest solution, which I suggested earlier [1], would be to explicitly call vga_arbiter_add_pci_device() directly from the PCI core when it enumerates a VGA device. Then there's no initcall and no need for the BUS_NOTIFY_ADD/DEL_DEVICE stuff. vga_arbiter_add_pci_device() could set the default VGA device when it is enumerated, and change the default device if we enumerate a "better" one. And hotplug VGA devices would work automatically.
Emm, It seems that your solution has some difficulties to remove the whole initcall(vga_arb_device_init): we call vga_arbiter_add_pci_device() in pci_bus_add_device(), the list_for_each_entry() loop can be moved to vga_arbiter_check_bridge_sharing(), vga_arb_select_default_device() can be renamed to vga_arb_update_default_device() and be called in vga_arbiter_add_pci_device(), but how to handle misc_register(&vga_arb_device)?
Huacai
Whether we move vgaarb into drivers/pci or not is then kinda orthogonal.
I'm fine with moving it to drivers/pci if that makes anything easier. It definitely is PCI-related stuff, not GPU-related stuff.
[1] https://lore.kernel.org/r/20210526182940.GA1303599@bjorn-Precision-5520
On Fri, Jun 04, 2021 at 12:50:03PM +0800, Huacai Chen wrote:
On Thu, Jun 3, 2021 at 2:31 AM Bjorn Helgaas bhelgaas@google.com wrote:
[+cc linux-pci]
On Wed, Jun 2, 2021 at 11:22 AM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jun 02, 2021 at 06:36:03PM +0800, Huacai Chen wrote:
On Wed, Jun 2, 2021 at 2:03 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote:
On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote: > On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote: > > We should call vga_arb_device_init() after PCI enumeration, otherwise it > > may fail to select the default VGA device. Since vga_arb_device_init() > > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are > > both wrapped by subsys_initcall(), their sequence is not assured. So, we > > use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ > > device_init().
Trying to juggle levels like this always fails if you build the code as a module.
Why not fix it properly and handle the out-of-order loading by returning a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Emmm, this patch cannot handle the hotplug case and module case, it just handles the case that vgaarb, drm driver and pci all built-in. But I think this is enough, because the original problem only happens on very few BMC-based VGA cards (BMC doesn't set the VGA Enable bit on the bridge, which breaks vgaarb).
I'm not talking aout hotplug, just ordering the various pieces correctly. That vgaarb isn't really a driver and also can't really handle hotplug is my point. I guess that got lost a bit?
Anyway my proposal is essentially to do a
$ git move drivers/gpu/vga/vgaarb.c drivers/pci
But I just realized that vgaarb is a bool option, so module isn't possible anyway, and we could fix this by calling vgaarb from pcibios init (with an empty static inline in the header if vgaarb is disabled). That makes the dependency very explicit and guarantees it works correctly.
pcibios_init() is also an initcall and is implemented by every arch. I agree that calling vga_arb_device_init() directly from pcibios_init() would probably fix this problem, and it would be really nice to have it not be an initcall. But it's also kind of a pain to have to update all those copies of pcibios_init(), and I would be looking for a way to unify it since it's not really an arch-specific thing.
I think the simplest solution, which I suggested earlier [1], would be to explicitly call vga_arbiter_add_pci_device() directly from the PCI core when it enumerates a VGA device. Then there's no initcall and no need for the BUS_NOTIFY_ADD/DEL_DEVICE stuff. vga_arbiter_add_pci_device() could set the default VGA device when it is enumerated, and change the default device if we enumerate a "better" one. And hotplug VGA devices would work automatically.
Emm, It seems that your solution has some difficulties to remove the whole initcall(vga_arb_device_init): we call vga_arbiter_add_pci_device() in pci_bus_add_device(), the list_for_each_entry() loop can be moved to vga_arbiter_check_bridge_sharing(), vga_arb_select_default_device() can be renamed to vga_arb_update_default_device() and be called in vga_arbiter_add_pci_device(), but how to handle misc_register(&vga_arb_device)?
Might need to keep vga_arb_device_init() as an initcall, but remove everything from it except the misc_register().
Hi, Bjorn,
On Sat, Jun 5, 2021 at 3:56 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Fri, Jun 04, 2021 at 12:50:03PM +0800, Huacai Chen wrote:
On Thu, Jun 3, 2021 at 2:31 AM Bjorn Helgaas bhelgaas@google.com wrote:
[+cc linux-pci]
On Wed, Jun 2, 2021 at 11:22 AM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jun 02, 2021 at 06:36:03PM +0800, Huacai Chen wrote:
On Wed, Jun 2, 2021 at 2:03 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 01, 2021 at 07:12:27PM +0200, Greg KH wrote: > On Tue, Jun 01, 2021 at 05:56:40PM +0200, Daniel Vetter wrote: > > On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote: > > > We should call vga_arb_device_init() after PCI enumeration, otherwise it > > > may fail to select the default VGA device. Since vga_arb_device_init() > > > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are > > > both wrapped by subsys_initcall(), their sequence is not assured. So, we > > > use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ > > > device_init(). > > Trying to juggle levels like this always fails if you build the code as > a module. > > Why not fix it properly and handle the out-of-order loading by returning > a "deferred" error if you do not have your resources yet?
It's not a driver, it's kinda a bolted-on-the-side subsytem of pci. So not something you can -EPROBE_DEFER I think, without potentially upsetting the drivers that need this.
Which might mean we should move this into pci subsystem proper perhaps? Then adding the init call at the right time becomes trivial since we just plug it in at the end of pci init.
Also maybe that's how distros avoid this pain, pci is built-in, vgaarb is generally a module, problem solved.
Bjorn, would you take this entire vgaarb.c thing? From a quick look I don't think it has a drm-ism in it (unlike vga_switcheroo, but that works a bit differently and doesn't have this init order issue).
Emmm, this patch cannot handle the hotplug case and module case, it just handles the case that vgaarb, drm driver and pci all built-in. But I think this is enough, because the original problem only happens on very few BMC-based VGA cards (BMC doesn't set the VGA Enable bit on the bridge, which breaks vgaarb).
I'm not talking aout hotplug, just ordering the various pieces correctly. That vgaarb isn't really a driver and also can't really handle hotplug is my point. I guess that got lost a bit?
Anyway my proposal is essentially to do a
$ git move drivers/gpu/vga/vgaarb.c drivers/pci
But I just realized that vgaarb is a bool option, so module isn't possible anyway, and we could fix this by calling vgaarb from pcibios init (with an empty static inline in the header if vgaarb is disabled). That makes the dependency very explicit and guarantees it works correctly.
pcibios_init() is also an initcall and is implemented by every arch. I agree that calling vga_arb_device_init() directly from pcibios_init() would probably fix this problem, and it would be really nice to have it not be an initcall. But it's also kind of a pain to have to update all those copies of pcibios_init(), and I would be looking for a way to unify it since it's not really an arch-specific thing.
I think the simplest solution, which I suggested earlier [1], would be to explicitly call vga_arbiter_add_pci_device() directly from the PCI core when it enumerates a VGA device. Then there's no initcall and no need for the BUS_NOTIFY_ADD/DEL_DEVICE stuff. vga_arbiter_add_pci_device() could set the default VGA device when it is enumerated, and change the default device if we enumerate a "better" one. And hotplug VGA devices would work automatically.
Emm, It seems that your solution has some difficulties to remove the whole initcall(vga_arb_device_init): we call vga_arbiter_add_pci_device() in pci_bus_add_device(), the list_for_each_entry() loop can be moved to vga_arbiter_check_bridge_sharing(), vga_arb_select_default_device() can be renamed to vga_arb_update_default_device() and be called in vga_arbiter_add_pci_device(), but how to handle misc_register(&vga_arb_device)?
Might need to keep vga_arb_device_init() as an initcall, but remove everything from it except the misc_register().
OK, let me try. But I think call vga_arbiter_add_pci_device() in pci core is nearly the same as notifier. Anyway, I will send a new patch later.
Huacai
On Sat, Jun 05, 2021 at 10:02:05AM +0800, Huacai Chen wrote:
On Sat, Jun 5, 2021 at 3:56 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Fri, Jun 04, 2021 at 12:50:03PM +0800, Huacai Chen wrote:
On Thu, Jun 3, 2021 at 2:31 AM Bjorn Helgaas bhelgaas@google.com wrote:
I think the simplest solution, which I suggested earlier [1], would be to explicitly call vga_arbiter_add_pci_device() directly from the PCI core when it enumerates a VGA device. Then there's no initcall and no need for the BUS_NOTIFY_ADD/DEL_DEVICE stuff. vga_arbiter_add_pci_device() could set the default VGA device when it is enumerated, and change the default device if we enumerate a "better" one. And hotplug VGA devices would work automatically.
Emm, It seems that your solution has some difficulties to remove the whole initcall(vga_arb_device_init): we call vga_arbiter_add_pci_device() in pci_bus_add_device(), the list_for_each_entry() loop can be moved to vga_arbiter_check_bridge_sharing(), vga_arb_select_default_device() can be renamed to vga_arb_update_default_device() and be called in vga_arbiter_add_pci_device(), but how to handle misc_register(&vga_arb_device)?
Might need to keep vga_arb_device_init() as an initcall, but remove everything from it except the misc_register().
OK, let me try. But I think call vga_arbiter_add_pci_device() in pci core is nearly the same as notifier. Anyway, I will send a new patch later.
Notifiers are useful in some situations, for example, if a loadable module needs to be called when a device is added or removed.
But when possible, I will always choose a direct call instead because it's much less complicated. The VGA arbiter cannot be a loadable module, so I don't think there's any reason to use a notifier in this case.
Bjorn
Hi, Bjorn,
On Sun, Jun 6, 2021 at 1:59 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Sat, Jun 05, 2021 at 10:02:05AM +0800, Huacai Chen wrote:
On Sat, Jun 5, 2021 at 3:56 AM Bjorn Helgaas helgaas@kernel.org wrote:
On Fri, Jun 04, 2021 at 12:50:03PM +0800, Huacai Chen wrote:
On Thu, Jun 3, 2021 at 2:31 AM Bjorn Helgaas bhelgaas@google.com wrote:
I think the simplest solution, which I suggested earlier [1], would be to explicitly call vga_arbiter_add_pci_device() directly from the PCI core when it enumerates a VGA device. Then there's no initcall and no need for the BUS_NOTIFY_ADD/DEL_DEVICE stuff. vga_arbiter_add_pci_device() could set the default VGA device when it is enumerated, and change the default device if we enumerate a "better" one. And hotplug VGA devices would work automatically.
Emm, It seems that your solution has some difficulties to remove the whole initcall(vga_arb_device_init): we call vga_arbiter_add_pci_device() in pci_bus_add_device(), the list_for_each_entry() loop can be moved to vga_arbiter_check_bridge_sharing(), vga_arb_select_default_device() can be renamed to vga_arb_update_default_device() and be called in vga_arbiter_add_pci_device(), but how to handle misc_register(&vga_arb_device)?
Might need to keep vga_arb_device_init() as an initcall, but remove everything from it except the misc_register().
OK, let me try. But I think call vga_arbiter_add_pci_device() in pci core is nearly the same as notifier. Anyway, I will send a new patch later.
Notifiers are useful in some situations, for example, if a loadable module needs to be called when a device is added or removed.
But when possible, I will always choose a direct call instead because it's much less complicated. The VGA arbiter cannot be a loadable module, so I don't think there's any reason to use a notifier in this case.
I have done a new patch (see below) to solve this problem, but I still use a notifier. Because in my opinion, direct call is suitable in the same subsystem, and notifier is suitable when it is cross subsystem (vgaarb and pci are two different subsystems, at least now). If you still think it better to use direct call, I will send a new version.
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 5180c5687ee5..9bb3325cb26b 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -585,6 +585,78 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc) } EXPORT_SYMBOL(vga_put);
+static void vga_arb_update_default_device(struct vga_device *vgadev) +{ + struct pci_dev *pdev = vgadev->pdev; + struct device *dev = &pdev->dev; + struct vga_device *vgadev_default; +#if defined(CONFIG_X86) || defined(CONFIG_IA64) + int i; + unsigned long flags; + u64 base = screen_info.lfb_base; + u64 size = screen_info.lfb_size; + u64 limit; + resource_size_t start, end; +#endif + + /* Deal with VGA default device. Use first enabled one + * by default if arch doesn't have it's own hook + */ + if (!vga_default_device()) { + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) + vgaarb_info(dev, "setting as boot VGA device\n"); + else + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); + vga_set_default_device(pdev); + } + + vgadev_default = vgadev_find(vga_default); + + /* Overrided by a better device */ + if (((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0) && + ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { + vgaarb_info(dev, "overriding boot VGA device\n"); + vga_set_default_device(pdev); + } + +#if defined(CONFIG_X86) || defined(CONFIG_IA64) + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) + base |= (u64)screen_info.ext_lfb_base << 32; + + limit = base + size; + + /* + * Override vga_arbiter_add_pci_device()'s I/O based detection + * as it may take the wrong device (e.g. on Apple system under + * EFI). + * + * Select the device owning the boot framebuffer if there is + * one. + */ + + /* Does firmware framebuffer belong to us? */ + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { + flags = pci_resource_flags(vgadev->pdev, i); + + if ((flags & IORESOURCE_MEM) == 0) + continue; + + start = pci_resource_start(vgadev->pdev, i); + end = pci_resource_end(vgadev->pdev, i); + + if (!start || !end) + continue; + + if (base < start || limit >= end) + continue; + + if (vgadev->pdev != vga_default_device()) + vgaarb_info(dev, "overriding boot device\n"); + vga_set_default_device(vgadev->pdev); + } +#endif +} + /* * Rules for using a bridge to control a VGA descendant decoding: if a bridge * has only one VGA descendant then it can be used to control the VGA routing @@ -642,6 +714,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) } new_bus = new_bus->parent; } + + if (vgadev->bridge_has_one_vga == true) + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n"); + else + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n"); }
/* @@ -712,15 +789,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) bus = bus->parent; }
- /* Deal with VGA default device. Use first enabled one - * by default if arch doesn't have it's own hook - */ - if (vga_default == NULL && - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { - vgaarb_info(&pdev->dev, "setting as boot VGA device\n"); - vga_set_default_device(pdev); - } - + vga_arb_update_default_device(vgadev); vga_arbiter_check_bridge_sharing(vgadev);
/* Add to the list */ @@ -1450,91 +1519,9 @@ static struct miscdevice vga_arb_device = { MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops };
-static void __init vga_arb_select_default_device(void) -{ - struct pci_dev *pdev; - struct vga_device *vgadev; - -#if defined(CONFIG_X86) || defined(CONFIG_IA64) - u64 base = screen_info.lfb_base; - u64 size = screen_info.lfb_size; - u64 limit; - resource_size_t start, end; - unsigned long flags; - int i; - - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) - base |= (u64)screen_info.ext_lfb_base << 32; - - limit = base + size; - - list_for_each_entry(vgadev, &vga_list, list) { - struct device *dev = &vgadev->pdev->dev; - /* - * Override vga_arbiter_add_pci_device()'s I/O based detection - * as it may take the wrong device (e.g. on Apple system under - * EFI). - * - * Select the device owning the boot framebuffer if there is - * one. - */ - - /* Does firmware framebuffer belong to us? */ - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { - flags = pci_resource_flags(vgadev->pdev, i); - - if ((flags & IORESOURCE_MEM) == 0) - continue; - - start = pci_resource_start(vgadev->pdev, i); - end = pci_resource_end(vgadev->pdev, i); - - if (!start || !end) - continue; - - if (base < start || limit >= end) - continue; - - if (!vga_default_device()) - vgaarb_info(dev, "setting as boot device\n"); - else if (vgadev->pdev != vga_default_device()) - vgaarb_info(dev, "overriding boot device\n"); - vga_set_default_device(vgadev->pdev); - } - } -#endif - if (!vga_default_device()) { - list_for_each_entry(vgadev, &vga_list, list) { - struct device *dev = &vgadev->pdev->dev; - u16 cmd; - - pdev = vgadev->pdev; - pci_read_config_word(pdev, PCI_COMMAND, &cmd); - if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); - vga_set_default_device(pdev); - break; - } - } - } - - if (!vga_default_device()) { - vgadev = list_first_entry_or_null(&vga_list, - struct vga_device, list); - if (vgadev) { - struct device *dev = &vgadev->pdev->dev; - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); - vga_set_default_device(vgadev->pdev); - } - } -} - static int __init vga_arb_device_init(void) { int rc; - struct pci_dev *pdev; - struct vga_device *vgadev;
rc = misc_register(&vga_arb_device); if (rc < 0) @@ -1542,26 +1529,6 @@ static int __init vga_arb_device_init(void)
bus_register_notifier(&pci_bus_type, &pci_notifier);
- /* We add all PCI devices satisfying VGA class in the arbiter by - * default */ - pdev = NULL; - while ((pdev = - pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, - PCI_ANY_ID, pdev)) != NULL) - vga_arbiter_add_pci_device(pdev); - - list_for_each_entry(vgadev, &vga_list, list) { - struct device *dev = &vgadev->pdev->dev; - - if (vgadev->bridge_has_one_vga) - vgaarb_info(dev, "bridge control possible\n"); - else - vgaarb_info(dev, "no bridge control possible\n"); - } - - vga_arb_select_default_device(); - - pr_info("loaded\n"); return rc; } subsys_initcall(vga_arb_device_init);
Bjorn
Hi, Daniel,
On Tue, Jun 1, 2021 at 11:56 PM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 28, 2021 at 04:26:07PM +0800, Huacai Chen wrote:
We should call vga_arb_device_init() after PCI enumeration, otherwise it may fail to select the default VGA device. Since vga_arb_device_init() and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are both wrapped by subsys_initcall(), their sequence is not assured. So, we use subsys_initcall_sync() instead of subsys_initcall() to wrap vga_arb_ device_init().
Signed-off-by: Huacai Chen chenhuacai@loongson.cn
Is there a bug report for this? Should we mark this cc: stable?
No bug report yet, we just found vga_arb_device_init() has no effect on Loongson platform.
Huacai
Also adding Bjorn/Greg since I have no idea whether this is the right fix. It does do what it says on the tin, but that's maybe not the best approach here. -Daniel
drivers/gpu/vga/vgaarb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 5180c5687ee5..4b8a62af34cf 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -1564,4 +1564,4 @@ static int __init vga_arb_device_init(void) pr_info("loaded\n"); return rc; } -subsys_initcall(vga_arb_device_init);
+subsys_initcall_sync(vga_arb_device_init);
2.27.0
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Bjorn Helgaas -
Daniel Vetter -
Greg KH -
Huacai Chen -
Huacai Chen