[PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[Alex Deucher]

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

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[Aaron Ma]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert "vgaarb: Keep adding VGA device in queue"

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel