[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230719193233.GA511659@bhelgaas>
Date: Wed, 19 Jul 2023 14:32:33 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Sui Jingfeng <sui.jingfeng@...ux.dev>
Cc: David Airlie <airlied@...il.com>, amd-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
intel-gfx@...ts.freedesktop.org, kvm@...r.kernel.org,
linux-pci@...r.kernel.org, linux-fbdev@...r.kernel.org,
Sui Jingfeng <suijingfeng@...ngson.cn>,
Alex Deucher <alexander.deucher@....com>,
Christian Konig <christian.koenig@....com>,
Pan Xinhui <Xinhui.Pan@....com>,
Daniel Vetter <daniel@...ll.ch>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
Ben Skeggs <bskeggs@...hat.com>,
Karol Herbst <kherbst@...hat.com>,
Lyude Paul <lyude@...hat.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Alex Williamson <alex.williamson@...hat.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
Hawking Zhang <Hawking.Zhang@....com>,
Mario Limonciello <mario.limonciello@....com>,
Lijo Lazar <lijo.lazar@....com>,
YiPeng Chai <YiPeng.Chai@....com>,
Bokun Zhang <Bokun.Zhang@....com>,
Likun Gao <Likun.Gao@....com>,
Ville Syrjala <ville.syrjala@...ux.intel.com>,
Jason Gunthorpe <jgg@...pe.ca>,
Kevin Tian <kevin.tian@...el.com>,
Cornelia Huck <cohuck@...hat.com>,
Yishai Hadas <yishaih@...dia.com>,
Abhishek Sahu <abhsahu@...dia.com>,
Yi Liu <yi.l.liu@...el.com>,
Jani Nikula <jani.nikula@...el.com>
Subject: Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
[+cc linux-pci (please cc in the future since the bulk of this patch
is in drivers/pci/)]
On Wed, Jul 12, 2023 at 12:43:05AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@...ngson.cn>
>
> Currently, the strategy of selecting the default boot on a multiple video
> card coexistence system is not perfect. Potential problems are:
>
> 1) This function is a no-op on non-x86 architectures.
Which function in particular is a no-op for non-x86?
> 2) It does not take the PCI Bar may get relocated into consideration.
> 3) It is not effective for the PCI device without a dedicated VRAM Bar.
> 4) It is device-agnostic, thus it has to waste the effort to iterate all
> of the PCI Bar to find the VRAM aperture.
> 5) It has invented lots of methods to determine which one is the default
> boot device, but this is still a policy because it doesn't give the
> user a choice to override.
I don't think we need a list of *potential* problems. We need an
example of the specific problem this will solve, i.e., what currently
does not work?
The drm/ast and maybe drm/loongson patches are the only ones that use
the new callback, so I assume there are real problems with those
drivers.
CONFIG_DRM_AST is a tristate. We're talking about identifying the
boot-time console device. So if CONFIG_DRM_AST=m, I guess we don't
get the benefit of the new callback unless the module gets loaded?
> Also honor the comment: "Clients have *TWO* callback mechanisms they
> can use"
This refers to the existing vga_client_register() function comment:
* vga_client_register - register or unregister a VGA arbitration client
* @pdev: pci device of the VGA client
* @set_decode: vga decode change callback
*
* Clients have two callback mechanisms they can use.
*
* @set_decode callback: If a client can disable its GPU VGA resource, it
* will get a callback from this to set the encode/decode state.
and the fact that struct vga_device currently only contains *one*
callback function pointer:
unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
Adding the .is_primary_gpu() callback does mean there will now be two
callbacks, as the comment says, but I think it's just confusing to
mention this in the commit log, so I would just remove it.
> @@ -1509,13 +1543,24 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
> * cases of hotplugable vga cards.
> */
>
> - if (action == BUS_NOTIFY_ADD_DEVICE)
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> notify = vga_arbiter_add_pci_device(pdev);
> - else if (action == BUS_NOTIFY_DEL_DEVICE)
> + if (notify)
> + vga_arbiter_notify_clients();
> + break;
> + case BUS_NOTIFY_DEL_DEVICE:
> notify = vga_arbiter_del_pci_device(pdev);
> + if (notify)
> + vga_arbiter_notify_clients();
> + break;
> + case BUS_NOTIFY_BOUND_DRIVER:
> + vga_arbiter_do_arbitration(pdev);
> + break;
> + default:
> + break;
> + }
Changing from if/else to switch makes the patch bigger than necessary
for no real benefit and obscures what is really changing.
Bjorn
Powered by blists - more mailing lists