| Message ID | 20190510142958.28017-1-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "vgaarb: Keep adding VGA device in queue" | expand |
On Fri, May 10, 2019 at 09:29:58AM -0500, Alex Deucher wrote: > This breaks multiple graphics cards in the Amigaone x5000 > on PPC. > > This reverts commit 3d42f1ddc47a69c0ce155f9f30d764c4d689a5fa. > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=109345 > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > CC: Aaron Ma <aaron.ma@canonical.com> Given that the bug is a bit a mess I think we need to add a bit more context here in the commit message. My understanding: Goal of the revert commit was to make the integrated boot device the primary one, if we can't detect which one is the boot device, instead of the last one. Which makes some sense. Now people have relied on the kernel picking the last one, which usually is an add-on card, and therefore simply plugging in an add-on card allows them to overwrite the default choice. Which also makes sense, and since it's the older behaviour, wins. I think it'd be good to add a comment here that this behaviour has become uapi, e.g. /* Add at the front so that we pick the last device as fallback * default, with the usual result that plug in cards are preferred * over integrated graphics. */ With that (or similar) and more commit message context: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > 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 f2f3ef8af271..8a3c45219a2a 100644 > --- a/drivers/gpu/vga/vgaarb.c > +++ b/drivers/gpu/vga/vgaarb.c > @@ -725,7 +725,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > vga_arbiter_check_bridge_sharing(vgadev); > > /* Add to the list */ > - list_add_tail(&vgadev->list, &vga_list); > + list_add(&vgadev->list, &vga_list); > vga_count++; > vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", > vga_iostate_to_str(vgadev->decodes), > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2019-05-10 4:29 p.m., Alex Deucher wrote: > This breaks multiple graphics cards in the Amigaone x5000 > on PPC. > > This reverts commit 3d42f1ddc47a69c0ce155f9f30d764c4d689a5fa. > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=109345 It's not clear to me yet that this was really a regression that requires a revert. It looks like the bug reporter was relying on Xorg with no xorg.conf picking one particular graphics card out of two in the system, and this change resulted in it using the other one instead. I asked for the corresponding Xorg log files to see if they show why Xorg picks one card or the other in each case.
On 2019-05-10 5:42 p.m., Daniel Vetter wrote: > On Fri, May 10, 2019 at 09:29:58AM -0500, Alex Deucher wrote: >> This breaks multiple graphics cards in the Amigaone x5000 >> on PPC. >> >> This reverts commit 3d42f1ddc47a69c0ce155f9f30d764c4d689a5fa. >> >> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=109345 >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >> CC: Aaron Ma <aaron.ma@canonical.com> > > Given that the bug is a bit a mess I think we need to add a bit more > context here in the commit message. My understanding: > > Goal of the revert commit was to make the integrated boot device the > primary one, if we can't detect which one is the boot device, instead of > the last one. Which makes some sense. > > Now people have relied on the kernel picking the last one, which usually > is an add-on card, and therefore simply plugging in an add-on card allows > them to overwrite the default choice. Which also makes sense, and since > it's the older behaviour, wins. > > I think it'd be good to add a comment here that this behaviour has become > uapi, e.g. > > /* Add at the front so that we pick the last device as fallback > * default, with the usual result that plug in cards are preferred > * over integrated graphics. */ > > With that (or similar) and more commit message context: The bug reporter's system doesn't have integrated graphics though, just two plug-in cards. It's not clear to me yet that their expectation of Xorg to pick any particular one of them without configuration was justified.
On 5/10/19 11:46 PM, Michel Dänzer wrote: >> Given that the bug is a bit a mess I think we need to add a bit more >> context here in the commit message. My understanding: >> >> Goal of the revert commit was to make the integrated boot device the >> primary one, if we can't detect which one is the boot device, instead of >> the last one. Which makes some sense. >> >> Now people have relied on the kernel picking the last one, which usually >> is an add-on card, and therefore simply plugging in an add-on card allows >> them to overwrite the default choice. Which also makes sense, and since >> it's the older behaviour, wins. >> >> I think it'd be good to add a comment here that this behaviour has become >> uapi, e.g. >> >> /* Add at the front so that we pick the last device as fallback >> * default, with the usual result that plug in cards are preferred >> * over integrated graphics. */ >> >> With that (or similar) and more commit message context: > The bug reporter's system doesn't have integrated graphics though, just > two plug-in cards. It's not clear to me yet that their expectation of > Xorg to pick any particular one of them without configuration was justified. This code is kind of fail safe. When in hybrid graphic mode. It should fallback to integrated GPU, which should always be primary fb. So picking 1st (minor PCI ID number) GPU should make more sense. When with multiple d-GPUs, we can't say which card should be the right one for users when no Xorg conf set. Usually BIOS will set the 1st (minor PCI ID number) d-GPU as primary. So aligning with BIOS setting will be better I think. Aaron
On 2019-05-10 8:01 p.m., Aaron Ma wrote: > On 5/10/19 11:46 PM, Michel Dänzer wrote: >>> Given that the bug is a bit a mess I think we need to add a bit more >>> context here in the commit message. My understanding: >>> >>> Goal of the revert commit was to make the integrated boot device the >>> primary one, if we can't detect which one is the boot device, instead of >>> the last one. Which makes some sense. >>> >>> Now people have relied on the kernel picking the last one, which usually >>> is an add-on card, and therefore simply plugging in an add-on card allows >>> them to overwrite the default choice. Which also makes sense, and since >>> it's the older behaviour, wins. >>> >>> I think it'd be good to add a comment here that this behaviour has become >>> uapi, e.g. >>> >>> /* Add at the front so that we pick the last device as fallback >>> * default, with the usual result that plug in cards are preferred >>> * over integrated graphics. */ >>> >>> With that (or similar) and more commit message context: >> The bug reporter's system doesn't have integrated graphics though, just >> two plug-in cards. It's not clear to me yet that their expectation of >> Xorg to pick any particular one of them without configuration was justified. > > > This code is kind of fail safe. > > When in hybrid graphic mode. > It should fallback to integrated GPU, which should always be primary fb. > So picking 1st (minor PCI ID number) GPU should make more sense. > > When with multiple d-GPUs, we can't say which card should be the right > one for users when no Xorg conf set. > Usually BIOS will set the 1st (minor PCI ID number) d-GPU as primary. > So aligning with BIOS setting will be better I think. Right. The bug description even says that the card the reporter expected Xorg to come up on is the "secondary" card in a PCIe 1x slot (whereas the primary is in PCIe 16x). It seems pretty clear that your change actually made things better, and the reporter was just relying on the previous wrong behaviour. Therefore I resolved the report as not a bug, and this patch should be dropped.
On Mon, May 13, 2019 at 12:14:29PM +0200, Michel Dänzer wrote: > On 2019-05-10 8:01 p.m., Aaron Ma wrote: > > On 5/10/19 11:46 PM, Michel Dänzer wrote: > >>> Given that the bug is a bit a mess I think we need to add a bit more > >>> context here in the commit message. My understanding: > >>> > >>> Goal of the revert commit was to make the integrated boot device the > >>> primary one, if we can't detect which one is the boot device, instead of > >>> the last one. Which makes some sense. > >>> > >>> Now people have relied on the kernel picking the last one, which usually > >>> is an add-on card, and therefore simply plugging in an add-on card allows > >>> them to overwrite the default choice. Which also makes sense, and since > >>> it's the older behaviour, wins. > >>> > >>> I think it'd be good to add a comment here that this behaviour has become > >>> uapi, e.g. > >>> > >>> /* Add at the front so that we pick the last device as fallback > >>> * default, with the usual result that plug in cards are preferred > >>> * over integrated graphics. */ > >>> > >>> With that (or similar) and more commit message context: > >> The bug reporter's system doesn't have integrated graphics though, just > >> two plug-in cards. It's not clear to me yet that their expectation of > >> Xorg to pick any particular one of them without configuration was justified. > > > > > > This code is kind of fail safe. > > > > When in hybrid graphic mode. > > It should fallback to integrated GPU, which should always be primary fb. > > So picking 1st (minor PCI ID number) GPU should make more sense. > > > > When with multiple d-GPUs, we can't say which card should be the right > > one for users when no Xorg conf set. > > Usually BIOS will set the 1st (minor PCI ID number) d-GPU as primary. > > So aligning with BIOS setting will be better I think. > > Right. The bug description even says that the card the reporter expected > Xorg to come up on is the "secondary" card in a PCIe 1x slot (whereas > the primary is in PCIe 16x). It seems pretty clear that your change > actually made things better, and the reporter was just relying on the > previous wrong behaviour. Therefore I resolved the report as not a bug, > and this patch should be dropped. Hm I missed that this is about 2 external pci slots. Agreeing with you reasoning here now, retracting my r-b. -Daniel
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index f2f3ef8af271..8a3c45219a2a 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -725,7 +725,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) vga_arbiter_check_bridge_sharing(vgadev); /* Add to the list */ - list_add_tail(&vgadev->list, &vga_list); + list_add(&vgadev->list, &vga_list); vga_count++; vgaarb_info(&pdev->dev, "VGA device added: decodes=%s,owns=%s,locks=%s\n", vga_iostate_to_str(vgadev->decodes),
This breaks multiple graphics cards in the Amigaone x5000 on PPC. This reverts commit 3d42f1ddc47a69c0ce155f9f30d764c4d689a5fa. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=109345 Signed-off-by: Alex Deucher <alexander.deucher@amd.com> CC: Aaron Ma <aaron.ma@canonical.com> --- drivers/gpu/vga/vgaarb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)