Received: by 2002:a05:7412:3784:b0:e2:908c:2ebd with SMTP id jk4csp2626063rdb; Wed, 4 Oct 2023 07:01:22 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG1G9yVi3iGEHWetM0OTGgosQo47NQfmq6pHl4aI4sK2OjK7ua32AevPI26n3k69b9zE6VB X-Received: by 2002:a17:90a:3f16:b0:274:6839:6a8c with SMTP id l22-20020a17090a3f1600b0027468396a8cmr2193196pjc.6.1696428081676; Wed, 04 Oct 2023 07:01:21 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696428081; cv=none; d=google.com; s=arc-20160816; b=Ag7UKDqVzax/twXNH2Fkub+3wnpWtRUmQ8o9egxxymruKYWTwmGV7Slb2cvLJmdzvu qAkAvuDjoKgYA9pKdHZb8nLj5iTkNbjZUht39o8lmqRkMF9SVmqEN2uhJR+N37hhdYdd ddzmVR7JvRK3+YBPV254ex23sF13wmrEgTACtZ7m+hHbmuqYctZ256wtFnEGZG04NF8f s1t0xndUFPDBpLr70cesXhbVbjMDuo7XErzh/tTjHcJVw1NqOkJKkvv/oghuEiFbwf0F qM91kGF4w0oyzpGxCIBslHFxfwXQ8sDQskrfcKfhfIuhHprHhqqoNO+unhYECSkURaQm ILlQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :message-id:subject:cc:to:from:date:dkim-signature; bh=BrjfC+CWGDcvCPqSiPmm0vqKP4CIjXhc41LrhGKfRnY=; fh=efWFjYi1TpDh4jejx7hVnPAH2rPPwktRo9DYbdqk2/k=; b=pdVpdPnPpT/Wl1swRG/TVQ4a9Mcq0ZStLOTUgyBEOK1G4QpBdYUsSxGnI7ONuSpFFQ nUEVmjc74psmsV3OCYr+SCDQUjGtrOVEZUoAx6T0qRZj47j8bY3OSCpe/Vj8cq7qOTvr ASApOrMciYqEAzgIdwohtxG/NkYxzm7A2VTRUmaCrIzb2UlugNZmxMN+HtV/9GQ7uLNj cCpxS9nJ6Ffop1NoklH3POPfEmZNVMcnAO8i+Nmbdwdwh+LlDJBdXOxHFRyUG2UE3MRu q827nxHn7VRYIldvfRnVmkMsMyaTPp1rTlVbYHtlkwKjLCN34J01uD+EICJCfGuCQsay OemQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=Q2Kyno5R; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from snail.vger.email (snail.vger.email. [23.128.96.37]) by mx.google.com with ESMTPS id i34-20020a17090a3da500b0025c1d114af0si1515744pjc.93.2023.10.04.07.01.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Oct 2023 07:01:21 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=Q2Kyno5R; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 5B79E8218238; Wed, 4 Oct 2023 07:01:16 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242753AbjJDOBL (ORCPT + 99 others); Wed, 4 Oct 2023 10:01:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60656 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242747AbjJDOBJ (ORCPT ); Wed, 4 Oct 2023 10:01:09 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 36D1AD8; Wed, 4 Oct 2023 07:01:05 -0700 (PDT) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 76ADAC433C8; Wed, 4 Oct 2023 14:01:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1696428064; bh=ZXfdVsNac93kNUS4gIvDRTAkfJjEGoRUuo+eHBQdlrk=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=Q2Kyno5RZ4oCSaQTGQLDY+x4/1Ucb0kiGgSJiAP00lVdDiNXqDfgBnPja1ASnP0c8 SI09gqkq6yQ2RnvxXkIOVZ2pl7xB1phV01hclnTT8E0fXbz3Vtf7J91b1lE0tNE2Zb vb0FX5r9s8DvYC2CDCHJnpy3dNKZ+5DTCMdGHWcoP7sawxk5mzJgI02x1GvusiDQon Os8fkceU1jck+ycS+U9pMj1S4WJ7bb3IQ0+yNT2ivUnquaZzXQI7Ym+QwDzbq/6hMU Addw4/HhUGz3DwpJSWfUl03cj051xDzfqd5q0HhQ2Ltqq6MBLocqrZ8J8cImbwaWRn LFM0bTS0UrMaQ== Date: Wed, 4 Oct 2023 09:01:02 -0500 From: Bjorn Helgaas To: Sui Jingfeng Cc: Bjorn Helgaas , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH pci-next v6 1/2] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent Message-ID: <20231004140102.GA709356@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 04 Oct 2023 07:01:16 -0700 (PDT) On Wed, Oct 04, 2023 at 08:55:04PM +0800, Sui Jingfeng wrote: > On 2023/10/3 23:54, Bjorn Helgaas wrote: > > On Mon, Oct 02, 2023 at 08:05:10PM +0800, Sui Jingfeng wrote: > > > Currently, the vga_is_firmware_default() function only works on x86 and > > > ia64, it is a no-op on the rest of the architectures. This patch completes > > > the implementation for it, the added code tries to capture the PCI (e) VGA > > > device that owns the firmware framebuffer, since only one GPU could own > > > the firmware fb, things are almost done once we have determined the boot > > > VGA device. As the PCI resource relocation do have a influence on the > > > results of identification, we make it available on architectures where PCI > > > resource relocation does happen at first. Because this patch is more > > > important for those architectures(such as arm, arm64, loongarch, mips and > > > risc-v etc). > > > > There's a little too much going on this this patch. The problem is > > very simple: currently we compare firmware BAR assignments with BARs > > that may have been reassigned by Linux. > > > > What if we did something like the patch below? I think it will be > > less confusing if we only have one copy of the code related to > > screen_info. > > > > > Signed-off-by: Sui Jingfeng > > > --- > > > drivers/pci/vgaarb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 76 insertions(+) > > > > > > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c > > > index 5e6b1eb54c64..02821c0f4cd0 100644 > > > --- a/drivers/pci/vgaarb.c > > > +++ b/drivers/pci/vgaarb.c > > > @@ -60,6 +60,9 @@ static bool vga_arbiter_used; > > > static DEFINE_SPINLOCK(vga_lock); > > > static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue); > > > +/* The PCI(e) VGA device which owns the firmware framebuffer */ > > > +static struct pci_dev *pdev_boot_vga; > > > + > > > static const char *vga_iostate_to_str(unsigned int iostate) > > > { > > > /* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */ > > > @@ -582,6 +585,9 @@ static bool vga_is_firmware_default(struct pci_dev *pdev) > > > return true; > > > } > > > +#else > > > + if (pdev_boot_vga && pdev_boot_vga == pdev) > > > + return true; > > > #endif > > > return false; > > > } > > > @@ -1557,3 +1563,73 @@ static int __init vga_arb_device_init(void) > > > return rc; > > > } > > > subsys_initcall_sync(vga_arb_device_init); > > > + > > > +/* > > > + * Get the physical address range that the firmware framebuffer occupies. > > > + * > > > + * Note that the global screen_info is arch-specific, thus CONFIG_SYSFB is > > > + * chosen as compile-time conditional to suppress linkage problems on non-x86 > > > + * architectures. > > > + * > > > + * Returns true on success, otherwise return false. > > > + */ > > > +static bool vga_arb_get_firmware_fb_range(u64 *start, u64 *end) > > > +{ > > > + u64 fb_start = 0; > > > + u64 fb_size = 0; > > > + u64 fb_end; > > > + > > > +#if defined(CONFIG_X86) || defined(CONFIG_IA64) || defined(CONFIG_SYSFB) > > > + fb_start = screen_info.lfb_base; > > > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > > > + fb_start |= (u64)screen_info.ext_lfb_base << 32; > > > + > > > + fb_size = screen_info.lfb_size; > > > +#endif > > > + > > > + /* No firmware framebuffer support */ > > > + if (!fb_start || !fb_size) > > > + return false; > > > + > > > + fb_end = fb_start + fb_size - 1; > > > + > > > + *start = fb_start; > > > + *end = fb_end; > > > + > > > + return true; > > > +} > > > + > > > +/* > > > + * Identify the PCI VGA device that contains the firmware framebuffer > > > + */ > > > +static void pci_boot_vga_capturer(struct pci_dev *pdev) > > > +{ > > > + u64 fb_start, fb_end; > > > + struct resource *res; > > > + unsigned int i; > > > + > > > + if (pdev_boot_vga) > > > + return; > > > + > > > + if (!vga_arb_get_firmware_fb_range(&fb_start, &fb_end)) > > > + return; > > > + > > > + pci_dev_for_each_resource(pdev, res, i) { > > > + if (resource_type(res) != IORESOURCE_MEM) > > > + continue; > > > + > > > + if (!res->start || !res->end) > > > + continue; > > > + > > > + if (res->start <= fb_start && fb_end <= res->end) { > > > + pdev_boot_vga = pdev; > > > + > > > + vgaarb_info(&pdev->dev, > > > + "BAR %u: %pR contains firmware FB [0x%llx-0x%llx]\n", > > > + i, res, fb_start, fb_end); > > > + break; > > > + } > > > + } > > > +} > > > +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, > > > + 8, pci_boot_vga_capturer); > > > > PCI/VGA: Match firmware framebuffer before BAR reassignment > > > > vga_is_firmware_default() decides a device is the firmware default VGA > > device if it has a BAR that contains the framebuffer described by > > screen_info. > > > > Previously this was unreliable because the screen_info framebuffer address > > comes from firmware, and the Linux PCI core may reassign device BARs before > > vga_is_firmware_default() runs. This reassignment means the BAR may not > > match the screen_info values, but we still want to select the device as the > > firmware default. > > > > Make vga_is_firmware_default() more reliable by running it as a quirk so it > > happens before any BAR reassignment. > > > > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c > > index 5e6b1eb54c64..4a53e76caddd 100644 > > --- a/drivers/pci/vgaarb.c > > +++ b/drivers/pci/vgaarb.c > > @@ -60,6 +60,8 @@ static bool vga_arbiter_used; > > static DEFINE_SPINLOCK(vga_lock); > > static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue); > > +static struct pci_dev *vga_firmware_device; > > + > > static const char *vga_iostate_to_str(unsigned int iostate) > > { > > /* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */ > > @@ -560,6 +562,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev) > > u64 base = screen_info.lfb_base; > > u64 size = screen_info.lfb_size; > > struct resource *r; > > + unsigned int i; > > u64 limit; > > /* Select the device owning the boot framebuffer if there is one */ > > @@ -570,7 +573,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev) > > limit = base + size; > > /* Does firmware framebuffer belong to us? */ > > - pci_dev_for_each_resource(pdev, r) { > > + pci_dev_for_each_resource(pdev, r, i) { > > if (resource_type(r) != IORESOURCE_MEM) > > continue; > > @@ -580,6 +583,8 @@ static bool vga_is_firmware_default(struct pci_dev *pdev) > > if (base < r->start || limit >= r->end) > > continue; > > + vgaarb_info(&pdev->dev, "BAR %u: %pR contains firmware framebuffer [%#010llx-%#010llx]\n", > > + i, r, base, limit - 1); > > return true; > > } > > #endif > > @@ -623,7 +628,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev) > > if (boot_vga && boot_vga->is_firmware_default) > > return false; > > - if (vga_is_firmware_default(pdev)) { > > + if (pdev == vga_firmware_device) { > > vgadev->is_firmware_default = true; > > return true; > > } > > @@ -1557,3 +1562,14 @@ static int __init vga_arb_device_init(void) > > return rc; > > } > > subsys_initcall_sync(vga_arb_device_init); > > + > > +static void vga_match_firmware_framebuffer(struct pci_dev *pdev) > > +{ > > + if (vga_firmware_device) > > + return; > > + > > + if (vga_is_firmware_default(pdev)) > > + vga_firmware_device = pdev; > > +} > > +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, > > + 8, vga_match_firmware_framebuffer); > > > Q: What if we did something like the patch below? > > A: > > But the vga_is_firmware_default() function only works on X86 and IA64, > you patch doesn't solve the problems on ARM64 and LoongArch. Yes, that's true. Ideally a patch solves a single problem. This one solves an issue for x86 and ia64. A subsequent patch can solve the problems on ARM64 and LoongArch. Doing them separately means that each patch is easier to understand and if we accidentally break something, a bisection can give more specific information about what broke. Bjorn