From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6009C4332F for ; Fri, 17 Sep 2021 16:55:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CC40761100 for ; Fri, 17 Sep 2021 16:55:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243837AbhIQQ41 (ORCPT ); Fri, 17 Sep 2021 12:56:27 -0400 Received: from mail.kernel.org ([198.145.29.99]:36212 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243102AbhIQQ4Y (ORCPT ); Fri, 17 Sep 2021 12:56:24 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 97AAF61108; Fri, 17 Sep 2021 16:55:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1631897701; bh=gIN/1/UJ9KFOMMzT+R4qCGsl0N3O4XSWxGatfxzx4dc=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=GstmkVwl1605P/G9i50cL7p85qHP8Iyk2IMhCZ8dxd+PX9nRyrirBnIPfakhJHjJb UndfnJAdTIIzPQX23o7ZIuCJLdUmwOh9LL2FCUlxH5OLoCG7NyK7dQLrfs995Y9QIK AUz1THxXHlNFFArXZn5oxY6Yf3cFa42NFhNzARvsWwfOlXt8pDzSycne3mmt9BA7Ln X/in+c1wMbiMO466tFyubRG+l5o63b9PYxxA6yL1O4q1WApo80Bi3JHO92quZI8q4m gGlRgpP/lbKk+cEooMFWX9W0GyfW0a20JSGTBcx3wG07nq112b2Bg3ObpXRIHhkIF7 Hkxna/hyXqpoA== Date: Fri, 17 Sep 2021 11:55:00 -0500 From: Bjorn Helgaas To: Kai-Heng Feng Cc: David Airlie , Daniel Vetter , Maarten Lankhorst , mripard@kernel.org, Thomas Zimmermann , "Deucher, Alexander" , "open list:DRM DRIVERS" , LKML , Linux PCI , Huacai Chen Subject: Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU Message-ID: <20210917165500.GA1723244@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 17, 2021 at 11:49:45AM +0800, Kai-Heng Feng wrote: > On Fri, Sep 17, 2021 at 12:38 AM Bjorn Helgaas wrote: > > > > [+cc Huacai, linux-pci] > > > > On Wed, May 19, 2021 at 09:57:23PM +0800, Kai-Heng Feng wrote: > > > Commit 3d42f1ddc47a ("vgaarb: Keep adding VGA device in queue") assumes > > > the first device is an integrated GPU. However, on AMD platforms an > > > integrated GPU can have higher PCI device number than a discrete GPU. > > > > > > Integrated GPU on ACPI platform generally has _DOD and _DOS method, so > > > use that as predicate to find integrated GPU. If the new strategy > > > doesn't work, fallback to use the first device as boot VGA. > > > > > > Signed-off-by: Kai-Heng Feng > > > --- > > > drivers/gpu/vga/vgaarb.c | 31 ++++++++++++++++++++++++++----- > > > 1 file changed, 26 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > > > index 5180c5687ee5..949fde433ea2 100644 > > > --- a/drivers/gpu/vga/vgaarb.c > > > +++ b/drivers/gpu/vga/vgaarb.c > > > @@ -50,6 +50,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #include > > > > > > @@ -1450,9 +1451,23 @@ static struct miscdevice vga_arb_device = { > > > MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops > > > }; > > > > > > +#if defined(CONFIG_ACPI) > > > +static bool vga_arb_integrated_gpu(struct device *dev) > > > +{ > > > + struct acpi_device *adev = ACPI_COMPANION(dev); > > > + > > > + return adev && !strcmp(acpi_device_hid(adev), ACPI_VIDEO_HID); > > > +} > > > +#else > > > +static bool vga_arb_integrated_gpu(struct device *dev) > > > +{ > > > + return false; > > > +} > > > +#endif > > > + > > > static void __init vga_arb_select_default_device(void) > > > { > > > - struct pci_dev *pdev; > > > + struct pci_dev *pdev, *found = NULL; > > > struct vga_device *vgadev; > > > > > > #if defined(CONFIG_X86) || defined(CONFIG_IA64) > > > @@ -1505,20 +1520,26 @@ static void __init vga_arb_select_default_device(void) > > > #endif > > > > > > if (!vga_default_device()) { > > > - list_for_each_entry(vgadev, &vga_list, list) { > > > + list_for_each_entry_reverse(vgadev, &vga_list, list) { > > > > Hi Kai-Heng, do you remember why you changed the order of this list > > traversal? > > The descending order is to keep the original behavior. > > Before this patch, it breaks out of the loop as early as possible, so > the lower numbered device is picked. > This patch makes it only break out of the loop when ACPI_VIDEO_HID > device is found. > So if there are more than one device that meet "cmd & (PCI_COMMAND_IO > | PCI_COMMAND_MEMORY)", higher numbered device will be selected. > So the traverse order reversal is to keep the original behavior. Can you give an example of what you mean? I don't quite follow how it keeps the original behavior. If we have this: 0 PCI_COMMAND_MEMORY set ACPI_VIDEO_HID 1 PCI_COMMAND_MEMORY set ACPI_VIDEO_HID Previously we didn't look for ACPI_VIDEO_HID, so we chose 0, now we choose 1, which seems wrong. In the absence of other information, I would prefer the lower-numbered device. Or this: 0 PCI_COMMAND_MEMORY set 1 PCI_COMMAND_MEMORY set ACPI_VIDEO_HID Previously we chose 0; now we choose 1, which does seem right, but we'd choose 1 regardless of the order. Or this: 0 PCI_COMMAND_MEMORY set ACPI_VIDEO_HID 1 PCI_COMMAND_MEMORY set Previously we chose 0, now we still choose 0, which seems right but again doesn't depend on the order. The first case, where both devices are ACPI_VIDEO_HID, is the only one where the order matters, and I suggest that we should be using the original order, not the reversed order. > > I guess the list_add_tail() in vga_arbiter_add_pci_device() means > > vga_list is generally ordered with small device numbers first and > > large ones last. > > > > So you pick the integrated GPU with the largest device number. Are > > there systems with more than one integrated GPU? If so, I would > > naively expect that in the absence of an indication otherwise, we'd > > want the one with the *smallest* device number. > > There's only one integrated GPU on the affected system. > > The approach is to keep the list traversal in one pass. > Is there any regression introduce by this patch? > If that's the case, we can separate the logic and find the > ACPI_VIDEO_HID in second pass. No regression, I'm just looking at Huacai's VGA patches, which affect this area.