Re: [PATCH 04/11] video/aperture: use generic code to figure out the vga default device

On Wed, Jan 11, 2023 at 04:59:30PM +0100, Thomas Zimmermann wrote:

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:

quoted

Since vgaarb has been promoted to be a core piece of the pci subsystem
we don't have to open code random guesses anymore, we actually know
this in a platform agnostic way, and there's no need for an x86
specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
drivers/pci")

This should not result in any functional change, and the non-x86
multi-gpu pci systems are probably rare enough to not matter (I don't
know of any tbh). But it's a nice cleanup, so let's do it.

Signed-off-by: Daniel Vetter <redacted>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
  drivers/video/aperture.c | 6 ++----
  1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 41e77de1ea82..3d8c925c7365 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c

@@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
   */
  int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
  {
-	bool primary = false;
+	bool primary;
  	resource_size_t base, size;
  	int bar, ret;
-#ifdef CONFIG_X86
-	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
-#endif
+	primary = pdev == vga_default_device();

vga_default_device() is part of vgaarb and can return NULL. [1] That new
test is likely to be incorrect on many systems.

I suggest to implement a helper like fb_is_primary_device() on x86: it uses
the default VGA if set, or falls back to the original test. [2]

It's noteworthy that on most architectures, fb_is_primary_device() returns
0. But at least on Sparc [3] and some Parisc [4] machines, it does not.

Afaik these neither do legacy vga nor sysfb, so we should be fine. I'll
augment the commit message.

I've long wanted to rework this helper anyway. So this is a good
opportunity.

Hm yeah that sounds like a good thing to copy. I'm honestly not sure
whether it's needed, but at least it shouldn't hurt. I thought at least
that the boot default device should be set on all pci architectures.

I'll also reorder this with the previous patch to avoid the compile fail.
-Daniel

Best regards
Thomas

[1]
https://elixir.bootlin.com/linux/latest/source/include/linux/vgaarb.h#L69
[2]
https://elixir.bootlin.com/linux/latest/source/arch/x86/video/fbdev.c#L14
[3] https://elixir.bootlin.com/linux/latest/source/arch/sparc/include/asm/fb.h#L18
[4] https://elixir.bootlin.com/linux/latest/source/drivers/video/console/sticore.c#L1153

quoted

  	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
  		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch