thr3ads.net - Nouveau - [Nouveau] [PATCH v3 0/4] PCI/VGA: introduce is_boot_device function callback to vga_client

If this information is useful, please help other people find it:
Share via:

Sui Jingfeng

2023-Jun-08 11:43 UTC

[Nouveau] [PATCH v3 0/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

From: Sui Jingfeng <suijingfeng at loongson.cn>

Patch 1,2 and 3 do basic clean up to the vgaarb module.
Patch 4 introduce is_boot_device function callback to vga_client_register

Sui Jingfeng (4):
  PCI/VGA: tidy up the code and comment format
  PCI/VGA: Use unsigned type for the io_state variable
  PCI/VGA: only deal with VGA class devices
  PCI/VGA: introduce is_boot_device function callback to
    vga_client_register

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 +-
 drivers/gpu/drm/i915/display/intel_vga.c   |  3 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c      |  2 +-
 drivers/gpu/drm/radeon/radeon_device.c     |  2 +-
 drivers/pci/vgaarb.c                       | 72 +++++++++++++---------
 drivers/vfio/pci/vfio_pci_core.c           |  2 +-
 include/linux/vgaarb.h                     | 16 ++---
 7 files changed, 57 insertions(+), 42 deletions(-)

-- 
2.25.1

Sui Jingfeng

2023-Jun-08 11:43 UTC

head link

[Nouveau] [PATCH v3 1/4] PCI/VGA: tidy up the code and comment format

From: Sui Jingfeng <suijingfeng at loongson.cn>

This patch replaces the leading space with a tab and removes the double
blank line, no functional change.

Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
---
 drivers/pci/vgaarb.c   | 26 +++++++++++++-------------
 include/linux/vgaarb.h |  8 +++-----
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5a696078b382..bf2921e3cb06 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -61,7 +61,6 @@ static bool vga_arbiter_used;
 static DEFINE_SPINLOCK(vga_lock);
 static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
 
-
 static const char *vga_iostate_to_str(unsigned int iostate)
 {
 	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
@@ -80,7 +79,8 @@ static const char *vga_iostate_to_str(unsigned int iostate)
 static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
 {
 	/* we could in theory hand out locks on IO and mem
-	 * separately to userspace but it can cause deadlocks */
+	 * separately to userspace but it can cause deadlocks
+	 */
 	if (strncmp(buf, "none", 4) == 0) {
 		*io_state = VGA_RSRC_NONE;
 		return 1;
@@ -99,7 +99,7 @@ static int vga_str_to_iostate(char *buf, int str_size, int
*io_state)
 	return 1;
 }
 
-/* this is only used a cookie - it should not be dereferenced */
+/* This is only used as cookie, it should not be dereferenced */
 static struct pci_dev *vga_default;
 
 /* Find somebody in our list */
@@ -194,13 +194,15 @@ int vga_remove_vgacon(struct pci_dev *pdev)
 EXPORT_SYMBOL(vga_remove_vgacon);
 
 /* If we don't ever use VGA arb we should avoid
-   turning off anything anywhere due to old X servers getting
-   confused about the boot device not being VGA */
+ * turning off anything anywhere due to old X servers getting
+ * confused about the boot device not being VGA
+ */
 static void vga_check_first_use(void)
 {
 	/* we should inform all GPUs in the system that
 	 * VGA arb has occurred and to try and disable resources
-	 * if they can */
+	 * if they can
+	 */
 	if (!vga_arbiter_used) {
 		vga_arbiter_used = true;
 		vga_arbiter_notify_clients();
@@ -956,9 +958,9 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
  * @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.
  *
- * Rationale: we cannot disable VGA decode resources unconditionally some
single
- * GPU laptops seem to require ACPI or BIOS access to the VGA registers to
- * control things like backlights etc.  Hopefully newer multi-GPU laptops do
+ * Rationale: we cannot disable VGA decode resources unconditionally, some
+ * single GPU laptops seem to require ACPI or BIOS access to the VGA registers
+ * to control things like backlights etc. Hopefully newer multi-GPU laptops do
  * something saner, and desktops won't have any special ACPI for this. The
  * driver will get a callback when VGA arbitration is first used by userspace
  * since some older X servers have issues.
@@ -988,7 +990,6 @@ int vga_client_register(struct pci_dev *pdev,
 bail:
 	spin_unlock_irqrestore(&vga_lock, flags);
 	return ret;
-
 }
 EXPORT_SYMBOL(vga_client_register);
 
@@ -1079,7 +1080,6 @@ static int vga_pci_str_to_vars(char *buf, int count,
unsigned int *domain,
 	int n;
 	unsigned int slot, func;
 
-
 	n = sscanf(buf, "PCI:%x:%x:%x.%x", domain, bus, &slot,
&func);
 	if (n != 4)
 		return 0;
@@ -1431,7 +1431,6 @@ static int vga_arb_open(struct inode *inode, struct file
*file)
 	priv->cards[0].io_cnt = 0;
 	priv->cards[0].mem_cnt = 0;
 
-
 	return 0;
 }
 
@@ -1544,7 +1543,8 @@ static int __init vga_arb_device_init(void)
 	bus_register_notifier(&pci_bus_type, &pci_notifier);
 
 	/* We add all PCI devices satisfying VGA class in the arbiter by
-	 * default */
+	 * default
+	 */
 	pdev = NULL;
 	while ((pdev  		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index b4b9137f9792..d36225c582ee 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -23,9 +23,7 @@
  * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
  * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
  * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS
- * IN THE SOFTWARE.
- *
+ * DEALINGS IN THE SOFTWARE.
  */
 
 #ifndef LINUX_VGA_H
@@ -96,7 +94,7 @@ static inline int vga_client_register(struct pci_dev *pdev,
 static inline int vga_get_interruptible(struct pci_dev *pdev,
 					unsigned int rsrc)
 {
-       return vga_get(pdev, rsrc, 1);
+	return vga_get(pdev, rsrc, 1);
 }
 
 /**
@@ -111,7 +109,7 @@ static inline int vga_get_interruptible(struct pci_dev
*pdev,
 static inline int vga_get_uninterruptible(struct pci_dev *pdev,
 					  unsigned int rsrc)
 {
-       return vga_get(pdev, rsrc, 0);
+	return vga_get(pdev, rsrc, 0);
 }
 
 static inline void vga_client_unregister(struct pci_dev *pdev)
-- 
2.25.1

Sui Jingfeng

2023-Jun-08 11:43 UTC

head link

[Nouveau] [PATCH v3 2/4] PCI/VGA: Use unsigned type for the io_state variable

From: Sui Jingfeng <suijingfeng at loongson.cn>

The io_state variable in the vga_arb_write() function is declared with
unsigned int type, while the vga_str_to_iostate() function takes int *
type. To keep them consistent, replace the third argument of
vga_str_to_iostate() function with the unsigned int * type.

Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
---
 drivers/pci/vgaarb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index bf2921e3cb06..7f0347f4f6fd 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -76,7 +76,7 @@ static const char *vga_iostate_to_str(unsigned int iostate)
 	return "none";
 }
 
-static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
+static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
 {
 	/* we could in theory hand out locks on IO and mem
 	 * separately to userspace but it can cause deadlocks
-- 
2.25.1

Sui Jingfeng

2023-Jun-08 11:43 UTC

head link

[Nouveau] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices

From: Sui Jingfeng <suijingfeng at loongson.cn>

vgaarb only deal with the VGA devcie(pdev->class == 0x0300), so replace the
pci_get_subsys() function with pci_get_class(). Filter the non pci display
device out. There no need to process the non display PCI device.

Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
---
 drivers/pci/vgaarb.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 7f0347f4f6fd..b0bf4952a95d 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -756,10 +756,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev
*pdev)
 	struct pci_dev *bridge;
 	u16 cmd;
 
-	/* Only deal with VGA class devices */
-	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
-		return false;
-
 	/* Allocate structure */
 	vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
 	if (vgadev == NULL) {
@@ -1499,7 +1495,9 @@ static int pci_notify(struct notifier_block *nb, unsigned
long action,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	bool notify = false;
 
-	vgaarb_dbg(dev, "%s\n", __func__);
+	/* Only deal with VGA class devices */
+	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+		return 0;
 
 	/* For now we're only intereted in devices added and removed. I didn't
 	 * test this thing here, so someone needs to double check for the
@@ -1509,6 +1507,8 @@ static int pci_notify(struct notifier_block *nb, unsigned
long action,
 	else if (action == BUS_NOTIFY_DEL_DEVICE)
 		notify = vga_arbiter_del_pci_device(pdev);
 
+	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
+
 	if (notify)
 		vga_arbiter_notify_clients();
 	return 0;
@@ -1533,8 +1533,8 @@ static struct miscdevice vga_arb_device = {
 
 static int __init vga_arb_device_init(void)
 {
+	struct pci_dev *pdev = NULL;
 	int rc;
-	struct pci_dev *pdev;
 
 	rc = misc_register(&vga_arb_device);
 	if (rc < 0)
@@ -1545,11 +1545,13 @@ static int __init vga_arb_device_init(void)
 	/* We add all PCI devices satisfying VGA class in the arbiter by
 	 * default
 	 */
-	pdev = NULL;
-	while ((pdev -		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
-			       PCI_ANY_ID, pdev)) != NULL)
+	while (1) {
+		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
+		if (!pdev)
+			break;
+
 		vga_arbiter_add_pci_device(pdev);
+	};
 
 	pr_info("loaded\n");
 	return rc;
-- 
2.25.1

Sui Jingfeng

2023-Jun-08 11:43 UTC

head link

[Nouveau] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

From: Sui Jingfeng <suijingfeng at loongson.cn>

The vga_is_firmware_default() function is arch-dependent, which doesn't
sound right. At least, it also works on the Mips and LoongArch platforms.
Tested with the drm/amdgpu and drm/radeon drivers. However, it's difficult
to enumerate all arch-driver combinations. I'm wrong if there is only one
exception.

With the observation that device drivers typically have better knowledge
about which PCI bar contains the firmware framebuffer, which could avoid
the need to iterate all of the PCI BARs.

But as a PCI function at pci/vgaarb.c, vga_is_firmware_default() is
probably not suitable to make such an optimization for a specific device.

There are PCI display controllers that don't have a dedicated VRAM bar,
this function will lose its effectiveness in such a case. Luckily, the
device driver can provide an accurate workaround.

Therefore, this patch introduces a callback that allows the device driver
to tell the VGAARB if the device is the default boot device. This patch
only intends to introduce the mechanism, while the implementation is left
to the device driver authors. Also honor the comment: "Clients have two
callback mechanisms they can use"

Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 +-
 drivers/gpu/drm/i915/display/intel_vga.c   |  3 +--
 drivers/gpu/drm/nouveau/nouveau_vga.c      |  2 +-
 drivers/gpu/drm/radeon/radeon_device.c     |  2 +-
 drivers/pci/vgaarb.c                       | 22 ++++++++++++++++++----
 drivers/vfio/pci/vfio_pci_core.c           |  2 +-
 include/linux/vgaarb.h                     |  8 +++++---
 7 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5c7d40873ee2..7a096f2d5c16 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3960,7 +3960,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	/* this will fail for cards that aren't VGA class devices, just
 	 * ignore it */
 	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
-		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
+		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode, NULL);
 
 	px = amdgpu_device_supports_px(ddev);
 
diff --git a/drivers/gpu/drm/i915/display/intel_vga.c
b/drivers/gpu/drm/i915/display/intel_vga.c
index 286a0bdd28c6..98d7d4dffe9f 100644
--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -115,7 +115,6 @@ intel_vga_set_decode(struct pci_dev *pdev, bool
enable_decode)
 
 int intel_vga_register(struct drm_i915_private *i915)
 {
-
 	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
 	int ret;
 
@@ -127,7 +126,7 @@ int intel_vga_register(struct drm_i915_private *i915)
 	 * then we do not take part in VGA arbitration and the
 	 * vga_client_register() fails with -ENODEV.
 	 */
-	ret = vga_client_register(pdev, intel_vga_set_decode);
+	ret = vga_client_register(pdev, intel_vga_set_decode, NULL);
 	if (ret && ret != -ENODEV)
 		return ret;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c
b/drivers/gpu/drm/nouveau/nouveau_vga.c
index f8bf0ec26844..162b4f4676c7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -92,7 +92,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
 		return;
 	pdev = to_pci_dev(dev->dev);
 
-	vga_client_register(pdev, nouveau_vga_set_decode);
+	vga_client_register(pdev, nouveau_vga_set_decode, NULL);
 
 	/* don't register Thunderbolt eGPU with vga_switcheroo */
 	if (pci_is_thunderbolt_attached(pdev))
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index afbb3a80c0c6..71f2ff39d6a1 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1425,7 +1425,7 @@ int radeon_device_init(struct radeon_device *rdev,
 	/* if we have > 1 VGA cards, then disable the radeon VGA resources */
 	/* this will fail for cards that aren't VGA class devices, just
 	 * ignore it */
-	vga_client_register(rdev->pdev, radeon_vga_set_decode);
+	vga_client_register(rdev->pdev, radeon_vga_set_decode, NULL);
 
 	if (rdev->flags & RADEON_IS_PX)
 		runtime = true;
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index b0bf4952a95d..d3dab61e0ef2 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -53,6 +53,7 @@ struct vga_device {
 	bool bridge_has_one_vga;
 	bool is_firmware_default;	/* device selected by firmware */
 	unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
+	bool (*is_boot_device)(struct pci_dev *pdev);
 };
 
 static LIST_HEAD(vga_list);
@@ -614,10 +615,17 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
 	if (boot_vga && boot_vga->is_firmware_default)
 		return false;
 
-	if (vga_is_firmware_default(pdev)) {
-		vgadev->is_firmware_default = true;
+	/*
+	 * Ask the device driver first, if registered. Fallback to the
+	 * default implement if the callback is non-exist.
+	 */
+	if (vgadev->is_boot_device)
+		vgadev->is_firmware_default = vgadev->is_boot_device(pdev);
+	else
+		vgadev->is_firmware_default = vga_is_firmware_default(pdev);
+
+	if (vgadev->is_firmware_default)
 		return true;
-	}
 
 	/*
 	 * A legacy VGA device has MEM and IO enabled and any bridges
@@ -954,6 +962,10 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
  * @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.
  *
+ * @is_boot_device: callback to the device driver, query if a client is the
+ * default boot device, as the device driver typically has better knowledge
+ * if specific device is the boot device. But this callback is optional.
+ *
  * Rationale: we cannot disable VGA decode resources unconditionally, some
  * single GPU laptops seem to require ACPI or BIOS access to the VGA registers
  * to control things like backlights etc. Hopefully newer multi-GPU laptops do
@@ -969,7 +981,8 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
  * Returns: 0 on success, -1 on failure
  */
 int vga_client_register(struct pci_dev *pdev,
-		unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
+		unsigned int (*set_decode)(struct pci_dev *pdev, bool decode),
+		bool (*is_boot_device)(struct pci_dev *pdev))
 {
 	int ret = -ENODEV;
 	struct vga_device *vgadev;
@@ -981,6 +994,7 @@ int vga_client_register(struct pci_dev *pdev,
 		goto bail;
 
 	vgadev->set_decode = set_decode;
+	vgadev->is_boot_device = is_boot_device;
 	ret = 0;
 
 bail:
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index a5ab416cf476..2a8873a330ba 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2067,7 +2067,7 @@ static int vfio_pci_vga_init(struct vfio_pci_core_device
*vdev)
 	if (ret)
 		return ret;
 
-	ret = vga_client_register(pdev, vfio_pci_set_decode);
+	ret = vga_client_register(pdev, vfio_pci_set_decode, NULL);
 	if (ret)
 		return ret;
 	vga_set_legacy_decoding(pdev, vfio_pci_set_decode(pdev, false));
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index d36225c582ee..66fe80ffad76 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -50,7 +50,8 @@ struct pci_dev *vga_default_device(void);
 void vga_set_default_device(struct pci_dev *pdev);
 int vga_remove_vgacon(struct pci_dev *pdev);
 int vga_client_register(struct pci_dev *pdev,
-		unsigned int (*set_decode)(struct pci_dev *pdev, bool state));
+		unsigned int (*set_decode)(struct pci_dev *pdev, bool state),
+		bool (*is_boot_device)(struct pci_dev *pdev));
 #else /* CONFIG_VGA_ARB */
 static inline void vga_set_legacy_decoding(struct pci_dev *pdev,
 		unsigned int decodes)
@@ -76,7 +77,8 @@ static inline int vga_remove_vgacon(struct pci_dev *pdev)
 	return 0;
 }
 static inline int vga_client_register(struct pci_dev *pdev,
-		unsigned int (*set_decode)(struct pci_dev *pdev, bool state))
+		unsigned int (*set_decode)(struct pci_dev *pdev, bool state),
+		bool (*is_boot_device)(struct pci_dev *pdev))
 {
 	return 0;
 }
@@ -114,7 +116,7 @@ static inline int vga_get_uninterruptible(struct pci_dev
*pdev,
 
 static inline void vga_client_unregister(struct pci_dev *pdev)
 {
-	vga_client_register(pdev, NULL);
+	vga_client_register(pdev, NULL, NULL);
 }
 
 #endif /* LINUX_VGA_H */
-- 
2.25.1

Bjorn Helgaas

2023-Jun-08 19:12 UTC

head link

[Nouveau] [Intel-gfx] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices

Start with verb and capitalize to match ("Deal only with ...")

On Thu, Jun 08, 2023 at 07:43:21PM +0800, Sui Jingfeng
wrote:> From: Sui Jingfeng <suijingfeng at loongson.cn>
> 
> vgaarb only deal with the VGA devcie(pdev->class == 0x0300), so replace
the
> pci_get_subsys() function with pci_get_class(). Filter the non pci display
> device out. There no need to process the non display PCI device.
s/non pci/non-PCI/
s/non display/non-display/

This is fine, and I'll merge this, but someday I would like to get rid
of the bus_register_notifier() and call vga_arbiter_add_pci_device()
and vga_arbiter_del_pci_device() directly from the PCI core.

Or if you wanted to do that now, that would be even better :)

Bjorn
> Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
> ---
>  drivers/pci/vgaarb.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 7f0347f4f6fd..b0bf4952a95d 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -756,10 +756,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev
*pdev)
>  	struct pci_dev *bridge;
>  	u16 cmd;
>  
> -	/* Only deal with VGA class devices */
> -	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> -		return false;
> -
>  	/* Allocate structure */
>  	vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>  	if (vgadev == NULL) {
> @@ -1499,7 +1495,9 @@ static int pci_notify(struct notifier_block *nb,
unsigned long action,
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	bool notify = false;
>  
> -	vgaarb_dbg(dev, "%s\n", __func__);
> +	/* Only deal with VGA class devices */
> +	if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +		return 0;
>  
>  	/* For now we're only intereted in devices added and removed. I
didn't
>  	 * test this thing here, so someone needs to double check for the
> @@ -1509,6 +1507,8 @@ static int pci_notify(struct notifier_block *nb,
unsigned long action,
>  	else if (action == BUS_NOTIFY_DEL_DEVICE)
>  		notify = vga_arbiter_del_pci_device(pdev);
>  
> +	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
> +
>  	if (notify)
>  		vga_arbiter_notify_clients();
>  	return 0;
> @@ -1533,8 +1533,8 @@ static struct miscdevice vga_arb_device = {
>  
>  static int __init vga_arb_device_init(void)
>  {
> +	struct pci_dev *pdev = NULL;
>  	int rc;
> -	struct pci_dev *pdev;
>  
>  	rc = misc_register(&vga_arb_device);
>  	if (rc < 0)
> @@ -1545,11 +1545,13 @@ static int __init vga_arb_device_init(void)
>  	/* We add all PCI devices satisfying VGA class in the arbiter by
>  	 * default
>  	 */
> -	pdev = NULL;
> -	while ((pdev > -		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> -			       PCI_ANY_ID, pdev)) != NULL)
> +	while (1) {
> +		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
> +		if (!pdev)
> +			break;
> +
>  		vga_arbiter_add_pci_device(pdev);
> +	};
>  
>  	pr_info("loaded\n");
>  	return rc;
> -- 
> 2.25.1
>

Bjorn Helgaas

2023-Jun-08 19:19 UTC

head link

[Nouveau] [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng
wrote:> From: Sui Jingfeng <suijingfeng at loongson.cn>
> 
> The vga_is_firmware_default() function is arch-dependent, which doesn't
> sound right. At least, it also works on the Mips and LoongArch platforms.
> Tested with the drm/amdgpu and drm/radeon drivers. However, it's
difficult
> to enumerate all arch-driver combinations. I'm wrong if there is only
one
> exception.
> 
> With the observation that device drivers typically have better knowledge
> about which PCI bar contains the firmware framebuffer, which could avoid
> the need to iterate all of the PCI BARs.
> 
> But as a PCI function at pci/vgaarb.c, vga_is_firmware_default() is
> probably not suitable to make such an optimization for a specific device.
> 
> There are PCI display controllers that don't have a dedicated VRAM bar,
> this function will lose its effectiveness in such a case. Luckily, the
> device driver can provide an accurate workaround.
> 
> Therefore, this patch introduces a callback that allows the device driver
> to tell the VGAARB if the device is the default boot device. This patch
> only intends to introduce the mechanism, while the implementation is left
> to the device driver authors. Also honor the comment: "Clients have
two
> callback mechanisms they can use"
s/bar/BAR/ (several)

Nothing here uses the callback.  I don't want to merge this until we
have a user.

I'm not sure why the device driver should know whether its device is
the default boot device.
> Signed-off-by: Sui Jingfeng <suijingfeng at loongson.cn>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 +-
>  drivers/gpu/drm/i915/display/intel_vga.c   |  3 +--
>  drivers/gpu/drm/nouveau/nouveau_vga.c      |  2 +-
>  drivers/gpu/drm/radeon/radeon_device.c     |  2 +-
>  drivers/pci/vgaarb.c                       | 22 ++++++++++++++++++----
>  drivers/vfio/pci/vfio_pci_core.c           |  2 +-
>  include/linux/vgaarb.h                     |  8 +++++---
>  7 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5c7d40873ee2..7a096f2d5c16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3960,7 +3960,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  	/* this will fail for cards that aren't VGA class devices, just
>  	 * ignore it */
>  	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> -		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> +		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode, NULL);
>  
>  	px = amdgpu_device_supports_px(ddev);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c
b/drivers/gpu/drm/i915/display/intel_vga.c
> index 286a0bdd28c6..98d7d4dffe9f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -115,7 +115,6 @@ intel_vga_set_decode(struct pci_dev *pdev, bool
enable_decode)
>  
>  int intel_vga_register(struct drm_i915_private *i915)
>  {
> -
>  	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>  	int ret;
>  
> @@ -127,7 +126,7 @@ int intel_vga_register(struct drm_i915_private *i915)
>  	 * then we do not take part in VGA arbitration and the
>  	 * vga_client_register() fails with -ENODEV.
>  	 */
> -	ret = vga_client_register(pdev, intel_vga_set_decode);
> +	ret = vga_client_register(pdev, intel_vga_set_decode, NULL);
>  	if (ret && ret != -ENODEV)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c
b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index f8bf0ec26844..162b4f4676c7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -92,7 +92,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
>  		return;
>  	pdev = to_pci_dev(dev->dev);
>  
> -	vga_client_register(pdev, nouveau_vga_set_decode);
> +	vga_client_register(pdev, nouveau_vga_set_decode, NULL);
>  
>  	/* don't register Thunderbolt eGPU with vga_switcheroo */
>  	if (pci_is_thunderbolt_attached(pdev))
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
> index afbb3a80c0c6..71f2ff39d6a1 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1425,7 +1425,7 @@ int radeon_device_init(struct radeon_device *rdev,
>  	/* if we have > 1 VGA cards, then disable the radeon VGA resources */
>  	/* this will fail for cards that aren't VGA class devices, just
>  	 * ignore it */
> -	vga_client_register(rdev->pdev, radeon_vga_set_decode);
> +	vga_client_register(rdev->pdev, radeon_vga_set_decode, NULL);
>  
>  	if (rdev->flags & RADEON_IS_PX)
>  		runtime = true;
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index b0bf4952a95d..d3dab61e0ef2 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -53,6 +53,7 @@ struct vga_device {
>  	bool bridge_has_one_vga;
>  	bool is_firmware_default;	/* device selected by firmware */
>  	unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
> +	bool (*is_boot_device)(struct pci_dev *pdev);
>  };
>  
>  static LIST_HEAD(vga_list);
> @@ -614,10 +615,17 @@ static bool vga_is_boot_device(struct vga_device
*vgadev)
>  	if (boot_vga && boot_vga->is_firmware_default)
>  		return false;
>  
> -	if (vga_is_firmware_default(pdev)) {
> -		vgadev->is_firmware_default = true;
> +	/*
> +	 * Ask the device driver first, if registered. Fallback to the
> +	 * default implement if the callback is non-exist.
> +	 */
> +	if (vgadev->is_boot_device)
> +		vgadev->is_firmware_default = vgadev->is_boot_device(pdev);
> +	else
> +		vgadev->is_firmware_default = vga_is_firmware_default(pdev);
> +
> +	if (vgadev->is_firmware_default)
>  		return true;
> -	}
>  
>  	/*
>  	 * A legacy VGA device has MEM and IO enabled and any bridges
> @@ -954,6 +962,10 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>   * @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.
>   *
> + * @is_boot_device: callback to the device driver, query if a client is
the
> + * default boot device, as the device driver typically has better
knowledge
> + * if specific device is the boot device. But this callback is optional.
> + *
>   * Rationale: we cannot disable VGA decode resources unconditionally, some
>   * single GPU laptops seem to require ACPI or BIOS access to the VGA
registers
>   * to control things like backlights etc. Hopefully newer multi-GPU
laptops do
> @@ -969,7 +981,8 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
>   * Returns: 0 on success, -1 on failure
>   */
>  int vga_client_register(struct pci_dev *pdev,
> -		unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
> +		unsigned int (*set_decode)(struct pci_dev *pdev, bool decode),
> +		bool (*is_boot_device)(struct pci_dev *pdev))
>  {
>  	int ret = -ENODEV;
>  	struct vga_device *vgadev;
> @@ -981,6 +994,7 @@ int vga_client_register(struct pci_dev *pdev,
>  		goto bail;
>  
>  	vgadev->set_decode = set_decode;
> +	vgadev->is_boot_device = is_boot_device;
>  	ret = 0;
>  
>  bail:
> diff --git a/drivers/vfio/pci/vfio_pci_core.c
b/drivers/vfio/pci/vfio_pci_core.c
> index a5ab416cf476..2a8873a330ba 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -2067,7 +2067,7 @@ static int vfio_pci_vga_init(struct
vfio_pci_core_device *vdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = vga_client_register(pdev, vfio_pci_set_decode);
> +	ret = vga_client_register(pdev, vfio_pci_set_decode, NULL);
>  	if (ret)
>  		return ret;
>  	vga_set_legacy_decoding(pdev, vfio_pci_set_decode(pdev, false));
> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
> index d36225c582ee..66fe80ffad76 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -50,7 +50,8 @@ struct pci_dev *vga_default_device(void);
>  void vga_set_default_device(struct pci_dev *pdev);
>  int vga_remove_vgacon(struct pci_dev *pdev);
>  int vga_client_register(struct pci_dev *pdev,
> -		unsigned int (*set_decode)(struct pci_dev *pdev, bool state));
> +		unsigned int (*set_decode)(struct pci_dev *pdev, bool state),
> +		bool (*is_boot_device)(struct pci_dev *pdev));
>  #else /* CONFIG_VGA_ARB */
>  static inline void vga_set_legacy_decoding(struct pci_dev *pdev,
>  		unsigned int decodes)
> @@ -76,7 +77,8 @@ static inline int vga_remove_vgacon(struct pci_dev *pdev)
>  	return 0;
>  }
>  static inline int vga_client_register(struct pci_dev *pdev,
> -		unsigned int (*set_decode)(struct pci_dev *pdev, bool state))
> +		unsigned int (*set_decode)(struct pci_dev *pdev, bool state),
> +		bool (*is_boot_device)(struct pci_dev *pdev))
>  {
>  	return 0;
>  }
> @@ -114,7 +116,7 @@ static inline int vga_get_uninterruptible(struct
pci_dev *pdev,
>  
>  static inline void vga_client_unregister(struct pci_dev *pdev)
>  {
> -	vga_client_register(pdev, NULL);
> +	vga_client_register(pdev, NULL, NULL);
>  }
>  
>  #endif /* LINUX_VGA_H */
> -- 
> 2.25.1
>

Apparently Analagous Threads

Search for more possibly parallel threads

Nouveau - Jun 2023 - [PATCH v3 0/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

[Nouveau] [PATCH v3 0/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

[Nouveau] [PATCH v3 1/4] PCI/VGA: tidy up the code and comment format

[Nouveau] [PATCH v3 2/4] PCI/VGA: Use unsigned type for the io_state variable

[Nouveau] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices

[Nouveau] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

[Nouveau] [Intel-gfx] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices

[Nouveau] [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

Apparently Analagous Threads