Re: [Intel-gfx] [PATCH 2/8] video/aperture: use generic code to figure out the vga default device

[Javier Martinez Canillas <javierm@redhat.com>]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

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

Agreed and if there are regressions reported then could be added there.

> - 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.
>
> 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 b009468ffdff..8835d3bc39bf 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();
>

Maybe enclose the check in parenthesis to make it easier to read ?

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat