[PATCH] PCI/VGA: Don't assume only VGA device found is the boot VGA device

[Kai-Heng Feng]

On 2024/10/25 8:55 PM, Alex Deucher wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Fri, Oct 25, 2024 at 3:51 AM Kai-Heng Feng <kaihengf at nvidia.com> wrote:
>>
>>
>>
>> On 2024/10/23 11:27 PM, Alex Deucher wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, Oct 22, 2024 at 9:27 PM Kai-Heng Feng <kaihengf at nvidia.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2024/10/22 9:04 PM, Alex Deucher wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Tue, Oct 22, 2024 at 2:31 AM Kai-Heng Feng <kaihengf at nvidia.com> wrote:
>>>>>>
>>>>>> Hi Luke,
>>>>>>
>>>>>> On 2024/10/15 4:04 PM, Luke Jones wrote:
>>>>>>> On Mon, 14 Oct 2024, at 5:25 PM, Mario Limonciello wrote:
>>>>>>>> From: Mario Limonciello <mario.limonciello at amd.com>
>>>>>>>>
>>>>>>>> The ASUS GA605W has a NVIDIA PCI VGA device and an AMD PCI display device.
>>>>>>>>
>>>>>>>> ```
>>>>>>>> 65:00.0 VGA compatible controller: NVIDIA Corporation AD106M [GeForce
>>>>>>>> RTX 4070 Max-Q / Mobile] (rev a1)
>>>>>>>> 66:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI]
>>>>>>>> Strix [Radeon 880M / 890M] (rev c1)
>>>>>>>> ```
>>>>>>>>
>>>>>>>> The fallback logic in vga_is_boot_device() flags the NVIDIA dGPU as the
>>>>>>>> boot VGA device, but really the eDP is connected to the AMD PCI display
>>>>>>>> device.
>>>>>>>>
>>>>>>>> Drop this case to avoid marking the NVIDIA dGPU as the boot VGA device.
>>>>>>>>
>>>>>>>> Suggested-by: Alex Deucher <alexander.deucher at amd.com>
>>>>>>>> Reported-by: Luke D. Jones <luke at ljones.dev>
>>>>>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3673
>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>>>>>>>> ---
>>>>>>>>      drivers/pci/vgaarb.c | 7 -------
>>>>>>>>      1 file changed, 7 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>>>>>>> index 78748e8d2dba..05ac2b672d4b 100644
>>>>>>>> --- a/drivers/pci/vgaarb.c
>>>>>>>> +++ b/drivers/pci/vgaarb.c
>>>>>>>> @@ -675,13 +675,6 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>>>>>>>>                 return true;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -    /*
>>>>>>>> -     * Vgadev has neither IO nor MEM enabled.  If we haven't found any
>>>>>>>> -     * other VGA devices, it is the best candidate so far.
>>>>>>>> -     */
>>>>>>>> -    if (!boot_vga)
>>>>>>>> -            return true;
>>>>>>>> -
>>>>>>>>         return false;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.43.0
>>>>>>>
>>>>>>> Hi Mario,
>>>>>>>
>>>>>>> I can verify that this does leave the `boot_vga` attribute set as 0 for the NVIDIA device.
>>>>>>
>>>>>> Does the following diff work for you?
>>>>>> This variant should be less risky for most systems.
>>>>>>
>>>>>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>>>>>> index 78748e8d2dba..3fb734cb9c1b 100644
>>>>>> --- a/drivers/pci/vgaarb.c
>>>>>> +++ b/drivers/pci/vgaarb.c
>>>>>> @@ -675,6 +675,9 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
>>>>>>                     return true;
>>>>>>             }
>>>>>>
>>>>>> +       if (vga_arb_integrated_gpu(&pdev->dev))
>>>>>> +               return true;
>>>>>> +
>>>>>
>>>>> The problem is that the integrated graphics does not support VGA.
>>>>
>>>> Right, so the check has to be used much earlier.
>>>>
>>>> I wonder does the integrated GFX have _DOD/_DOS while the discrete one doesn't?
>>>> If that's the case, vga_arb_integrated_gpu() can be used to differentiate which
>>>> one is the boot GFX.
>>>
>>> I think the problem is that the boot GPU is being conflated with vga
>>> arb.  In this case the iGPU has no VGA so has no reason to be involved
>>> in vga arb.  Trying to mess with any vga related resources on it could
>>> be problematic.  Do higher levels of the stack look at vga arb to
>>> determine the "primary" GPU?
>>
>> Hmm, I wonder if all those heuristic are needed for EFI based system?
>>
>> Can we assume that what being used by UEFI GOP is the primary GFX device?
> 
> Yes, I believe so.  The SBIOS should use the GOP device as determined
> by the user preference.  I.e.., in the bios configuration you can
> generally select iGPU or PEG for the primary display.

UEFI spec, 10.3.3.1 ACPI _ADR Device Path

"The _ADR device path is used to contain video output device attributes to 
support the Graphics Output Protocol. The device path can contain multiple _ADR 
entries if multiple video output devices are displaying the same output."

Luke, can you please see what are the _ADR values of the iGPU and dGPU? Maybe we 
can find which one was used by GOP this way.

Kai-Heng

> 
> Alex
> 
>>
>> Kai-Heng
>>
>>>
>>> Alex
>>>
>>>>
>>>> Kai-Heng
>>>>
>>>>>
>>>>> Alex
>>>>>
>>>>>>             /*
>>>>>>              * Vgadev has neither IO nor MEM enabled.  If we haven't found any
>>>>>>              * other VGA devices, it is the best candidate so far.
>>>>>>
>>>>>>
>>>>>> Kai-Heng
>>>>>>
>>>>>>>
>>>>>>> Tested-by: Luke D. Jones <luke at ljones.dev>
>>>>>>
>>>>
>>