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=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 A7D9CC07E95 for ; Wed, 7 Jul 2021 11:44:04 +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 71F4061C98 for ; Wed, 7 Jul 2021 11:44:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 71F4061C98 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D8F8D6E0D5; Wed, 7 Jul 2021 11:44:03 +0000 (UTC) Received: from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com [IPv6:2a00:1450:4864:20::32b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4240D6E0FE for ; Wed, 7 Jul 2021 11:44:02 +0000 (UTC) Received: by mail-wm1-x32b.google.com with SMTP id n33so1644843wms.1 for ; Wed, 07 Jul 2021 04:44:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=Ee0/Z4V5YdQ4Ev+stQInHtN+UUMT1hp4hha9v4lQ+Ds=; b=Ud9LFHVqWn3wWM/rQIWqywsiayo1KQITn+44FhMP38FUR5j4d39eLLW0oXvie6kqJz cPV0QkeMrrJ0oE6bJ97YxQvrCfrtvxJlEwA8izssm9itqPfEA31H9dWhaAkY38pnT9Bw CJd09kncuYzgBS77/sCPs8VRJsoGr2pqVMzU4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Ee0/Z4V5YdQ4Ev+stQInHtN+UUMT1hp4hha9v4lQ+Ds=; b=hkZs6tHE4Vpbo2i7whnD9flhljvyIjPdhnip37MeUuSR05iZelNQ5WlrkUoxwUT2fG rB4aBpdSRnDOWKdy6c3darLKJPsoBxg9w00Wl/OUX0ucIxYnwZVa+R7odRiew1xE+XGV 0OzvmmRGmlUYMdusIPCrujhX5zdgjPhE3cChHNvf9rN1xr8Q0C/7A1EtowI7e04vFsbv VVFTyfH1TBa271Iy/VnMgiTxGLBpOG+AW3u4FPb+II9sxwm832xhg6SfbQSL3atwL4aq S7SEGoDeSj3kCtXjQaLkIny6UT8Lsuei3iAVODf5ICB1/bqpuqmca2Vpl72otkPYYnVq R7Vg== X-Gm-Message-State: AOAM533H5/kuV9tH15V5qaTVu1mPvpBRBM3mROzT4imBEo9JX319Mt8l 0pCQEgoXZf3a3QFo5UrOeA4rug== X-Google-Smtp-Source: ABdhPJxYvuikFytoRXCPDA0CdmpNIy2xxYw3y/0x945J+7ETO1sAZeccDjOPdpuNNZz7Bqf3UzQvBQ== X-Received: by 2002:a1c:9d46:: with SMTP id g67mr6361753wme.188.1625658240850; Wed, 07 Jul 2021 04:44:00 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id f12sm5829752wmg.16.2021.07.07.04.43.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Jul 2021 04:44:00 -0700 (PDT) Date: Wed, 7 Jul 2021 13:43:58 +0200 From: Daniel Vetter To: Huacai Chen Subject: Re: [PATCH] vgaarb: Rework default VGA device selection Message-ID: References: <20210705100503.1120643-1-chenhuacai@loongson.cn> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 5.10.0-7-amd64 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: , Cc: David Airlie , Maling list - DRI developers , Bjorn Helgaas , Xuefeng Li , Huacai Chen Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, Jul 06, 2021 at 05:21:42PM +0800, Huacai Chen wrote: > Hi, Daniel, > > On Tue, Jul 6, 2021 at 4:21 PM Daniel Vetter wrote: > > > > On Mon, Jul 05, 2021 at 06:05:03PM +0800, Huacai Chen wrote: > > > Currently, vga_arb_device_init() selects the first probed VGA device > > > with VGA legacy resources enabled as the default device. However, some > > > BMC-based VGA cards (e.g., AST2500 and HiSilicon D05) don't enable VGA > > > legacy resources because their built-in upstream bridges don't support > > > PCI_BRIDGE_CTL_VGA. This makes "no default VGA card" and breaks X.org > > > auto-detection. > > > > > > Commit a37c0f48950b56f6ef2e ("vgaarb: Select a default VGA device even > > > if there's no legacy VGA") try to solve this problem but fails on some > > > platforms, because it relies on the initcall order: > > > > > > We should call vga_arb_device_init() after PCI enumeration, otherwise it > > > may fail to select the default VGA device. Since vga_arb_device_init() > > > and PCI enumeration function (i.e., pcibios_init() or acpi_init()) are > > > both wrapped by subsys_initcall(), their sequence is not assured. So, it > > > is possible to use subsys_initcall_sync() instead of subsys_initcall() > > > to wrap vga_arb_device_init(). > > > > > > However, the above approach still has drawbacks, it cannot handle the > > > cases that a VGA card is hot-plugged, or the gpu driver is compiled as a > > > module. > > > > > > So, as suggested by Bjorn Helgaas, this patch rework the selection: > > > 1, Remove direct vga_arb_select_default_device() calls in vga_arb_ > > > device_init(). > > > 2, Rename vga_arb_select_default_device() to vga_arb_update_default_ > > > device(), which selects the first probed VGA device as the default > > > (whether legacy resources enabled or not), and update the default > > > device if a better one is found (device with legacy resources enabled > > > is better, device owns the firmware framebuffer is even better). > > > 3, Every time a new VGA device is probed, vga_arbiter_add_pci_device() > > > is called, and vga_arb_update_default_device() is also called. So the > > > hotplug case and the module case can also be handled. > > > > > > Signed-off-by: Huacai Chen > > > > Sounds reasonable, but because this is defacto pci stuff that just isn't > > in drivers/pci (maybe we should move it) I'll defer to Bjorn's review > > before I apply this patch. > > -Daniel > This (move to drivers/pci) has been disscussed before, I think that > can be another patch. Oh definitely, I just wanted to explain why I won't simply apply this without Bjorn's review. -Daniel > > Huacai > > > > > --- > > > drivers/gpu/vga/vgaarb.c | 219 +++++++++++++++++---------------------- > > > 1 file changed, 97 insertions(+), 122 deletions(-) > > > > > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > > > index 949fde433ea2..07770aae3aaf 100644 > > > --- a/drivers/gpu/vga/vgaarb.c > > > +++ b/drivers/gpu/vga/vgaarb.c > > > @@ -586,6 +586,97 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc) > > > } > > > EXPORT_SYMBOL(vga_put); > > > > > > +#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 vga_arb_update_default_device(struct vga_device *vgadev) > > > +{ > > > + struct pci_dev *pdev = vgadev->pdev; > > > + struct device *dev = &pdev->dev; > > > + struct vga_device *vgadev_default; > > > +#if defined(CONFIG_X86) || defined(CONFIG_IA64) > > > + int i; > > > + unsigned long flags; > > > + u64 base = screen_info.lfb_base; > > > + u64 size = screen_info.lfb_size; > > > + u64 limit; > > > + resource_size_t start, end; > > > +#endif > > > + > > > + /* Deal with VGA default device. Use first enabled one > > > + * by default if arch doesn't have it's own hook > > > + */ > > > + if (!vga_default_device()) { > > > + if ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK) > > > + vgaarb_info(dev, "setting as boot VGA device\n"); > > > + else > > > + vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); > > > + vga_set_default_device(pdev); > > > + } > > > + > > > + vgadev_default = vgadev_find(vga_default); > > > + > > > + /* Overridden by a better device */ > > > + if (vgadev_default && ((vgadev_default->owns & VGA_RSRC_LEGACY_MASK) == 0) > > > + && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { > > > + vgaarb_info(dev, "overriding boot VGA device\n"); > > > + vga_set_default_device(pdev); > > > + } > > > + > > > + if (vga_arb_integrated_gpu(dev)) { > > > + vgaarb_info(dev, "overriding boot VGA device\n"); > > > + vga_set_default_device(pdev); > > > + } > > > + > > > +#if defined(CONFIG_X86) || defined(CONFIG_IA64) > > > + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > > > + base |= (u64)screen_info.ext_lfb_base << 32; > > > + > > > + limit = base + size; > > > + > > > + /* > > > + * Override vga_arbiter_add_pci_device()'s I/O based detection > > > + * as it may take the wrong device (e.g. on Apple system under > > > + * EFI). > > > + * > > > + * Select the device owning the boot framebuffer if there is > > > + * one. > > > + */ > > > + > > > + /* Does firmware framebuffer belong to us? */ > > > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { > > > + flags = pci_resource_flags(vgadev->pdev, i); > > > + > > > + if ((flags & IORESOURCE_MEM) == 0) > > > + continue; > > > + > > > + start = pci_resource_start(vgadev->pdev, i); > > > + end = pci_resource_end(vgadev->pdev, i); > > > + > > > + if (!start || !end) > > > + continue; > > > + > > > + if (base < start || limit >= end) > > > + continue; > > > + > > > + if (vgadev->pdev != vga_default_device()) > > > + vgaarb_info(dev, "overriding boot device\n"); > > > + vga_set_default_device(vgadev->pdev); > > > + } > > > +#endif > > > +} > > > + > > > /* > > > * Rules for using a bridge to control a VGA descendant decoding: if a bridge > > > * has only one VGA descendant then it can be used to control the VGA routing > > > @@ -643,6 +734,11 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev) > > > } > > > new_bus = new_bus->parent; > > > } > > > + > > > + if (vgadev->bridge_has_one_vga == true) > > > + vgaarb_info(&vgadev->pdev->dev, "bridge control possible\n"); > > > + else > > > + vgaarb_info(&vgadev->pdev->dev, "no bridge control possible\n"); > > > } > > > > > > /* > > > @@ -713,15 +809,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > > > bus = bus->parent; > > > } > > > > > > - /* Deal with VGA default device. Use first enabled one > > > - * by default if arch doesn't have it's own hook > > > - */ > > > - if (vga_default == NULL && > > > - ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) { > > > - vgaarb_info(&pdev->dev, "setting as boot VGA device\n"); > > > - vga_set_default_device(pdev); > > > - } > > > - > > > + vga_arb_update_default_device(vgadev); > > > vga_arbiter_check_bridge_sharing(vgadev); > > > > > > /* Add to the list */ > > > @@ -1451,111 +1539,10 @@ 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, *found = NULL; > > > - struct vga_device *vgadev; > > > - > > > -#if defined(CONFIG_X86) || defined(CONFIG_IA64) > > > - u64 base = screen_info.lfb_base; > > > - u64 size = screen_info.lfb_size; > > > - u64 limit; > > > - resource_size_t start, end; > > > - unsigned long flags; > > > - int i; > > > - > > > - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > > > - base |= (u64)screen_info.ext_lfb_base << 32; > > > - > > > - limit = base + size; > > > - > > > - list_for_each_entry(vgadev, &vga_list, list) { > > > - struct device *dev = &vgadev->pdev->dev; > > > - /* > > > - * Override vga_arbiter_add_pci_device()'s I/O based detection > > > - * as it may take the wrong device (e.g. on Apple system under > > > - * EFI). > > > - * > > > - * Select the device owning the boot framebuffer if there is > > > - * one. > > > - */ > > > - > > > - /* Does firmware framebuffer belong to us? */ > > > - for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { > > > - flags = pci_resource_flags(vgadev->pdev, i); > > > - > > > - if ((flags & IORESOURCE_MEM) == 0) > > > - continue; > > > - > > > - start = pci_resource_start(vgadev->pdev, i); > > > - end = pci_resource_end(vgadev->pdev, i); > > > - > > > - if (!start || !end) > > > - continue; > > > - > > > - if (base < start || limit >= end) > > > - continue; > > > - > > > - if (!vga_default_device()) > > > - vgaarb_info(dev, "setting as boot device\n"); > > > - else if (vgadev->pdev != vga_default_device()) > > > - vgaarb_info(dev, "overriding boot device\n"); > > > - vga_set_default_device(vgadev->pdev); > > > - } > > > - } > > > -#endif > > > - > > > - if (!vga_default_device()) { > > > - list_for_each_entry_reverse(vgadev, &vga_list, list) { > > > - 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)) { > > > - 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); > > > - if (vgadev) { > > > - struct device *dev = &vgadev->pdev->dev; > > > - vgaarb_info(dev, "setting as boot device (VGA legacy resources not available)\n"); > > > - vga_set_default_device(vgadev->pdev); > > > - } > > > - } > > > -} > > > - > > > static int __init vga_arb_device_init(void) > > > { > > > int rc; > > > struct pci_dev *pdev; > > > - struct vga_device *vgadev; > > > > > > rc = misc_register(&vga_arb_device); > > > if (rc < 0) > > > @@ -1571,18 +1558,6 @@ static int __init vga_arb_device_init(void) > > > PCI_ANY_ID, pdev)) != NULL) > > > vga_arbiter_add_pci_device(pdev); > > > > > > - list_for_each_entry(vgadev, &vga_list, list) { > > > - struct device *dev = &vgadev->pdev->dev; > > > - > > > - if (vgadev->bridge_has_one_vga) > > > - vgaarb_info(dev, "bridge control possible\n"); > > > - else > > > - vgaarb_info(dev, "no bridge control possible\n"); > > > - } > > > - > > > - vga_arb_select_default_device(); > > > - > > > - pr_info("loaded\n"); > > > return rc; > > > } > > > subsys_initcall(vga_arb_device_init); > > > -- > > > 2.27.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch