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=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,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 2FCD7C433EF for ; Fri, 17 Sep 2021 03:50:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0ED3460F13 for ; Fri, 17 Sep 2021 03:50:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244121AbhIQDv2 (ORCPT ); Thu, 16 Sep 2021 23:51:28 -0400 Received: from smtp-relay-internal-0.canonical.com ([185.125.188.122]:46068 "EHLO smtp-relay-internal-0.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234543AbhIQDv1 (ORCPT ); Thu, 16 Sep 2021 23:51:27 -0400 Received: from mail-oo1-f69.google.com (mail-oo1-f69.google.com [209.85.161.69]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 3EC44402D3 for ; Fri, 17 Sep 2021 03:49:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1631850598; bh=o2WwArOaqXO510rowzIQD3yCaqD7PjB7l55ID7TA3xc=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=KC/wMMq6u4xVTIC9gWwtWw3qFPdmYtz6bE0+rjN44ZoRNnma/rFGmlEFwdE+tN1vu IomElIncX/Wfo9BttVhsPysERGTi6sgoRZDtDaVpIGx8+dP75WV+wrpa+oRoBKMiKQ lbtTSAJQ6UAwoWZtiXZ13JPJFNb8wCuGfQTgT/tGtEr/VcdBNmlzR1BCyjthRQZwcO G6LeIy+gbZXkWoNL88gMLndA2hHOm44Hc4VA/qdwEg6tPjrsqXtjcl3azNgtzmN5wO HRLszHr1z4BXuRjitmpzM7VQqcBb73I1tVFx06Z4miObasxZxclDOup3CXV58llczG w8nLNVLFHrU4w== Received: by mail-oo1-f69.google.com with SMTP id e26-20020a4ada1a000000b00290b0695382so37943206oou.0 for ; Thu, 16 Sep 2021 20:49:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=o2WwArOaqXO510rowzIQD3yCaqD7PjB7l55ID7TA3xc=; b=XbXO7BvSNCWhtYw92zO1AAVJohJozF5GOMRPT0kPo3LOmHyB+pPYoi0Lg2Xs0d8vL4 kSTQgJFNXwLGK19pj/nrih3BEHC3joxG/34T96aS5fUKMDdLApd+oYrnPqeS9Gpp/MOV IoZImqv0r0yFChiu2a7SpFfMmqWx9hD7IVifBMFoh28MFoWsJNlMaejjHrQW8A1KHnu9 xkSzjrBLuvhQo0own09YZHhcrPoX98usqZzGx0GfhJwegBg/QI/G2Qz5dR3YgVVOkuIJ FD3SWR7nFD/n1UvzXQqPRAD+YmJEnrMZ7p4s7rOgFOhT5/UzRsUBM2jHw1GgWmBDzK4L RCDA== X-Gm-Message-State: AOAM532se4FuWS0gTdB1n7n5qWJptOhy1VIha777ZA8Sd8eG1A7jSj7T NSNr/fQh39+xVkzJaAjyzSfUhoyEUSESVyzQuD6lrBdct3aKYG7Zx+jrkrHLP4n87nVQoc62JaX ENYh8nC+ZtN/dA4KdhkXC8Mt9ffAEJHQwxaUW9GxkB1tQg6OJgy0BTDH51g== X-Received: by 2002:a9d:1708:: with SMTP id i8mr37631ota.233.1631850596967; Thu, 16 Sep 2021 20:49:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzw7/JqD91vFn6Wh+g3/wCVkduUO+JuLE1VRUyT5ut6P5TQKvgQDp/KcbJC9Gr75Lp89fxAxavZwAorE2CYMic= X-Received: by 2002:a9d:1708:: with SMTP id i8mr37613ota.233.1631850596710; Thu, 16 Sep 2021 20:49:56 -0700 (PDT) MIME-Version: 1.0 References: <20210519135723.525997-1-kai.heng.feng@canonical.com> <20210916163755.GA1620802@bjorn-Precision-5520> In-Reply-To: <20210916163755.GA1620802@bjorn-Precision-5520> From: Kai-Heng Feng Date: Fri, 17 Sep 2021 11:49:45 +0800 Message-ID: Subject: Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU To: Bjorn Helgaas Cc: David Airlie , Daniel Vetter , Maarten Lankhorst , mripard@kernel.org, Thomas Zimmermann , "Deucher, Alexander" , "open list:DRM DRIVERS" , LKML , Linux PCI , Huacai Chen Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > 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. Kai-Heng > > > struct device *dev = &vgadev->pdev->dev; > > u16 cmd; > > > > pdev = vgadev->pdev; > > pci_read_config_word(pdev, PCI_COMMAND, &cmd); > > if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { > > - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); > > - vga_set_default_device(pdev); > > - break; > > + found = pdev; > > + if (vga_arb_integrated_gpu(dev)) > > + break; > > } > > } > > } > > > > + if (found) { > > + vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n"); > > + vga_set_default_device(found); > > + return; > > + } > > + > > if (!vga_default_device()) { > > vgadev = list_first_entry_or_null(&vga_list, > > struct vga_device, list); > > -- > > 2.31.1 > > 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=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 526C5C433F5 for ; Fri, 17 Sep 2021 03:50:03 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E98A260EDF for ; Fri, 17 Sep 2021 03:50:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E98A260EDF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=canonical.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 325AF6EB41; Fri, 17 Sep 2021 03:50:02 +0000 (UTC) Received: from smtp-relay-internal-0.canonical.com (smtp-relay-internal-0.canonical.com [185.125.188.122]) by gabe.freedesktop.org (Postfix) with ESMTPS id 10C176EB41 for ; Fri, 17 Sep 2021 03:50:00 +0000 (UTC) Received: from mail-oi1-f197.google.com (mail-oi1-f197.google.com [209.85.167.197]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 38E5A40262 for ; Fri, 17 Sep 2021 03:49:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1631850598; bh=o2WwArOaqXO510rowzIQD3yCaqD7PjB7l55ID7TA3xc=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=KC/wMMq6u4xVTIC9gWwtWw3qFPdmYtz6bE0+rjN44ZoRNnma/rFGmlEFwdE+tN1vu IomElIncX/Wfo9BttVhsPysERGTi6sgoRZDtDaVpIGx8+dP75WV+wrpa+oRoBKMiKQ lbtTSAJQ6UAwoWZtiXZ13JPJFNb8wCuGfQTgT/tGtEr/VcdBNmlzR1BCyjthRQZwcO G6LeIy+gbZXkWoNL88gMLndA2hHOm44Hc4VA/qdwEg6tPjrsqXtjcl3azNgtzmN5wO HRLszHr1z4BXuRjitmpzM7VQqcBb73I1tVFx06Z4miObasxZxclDOup3CXV58llczG w8nLNVLFHrU4w== Received: by mail-oi1-f197.google.com with SMTP id n9-20020a0568080a0900b002689a4834e4so33202635oij.12 for ; Thu, 16 Sep 2021 20:49:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=o2WwArOaqXO510rowzIQD3yCaqD7PjB7l55ID7TA3xc=; b=CzxNaqRqWIoe/mogaRjbGimt9Pb6i73ULQoZKL9aRdgCd1sldhY/9OeU1ZXddDA54D /umqwjBxd0jmZTIFFTfLbT6eh0eszs9aiwLVrWukryeI9Fc1xAq8qMxL6YE/46DHzZXq RJ3hGCQSWaTj/snjHIVaCjynDyHPxJGSR3uj5G8DbNW06cedBI9MFWP/YlmFHMwX6RSf OFfSoMLHaTQsC6T2CZ6PTYSYgKyQQ4XWbNi1Ly7zxsw06soj534ksjIhp5DfaJS5mDYz F+nSyodMJR57kQz2d4zYxZafpD6xPzbmFPx7+QQANkhc4mtheG1qG5azkzSHmoYgGzzV gGvA== X-Gm-Message-State: AOAM530hCAAncIJpNvbEwzbmkLR89dYJhtJ2wPGWHWkOb/sd/IYVBHvs +8RkTAkWafrFQkcUv7tnXi8dJbomfC5LUwxADrs5AltMYbm/Q++rEEa7byOQQFd2Cq7Bpi8N/Dv wxtJL4+Y67lWpReGLDG635pSKIPSuf9QCZviixGSM/iKN0F6KNtNXPc0CIebGhQ== X-Received: by 2002:a9d:1708:: with SMTP id i8mr37626ota.233.1631850596966; Thu, 16 Sep 2021 20:49:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzw7/JqD91vFn6Wh+g3/wCVkduUO+JuLE1VRUyT5ut6P5TQKvgQDp/KcbJC9Gr75Lp89fxAxavZwAorE2CYMic= X-Received: by 2002:a9d:1708:: with SMTP id i8mr37613ota.233.1631850596710; Thu, 16 Sep 2021 20:49:56 -0700 (PDT) MIME-Version: 1.0 References: <20210519135723.525997-1-kai.heng.feng@canonical.com> <20210916163755.GA1620802@bjorn-Precision-5520> In-Reply-To: <20210916163755.GA1620802@bjorn-Precision-5520> From: Kai-Heng Feng Date: Fri, 17 Sep 2021 11:49:45 +0800 Message-ID: Subject: Re: [PATCH] vgaarb: Use ACPI HID name to find integrated GPU To: Bjorn Helgaas Cc: David Airlie , Daniel Vetter , Maarten Lankhorst , mripard@kernel.org, Thomas Zimmermann , "Deucher, Alexander" , "open list:DRM DRIVERS" , LKML , Linux PCI , Huacai Chen Content-Type: text/plain; charset="UTF-8" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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. > > 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. Kai-Heng > > > struct device *dev = &vgadev->pdev->dev; > > u16 cmd; > > > > pdev = vgadev->pdev; > > pci_read_config_word(pdev, PCI_COMMAND, &cmd); > > if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) { > > - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); > > - vga_set_default_device(pdev); > > - break; > > + found = pdev; > > + if (vga_arb_integrated_gpu(dev)) > > + break; > > } > > } > > } > > > > + if (found) { > > + vgaarb_info(&found->dev, "setting as boot device (VGA legacy resources not available)\n"); > > + vga_set_default_device(found); > > + return; > > + } > > + > > if (!vga_default_device()) { > > vgadev = list_first_entry_or_null(&vga_list, > > struct vga_device, list); > > -- > > 2.31.1 > >