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

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

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@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

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

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@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@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

Re: [Intel-gfx] [PATCH v3 1/4] PCI/VGA: tidy up the code and comment format

[Bjorn Helgaas]

Capitalize subject to match ("Tidy ...")

On Thu, Jun 08, 2023 at 07:43:19PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> This patch replaces the leading space with a tab and removes the double
> blank line, no functional change.

Can you move this to the end of the series?  The functional changes
are more likely to be backported, and I think the backport may be a
little easier without the cleanup in the middle.

>  	/* 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
> +	 */

Since you're touching this anyway, can you update it to the
conventional multi-line comment style:

  /*
   * We could in theory ...
   */

And capitalize "We", add a period at end, and rewrap to fill 78
columns or so?  Same for other comments below.

> +++ 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.
>   */

Can you make a separate patch to replace this entire copyright notice
with the appropriate SPDX-License-Identifier header?
Documentation/process/license-rules.rst has details.

Bjorn

Re: [Intel-gfx] [PATCH v3 1/4] PCI/VGA: tidy up the code and comment format

[Sui Jingfeng]

Hi,

On 2023/6/9 03:07, Bjorn Helgaas wrote:
> Capitalize subject to match ("Tidy ...")
>
> On Thu, Jun 08, 2023 at 07:43:19PM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> This patch replaces the leading space with a tab and removes the double
>> blank line, no functional change.
> Can you move this to the end of the series?  The functional changes
> are more likely to be backported, and I think the backport may be a
> little easier without the cleanup in the middle.

OK, acceptable.

>>   	/* 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
>> +	 */
> Since you're touching this anyway, can you update it to the
> conventional multi-line comment style:
>
>    /*
>     * We could in theory ...
>     */
>
> And capitalize "We", add a period at end, and rewrap to fill 78
> columns or so?  Same for other comments below.
OK, I could improve this at next version.
>> +++ 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.
>>    */
> Can you make a separate patch to replace this entire copyright notice
> with the appropriate SPDX-License-Identifier header?
> Documentation/process/license-rules.rst has details.

Wow ...

> Bjorn

-- 
Jingfeng

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

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@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@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

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

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@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@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

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

[Bjorn Helgaas]

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

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

[Sui Jingfeng]

Hi,

On 2023/6/9 03:12, Bjorn Helgaas wrote:
> 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@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/

Will be fixed at next version, thanks for the strict checking,

I will try to bring this rigorous to the other patch of myself in the 
future.

> 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 :)

I think, I probably should only do what I'm good at.


The "good at" here means that It's under prepared,

already matured with thinking(or consideration) by myself.

And also including testing(on two or three architecture).


That idea is belong to you, I would like to see how does it going to 
happen only.

> Bjorn
>
>> Signed-off-by: Sui Jingfeng <suijingfeng@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
>>
-- 
Jingfeng

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

[Sui Jingfeng]

Hi,

On 2023/6/9 03:12, Bjorn Helgaas wrote:
> 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@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.

Nice idea!


But I'm wondering there are traps in this.

The pci_notifier in vgaarb.c is still need on Mips platform.

Because of loading order problems.

On MIPS system, PCI devices are enumerated by pcibios_init(),

which runs after vga_arb_device_init(). This is true until now.

When vga_arb_device_init() function get called, it will capture nothing.

On that time, the system have no PCI device enumerated.

Because of this, there are various problems in the past.

This is the reason we still need the notifier,

we need a way to capture the PCI display device after vgaarb already 
loaded(finished).


On complex case, there are ASpeed BMC, loongson integrated display 
controller,

and radeon discrete video card co-exist on Loongson 3B4000 server.

I have fixed various bug at our downstream(linux-4.19) environment.


I have fixed a bug on downstream involved with aspeed bmc, by workaround[1].

Chen Huacai grabbing my patch[1] and rewrite it, submit together with 
him-self's.

Its fine,  but those patch still look paleness in the front of Loongson 
integrated display controller.

Loongson integrated display controller don't has a dedicated VRAM bar.

vga_is_firmware_default() will lose its effectiveness then.

This is the reason we reveal our patch(0004 in this series) to face 
upstream.

Its not only for loongson integrated display controller through.

Its not uncommon that ARM servers have A aspeed BMC and discrete amdgpu 
or old radeon card.

Let the device drivers gave us a hint probably can help to resolve 
multi-video card co-exist

problem.

Consider that sometime user want to use integrate gpu, sometime user 
want to use discrete gpu.

Also, during driver developing(or debugging),

driver writer may want override the default boot device.


vgaarb probable shouldn't make the decision

without giving the device driver a chance to override.


Beside,  vga_is_firmware_default() only apply for PCI display device.

On ARM64 system, there are a lot of platform device.

If we move this function back to the device driver,

it probably applicable for a platform display controller + one/two PCI 
display devices case.


We find a method at downstream during we get efifb works LoongArch platform.

We can utilize the screen_info, screen_info say where's the firmware 
framebuffer is located at.

Drivers for specific hardware platform perhaps know more clearly than 
vgaarb.

if it is the default boot device.


[1] 
https://lore.kernel.org/all/20210514080025.1828197-6-chenhuacai@loongson.cn/

> Or if you wanted to do that now, that would be even better :)
>
> Bjorn
>
>> Signed-off-by: Sui Jingfeng <suijingfeng@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
>>
-- 
Jingfeng

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

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@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@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

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

[Bjorn Helgaas]

On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@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@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
> 

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

[Sui Jingfeng]

Hi,

On 2023/6/9 03:19, Bjorn Helgaas wrote:
> On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng <suijingfeng@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.

This is chicken and egg question.

If you could help get this merge first, I will show you the first user.

> I'm not sure why the device driver should know whether its device is
> the default boot device.

It's not that the device driver should know,

but it's about that the device driver has the right to override.

Device driver may have better approach to identify the default boot device.

>> Signed-off-by: Sui Jingfeng <suijingfeng@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
>>
-- 
Jingfeng

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

[Bjorn Helgaas]

On Fri, Jun 09, 2023 at 10:27:39AM +0800, Sui Jingfeng wrote:
> On 2023/6/9 03:19, Bjorn Helgaas wrote:
> > On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote:
> > > From: Sui Jingfeng <suijingfeng@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.
> 
> This is chicken and egg question.
> 
> If you could help get this merge first, I will show you the first user.
> 
> > I'm not sure why the device driver should know whether its device is
> > the default boot device.
> 
> It's not that the device driver should know,
> 
> but it's about that the device driver has the right to override.
> 
> Device driver may have better approach to identify the default boot
> device.

The way we usually handle this is to include the new callback in the
same series as the first user of it.  That has two benefits:
(1) everybody can review the whole picture and possibly suggest
different approaches, and (2) when we merge the infrastructure,
we also merge a user of it at the same time, so the whole thing can be
tested and we don't end up with unused code.

Bjorn

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

[Sui Jingfeng]

On 2023/6/10 00:48, Bjorn Helgaas wrote:
> On Fri, Jun 09, 2023 at 10:27:39AM +0800, Sui Jingfeng wrote:
>> On 2023/6/9 03:19, Bjorn Helgaas wrote:
>>> On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote:
>>>> From: Sui Jingfeng <suijingfeng@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.
>> This is chicken and egg question.
>>
>> If you could help get this merge first, I will show you the first user.
>>
>>> I'm not sure why the device driver should know whether its device is
>>> the default boot device.
>> It's not that the device driver should know,
>>
>> but it's about that the device driver has the right to override.
>>
>> Device driver may have better approach to identify the default boot
>> device.
> The way we usually handle this is to include the new callback in the
> same series as the first user of it.  That has two benefits:
> (1) everybody can review the whole picture and possibly suggest
> different approaches, and (2) when we merge the infrastructure,
> we also merge a user of it at the same time, so the whole thing can be
> tested and we don't end up with unused code.

OK, acceptable

I will try to prepare the user code of this callback and respin the patch.

I may resend it with another patch set in the future, this series 
already drop it,

see v5[1]

[1] https://patchwork.freedesktop.org/series/119134/

> Bjorn