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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 31 Oct 2024, at 1:58 AM, Kai-Heng Feng wrote:
> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxx>
>>>>>>>>>
>>>>>>>>> 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@xxxxxxx>
>>>>>>>>> Reported-by: Luke D. Jones <luke@xxxxxxxxxx>
>>>>>>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3673
>>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
>>>>>>>>> ---
>>>>>>>>>      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.

I'm not sure what I'm looking at here, but initial search shows:

                Device (VGA)
                {
                    Name (_ADR, Zero)  // _ADR: Address
                    Name (DOSA, Zero)
                    Method (_DOS, 1, NotSerialized)  // _DOS: Disable Output Switching
                    {
                        DOSA = Arg0
                    }

                    Method (_DOD, 0, NotSerialized)  // _DOD: Display Output Devices
                    {
                        M460 ("PLA-ASL-\\_SB.PCI0.GPPA.VGA._DOD\n", Zero, Zero, Zero, Zero, Zero, Zero)
                        Return (Package (0x07)
                        {
                            0x00010110, 
                            0x00010210, 
                            0x00010220, 
                            0x00010230, 
                            0x00010240, 
                            0x00031000, 
                            0x00032000
                        })
                    }

                    Device (LCD)
                    {
                        Name (_ADR, 0x0110)  // _ADR: Address
                        Name (BCLB, Package (0x34)
                        {

https://gitlab.com/asus-linux/reverse-engineering/-/blob/master/uncategorized/GA605WI/dsdt.dsl?ref_type=heads#L4666

And:

            Method (_DOD, 0, NotSerialized)  // _DOD: Display Output Devices
            {
                Return (Package (0x01)
                {
                    0x8000A450
                })
            }

            Device (LCD0)
            {
                Method (_ADR, 0, Serialized)  // _ADR: Address
                {
                    Return (0x8000A450)
                }

https://gitlab.com/asus-linux/reverse-engineering/-/blob/master/uncategorized/GA605WI/ssdt1.dsl?ref_type=heads#L411

The links are direct to the lines I thought were relevant in the dumped DSDT.

Luke.

>
> 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@xxxxxxxxxx>
>>>>>>>
>>>>>
>>>





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux