[v5,2/9] video/aperture: use generic code to figure out the vga default device

Commit Message

Thomas Zimmermann April 6, 2023, 1:21 p.m. UTC

From: Daniel Vetter <[e-mail]>

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 commit 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.

There's been a few questions on previous iterations on dri-devel and
irc:

- fb_is_primary_device() seems to be yet another implementation of
  this theme, and at least on x86 it checks for both
  vga_default_device OR rom shadowing. There shouldn't ever be a case
  where rom shadowing gives any additional hints about the boot vga
  device, but if there is then the default vga selection in vgaarb
  should probably be fixed. And not special-case checks replicated all
  over.

- Thomas also brought up that on most !x86 systems
  fb_is_primary_device() returns 0, except on sparc/parisc. But these
  2 special cases are about platform specific devices and not pci, so
  shouldn't have any interactions.

- Furthermore fb_is_primary_device() is a bit a red herring since it's
  only used to select the right fbdev driver for fbcon, and not for
  the fw handover dance which the aperture helpers handle. At least
  for x86 we might want to look into unifying them, but that's a
  separate thing.

v2: Extend commit message trying to summarize various discussions.

v4:
- make the test for the primary device easier to read (Javier)
- fix commit message style (i.e., commit 1234 ("..."))
- fix Daniel's S-o-b address

v5:
- add back an S-o-b tag with Daniel's Intel address

Signed-off-by: Daniel Vetter <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Javier Martinez Canillas <[e-mail]>
Cc: Helge Deller <[e-mail]>
Cc: [e-mail]
Cc: Bjorn Helgaas <[e-mail]>
Cc: [e-mail]
Reviewed-by: Javier Martinez Canillas <[e-mail]>
---
 drivers/video/aperture.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Helge Deller April 7, 2023, 8:54 p.m. UTC | #1

On 4/6/23 15:21, Thomas Zimmermann wrote:
> From: Daniel Vetter <[e-mail]>
>
> 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 commit 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.
>
> There's been a few questions on previous iterations on dri-devel and
> irc:
>
> - fb_is_primary_device() seems to be yet another implementation of
>    this theme, and at least on x86 it checks for both
>    vga_default_device OR rom shadowing. There shouldn't ever be a case
>    where rom shadowing gives any additional hints about the boot vga
>    device, but if there is then the default vga selection in vgaarb
>    should probably be fixed. And not special-case checks replicated all
>    over.
>
> - Thomas also brought up that on most !x86 systems
>    fb_is_primary_device() returns 0, except on sparc/parisc. But these
>    2 special cases are about platform specific devices and not pci, so
>    shouldn't have any interactions.

Nearly all graphics cards on parisc machines are actually PCI cards,
but the way we handle the handover to graphics mode with STIcore doesn't
conflicts with your planned aperture changes.
So no problem as far as I can see for parisc...

Helge

Daniel Vetter April 11, 2023, 2:36 p.m. UTC | #2

On Fri, Apr 07, 2023 at 10:54:00PM +0200, Helge Deller wrote:
> On 4/6/23 15:21, Thomas Zimmermann wrote:
> > From: Daniel Vetter <[e-mail]>
> > 
> > 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 commit 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.
> > 
> > There's been a few questions on previous iterations on dri-devel and
> > irc:
> > 
> > - fb_is_primary_device() seems to be yet another implementation of
> >    this theme, and at least on x86 it checks for both
> >    vga_default_device OR rom shadowing. There shouldn't ever be a case
> >    where rom shadowing gives any additional hints about the boot vga
> >    device, but if there is then the default vga selection in vgaarb
> >    should probably be fixed. And not special-case checks replicated all
> >    over.
> > 
> > - Thomas also brought up that on most !x86 systems
> >    fb_is_primary_device() returns 0, except on sparc/parisc. But these
> >    2 special cases are about platform specific devices and not pci, so
> >    shouldn't have any interactions.
> 
> Nearly all graphics cards on parisc machines are actually PCI cards,
> but the way we handle the handover to graphics mode with STIcore doesn't
> conflicts with your planned aperture changes.
> So no problem as far as I can see for parisc...

Ah I thought sticore was some very special bus, if those can be pci cards
underneath then I guess some cleanup eventually might be a good idea? For
anything with a pci bus it's rather strange when vgaarb and
fb_is_primary_device() aren't a match ...
-Daniel

Helge Deller April 11, 2023, 3:25 p.m. UTC | #3

On 4/11/23 16:36, Daniel Vetter wrote:
> On Fri, Apr 07, 2023 at 10:54:00PM +0200, Helge Deller wrote:
>> On 4/6/23 15:21, Thomas Zimmermann wrote:
>>> From: Daniel Vetter <[e-mail]>
>>>
>>> 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 commit 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.
>>>
>>> There's been a few questions on previous iterations on dri-devel and
>>> irc:
>>>
>>> - fb_is_primary_device() seems to be yet another implementation of
>>>     this theme, and at least on x86 it checks for both
>>>     vga_default_device OR rom shadowing. There shouldn't ever be a case
>>>     where rom shadowing gives any additional hints about the boot vga
>>>     device, but if there is then the default vga selection in vgaarb
>>>     should probably be fixed. And not special-case checks replicated all
>>>     over.
>>>
>>> - Thomas also brought up that on most !x86 systems
>>>     fb_is_primary_device() returns 0, except on sparc/parisc. But these
>>>     2 special cases are about platform specific devices and not pci, so
>>>     shouldn't have any interactions.
>>
>> Nearly all graphics cards on parisc machines are actually PCI cards,
>> but the way we handle the handover to graphics mode with STIcore doesn't
>> conflicts with your planned aperture changes.
>> So no problem as far as I can see for parisc...
>
> Ah I thought sticore was some very special bus, if those can be pci cards

STI stands for "Standard Text Interface" [1], which is a API of ROM functions
to output text chars on a console. It's comparable to the text output functions
in a PC-BIOS on x86 and dependend on the ROM it drives any supported card which has
a parisc ROM. So, STI supports cards on PCI & AGP busses, as well on older GSC busses.
[1] https://parisc.wiki.kernel.org/images-parisc/e/e3/Sti.pdf

> underneath then I guess some cleanup eventually might be a good idea? For
> anything with a pci bus it's rather strange when vgaarb and
> fb_is_primary_device() aren't a match ...

There is no VGA on parisc, so there is no conflict. Cards come either with
a parisc STI ROM to support text mode, or they will only be used as secondary
cards only.  The graphics mode is only done in userspace by specific drivers, e.g.
by the X11 server in HP-UX.
Even on x86 the BIOS will only show text output if the graphics card comes
with a VGA-compatible BIOS.

Helge

Daniel Vetter April 13, 2023, 8:54 a.m. UTC | #4

On Tue, Apr 11, 2023 at 05:25:47PM +0200, Helge Deller wrote:
> On 4/11/23 16:36, Daniel Vetter wrote:
> > On Fri, Apr 07, 2023 at 10:54:00PM +0200, Helge Deller wrote:
> > > On 4/6/23 15:21, Thomas Zimmermann wrote:
> > > > From: Daniel Vetter <[e-mail]>
> > > > 
> > > > 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 commit 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.
> > > > 
> > > > There's been a few questions on previous iterations on dri-devel and
> > > > irc:
> > > > 
> > > > - fb_is_primary_device() seems to be yet another implementation of
> > > >     this theme, and at least on x86 it checks for both
> > > >     vga_default_device OR rom shadowing. There shouldn't ever be a case
> > > >     where rom shadowing gives any additional hints about the boot vga
> > > >     device, but if there is then the default vga selection in vgaarb
> > > >     should probably be fixed. And not special-case checks replicated all
> > > >     over.
> > > > 
> > > > - Thomas also brought up that on most !x86 systems
> > > >     fb_is_primary_device() returns 0, except on sparc/parisc. But these
> > > >     2 special cases are about platform specific devices and not pci, so
> > > >     shouldn't have any interactions.
> > > 
> > > Nearly all graphics cards on parisc machines are actually PCI cards,
> > > but the way we handle the handover to graphics mode with STIcore doesn't
> > > conflicts with your planned aperture changes.
> > > So no problem as far as I can see for parisc...
> > 
> > Ah I thought sticore was some very special bus, if those can be pci cards
> 
> STI stands for "Standard Text Interface" [1], which is a API of ROM functions
> to output text chars on a console. It's comparable to the text output functions
> in a PC-BIOS on x86 and dependend on the ROM it drives any supported card which has
> a parisc ROM. So, STI supports cards on PCI & AGP busses, as well on older GSC busses.
> [1] https://parisc.wiki.kernel.org/images-parisc/e/e3/Sti.pdf
> 
> > underneath then I guess some cleanup eventually might be a good idea? For
> > anything with a pci bus it's rather strange when vgaarb and
> > fb_is_primary_device() aren't a match ...
> 
> There is no VGA on parisc, so there is no conflict. Cards come either with
> a parisc STI ROM to support text mode, or they will only be used as secondary
> cards only.  The graphics mode is only done in userspace by specific drivers, e.g.
> by the X11 server in HP-UX.
> Even on x86 the BIOS will only show text output if the graphics card comes
> with a VGA-compatible BIOS.

tbf after reading vt.c and fbcon.c some more I'm pretty sure my patch is
nonsense. As sure as you can be with vt/fbcon :-/

Since it sounds like it's a driver bug, maybe a patch to do a pr_warn in
register_framebuffer if there's no mode? I read a pile of code around
modedb.c right now, and there's a lot of things that silently assume that
you always have a mode. So would be good thing to check.
-Daniel

Patch

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 41e77de1ea82..d0eccc4ed60b 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -328,9 +328,8 @@  int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
 	resource_size_t base, size;
 	int bar, ret;
 
-#ifdef CONFIG_X86
-	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
-#endif
+	if (pdev == vga_default_device())
+		primary = true;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
 		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))