Re: [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge

From: Dave Airlie <airlied@gmail.com>
To: Huacai Chen <chenhuacai@kernel.org>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Maling list - DRI developers  <dri-devel@lists.freedesktop.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci <linux-pci@vger.kernel.org>,
	Jingfeng Sui <suijingfeng@loongson.cn>,
	Jiaxun Yang <jiaxun.yang@flygoat.com>,
	Huacai Chen <chenhuacai@loongson.cn>
Subject: Re: [PATCH 5/5] PCI: Support ASpeed VGA cards behind a misbehaving bridge
Date: Wed, 26 May 2021 13:00:33 +1000	[thread overview]
Message-ID: <CAPM=9tx0kr7xdA8eB2+u6Xg0C7FSMbZEKGVKOTZEkdA7Kay42A@mail.gmail.com> (raw)
In-Reply-To: <CAAhV-H4Ayg9QyPBsXRqBbsn8OTEmN2yw7Bf3on63UVM950rARA@mail.gmail.com>

> > > > I think I would see if it's possible to call
> > > > vga_arb_select_default_device() from vga_arbiter_add_pci_device()
> > > > instead of from vga_arb_device_init().
> > > >
> > > > I would also (as a separate patch) try to get rid of this loop in
> > > > vga_arb_device_init():
> > > >
> > > >         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");
> > > >         }
> > > >
> > > > and do the vgaarb_info() in vga_arbiter_check_bridge_sharing(), where
> > > > the loop would not be needed.
> > >
> > > Any updates?
> >
> > Are you waiting for me to do something else?
> >
> > I suggested an approach above, but I don't have time to actually do
> > the work for you.
> Yes, I am really waiting... but I am also investigating history and thinking.
>
> If I haven't missed something (correct me if I'm wrong). For the
> original HiSilicon problem, the first attempt is to modify
> vga_arbiter_add_pci_device() and remove the VGA_RSRC_LEGACY_MASK
> check. But vga_arbiter_add_pci_device() is called for each PCI device,
> so removing that check will cause the first VGA device to be the
> default VGA device. This breaks some x86 platforms, so after that you
> don't touch vga_arbiter_add_pci_device(), but add
> vga_arb_select_default_device() in vga_arb_device_init().
>
> If the above history is correct, then we cannot add
> vga_arb_select_default_device() in vga_arbiter_add_pci_device()
> directly. So it seems we can only add vga_arb_select_default_device()
> in pci_notify(). And if we don't care about hotplug, we can simply use
> subsys_initcall_sync() to wrap vga_arb_device_init().
>
> And DRM developers, please let me know what do you think about?

I'm not 100% following what is going on here.

Do you need call vga_arb_select_default_device after hotplug for some
reason, or it this just a race with subsys_init?

I think just adding subsys_initcall_sync should be fine

I don't see why you'd want to care about making a hotplug VGA device
the default at this point.

Dave.