[PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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);
-- 
2.27.0

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Greg KH]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Bjorn Helgaas]

[+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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Bjorn Helgaas]

[+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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Bjorn Helgaas]

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().

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Bjorn Helgaas]

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().

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Bjorn Helgaas]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Bjorn Helgaas]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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

Re: [PATCH] vgaarb: Call vga_arb_device_init() after PCI enumeration

[Huacai Chen]

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