| Message ID | 20230111154112.90575-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers | expand |
Hi Am 11.01.23 um 16:41 schrieb Daniel Vetter: > 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 <daniel.vetter@intel.com> > 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. I've long wanted to rework this helper anyway. So this is a good opportunity. 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 > > for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) { > if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
On Wed, Jan 11, 2023 at 04:59:30PM +0100, Thomas Zimmermann wrote: > Hi > > Am 11.01.23 um 16:41 schrieb Daniel Vetter: > > 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 <daniel.vetter@intel.com> > > 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 > > > > 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
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(); for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) { if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
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 <daniel.vetter@intel.com> 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(-)