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

[Kai-Heng Feng <kaihengf@nvidia.com>]

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@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@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@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@amd.com>
>>>>>> Reported-by: Luke D. Jones <luke@ljones.dev>
>>>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3673
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@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?

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@ljones.dev>
>>>>
>>