From: Sui Jingfeng <[email protected]>
Various improve.
Sui Jingfeng (6):
PCI/VGA: Use unsigned type for the io_state variable
PCI/VGA: Deal with PCI VGA compatible devices only
PCI/VGA: drop the inline of vga_update_device_decodes() function
PCI/VGA: Move the new_state assignment out the loop
PCI/VGA: Tidy up the code and comment format
PCI/VGA: Replace full MIT license text with SPDX identifier
drivers/pci/vgaarb.c | 168 ++++++++++++++++++++++-------------------
include/linux/vgaarb.h | 27 +------
2 files changed, 96 insertions(+), 99 deletions(-)
--
2.25.1
From: Sui Jingfeng <[email protected]>
Currently, vgaarb only cares about PCI VGA-compatible class devices.
While vga_arbiter_del_pci_device() gets called unbalanced when some PCI
device is about to be removed. This happens even during the boot process.
Another reason is that the vga_arb_device_init() function is not efficient.
Since we only care about VGA-compatible devices (pdev->class == 0x030000),
We could filter the unqualified devices out in the vga_arb_device_init()
function. While the current implementation is to search all PCI devices
in a system, this is not necessary.
Reviewed-by: Mario Limonciello <[email protected]>
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/pci/vgaarb.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index c1bc6c983932..021116ed61cb 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -754,10 +754,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) {
@@ -1502,6 +1498,10 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
vgaarb_dbg(dev, "%s\n", __func__);
+ /* Deal with VGA compatible devices only */
+ 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
* cases of hotplugable vga cards. */
@@ -1534,8 +1534,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)
@@ -1543,13 +1543,14 @@ 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 */
- pdev = NULL;
- while ((pdev =
- pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
- PCI_ANY_ID, pdev)) != NULL)
- vga_arbiter_add_pci_device(pdev);
+ /*
+ * We add all PCI VGA compatible devices in the arbiter by default
+ */
+ do {
+ pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
+ if (pdev)
+ vga_arbiter_add_pci_device(pdev);
+ } while (pdev);
pr_info("loaded\n");
return rc;
--
2.25.1
From: Sui Jingfeng <[email protected]>
In the vga_arbiter_notify_clients() function, the value of the 'new_state'
variable will be 'false' on systems that have more than one PCI VGA card.
Its value will be 'true' on systems that have one or no PCI VGA compatible
card. In other words, its value is not relevant to the iteration, so move
the assignment () out of the loop.
For a system with multiple video cards, this patch saves the redundant
assignment.
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/pci/vgaarb.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 668139f7c247..4c448c758bab 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1468,22 +1468,20 @@ static void vga_arbiter_notify_clients(void)
{
struct vga_device *vgadev;
unsigned long flags;
- uint32_t new_decodes;
- bool new_state;
+ bool state;
if (!vga_arbiter_used)
return;
+ state = (vga_count > 1) ? false : true;
+
spin_lock_irqsave(&vga_lock, flags);
list_for_each_entry(vgadev, &vga_list, list) {
- if (vga_count > 1)
- new_state = false;
- else
- new_state = true;
if (vgadev->set_decode) {
- new_decodes = vgadev->set_decode(vgadev->pdev,
- new_state);
- vga_update_device_decodes(vgadev, new_decodes);
+ unsigned int decodes;
+
+ decodes = vgadev->set_decode(vgadev->pdev, state);
+ vga_update_device_decodes(vgadev, decodes);
}
}
spin_unlock_irqrestore(&vga_lock, flags);
--
2.25.1
From: Sui Jingfeng <[email protected]>
The vga_update_device_decodes() function is NOT a trivial function, It is
not performance critical either. So, drop the inline.
This patch also make the parameter and argument consistent, use unsigned
int type instead of the signed type to store the decode. Change the second
argument of vga_update_device_decodes() function as 'unsigned int' type.
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/pci/vgaarb.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 021116ed61cb..668139f7c247 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -860,24 +860,24 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
return ret;
}
-/* this is called with the lock */
-static inline void vga_update_device_decodes(struct vga_device *vgadev,
- int new_decodes)
+/* This is called with the lock */
+static void vga_update_device_decodes(struct vga_device *vgadev,
+ unsigned int new_decodes)
{
struct device *dev = &vgadev->pdev->dev;
- int old_decodes, decodes_removed, decodes_unlocked;
+ unsigned int old_decodes = vgadev->decodes;
+ unsigned int decodes_removed = ~new_decodes & old_decodes;
+ unsigned int decodes_unlocked = vgadev->locks & decodes_removed;
- old_decodes = vgadev->decodes;
- decodes_removed = ~new_decodes & old_decodes;
- decodes_unlocked = vgadev->locks & decodes_removed;
vgadev->decodes = new_decodes;
- vgaarb_info(dev, "changed VGA decodes: olddecodes=%s,decodes=%s:owns=%s\n",
- vga_iostate_to_str(old_decodes),
- vga_iostate_to_str(vgadev->decodes),
- vga_iostate_to_str(vgadev->owns));
+ vgaarb_info(dev,
+ "VGA decodes changed: olddecodes=%s,decodes=%s:owns=%s\n",
+ vga_iostate_to_str(old_decodes),
+ vga_iostate_to_str(vgadev->decodes),
+ vga_iostate_to_str(vgadev->owns));
- /* if we removed locked decodes, lock count goes to zero, and release */
+ /* If we removed locked decodes, lock count goes to zero, and release */
if (decodes_unlocked) {
if (decodes_unlocked & VGA_RSRC_LEGACY_IO)
vgadev->io_lock_cnt = 0;
--
2.25.1
From: Sui Jingfeng <[email protected]>
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 <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
---
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 5a696078b382..c1bc6c983932 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -77,7 +77,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
From: Sui Jingfeng <[email protected]>
Per Documentation/process/license-rules.rst, the SPDX MIT identifier is
equivalent to including the entire MIT license text from
LICENSES/preferred/MIT.
Replace the MIT license text with the equivalent SPDX identifier.
Signed-off-by: Sui Jingfeng <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
---
include/linux/vgaarb.h | 23 ++---------------------
1 file changed, 2 insertions(+), 21 deletions(-)
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 6d5465f8c3f2..97129a1bbb7d 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -1,3 +1,5 @@
+/* SPDX-License-Identifier: MIT */
+
/*
* The VGA aribiter manages VGA space routing and VGA resource decode to
* allow multiple VGA devices to be used in a system in a safe way.
@@ -5,27 +7,6 @@
* (C) Copyright 2005 Benjamin Herrenschmidt <[email protected]>
* (C) Copyright 2007 Paulo R. Zanoni <[email protected]>
* (C) Copyright 2007, 2009 Tiago Vignatti <[email protected]>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * 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.
- *
*/
#ifndef LINUX_VGA_H
--
2.25.1
From: Sui Jingfeng <[email protected]>
This patch replaces the leading space with a tab and removes the double
blank line and fix various typos, no functional change.
Reviewed-by: Andi Shyti <[email protected]>
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/pci/vgaarb.c | 101 ++++++++++++++++++++++++-----------------
include/linux/vgaarb.h | 4 +-
2 files changed, 61 insertions(+), 44 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 4c448c758bab..bf96e085751d 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -32,10 +32,10 @@
#include <linux/acpi.h>
#include <linux/uaccess.h>
-
#include <linux/vgaarb.h>
static void vga_arbiter_notify_clients(void);
+
/*
* We keep a list of all vga devices in the system to speed
* up the various operations of the arbiter
@@ -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 */
@@ -79,8 +78,10 @@ static const char *vga_iostate_to_str(unsigned int iostate)
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 */
+ /*
+ * We could in theory hand out locks on IO and mem
+ * separately to userspace but it can cause deadlocks
+ */
if (strncmp(buf, "none", 4) == 0) {
*io_state = VGA_RSRC_NONE;
return 1;
@@ -99,7 +100,7 @@ static int vga_str_to_iostate(char *buf, int str_size, unsigned 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 */
@@ -193,14 +194,17 @@ int vga_remove_vgacon(struct pci_dev *pdev)
#endif
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 */
+/*
+ * 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
+ */
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 */
+ /*
+ * We should inform all GPUs in the system that
+ * vgaarb has occurred and to try and disable resources if they can
+ */
if (!vga_arbiter_used) {
vga_arbiter_used = true;
vga_arbiter_notify_clients();
@@ -216,7 +220,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
unsigned int pci_bits;
u32 flags = 0;
- /* Account for "normal" resources to lock. If we decode the legacy,
+ /*
+ * Account for "normal" resources to lock. If we decode the legacy,
* counterpart, we need to request it as well
*/
if ((rsrc & VGA_RSRC_NORMAL_IO) &&
@@ -236,7 +241,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
if (wants == 0)
goto lock_them;
- /* We don't need to request a legacy resource, we just enable
+ /*
+ * We don't need to request a legacy resource, we just enable
* appropriate decoding and go
*/
legacy_wants = wants & VGA_RSRC_LEGACY_MASK;
@@ -252,7 +258,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
if (vgadev == conflict)
continue;
- /* We have a possible conflict. before we go further, we must
+ /*
+ * We have a possible conflict. before we go further, we must
* check if we sit on the same bus as the conflicting device.
* if we don't, then we must tie both IO and MEM resources
* together since there is only a single bit controlling
@@ -263,13 +270,15 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
lwants = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
}
- /* Check if the guy has a lock on the resource. If he does,
+ /*
+ * Check if the guy has a lock on the resource. If he does,
* return the conflicting entry
*/
if (conflict->locks & lwants)
return conflict;
- /* Ok, now check if it owns the resource we want. We can
+ /*
+ * Ok, now check if it owns the resource we want. We can
* lock resources that are not decoded, therefore a device
* can own resources it doesn't decode.
*/
@@ -277,14 +286,16 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
if (!match)
continue;
- /* looks like he doesn't have a lock, we can steal
+ /*
+ * Looks like he doesn't have a lock, we can steal
* them from him
*/
flags = 0;
pci_bits = 0;
- /* If we can't control legacy resources via the bridge, we
+ /*
+ * If we can't control legacy resources via the bridge, we
* also need to disable normal decoding.
*/
if (!conflict->bridge_has_one_vga) {
@@ -311,7 +322,8 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
}
enable_them:
- /* ok dude, we got it, everybody conflicting has been disabled, let's
+ /*
+ * Ok dude, we got it, everybody conflicting has been disabled, let's
* enable us. Mark any bits in "owns" regardless of whether we
* decoded them. We can lock resources we don't decode, therefore
* we must track them via "owns".
@@ -353,7 +365,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
vgaarb_dbg(dev, "%s\n", __func__);
- /* Update our counters, and account for equivalent legacy resources
+ /*
+ * Update our counters, and account for equivalent legacy resources
* if we decode them
*/
if ((rsrc & VGA_RSRC_NORMAL_IO) && vgadev->io_norm_cnt > 0) {
@@ -371,7 +384,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
if ((rsrc & VGA_RSRC_LEGACY_MEM) && vgadev->mem_lock_cnt > 0)
vgadev->mem_lock_cnt--;
- /* Just clear lock bits, we do lazy operations so we don't really
+ /*
+ * Just clear lock bits, we do lazy operations so we don't really
* have to bother about anything else at this point
*/
if (vgadev->io_lock_cnt == 0)
@@ -379,7 +393,8 @@ static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
if (vgadev->mem_lock_cnt == 0)
vgadev->locks &= ~VGA_RSRC_LEGACY_MEM;
- /* Kick the wait queue in case somebody was waiting if we actually
+ /*
+ * Kick the wait queue in case somebody was waiting if we actually
* released something
*/
if (old_locks != vgadev->locks)
@@ -447,8 +462,8 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
if (conflict == NULL)
break;
-
- /* We have a conflict, we wait until somebody kicks the
+ /*
+ * We have a conflict, we wait until somebody kicks the
* work queue. Currently we have one work queue that we
* kick each time some resources are released, but it would
* be fairly easy to have a per device one so that we only
@@ -665,7 +680,7 @@ static bool vga_is_boot_device(struct vga_device *vgadev)
}
/*
- * vgadev has neither IO nor MEM enabled. If we haven't found any
+ * Vgadev has neither IO nor MEM enabled. If we haven't found any
* other VGA devices, it is the best candidate so far.
*/
if (!boot_vga)
@@ -706,7 +721,7 @@ static void vga_arbiter_check_bridge_sharing(struct vga_device *vgadev)
bus = same_bridge_vgadev->pdev->bus;
bridge = bus->self;
- /* see if the share a bridge with this device */
+ /* See if the share a bridge with this device */
if (new_bridge == bridge) {
/*
* If their direct parent bridge is the same
@@ -777,9 +792,10 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
vgadev->decodes = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
- /* by default mark it as decoding */
+ /* By default, mark it as decoding */
vga_decode_count++;
- /* Mark that we "own" resources based on our enables, we will
+ /*
+ * Mark that we "own" resources based on our enables, we will
* clear that below if the bridge isn't forwarding
*/
pci_read_config_word(pdev, PCI_COMMAND, &cmd);
@@ -886,7 +902,7 @@ static void vga_update_device_decodes(struct vga_device *vgadev,
__vga_put(vgadev, decodes_unlocked);
}
- /* change decodes counter */
+ /* Change decodes counter */
if (old_decodes & VGA_RSRC_LEGACY_MASK &&
!(new_decodes & VGA_RSRC_LEGACY_MASK))
vga_decode_count--;
@@ -910,14 +926,15 @@ static void __vga_set_legacy_decoding(struct pci_dev *pdev,
if (vgadev == NULL)
goto bail;
- /* don't let userspace futz with kernel driver decodes */
+ /* Don't let userspace futz with kernel driver decodes */
if (userspace && vgadev->set_decode)
goto bail;
- /* update the device decodes + counter */
+ /* Update the device decodes + counter */
vga_update_device_decodes(vgadev, decodes);
- /* XXX if somebody is going from "doesn't decode" to "decodes" state
+ /*
+ * XXX if somebody is going from "doesn't decode" to "decodes" state
* here, additional care must be taken as we may have pending owner
* ship of non-legacy region ...
*/
@@ -952,9 +969,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.
@@ -984,7 +1001,6 @@ int vga_client_register(struct pci_dev *pdev,
bail:
spin_unlock_irqrestore(&vga_lock, flags);
return ret;
-
}
EXPORT_SYMBOL(vga_client_register);
@@ -1075,7 +1091,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;
@@ -1310,7 +1325,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
curr_pos += 7;
remaining -= 7;
pr_debug("client 0x%p called 'target'\n", priv);
- /* if target is default */
+ /* If target is default */
if (!strncmp(curr_pos, "default", 7))
pdev = pci_dev_get(vga_default_device());
else {
@@ -1427,7 +1442,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;
}
@@ -1461,7 +1475,7 @@ static int vga_arb_release(struct inode *inode, struct file *file)
}
/*
- * callback any registered clients to let them know we have a
+ * Callback any registered clients to let them know we have a
* change in VGA cards
*/
static void vga_arbiter_notify_clients(void)
@@ -1500,9 +1514,12 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
return 0;
- /* For now we're only intereted in devices added and removed. I didn't
+ /*
+ * For now we're only interested in devices added and removed. I didn't
* test this thing here, so someone needs to double check for the
- * cases of hotplugable vga cards. */
+ * cases of hotplugable vga cards.
+ */
+
if (action == BUS_NOTIFY_ADD_DEVICE)
notify = vga_arbiter_add_pci_device(pdev);
else if (action == BUS_NOTIFY_DEL_DEVICE)
diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index b4b9137f9792..6d5465f8c3f2 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -96,7 +96,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 +111,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
On Tue, Jul 11, 2023 at 09:43:50PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <[email protected]>
>
> Currently, vgaarb only cares about PCI VGA-compatible class devices.
>
> While vga_arbiter_del_pci_device() gets called unbalanced when some PCI
> device is about to be removed. This happens even during the boot process.
The previous code calls vga_arbiter_add_pci_device() for every device
(every device present at boot and also every hot-added device). It
only allocates a vga_device if pdev->class is 0x0300XX.
It calls vga_arbiter_del_pci_device() for every device removal. It
does nothing unless it finds a vga_device.
This seems symmetric and reasonable to me. Did you observe a problem
with it?
> Another reason is that the vga_arb_device_init() function is not efficient.
> Since we only care about VGA-compatible devices (pdev->class == 0x030000),
> We could filter the unqualified devices out in the vga_arb_device_init()
> function. While the current implementation is to search all PCI devices
> in a system, this is not necessary.
Optimization is fine, but the most important thing here is to be clear
about what functional change this patch makes. As I mentioned at [1],
if this patch affects the class codes accepted, please make that clear
here.
> Reviewed-by: Mario Limonciello <[email protected]>
> Signed-off-by: Sui Jingfeng <[email protected]>
I do not see Mario's Reviewed-by on the list. I do see Mario's
Reviewed-by [2] for a previous version, but that version added this in
pci_notify():
+ if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
+ return 0;
while this version adds:
+ if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+ return 0;
It's OK to carry a review to future versions if there are
insignificant changes, but this is a functional change that seems
significant to me. The first matches only 0x030000, while the second
discards the low eight bits so it matches 0x0300XX.
[1] https://lore.kernel.org/r/20230718231400.GA496927@bhelgaas
[2] https://lore.kernel.org/all/[email protected]/
> ---
> drivers/pci/vgaarb.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index c1bc6c983932..021116ed61cb 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -754,10 +754,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) {
> @@ -1502,6 +1498,10 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>
> vgaarb_dbg(dev, "%s\n", __func__);
>
> + /* Deal with VGA compatible devices only */
> + 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
> * cases of hotplugable vga cards. */
> @@ -1534,8 +1534,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)
> @@ -1543,13 +1543,14 @@ 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 */
> - pdev = NULL;
> - while ((pdev =
> - pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> - PCI_ANY_ID, pdev)) != NULL)
> - vga_arbiter_add_pci_device(pdev);
> + /*
> + * We add all PCI VGA compatible devices in the arbiter by default
> + */
> + do {
> + pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
> + if (pdev)
> + vga_arbiter_add_pci_device(pdev);
> + } while (pdev);
>
> pr_info("loaded\n");
> return rc;
> --
> 2.25.1
>
Hi,
On 2023/7/20 02:26, Bjorn Helgaas wrote:
> On Tue, Jul 11, 2023 at 09:43:50PM +0800, Sui Jingfeng wrote:
[...]
>> Reviewed-by: Mario Limonciello <[email protected]>
>> Signed-off-by: Sui Jingfeng <[email protected]>
> I do not see Mario's Reviewed-by on the list. I do see Mario's
> Reviewed-by [2] for a previous version, but that version added this in
> pci_notify():
>
> + if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
> + return 0;
>
> while this version adds:
>
> + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> + return 0;
>
> It's OK to carry a review to future versions if there are
> insignificant changes, but this is a functional change that seems
> significant to me. The first matches only 0x030000, while the second
> discards the low eight bits so it matches 0x0300XX.
Yes, you are right.
But I suddenly realized that this may deserve another patch, desperate
trivial.
What this version adds here is *same* before this patch set is applied.
My explanation about the minor tweak being made before this version and
previous version
is that I want to keep my patch *less distraction*.
The major functional gains(benefit) is that we filter non VGA compatible
devices out.
As a start point, I should keep one patch do one thing (do one thing and
do it well).
On the other hand, even though the lest significant 8 but if pdev->class
is really matter.
I think I still need to wait the things(a bug emerged, for example)
became clear.
Instead of cleanup all potential problems with obvious motivation.
I think Mario will accept my explanation.
> [1] https://lore.kernel.org/r/20230718231400.GA496927@bhelgaas
> [2] https://lore.kernel.org/all/[email protected]/
>
>> ---
>> drivers/pci/vgaarb.c | 25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index c1bc6c983932..021116ed61cb 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -754,10 +754,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) {
>> @@ -1502,6 +1498,10 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>>
>> vgaarb_dbg(dev, "%s\n", __func__);
>>
>> + /* Deal with VGA compatible devices only */
>> + 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
>> * cases of hotplugable vga cards. */
>> @@ -1534,8 +1534,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)
>> @@ -1543,13 +1543,14 @@ 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 */
>> - pdev = NULL;
>> - while ((pdev =
>> - pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>> - PCI_ANY_ID, pdev)) != NULL)
>> - vga_arbiter_add_pci_device(pdev);
>> + /*
>> + * We add all PCI VGA compatible devices in the arbiter by default
>> + */
>> + do {
>> + pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
>> + if (pdev)
>> + vga_arbiter_add_pci_device(pdev);
>> + } while (pdev);
>>
>> pr_info("loaded\n");
>> return rc;
>> --
>> 2.25.1
>>
On 2023/7/20 03:58, Sui Jingfeng wrote:
> What this version adds here is *same* before this patch set is applied.
The filter method is *same* , in the cases of before this patch is
applied and after this patch is applied.
On 2023/7/20 03:58, Sui Jingfeng wrote:
> On the other hand, even though the lest significant 8 but if
> pdev->class is really matter.
If the low eight bits of pdev->class is really matters,
maybe we should wait the potential problems became severe.
Currently, it is not obvious.
On 2023/7/20 03:58, Sui Jingfeng wrote:
> My explanation about the minor tweak being made before this version
> and previous version
>
> is that I want to keep my patch *less distraction*.
The minor tweak being made between this version and previous version is
to keep my patch *less distraction*.
Hi,
On 2023/7/20 02:26, Bjorn Helgaas wrote:
> On Tue, Jul 11, 2023 at 09:43:50PM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng<[email protected]>
>>
>> Currently, vgaarb only cares about PCI VGA-compatible class devices.
>>
>> While vga_arbiter_del_pci_device() gets called unbalanced when some PCI
>> device is about to be removed. This happens even during the boot process.
> The previous code calls vga_arbiter_add_pci_device() for every device
> (every device present at boot and also every hot-added device). It
> only allocates a vga_device if pdev->class is 0x0300XX.
>
> It calls vga_arbiter_del_pci_device() for every device removal. It
> does nothing unless it finds a vga_device.
> This seems symmetric and reasonable to me. Did you observe a problem
> with it?
>
Not big deal, but the vgaarb does do some useless work there.
Right, it calls vga_arbiter_del_pci_device() for every device removal.
And it can not finds a vga_device at the most time. (Because on normal
case, a user only have one or two GPU device in the system.)
But even it can not finds a vga_device, vga_arbiter_del_pci_device()
still brings
additional(and it is unnecessary) overheads.
For an example, on my i3-8100 (the motherboard model is H110 D4L) machine,
The PCI device(0000:00:1f.1) will trigger the call to
vga_arbiter_del_pci_device().
Even though it can not finds a vga_device,
vga_arbiter_del_pci_device() is *NOT* a no-op still.
```
static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
{
struct vga_device *vgadev;
unsigned long flags;
bool ret = true;
spin_lock_irqsave(&vga_lock, flags);
vgadev = vgadev_find(pdev);
if (vgadev == NULL) {
ret = false;
goto bail;
}
// omit ...
bail:
spin_unlock_irqrestore(&vga_lock, flags);
kfree(vgadev);
return ret;
}
```
1) It call spin_lock_irqsave() and spin_unlock_irqrestore() pair for
complete irrelevant PCI devices
2) It try to find a vgadev with pdev pointer, which have to search the
whole list (All nodes in the list got accessed), because it can not find.
3) It call kfree() to free NULL pointer, it's just that kfree() will
just return if you pass a NULL, so no bug happen.
It is not efficient.
While the major contribution of my patch is to filter irrelevant PCI device.
Otherwise there 30+ noisy(useless) events got snooped. See below:
```
[ 0.246077] pci 0000:01:00.0: vgaarb: setting as boot VGA device
[ 0.246077] pci 0000:01:00.0: vgaarb: bridge control possible
[ 0.246077] pci 0000:01:00.0: vgaarb: VGA device added:
decodes=io+mem,owns=io+mem,locks=none
[ 0.246077] vgaarb: loaded
[ 0.294169] skl_uncore 0000:00:00.0: vgaarb: pci_notify: action=3
[ 0.294182] skl_uncore 0000:00:00.0: vgaarb: pci_notify: action=4
[ 0.301297] pcieport 0000:00:01.0: vgaarb: pci_notify: action=3
[ 0.301482] pcieport 0000:00:01.0: vgaarb: pci_notify: action=4
[ 0.301488] pcieport 0000:00:1c.0: vgaarb: pci_notify: action=3
[ 0.301705] pcieport 0000:00:1c.0: vgaarb: pci_notify: action=4
[ 1.806445] xhci_hcd 0000:00:14.0: vgaarb: pci_notify: action=3
[ 1.810976] ahci 0000:00:17.0: vgaarb: pci_notify: action=3
[ 1.824383] xhci_hcd 0000:00:14.0: vgaarb: pci_notify: action=4
[ 1.857470] ahci 0000:00:17.0: vgaarb: pci_notify: action=4
[ 4.692700] intel_pch_thermal 0000:00:14.2: vgaarb: pci_notify: action=3
[ 4.693110] intel_pch_thermal 0000:00:14.2: vgaarb: pci_notify: action=4
[ 4.746712] i801_smbus 0000:00:1f.4: vgaarb: pci_notify: action=3
[ 4.747212] pci 0000:00:1f.1: vgaarb: pci_notify: action=0
[ 4.747227] pci 0000:00:1f.1: vgaarb: pci_notify: action=1
[ 4.747250] pci 0000:00:1f.1: vgaarb: pci_notify: action=2
[ 4.749098] i801_smbus 0000:00:1f.4: vgaarb: pci_notify: action=4
[ 4.799217] mei_me 0000:00:16.0: vgaarb: pci_notify: action=3
[ 4.802503] mei_me 0000:00:16.0: vgaarb: pci_notify: action=4
[ 4.874880] intel-lpss 0000:00:15.0: vgaarb: pci_notify: action=3
[ 4.881227] intel-lpss 0000:00:15.0: vgaarb: pci_notify: action=4
[ 4.881240] intel-lpss 0000:00:15.1: vgaarb: pci_notify: action=3
[ 4.887578] intel-lpss 0000:00:15.1: vgaarb: pci_notify: action=4
[ 4.985796] r8169 0000:02:00.0: vgaarb: pci_notify: action=3
[ 4.991862] r8169 0000:02:00.0: vgaarb: pci_notify: action=4
[ 5.404835] snd_hda_intel 0000:00:1f.3: vgaarb: pci_notify: action=3
[ 5.405175] snd_hda_intel 0000:00:1f.3: vgaarb: pci_notify: action=4
[ 5.405401] snd_hda_intel 0000:01:00.1: vgaarb: pci_notify: action=3
[ 5.405973] snd_hda_intel 0000:01:00.1: vgaarb: pci_notify: action=4
[ 10.793665] i915 0000:00:02.0: vgaarb: pci_notify: action=3
[ 11.201384] i915 0000:00:02.0: vgaarb: pci_notify: action=4
[ 16.135842] amdgpu 0000:01:00.0: vgaarb: pci_notify: action=3
[ 16.140458] amdgpu 0000:01:00.0: vgaarb: deactivate vga console
[ 16.638564] amdgpu 0000:01:00.0: vgaarb: pci_notify: action=4
```
Hi,
On 2023/7/20 05:13, Sui Jingfeng wrote:
> Otherwise there 30+ noisy(useless) events got snooped. See below:
>
>
> ```
>
> [ 0.246077] pci 0000:01:00.0: vgaarb: setting as boot VGA device
> [ 0.246077] pci 0000:01:00.0: vgaarb: bridge control possible
> [ 0.246077] pci 0000:01:00.0: vgaarb: VGA device added:
> decodes=io+mem,owns=io+mem,locks=none
> [ 0.246077] vgaarb: loaded
> [ 0.294169] skl_uncore 0000:00:00.0: vgaarb: pci_notify: action=3
> [ 0.294182] skl_uncore 0000:00:00.0: vgaarb: pci_notify: action=4
> [ 0.301297] pcieport 0000:00:01.0: vgaarb: pci_notify: action=3
> [ 0.301482] pcieport 0000:00:01.0: vgaarb: pci_notify: action=4
> [ 0.301488] pcieport 0000:00:1c.0: vgaarb: pci_notify: action=3
> [ 0.301705] pcieport 0000:00:1c.0: vgaarb: pci_notify: action=4
> [ 1.806445] xhci_hcd 0000:00:14.0: vgaarb: pci_notify: action=3
> [ 1.810976] ahci 0000:00:17.0: vgaarb: pci_notify: action=3
> [ 1.824383] xhci_hcd 0000:00:14.0: vgaarb: pci_notify: action=4
> [ 1.857470] ahci 0000:00:17.0: vgaarb: pci_notify: action=4
> [ 4.692700] intel_pch_thermal 0000:00:14.2: vgaarb: pci_notify:
> action=3
> [ 4.693110] intel_pch_thermal 0000:00:14.2: vgaarb: pci_notify:
> action=4
> [ 4.746712] i801_smbus 0000:00:1f.4: vgaarb: pci_notify: action=3
> [ 4.747212] pci 0000:00:1f.1: vgaarb: pci_notify: action=0
> [ 4.747227] pci 0000:00:1f.1: vgaarb: pci_notify: action=1
> [ 4.747250] pci 0000:00:1f.1: vgaarb: pci_notify: action=2
> [ 4.749098] i801_smbus 0000:00:1f.4: vgaarb: pci_notify: action=4
> [ 4.799217] mei_me 0000:00:16.0: vgaarb: pci_notify: action=3
> [ 4.802503] mei_me 0000:00:16.0: vgaarb: pci_notify: action=4
> [ 4.874880] intel-lpss 0000:00:15.0: vgaarb: pci_notify: action=3
> [ 4.881227] intel-lpss 0000:00:15.0: vgaarb: pci_notify: action=4
> [ 4.881240] intel-lpss 0000:00:15.1: vgaarb: pci_notify: action=3
> [ 4.887578] intel-lpss 0000:00:15.1: vgaarb: pci_notify: action=4
> [ 4.985796] r8169 0000:02:00.0: vgaarb: pci_notify: action=3
> [ 4.991862] r8169 0000:02:00.0: vgaarb: pci_notify: action=4
> [ 5.404835] snd_hda_intel 0000:00:1f.3: vgaarb: pci_notify: action=3
> [ 5.405175] snd_hda_intel 0000:00:1f.3: vgaarb: pci_notify: action=4
> [ 5.405401] snd_hda_intel 0000:01:00.1: vgaarb: pci_notify: action=3
> [ 5.405973] snd_hda_intel 0000:01:00.1: vgaarb: pci_notify: action=4
> [ 10.793665] i915 0000:00:02.0: vgaarb: pci_notify: action=3
> [ 11.201384] i915 0000:00:02.0: vgaarb: pci_notify: action=4
> [ 16.135842] amdgpu 0000:01:00.0: vgaarb: pci_notify: action=3
> [ 16.140458] amdgpu 0000:01:00.0: vgaarb: deactivate vga console
> [ 16.638564] amdgpu 0000:01:00.0: vgaarb: pci_notify: action=4
>
> ```
After apply my patch, this events are still will notify me, it is just
that if we found it is irrelevant, we will return immediately.
No further process is needed.
Hi,
On 2023/7/20 02:26, Bjorn Helgaas wrote:
> Optimization is fine, but the most important thing here is to be clear
> about what functional change this patch makes. As I mentioned at [1],
> if this patch affects the class codes accepted, please make that clear
> here.
>
>> Reviewed-by: Mario Limonciello<[email protected]>
>> Signed-off-by: Sui Jingfeng<[email protected]>
> I do not see Mario's Reviewed-by on the list. I do see Mario's
> Reviewed-by [2] for a previous version, but that version added this in
> pci_notify():
>
> + if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
> + return 0;
>
> while this version adds:
>
> + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> + return 0;
>
> It's OK to carry a review to future versions if there are
> insignificant changes, but this is a functional change that seems
> significant to me. The first matches only 0x030000, while the second
> discards the low eight bits so it matches 0x0300XX.
>
> [1]https://lore.kernel.org/r/20230718231400.GA496927@bhelgaas
> [2]https://lore.kernel.org/all/[email protected]/
>
Yes, you are right. As you already told me at [1]:
According to the "PCI Code and ID Assignment" spec, r1.15, sec 1.4,
only mentions 0x0300 programming interface 0x00 as decoding
the legacy VGA addresses.
If the programming interface is 0x01, then it is a 8514-compatible
controller.
It is petty old card, about 30 years old(I think, it is nearly obsolete
for now).
I never have a chance to see such a card in real life.
Yes, we should adopt first matches method here. That is:
+ if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
+ return 0;
It seems that we are more rigorous to deal the VGA-compatible devices as
illustrated by above code here.
But who the still have that card (8514-compatible) and the hardware to
using such a card today ?
Please consider that the pci_dev_attrs_are_visible() function[3] also
ignore the
programming interface (the least significant 8 bits).
Therefore, at this version of my vgaarb cleanup patch set.
I choose to keep the original filtering rule,
but do the necessary optimization only which I think is meaningful.
In the future, we may want to expand VGAARB to deal all PCI display
class devices, with another patch.
if (pdev->class >> 16 == PCI_BASE_CLASS_DISPLAY)
// accept
else
// return immediately.
Then, we will have a more good chance to clarify the programmer interface.
Is this explanation feasible and reasonable, Bjorn and Mario ?
[3]
https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-sysfs.c#L1551
PING, please !
On 2023/7/11 21:43, Sui Jingfeng wrote:
> From: Sui Jingfeng <[email protected]>
>
> In the vga_arbiter_notify_clients() function, the value of the 'new_state'
> variable will be 'false' on systems that have more than one PCI VGA card.
> Its value will be 'true' on systems that have one or no PCI VGA compatible
> card. In other words, its value is not relevant to the iteration, so move
> the assignment () out of the loop.
>
> For a system with multiple video cards, this patch saves the redundant
> assignment.
>
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> drivers/pci/vgaarb.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 668139f7c247..4c448c758bab 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -1468,22 +1468,20 @@ static void vga_arbiter_notify_clients(void)
> {
> struct vga_device *vgadev;
> unsigned long flags;
> - uint32_t new_decodes;
> - bool new_state;
> + bool state;
>
> if (!vga_arbiter_used)
> return;
>
> + state = (vga_count > 1) ? false : true;
> +
> spin_lock_irqsave(&vga_lock, flags);
> list_for_each_entry(vgadev, &vga_list, list) {
> - if (vga_count > 1)
> - new_state = false;
> - else
> - new_state = true;
> if (vgadev->set_decode) {
> - new_decodes = vgadev->set_decode(vgadev->pdev,
> - new_state);
> - vga_update_device_decodes(vgadev, new_decodes);
> + unsigned int decodes;
> +
> + decodes = vgadev->set_decode(vgadev->pdev, state);
> + vga_update_device_decodes(vgadev, decodes);
> }
> }
> spin_unlock_irqrestore(&vga_lock, flags);
PING, please !
On 2023/7/11 21:43, Sui Jingfeng wrote:
> From: Sui Jingfeng <[email protected]>
>
> The vga_update_device_decodes() function is NOT a trivial function, It is
> not performance critical either. So, drop the inline.
>
> This patch also make the parameter and argument consistent, use unsigned
> int type instead of the signed type to store the decode. Change the second
> argument of vga_update_device_decodes() function as 'unsigned int' type.
>
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> drivers/pci/vgaarb.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 021116ed61cb..668139f7c247 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -860,24 +860,24 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
> return ret;
> }
>
> -/* this is called with the lock */
> -static inline void vga_update_device_decodes(struct vga_device *vgadev,
> - int new_decodes)
> +/* This is called with the lock */
> +static void vga_update_device_decodes(struct vga_device *vgadev,
> + unsigned int new_decodes)
> {
> struct device *dev = &vgadev->pdev->dev;
> - int old_decodes, decodes_removed, decodes_unlocked;
> + unsigned int old_decodes = vgadev->decodes;
> + unsigned int decodes_removed = ~new_decodes & old_decodes;
> + unsigned int decodes_unlocked = vgadev->locks & decodes_removed;
>
> - old_decodes = vgadev->decodes;
> - decodes_removed = ~new_decodes & old_decodes;
> - decodes_unlocked = vgadev->locks & decodes_removed;
> vgadev->decodes = new_decodes;
>
> - vgaarb_info(dev, "changed VGA decodes: olddecodes=%s,decodes=%s:owns=%s\n",
> - vga_iostate_to_str(old_decodes),
> - vga_iostate_to_str(vgadev->decodes),
> - vga_iostate_to_str(vgadev->owns));
> + vgaarb_info(dev,
> + "VGA decodes changed: olddecodes=%s,decodes=%s:owns=%s\n",
> + vga_iostate_to_str(old_decodes),
> + vga_iostate_to_str(vgadev->decodes),
> + vga_iostate_to_str(vgadev->owns));
>
> - /* if we removed locked decodes, lock count goes to zero, and release */
> + /* If we removed locked decodes, lock count goes to zero, and release */
> if (decodes_unlocked) {
> if (decodes_unlocked & VGA_RSRC_LEGACY_IO)
> vgadev->io_lock_cnt = 0;
On Mon, Jul 24, 2023 at 09:02:14PM +0800, suijingfeng wrote:
> PING, please !
Don't worry, these are not forgotten. Your other series seems more
important, so that's what I've been paying attention to.
> On 2023/7/11 21:43, Sui Jingfeng wrote:
> > From: Sui Jingfeng <[email protected]>
> >
> > In the vga_arbiter_notify_clients() function, the value of the 'new_state'
> > variable will be 'false' on systems that have more than one PCI VGA card.
> > Its value will be 'true' on systems that have one or no PCI VGA compatible
> > card. In other words, its value is not relevant to the iteration, so move
> > the assignment () out of the loop.
> >
> > For a system with multiple video cards, this patch saves the redundant
> > assignment.
> >
> > Signed-off-by: Sui Jingfeng <[email protected]>
> > ---
> > drivers/pci/vgaarb.c | 16 +++++++---------
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> > index 668139f7c247..4c448c758bab 100644
> > --- a/drivers/pci/vgaarb.c
> > +++ b/drivers/pci/vgaarb.c
> > @@ -1468,22 +1468,20 @@ static void vga_arbiter_notify_clients(void)
> > {
> > struct vga_device *vgadev;
> > unsigned long flags;
> > - uint32_t new_decodes;
> > - bool new_state;
> > + bool state;
> > if (!vga_arbiter_used)
> > return;
> > + state = (vga_count > 1) ? false : true;
> > +
> > spin_lock_irqsave(&vga_lock, flags);
> > list_for_each_entry(vgadev, &vga_list, list) {
> > - if (vga_count > 1)
> > - new_state = false;
> > - else
> > - new_state = true;
> > if (vgadev->set_decode) {
> > - new_decodes = vgadev->set_decode(vgadev->pdev,
> > - new_state);
> > - vga_update_device_decodes(vgadev, new_decodes);
> > + unsigned int decodes;
> > +
> > + decodes = vgadev->set_decode(vgadev->pdev, state);
> > + vga_update_device_decodes(vgadev, decodes);
> > }
> > }
> > spin_unlock_irqrestore(&vga_lock, flags);
>
On Sat, Jul 22, 2023 at 04:11:07PM +0800, suijingfeng wrote:
> ...
> In the future, we may want to expand VGAARB to deal all PCI display class
> devices, with another patch.
>
> if (pdev->class >> 16 == PCI_BASE_CLASS_DISPLAY)
>
> ???????? // accept
>
> else
>
> ????? // return immediately.
>
>
> Then, we will have a more good chance to clarify the programmer interface.
I would prefer not to expand vgaarb to deal with all display devices
unless they actually need the legacy resources like [pci 0xa0000-0xbffff].
But maybe the consumer of these interfaces relies on vgaarb even for
devices that don't need those resources? If so, please mention
examples of where they depend on vgaarb.
I expect the vgaarb interfaces are used by things that need to emulate
the option ROM to initialize the device. If the device has a
programming interface other than 0000 0000b, the option ROM should not
be using the [pci 0xa0000-0xbffff] resource, so vgaarb should not be
needed.
Bjorn
Hi,
On 2023/7/26 05:49, Bjorn Helgaas wrote:
> On Sat, Jul 22, 2023 at 04:11:07PM +0800, suijingfeng wrote:
>> ...
>> In the future, we may want to expand VGAARB to deal all PCI display class
>> devices, with another patch.
>>
>> if (pdev->class >> 16 == PCI_BASE_CLASS_DISPLAY)
>>
>> // accept
>>
>> else
>>
>> // return immediately.
>>
>>
>> Then, we will have a more good chance to clarify the programmer interface.
> I would prefer not to expand vgaarb to deal with all display devices
> unless they actually need the legacy resources like [pci 0xa0000-0xbffff].
What if a system have multiple non VGA-compatible GPU while all of them can display?
We still need to select a default for for user-space executable program (X server).
What if the VGA goes away someday?
I means that hardware vendors may abandon the old VGA standard.
After all, snooping a fixed address aperture is not absolute necessary for modern graphic card.
Modern graphic have dedicated VRAM Bar, the occupied address range can be relocatable.
Thus avoid the address overlap (or occlusion).
> But maybe the consumer of these interfaces relies on vgaarb even for
> devices that don't need those resources? If so, please mention
> examples of where they depend on vgaarb.
Yes, there do exist some PCI*NON* VGA-compatible display controllers,
Strictly speaking, there are not VGA-compatible in the sense that
they don't respond the fixed legacy VGA aperture.
Such a display controller also don't cares about the extension ROM (option ROM).
Loongson display controllers are one of the various examples.
Besides, Intel integrate GPU is capable switch to*NON* VGA-compatible.
especially in a multiple GPU co-exist hardware environment.
Old BIOS of Intel platform will change its class code from 0x0300 to 0x0380.
Newer BIOS do allow us to choose which one should be the primary GPU,
but if a user don't choose the Intel integrate GPU as primary,
the BIOS still will alter its PCI class code from 0x0300 to 0x0380.
By listing examples as above, I means that a PCI(e) GPU device do not
need to be VGA-compatible to display something on screen.
This is a very important point, I think,
which lead me to consider expand vgaarb.
I'm not sure if we should handle the programming interface thing here,
there are a lot of places where just ignore the programming interface.
> I expect the vgaarb interfaces are used by things that need to emulate
> the option ROM to initialize the device. If the device has a
> programming interface other than 0000 0000b, the option ROM should not
> be using the [pci 0xa0000-0xbffff] resource, so vgaarb should not be
> needed.
Also, I have another thought for this question.
The vga_set_default_device() function interface exported by vgaarb
is not ensure the restriction either.
I means that it does not check if a device is VGA-compatible,
it does not examine if the programming interface is 00000000b or 00000001b either.
In theory, a programmer could set a display device via the vga_set_default_device() interface.
Maybe this function is intentionally leave some space to workaround.
So, my idea is that leave programming interface related problems to the future.
I don't want to worry about a non-exist thing(programming interface == 0x01 for an example).
Back to my patch set, is this patch acceptable?
Or I still need to refine this series?
My other patches are queued up with this.
> Bjorn