[PATCH v7 0/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[PATCH v7 0/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Sui Jingfeng]

The vga_is_firmware_default() function is arch-dependent, it's probably
wrong if we simply remove the arch guard. As the VRAM BAR which contains
firmware framebuffer may move, while the lfb_base and lfb_size members of
the screen_info does not change accordingly. In short, it should take the
re-allocation of the PCI BAR into consideration.

With the observation that device drivers or video aperture helpers may
have better knowledge about which PCI bar contains the firmware fb,
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 not suitable to
make such an optimization since it is loaded too early.

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. Also honor
the comment: "Clients have two callback mechanisms they can use"

Sui Jingfeng (8):
  PCI/VGA: Use unsigned type for the io_state variable
  PCI/VGA: Deal only with VGA class devices
  PCI/VGA: Tidy up the code and comment format
  PCI/VGA: Replace full MIT license text with SPDX identifier
  video/aperture: Add a helper to detect if an aperture contains
    firmware FB
  PCI/VGA: Introduce is_boot_device function callback to
    vga_client_register
  drm/amdgpu: Implement the is_boot_device callback function
  drm/radeon: Implement the is_boot_device callback function

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  12 +-
 drivers/gpu/drm/drm_aperture.c             |  16 +++
 drivers/gpu/drm/i915/display/intel_vga.c   |   3 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c      |   2 +-
 drivers/gpu/drm/radeon/radeon_device.c     |  12 +-
 drivers/pci/vgaarb.c                       | 153 +++++++++++++--------
 drivers/vfio/pci/vfio_pci_core.c           |   2 +-
 drivers/video/aperture.c                   |  29 ++++
 include/drm/drm_aperture.h                 |   2 +
 include/linux/aperture.h                   |   7 +
 include/linux/vgaarb.h                     |  35 ++---
 11 files changed, 184 insertions(+), 89 deletions(-)

-- 
2.25.1

[PATCH v7 1/8] 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.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 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

[PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

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

Cc: Bjorn Helgaas <bhelgaas@google.com>
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 c1bc6c983932..22a505e877dc 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) {
@@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
+		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
@@ -1510,6 +1508,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;
@@ -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)
@@ -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: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
> device(pdev->class != 0x0300) out. There no need to process the non-display
> PCI device.
>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> 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 c1bc6c983932..22a505e877dc 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;
> -

Hi, here is probably a bug fixing.

For an example, nvidia render only GPU typically has 0x0380.

at its PCI class number, but  render only GPU should not participate in 
the arbitration.

As it shouldn't snoop the legacy fixed VGA address.

It(render only GPU) can not display anything.


But 0x0380 >> 8 = 0x03, the filter  failed.


>   	/* Allocate structure */
>   	vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>   	if (vgadev == NULL) {
> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
> +		return 0;

So here we only care 0x0300, my initial intent is to make an optimization,

nowadays sane display graphic card should all has 0x0300 as its PCI 
class number, is this complete right?

```

#define PCI_BASE_CLASS_DISPLAY        0x03
#define PCI_CLASS_DISPLAY_VGA        0x0300
#define PCI_CLASS_DISPLAY_XGA        0x0301
#define PCI_CLASS_DISPLAY_3D        0x0302
#define PCI_CLASS_DISPLAY_OTHER        0x0380

```

Any ideas ?

>   	/* 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
> @@ -1510,6 +1508,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;
> @@ -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)
> @@ -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;

-- 
Jingfeng

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Alex Deucher]

On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>
> Hi,
>
> On 2023/6/13 11:01, Sui Jingfeng wrote:
> > From: Sui Jingfeng <suijingfeng@loongson.cn>
> >
> > Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
> > pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
> > device(pdev->class != 0x0300) out. There no need to process the non-display
> > PCI device.
> >
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > 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 c1bc6c983932..22a505e877dc 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;
> > -
>
> Hi, here is probably a bug fixing.
>
> For an example, nvidia render only GPU typically has 0x0380.
>
> at its PCI class number, but  render only GPU should not participate in
> the arbitration.
>
> As it shouldn't snoop the legacy fixed VGA address.
>
> It(render only GPU) can not display anything.
>
>
> But 0x0380 >> 8 = 0x03, the filter  failed.
>
>
> >       /* Allocate structure */
> >       vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
> >       if (vgadev == NULL) {
> > @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
> > +             return 0;
>
> So here we only care 0x0300, my initial intent is to make an optimization,
>
> nowadays sane display graphic card should all has 0x0300 as its PCI
> class number, is this complete right?
>
> ```
>
> #define PCI_BASE_CLASS_DISPLAY        0x03
> #define PCI_CLASS_DISPLAY_VGA        0x0300
> #define PCI_CLASS_DISPLAY_XGA        0x0301
> #define PCI_CLASS_DISPLAY_3D        0x0302
> #define PCI_CLASS_DISPLAY_OTHER        0x0380
>
> ```
>
> Any ideas ?

I'm not quite sure what you are asking about here.  For vga_arb, we
only care about VGA class devices since those should be on the only
ones that might have VGA routed to them.  However, as VGA gets
deprecated, you'll have more non VGA PCI classes for devices which
could be the pre-OS console device.

Alex

>
> >       /* 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
> > @@ -1510,6 +1508,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;
> > @@ -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)
> > @@ -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;
>
> --
> Jingfeng
>

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/16 05:11, Alex Deucher wrote:
> On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>> Hi,
>>
>> On 2023/6/13 11:01, Sui Jingfeng wrote:
>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>
>>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
>>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
>>> device(pdev->class != 0x0300) out. There no need to process the non-display
>>> PCI device.
>>>
>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>> 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 c1bc6c983932..22a505e877dc 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;
>>> -
>> Hi, here is probably a bug fixing.
>>
>> For an example, nvidia render only GPU typically has 0x0380.
>>
>> as its PCI class number, but render only GPU should not participate in
>> the arbitration.
>>
>> As it shouldn't snoop the legacy fixed VGA address.
>>
>> It(render only GPU) can not display anything.
>>
>>
>> But 0x0380 >> 8 = 0x03, the filter  failed.
>>
>>
>>>        /* Allocate structure */
>>>        vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>>>        if (vgadev == NULL) {
>>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
>>> +             return 0;
>> So here we only care 0x0300, my initial intent is to make an optimization,
>>
>> nowadays sane display graphic card should all has 0x0300 as its PCI
>> class number, is this complete right?
>>
>> ```
>>
>> #define PCI_BASE_CLASS_DISPLAY        0x03
>> #define PCI_CLASS_DISPLAY_VGA        0x0300
>> #define PCI_CLASS_DISPLAY_XGA        0x0301
>> #define PCI_CLASS_DISPLAY_3D        0x0302
>> #define PCI_CLASS_DISPLAY_OTHER        0x0380
>>
>> ```
>>
>> Any ideas ?
> I'm not quite sure what you are asking about here.

To be honest, I'm worried about the PCI devices which has a

PCI_CLASS_DISPLAY_XGA as its PCI class number.

As those devices are very uncommon in the real world.


$ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"


Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,

there no code reference this macro. So I think it seems safe to ignore 
the XGA ?


PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate 
the render-only GPU.

And render-only GPU can't decode the fixed VGA address space, it is safe 
to ignore them.


>   For vga_arb, we
> only care about VGA class devices since those should be on the only
> ones that might have VGA routed to them.

>   However, as VGA gets deprecated,

We need the vgaarb for a system with multiple video card.

Not only because some Legacy VGA devices implemented

on PCI will typically have the same "hard-decoded" addresses;

But also these video card need to participate in the arbitration,

determine the default boot device.


Nowadays, the 'VGA devices' here is stand for the Graphics card

which is capable of display something on the screen.

We still need vgaarb to select the default boot device.


> you'll have more non VGA PCI classes for devices which
> could be the pre-OS console device.

Ah, we still want  do this(by applying this patch) first,

and then we will have the opportunity to see who will crying if 
something is broken. Will know more then.

But drop this patch or revise it with more consideration is also 
acceptable.


I asking about suggestion and/or review.

> Alex
>
>>>        /* 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
>>> @@ -1510,6 +1508,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;
>>> @@ -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)
>>> @@ -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;
>> --
>> Jingfeng
>>
-- 
Jingfeng

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Alex Deucher]

On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>
> Hi,
>
> On 2023/6/16 05:11, Alex Deucher wrote:
> > On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
> >> Hi,
> >>
> >> On 2023/6/13 11:01, Sui Jingfeng wrote:
> >>> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>
> >>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
> >>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
> >>> device(pdev->class != 0x0300) out. There no need to process the non-display
> >>> PCI device.
> >>>
> >>> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >>> 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 c1bc6c983932..22a505e877dc 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;
> >>> -
> >> Hi, here is probably a bug fixing.
> >>
> >> For an example, nvidia render only GPU typically has 0x0380.
> >>
> >> as its PCI class number, but render only GPU should not participate in
> >> the arbitration.
> >>
> >> As it shouldn't snoop the legacy fixed VGA address.
> >>
> >> It(render only GPU) can not display anything.
> >>
> >>
> >> But 0x0380 >> 8 = 0x03, the filter  failed.
> >>
> >>
> >>>        /* Allocate structure */
> >>>        vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
> >>>        if (vgadev == NULL) {
> >>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
> >>> +             return 0;
> >> So here we only care 0x0300, my initial intent is to make an optimization,
> >>
> >> nowadays sane display graphic card should all has 0x0300 as its PCI
> >> class number, is this complete right?
> >>
> >> ```
> >>
> >> #define PCI_BASE_CLASS_DISPLAY        0x03
> >> #define PCI_CLASS_DISPLAY_VGA        0x0300
> >> #define PCI_CLASS_DISPLAY_XGA        0x0301
> >> #define PCI_CLASS_DISPLAY_3D        0x0302
> >> #define PCI_CLASS_DISPLAY_OTHER        0x0380
> >>
> >> ```
> >>
> >> Any ideas ?
> > I'm not quite sure what you are asking about here.
>
> To be honest, I'm worried about the PCI devices which has a
>
> PCI_CLASS_DISPLAY_XGA as its PCI class number.
>
> As those devices are very uncommon in the real world.
>
>
> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
>
>
> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
>
> there no code reference this macro. So I think it seems safe to ignore
> the XGA ?
>
>
> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
> the render-only GPU.
>
> And render-only GPU can't decode the fixed VGA address space, it is safe
> to ignore them.
>
>
> >   For vga_arb, we
> > only care about VGA class devices since those should be on the only
> > ones that might have VGA routed to them.
>
> >   However, as VGA gets deprecated,
>
> We need the vgaarb for a system with multiple video card.
>
> Not only because some Legacy VGA devices implemented
>
> on PCI will typically have the same "hard-decoded" addresses;
>
> But also these video card need to participate in the arbitration,
>
> determine the default boot device.

But couldn't the boot device be determined via what whatever resources
were used by the pre-OS console?  I feel like that should be separate
from vgaarb.  vgaarb should handle PCI VGA routing and some other
mechanism should be used to determine what device provided the pre-OS
console.

Alex


>
>
> Nowadays, the 'VGA devices' here is stand for the Graphics card
>
> which is capable of display something on the screen.
>
> We still need vgaarb to select the default boot device.
>
>
> > you'll have more non VGA PCI classes for devices which
> > could be the pre-OS console device.
>
> Ah, we still want  do this(by applying this patch) first,
>
> and then we will have the opportunity to see who will crying if
> something is broken. Will know more then.
>
> But drop this patch or revise it with more consideration is also
> acceptable.
>
>
> I asking about suggestion and/or review.
>
> > Alex
> >
> >>>        /* 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
> >>> @@ -1510,6 +1508,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;
> >>> @@ -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)
> >>> @@ -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;
> >> --
> >> Jingfeng
> >>
> --
> Jingfeng
>

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

On 2023/6/16 21:41, Alex Deucher wrote:
> On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>> Hi,
>>
>> On 2023/6/16 05:11, Alex Deucher wrote:
>>> On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>> Hi,
>>>>
>>>> On 2023/6/13 11:01, Sui Jingfeng wrote:
>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>
>>>>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
>>>>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
>>>>> device(pdev->class != 0x0300) out. There no need to process the non-display
>>>>> PCI device.
>>>>>
>>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>>> 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 c1bc6c983932..22a505e877dc 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;
>>>>> -
>>>> Hi, here is probably a bug fixing.
>>>>
>>>> For an example, nvidia render only GPU typically has 0x0380.
>>>>
>>>> as its PCI class number, but render only GPU should not participate in
>>>> the arbitration.
>>>>
>>>> As it shouldn't snoop the legacy fixed VGA address.
>>>>
>>>> It(render only GPU) can not display anything.
>>>>
>>>>
>>>> But 0x0380 >> 8 = 0x03, the filter  failed.
>>>>
>>>>
>>>>>         /* Allocate structure */
>>>>>         vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>>>>>         if (vgadev == NULL) {
>>>>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
>>>>> +             return 0;
>>>> So here we only care 0x0300, my initial intent is to make an optimization,
>>>>
>>>> nowadays sane display graphic card should all has 0x0300 as its PCI
>>>> class number, is this complete right?
>>>>
>>>> ```
>>>>
>>>> #define PCI_BASE_CLASS_DISPLAY        0x03
>>>> #define PCI_CLASS_DISPLAY_VGA        0x0300
>>>> #define PCI_CLASS_DISPLAY_XGA        0x0301
>>>> #define PCI_CLASS_DISPLAY_3D        0x0302
>>>> #define PCI_CLASS_DISPLAY_OTHER        0x0380
>>>>
>>>> ```
>>>>
>>>> Any ideas ?
>>> I'm not quite sure what you are asking about here.
>> To be honest, I'm worried about the PCI devices which has a
>>
>> PCI_CLASS_DISPLAY_XGA as its PCI class number.
>>
>> As those devices are very uncommon in the real world.
>>
>>
>> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
>>
>>
>> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
>>
>> there no code reference this macro. So I think it seems safe to ignore
>> the XGA ?
>>
>>
>> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
>> the render-only GPU.
>>
>> And render-only GPU can't decode the fixed VGA address space, it is safe
>> to ignore them.
>>
>>
>>>    For vga_arb, we
>>> only care about VGA class devices since those should be on the only
>>> ones that might have VGA routed to them.
>>>    However, as VGA gets deprecated,
>> We need the vgaarb for a system with multiple video card.
>>
>> Not only because some Legacy VGA devices implemented
>>
>> on PCI will typically have the same "hard-decoded" addresses;
>>
>> But also these video card need to participate in the arbitration,
>>
>> determine the default boot device.
> But couldn't the boot device be determined via what whatever resources
> were used by the pre-OS console?

I don't know what you are refer to by saying  pre-OS console, UEFI 
SHELL,  UEFI GOP  or something like that.

If you are referring to the framebuffer driver which light up the screen 
before the Linux kernel is loaded .


Then, what you have said is true,  the boot device is determined by the 
pre-OS console.

But the problem is how does the Linux kernel(vgaarb) could know which 
one is the default boot device

on a multiple GPU machine.  Relaying on the firmware fb's address and 
size is what the mechanism

we already in using.


>   I feel like that should be separate from vgaarb.

Emm, this really deserved another patch, please ?

>   vgaarb should handle PCI VGA routing and some other
> mechanism should be used to determine what device provided the pre-OS
> console.

If the new mechanism need the firmware changed, then this probably break 
the old machine.

Also, this probably will get all arch involved. to get the new mechanism 
supported.

The testing pressure and review power needed is quite large.

drm/amdgpu and drm/radeon already being used on X86, ARM64,  Mips and 
more arch...

The reviewing process will became quite difficult then.

vgaarb is really what we already in use, and being used more than ten 
years ...


> Alex
>

>>
>> Nowadays, the 'VGA devices' here is stand for the Graphics card
>>
>> which is capable of display something on the screen.
>>
>> We still need vgaarb to select the default boot device.
>>
>>
>>> you'll have more non VGA PCI classes for devices which
>>> could be the pre-OS console device.
>> Ah, we still want  do this(by applying this patch) first,
>>
>> and then we will have the opportunity to see who will crying if
>> something is broken. Will know more then.
>>
>> But drop this patch or revise it with more consideration is also
>> acceptable.
>>
>>
>> I asking about suggestion and/or review.
>>
>>> Alex
>>>
>>>>>         /* 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
>>>>> @@ -1510,6 +1508,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;
>>>>> @@ -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)
>>>>> @@ -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;
>>>> --
>>>> Jingfeng
>>>>
>> --
>> Jingfeng
>>
-- 
Jingfeng

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Alex Deucher]

On Fri, Jun 16, 2023 at 10:22 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>
>
> On 2023/6/16 21:41, Alex Deucher wrote:
> > On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
> >> Hi,
> >>
> >> On 2023/6/16 05:11, Alex Deucher wrote:
> >>> On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
> >>>> Hi,
> >>>>
> >>>> On 2023/6/13 11:01, Sui Jingfeng wrote:
> >>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>>>
> >>>>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
> >>>>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
> >>>>> device(pdev->class != 0x0300) out. There no need to process the non-display
> >>>>> PCI device.
> >>>>>
> >>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >>>>> 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 c1bc6c983932..22a505e877dc 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;
> >>>>> -
> >>>> Hi, here is probably a bug fixing.
> >>>>
> >>>> For an example, nvidia render only GPU typically has 0x0380.
> >>>>
> >>>> as its PCI class number, but render only GPU should not participate in
> >>>> the arbitration.
> >>>>
> >>>> As it shouldn't snoop the legacy fixed VGA address.
> >>>>
> >>>> It(render only GPU) can not display anything.
> >>>>
> >>>>
> >>>> But 0x0380 >> 8 = 0x03, the filter  failed.
> >>>>
> >>>>
> >>>>>         /* Allocate structure */
> >>>>>         vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
> >>>>>         if (vgadev == NULL) {
> >>>>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
> >>>>> +             return 0;
> >>>> So here we only care 0x0300, my initial intent is to make an optimization,
> >>>>
> >>>> nowadays sane display graphic card should all has 0x0300 as its PCI
> >>>> class number, is this complete right?
> >>>>
> >>>> ```
> >>>>
> >>>> #define PCI_BASE_CLASS_DISPLAY        0x03
> >>>> #define PCI_CLASS_DISPLAY_VGA        0x0300
> >>>> #define PCI_CLASS_DISPLAY_XGA        0x0301
> >>>> #define PCI_CLASS_DISPLAY_3D        0x0302
> >>>> #define PCI_CLASS_DISPLAY_OTHER        0x0380
> >>>>
> >>>> ```
> >>>>
> >>>> Any ideas ?
> >>> I'm not quite sure what you are asking about here.
> >> To be honest, I'm worried about the PCI devices which has a
> >>
> >> PCI_CLASS_DISPLAY_XGA as its PCI class number.
> >>
> >> As those devices are very uncommon in the real world.
> >>
> >>
> >> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
> >>
> >>
> >> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
> >>
> >> there no code reference this macro. So I think it seems safe to ignore
> >> the XGA ?
> >>
> >>
> >> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
> >> the render-only GPU.
> >>
> >> And render-only GPU can't decode the fixed VGA address space, it is safe
> >> to ignore them.
> >>
> >>
> >>>    For vga_arb, we
> >>> only care about VGA class devices since those should be on the only
> >>> ones that might have VGA routed to them.
> >>>    However, as VGA gets deprecated,
> >> We need the vgaarb for a system with multiple video card.
> >>
> >> Not only because some Legacy VGA devices implemented
> >>
> >> on PCI will typically have the same "hard-decoded" addresses;
> >>
> >> But also these video card need to participate in the arbitration,
> >>
> >> determine the default boot device.
> > But couldn't the boot device be determined via what whatever resources
> > were used by the pre-OS console?
>
> I don't know what you are refer to by saying  pre-OS console, UEFI
> SHELL,  UEFI GOP  or something like that.
>

Right.  Before the OS loads the platform firmware generally sets up
something for display.  That could be GOP or vesa or some other
platform specific protocol.

> If you are referring to the framebuffer driver which light up the screen
> before the Linux kernel is loaded .
>
>
> Then, what you have said is true,  the boot device is determined by the
> pre-OS console.
>
> But the problem is how does the Linux kernel(vgaarb) could know which
> one is the default boot device
>
> on a multiple GPU machine.  Relaying on the firmware fb's address and
> size is what the mechanism
>
> we already in using.

Right.  It shouldn't need to depend on vgaarb.

>
>
> >   I feel like that should be separate from vgaarb.
>
> Emm, this really deserved another patch, please ?
>
> >   vgaarb should handle PCI VGA routing and some other
> > mechanism should be used to determine what device provided the pre-OS
> > console.
>
> If the new mechanism need the firmware changed, then this probably break
> the old machine.
>
> Also, this probably will get all arch involved. to get the new mechanism
> supported.
>
> The testing pressure and review power needed is quite large.
>
> drm/amdgpu and drm/radeon already being used on X86, ARM64,  Mips and
> more arch...
>
> The reviewing process will became quite difficult then.
>
> vgaarb is really what we already in use, and being used more than ten
> years ...

Yes, it works for x86 (and a few other platforms) today because of the
VGA legacy, so we can look at VGA routing to determine this.  But even
today, we don't need VGA routing to determine what was the primary
display before starting the OS.  We could probably have a platform
independent way to handle this by looking at the bread crumbs leftover
from the pre-OS environment.  E.g., for pre-UEFI platforms, we can
look at VGA routing.  For UEFI platforms we can look at what GOP left
us.  For various non-UEFI ARM/PPC/MIPS/etc. platforms we can look at
whatever breadcrumbs those pre-OS environments left.  That way when
VGA goes away, we can have a clean break and you won't need vgaarb if
the platform has no VGA devices.

Alex

>
>
> > Alex
> >
>
> >>
> >> Nowadays, the 'VGA devices' here is stand for the Graphics card
> >>
> >> which is capable of display something on the screen.
> >>
> >> We still need vgaarb to select the default boot device.
> >>
> >>
> >>> you'll have more non VGA PCI classes for devices which
> >>> could be the pre-OS console device.
> >> Ah, we still want  do this(by applying this patch) first,
> >>
> >> and then we will have the opportunity to see who will crying if
> >> something is broken. Will know more then.
> >>
> >> But drop this patch or revise it with more consideration is also
> >> acceptable.
> >>
> >>
> >> I asking about suggestion and/or review.
> >>
> >>> Alex
> >>>
> >>>>>         /* 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
> >>>>> @@ -1510,6 +1508,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;
> >>>>> @@ -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)
> >>>>> @@ -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;
> >>>> --
> >>>> Jingfeng
> >>>>
> >> --
> >> Jingfeng
> >>
> --
> Jingfeng
>

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/16 22:34, Alex Deucher wrote:
> On Fri, Jun 16, 2023 at 10:22 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>
>> On 2023/6/16 21:41, Alex Deucher wrote:
>>> On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>> Hi,
>>>>
>>>> On 2023/6/16 05:11, Alex Deucher wrote:
>>>>> On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2023/6/13 11:01, Sui Jingfeng wrote:
>>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>
>>>>>>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
>>>>>>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
>>>>>>> device(pdev->class != 0x0300) out. There no need to process the non-display
>>>>>>> PCI device.
>>>>>>>
>>>>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>> 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 c1bc6c983932..22a505e877dc 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;
>>>>>>> -
>>>>>> Hi, here is probably a bug fixing.
>>>>>>
>>>>>> For an example, nvidia render only GPU typically has 0x0380.
>>>>>>
>>>>>> as its PCI class number, but render only GPU should not participate in
>>>>>> the arbitration.
>>>>>>
>>>>>> As it shouldn't snoop the legacy fixed VGA address.
>>>>>>
>>>>>> It(render only GPU) can not display anything.
>>>>>>
>>>>>>
>>>>>> But 0x0380 >> 8 = 0x03, the filter  failed.
>>>>>>
>>>>>>
>>>>>>>          /* Allocate structure */
>>>>>>>          vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>>>>>>>          if (vgadev == NULL) {
>>>>>>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
>>>>>>> +             return 0;
>>>>>> So here we only care 0x0300, my initial intent is to make an optimization,
>>>>>>
>>>>>> nowadays sane display graphic card should all has 0x0300 as its PCI
>>>>>> class number, is this complete right?
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> #define PCI_BASE_CLASS_DISPLAY        0x03
>>>>>> #define PCI_CLASS_DISPLAY_VGA        0x0300
>>>>>> #define PCI_CLASS_DISPLAY_XGA        0x0301
>>>>>> #define PCI_CLASS_DISPLAY_3D        0x0302
>>>>>> #define PCI_CLASS_DISPLAY_OTHER        0x0380
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> Any ideas ?
>>>>> I'm not quite sure what you are asking about here.
>>>> To be honest, I'm worried about the PCI devices which has a
>>>>
>>>> PCI_CLASS_DISPLAY_XGA as its PCI class number.
>>>>
>>>> As those devices are very uncommon in the real world.
>>>>
>>>>
>>>> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
>>>>
>>>>
>>>> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
>>>>
>>>> there no code reference this macro. So I think it seems safe to ignore
>>>> the XGA ?
>>>>
>>>>
>>>> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
>>>> the render-only GPU.
>>>>
>>>> And render-only GPU can't decode the fixed VGA address space, it is safe
>>>> to ignore them.
>>>>
>>>>
>>>>>     For vga_arb, we
>>>>> only care about VGA class devices since those should be on the only
>>>>> ones that might have VGA routed to them.
>>>>>     However, as VGA gets deprecated,
>>>> We need the vgaarb for a system with multiple video card.
>>>>
>>>> Not only because some Legacy VGA devices implemented
>>>>
>>>> on PCI will typically have the same "hard-decoded" addresses;
>>>>
>>>> But also these video card need to participate in the arbitration,
>>>>
>>>> determine the default boot device.
>>> But couldn't the boot device be determined via what whatever resources
>>> were used by the pre-OS console?
>> I don't know what you are refer to by saying  pre-OS console, UEFI
>> SHELL,  UEFI GOP  or something like that.
>>
> Right.  Before the OS loads the platform firmware generally sets up
> something for display.  That could be GOP or vesa or some other
> platform specific protocol.
>
>> If you are referring to the framebuffer driver which light up the screen
>> before the Linux kernel is loaded .
>>
>>
>> Then, what you have said is true,  the boot device is determined by the
>> pre-OS console.
>>
>> But the problem is how does the Linux kernel(vgaarb) could know which
>> one is the default boot device
>>
>> on a multiple GPU machine.  Relaying on the firmware fb's address and
>> size is what the mechanism
>>
>> we already in using.
> Right.  It shouldn't need to depend on vgaarb.
>
>>
>>>    I feel like that should be separate from vgaarb.
>> Emm, this really deserved another patch, please ?
>>
>>>    vgaarb should handle PCI VGA routing and some other
>>> mechanism should be used to determine what device provided the pre-OS
>>> console.
>> If the new mechanism need the firmware changed, then this probably break
>> the old machine.
>>
>> Also, this probably will get all arch involved. to get the new mechanism
>> supported.
>>
>> The testing pressure and review power needed is quite large.
>>
>> drm/amdgpu and drm/radeon already being used on X86, ARM64,  Mips and
>> more arch...
>>
>> The reviewing process will became quite difficult then.
>>
>> vgaarb is really what we already in use, and being used more than ten
>> years ...
> Yes, it works for x86 (and a few other platforms) today because of the
> VGA legacy, so we can look at VGA routing to determine this.  But even
> today, we don't need VGA routing to determine what was the primary
> display before starting the OS.  We could probably have a platform
> independent way to handle this by looking at the bread crumbs leftover
> from the pre-OS environment.  E.g., for pre-UEFI platforms, we can
> look at VGA routing.  For UEFI platforms we can look at what GOP left
> us.  For various non-UEFI ARM/PPC/MIPS/etc. platforms we can look at
> whatever breadcrumbs those pre-OS environments left.  That way when
> VGA goes away, we can have a clean break and you won't need vgaarb if
> the platform has no VGA devices.

Yes, I'm complete agree the spirit you are convey here.

Patch 5[1] of this series is actually does what you have told me just now.

I put this function in video/aperture intended.


Its does not rely on "VGA routing".

Its also does not required the display device is PCI vga device.

Platform display controller drivers(drm/ingenic, drm/imx, drm/rockchip etc)

can also call this function(aperture_contain_firmware_fb).

It only require the platform has a firmware framebuffer driver 
registered successfully.

Both EFIFB are SIMPLEFB will be OK.


Beside  this, on systems with a platform display controller device and a 
PCI GPU device,

the current implement may always set the PCI GPU device as the default 
boot device.

This is probably true for the arm64 platform.


On the past, loongson LS2K1000 SoC  has a platform display controller,

the SoC also has PCIE controller, So drm/radeon driver can be used on it.

When discrete card is mounted on the system, the discrete gpu is always been

selected as the default boot device in the past.

Because radeon gpu is more faster, this behavior meet the user's 
expectation by accident.

Its funny and lucky.


But the problem is sometime you want to use the integrated one,

for example in the process of developing driver, or discrete GPU driver

has a bug panic X server,  the user(or the device driver) don't has the 
right to override.


Also with the current implement, even you disable the device driver.

vgaarb still make the decision for you,  which made the X server 
confused and panic.


Say on machine with loongson integrated display controller and a rx5700  
machine.

By passing the cmdline "modprobe.blacklist=amdgpu", then vgaarb will not 
agree.

He still mark the rx5700 as the default boot device. Then X server crash.

Because xf86-video-amdgpu(still loaded) has no root in the kernel do the 
service for it.


I'm doing such a thing because I need developing the drivers for this

integrated display controller, but I don't want unmounted the discrete 
GPU every time.

Because I also need to do the PRIME buffer sharing developing and test.


For AMD APU, this is also the case.

R5 200GE and 3000G processor has the integrated GPU,

On a machine with R5 5600G + nvidia gtx1060, a user may want to compare

which GPU is more powerful. But with the current implement, both the device

driver and the end user don't have a choice.


This is the reason why patch 6, 7 and 8[2] of this series is introduced.

Device driver could do the force override if it isn't the default device.

(by introducing another kernel cmd line)

Because of device driver(.ko) get loaded latter than vgaarb.


So feel free send a reviewed by ?


[1] https://patchwork.freedesktop.org/patch/542253/?series=119250&rev=1

[2] https://patchwork.freedesktop.org/patch/542255/?series=119250&rev=1

> Alex
>
>>
>>> Alex
>>>
>>>> Nowadays, the 'VGA devices' here is stand for the Graphics card
>>>>
>>>> which is capable of display something on the screen.
>>>>
>>>> We still need vgaarb to select the default boot device.
>>>>
>>>>
>>>>> you'll have more non VGA PCI classes for devices which
>>>>> could be the pre-OS console device.
>>>> Ah, we still want  do this(by applying this patch) first,
>>>>
>>>> and then we will have the opportunity to see who will crying if
>>>> something is broken. Will know more then.
>>>>
>>>> But drop this patch or revise it with more consideration is also
>>>> acceptable.
>>>>
>>>>
>>>> I asking about suggestion and/or review.
>>>>
>>>>> Alex
>>>>>
>>>>>>>          /* 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
>>>>>>> @@ -1510,6 +1508,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;
>>>>>>> @@ -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)
>>>>>>> @@ -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;
>>>>>> --
>>>>>> Jingfeng
>>>>>>
>>>> --
>>>> Jingfeng
>>>>
>> --
>> Jingfeng
>>
-- 
Jingfeng

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/16 22:34, Alex Deucher wrote:
> On Fri, Jun 16, 2023 at 10:22 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>
>> On 2023/6/16 21:41, Alex Deucher wrote:
>>> On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>> Hi,
>>>>
>>>> On 2023/6/16 05:11, Alex Deucher wrote:
>>>>> On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2023/6/13 11:01, Sui Jingfeng wrote:
>>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>
>>>>>>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
>>>>>>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
>>>>>>> device(pdev->class != 0x0300) out. There no need to process the non-display
>>>>>>> PCI device.
>>>>>>>
>>>>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>> 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 c1bc6c983932..22a505e877dc 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;
>>>>>>> -
>>>>>> Hi, here is probably a bug fixing.
>>>>>>
>>>>>> For an example, nvidia render only GPU typically has 0x0380.
>>>>>>
>>>>>> as its PCI class number, but render only GPU should not participate in
>>>>>> the arbitration.
>>>>>>
>>>>>> As it shouldn't snoop the legacy fixed VGA address.
>>>>>>
>>>>>> It(render only GPU) can not display anything.
>>>>>>
>>>>>>
>>>>>> But 0x0380 >> 8 = 0x03, the filter  failed.
>>>>>>
>>>>>>
>>>>>>>          /* Allocate structure */
>>>>>>>          vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>>>>>>>          if (vgadev == NULL) {
>>>>>>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
>>>>>>> +             return 0;
>>>>>> So here we only care 0x0300, my initial intent is to make an optimization,
>>>>>>
>>>>>> nowadays sane display graphic card should all has 0x0300 as its PCI
>>>>>> class number, is this complete right?
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> #define PCI_BASE_CLASS_DISPLAY        0x03
>>>>>> #define PCI_CLASS_DISPLAY_VGA        0x0300
>>>>>> #define PCI_CLASS_DISPLAY_XGA        0x0301
>>>>>> #define PCI_CLASS_DISPLAY_3D        0x0302
>>>>>> #define PCI_CLASS_DISPLAY_OTHER        0x0380
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> Any ideas ?
>>>>> I'm not quite sure what you are asking about here.
>>>> To be honest, I'm worried about the PCI devices which has a
>>>>
>>>> PCI_CLASS_DISPLAY_XGA as its PCI class number.
>>>>
>>>> As those devices are very uncommon in the real world.
>>>>
>>>>
>>>> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
>>>>
>>>>
>>>> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
>>>>
>>>> there no code reference this macro. So I think it seems safe to ignore
>>>> the XGA ?
>>>>
>>>>
>>>> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
>>>> the render-only GPU.
>>>>
>>>> And render-only GPU can't decode the fixed VGA address space, it is safe
>>>> to ignore them.
>>>>
>>>>
>>>>>     For vga_arb, we
>>>>> only care about VGA class devices since those should be on the only
>>>>> ones that might have VGA routed to them.
>>>>>     However, as VGA gets deprecated,
>>>> We need the vgaarb for a system with multiple video card.
>>>>
>>>> Not only because some Legacy VGA devices implemented
>>>>
>>>> on PCI will typically have the same "hard-decoded" addresses;
>>>>
>>>> But also these video card need to participate in the arbitration,
>>>>
>>>> determine the default boot device.
>>> But couldn't the boot device be determined via what whatever resources
>>> were used by the pre-OS console?
>> I don't know what you are refer to by saying  pre-OS console, UEFI
>> SHELL,  UEFI GOP  or something like that.
>>
> Right.  Before the OS loads the platform firmware generally sets up
> something for display.  That could be GOP or vesa or some other
> platform specific protocol.
>
>> If you are referring to the framebuffer driver which light up the screen
>> before the Linux kernel is loaded .
>>
>>
>> Then, what you have said is true,  the boot device is determined by the
>> pre-OS console.
>>
>> But the problem is how does the Linux kernel(vgaarb) could know which
>> one is the default boot device
>>
>> on a multiple GPU machine.  Relaying on the firmware fb's address and
>> size is what the mechanism
>>
>> we already in using.
> Right.  It shouldn't need to depend on vgaarb.
>
>>
>>>    I feel like that should be separate from vgaarb.
>> Emm, this really deserved another patch, please ?
>>
>>>    vgaarb should handle PCI VGA routing and some other
>>> mechanism should be used to determine what device provided the pre-OS
>>> console.
>> If the new mechanism need the firmware changed, then this probably break
>> the old machine.
>>
>> Also, this probably will get all arch involved. to get the new mechanism
>> supported.
>>
>> The testing pressure and review power needed is quite large.
>>
>> drm/amdgpu and drm/radeon already being used on X86, ARM64,  Mips and
>> more arch...
>>
>> The reviewing process will became quite difficult then.
>>
>> vgaarb is really what we already in use, and being used more than ten
>> years ...

This base class is defined for all types of display controllers.

For VGA devices (Sub-Class 00h), the Programming Interface byte is 
divided into a bit field that identifies additional video controller 
compatibilities.

A device can support multiple interfaces by using the bit map to 
indicate which interfaces are supported.

For the XGA devices (Sub-Class 01h), only the standard XGA interface is 
defined.

Sub-Class 02h is for controllers that have hardware support for 3D 
operations and are not VGA compatible.


> Yes, it works for x86 (and a few other platforms) today because of the
> VGA legacy, so we can look at VGA routing to determine this.  But even
> today, we don't need VGA routing to determine what was the primary
> display before starting the OS.  We could probably have a platform
> independent way to handle this by looking at the bread crumbs leftover
> from the pre-OS environment.

Yes, I agree with you.

>   E.g., for pre-UEFI platforms, we can
> look at VGA routing.  For UEFI platforms we can look at what GOP left
> us.  For various non-UEFI ARM/PPC/MIPS/etc. platforms we can look at
> whatever breadcrumbs those pre-OS environments left.  That way when
> VGA goes away, we can have a clean break and you won't need vgaarb if
> the platform has no VGA devices.

Yes, I agree with you.


Then, move vga_is_firmware_default() function to video/aperture.c also ?

Because this function shouldn't be platform dependent,

can be usable as long as the PCI resource relocation don't happen  (The 
vram bar don't move),

And screen_info is more about video specifci thing.


Yes your are right, it seems that the selection of the default boot 
device shouldn't depend on vgaarb.

As vgaarb is only for PCI vga compatible display controller.

It seems that platform display controller device should also 
participated in the arbitration.

Emm, but that may a bit difficult. Because we rely on the PCI notifier 
to snoop the bound between the drm driver and device,

call back to use if successful. So, what should I do next?  as a first step?

> Alex
>
>>
>>> Alex
>>>
>>>> Nowadays, the 'VGA devices' here is stand for the Graphics card
>>>>
>>>> which is capable of display something on the screen.
>>>>
>>>> We still need vgaarb to select the default boot device.
>>>>
>>>>
>>>>> you'll have more non VGA PCI classes for devices which
>>>>> could be the pre-OS console device.
>>>> Ah, we still want  do this(by applying this patch) first,
>>>>
>>>> and then we will have the opportunity to see who will crying if
>>>> something is broken. Will know more then.
>>>>
>>>> But drop this patch or revise it with more consideration is also
>>>> acceptable.
>>>>
>>>>
>>>> I asking about suggestion and/or review.
>>>>
>>>>> Alex
>>>>>
>>>>>>>          /* 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
>>>>>>> @@ -1510,6 +1508,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;
>>>>>>> @@ -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)
>>>>>>> @@ -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;
>>>>>> --
>>>>>> Jingfeng
>>>>>>
>>>> --
>>>> Jingfeng
>>>>
>> --
>> Jingfeng
>>
-- 
Jingfeng

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/18 20:11, Sui Jingfeng wrote:
> call back to use if successful


Call back to us if the drm device driver bound to the PCI GPU successfully

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

On 2023/6/18 20:11, Sui Jingfeng wrote:
> And screen_info is more about video specifci thing. 


screen_info is something more about video specific thing.

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/16 22:34, Alex Deucher wrote:
> On Fri, Jun 16, 2023 at 10:22 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>
>> On 2023/6/16 21:41, Alex Deucher wrote:
>>> On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>> Hi,
>>>>
>>>> On 2023/6/16 05:11, Alex Deucher wrote:
>>>>> On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2023/6/13 11:01, Sui Jingfeng wrote:
>>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>
>>>>>>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
>>>>>>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
>>>>>>> device(pdev->class != 0x0300) out. There no need to process the non-display
>>>>>>> PCI device.
>>>>>>>
>>>>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>> 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 c1bc6c983932..22a505e877dc 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;
>>>>>>> -
>>>>>> Hi, here is probably a bug fixing.
>>>>>>
>>>>>> For an example, nvidia render only GPU typically has 0x0380.
>>>>>>
>>>>>> as its PCI class number, but render only GPU should not participate in
>>>>>> the arbitration.
>>>>>>
>>>>>> As it shouldn't snoop the legacy fixed VGA address.
>>>>>>
>>>>>> It(render only GPU) can not display anything.
>>>>>>
>>>>>>
>>>>>> But 0x0380 >> 8 = 0x03, the filter  failed.
>>>>>>
>>>>>>
>>>>>>>          /* Allocate structure */
>>>>>>>          vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>>>>>>>          if (vgadev == NULL) {
>>>>>>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
>>>>>>> +             return 0;
>>>>>> So here we only care 0x0300, my initial intent is to make an optimization,
>>>>>>
>>>>>> nowadays sane display graphic card should all has 0x0300 as its PCI
>>>>>> class number, is this complete right?
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> #define PCI_BASE_CLASS_DISPLAY        0x03
>>>>>> #define PCI_CLASS_DISPLAY_VGA        0x0300
>>>>>> #define PCI_CLASS_DISPLAY_XGA        0x0301
>>>>>> #define PCI_CLASS_DISPLAY_3D        0x0302
>>>>>> #define PCI_CLASS_DISPLAY_OTHER        0x0380
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> Any ideas ?
>>>>> I'm not quite sure what you are asking about here.
>>>> To be honest, I'm worried about the PCI devices which has a
>>>>
>>>> PCI_CLASS_DISPLAY_XGA as its PCI class number.
>>>>
>>>> As those devices are very uncommon in the real world.
>>>>
>>>>
>>>> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
>>>>
>>>>
>>>> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
>>>>
>>>> there no code reference this macro. So I think it seems safe to ignore
>>>> the XGA ?
>>>>
>>>>
>>>> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
>>>> the render-only GPU.
>>>>
>>>> And render-only GPU can't decode the fixed VGA address space, it is safe
>>>> to ignore them.
>>>>
>>>>
>>>>>     For vga_arb, we
>>>>> only care about VGA class devices since those should be on the only
>>>>> ones that might have VGA routed to them.
>>>>>     However, as VGA gets deprecated,
>>>> We need the vgaarb for a system with multiple video card.
>>>>
>>>> Not only because some Legacy VGA devices implemented
>>>>
>>>> on PCI will typically have the same "hard-decoded" addresses;
>>>>
>>>> But also these video card need to participate in the arbitration,
>>>>
>>>> determine the default boot device.
>>> But couldn't the boot device be determined via what whatever resources
>>> were used by the pre-OS console?
>> I don't know what you are refer to by saying  pre-OS console, UEFI
>> SHELL,  UEFI GOP  or something like that.
>>
> Right.  Before the OS loads the platform firmware generally sets up
> something for display.  That could be GOP or vesa or some other
> platform specific protocol.
>
>> If you are referring to the framebuffer driver which light up the screen
>> before the Linux kernel is loaded .
>>
>>
>> Then, what you have said is true,  the boot device is determined by the
>> pre-OS console.
>>
>> But the problem is how does the Linux kernel(vgaarb) could know which
>> one is the default boot device
>>
>> on a multiple GPU machine.  Relaying on the firmware fb's address and
>> size is what the mechanism
>>
>> we already in using.
> Right.  It shouldn't need to depend on vgaarb.
>
>>
>>>    I feel like that should be separate from vgaarb.
>> Emm, this really deserved another patch, please ?
>>
>>>    vgaarb should handle PCI VGA routing and some other
>>> mechanism should be used to determine what device provided the pre-OS
>>> console.
>> If the new mechanism need the firmware changed, then this probably break
>> the old machine.
>>
>> Also, this probably will get all arch involved. to get the new mechanism
>> supported.
>>
>> The testing pressure and review power needed is quite large.
>>
>> drm/amdgpu and drm/radeon already being used on X86, ARM64,  Mips and
>> more arch...
>>
>> The reviewing process will became quite difficult then.
>>
>> vgaarb is really what we already in use, and being used more than ten
>> years ...
> Yes, it works for x86 (and a few other platforms) today because of the
> VGA legacy, so we can look at VGA routing to determine this.  But even
> today, we don't need VGA routing to determine what was the primary
> display before starting the OS.  We could probably have a platform
> independent way to handle this by looking at the bread crumbs leftover
> from the pre-OS environment.  E.g., for pre-UEFI platforms, we can
> look at VGA routing.  For UEFI platforms we can look at what GOP left
> us.  For various non-UEFI ARM/PPC/MIPS/etc. platforms we can look at
> whatever breadcrumbs those pre-OS environments left.  That way when
> VGA goes away, we can have a clean break and you won't need vgaarb if
> the platform has no VGA devices.


Thanks for the dear developers from AMDGPU,


Mario already give me a R-B to V6 of this series[1],  I link it to 
here,//for fear of lost it.

He may be busy,  not seeing the latest.


For this patch series, V6 is same with the V7.


[1] 
https://lore.kernel.org/all/5b6fdf65-b354-94a9-f883-be820157efad@amd.com/

> Alex
>
>>
>>> Alex
>>>
>>>> Nowadays, the 'VGA devices' here is stand for the Graphics card
>>>>
>>>> which is capable of display something on the screen.
>>>>
>>>> We still need vgaarb to select the default boot device.
>>>>
>>>>
>>>>> you'll have more non VGA PCI classes for devices which
>>>>> could be the pre-OS console device.
>>>> Ah, we still want  do this(by applying this patch) first,
>>>>
>>>> and then we will have the opportunity to see who will crying if
>>>> something is broken. Will know more then.
>>>>
>>>> But drop this patch or revise it with more consideration is also
>>>> acceptable.
>>>>
>>>>
>>>> I asking about suggestion and/or review.
>>>>
>>>>> Alex
>>>>>
>>>>>>>          /* 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
>>>>>>> @@ -1510,6 +1508,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;
>>>>>>> @@ -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)
>>>>>>> @@ -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;
>>>>>> --
>>>>>> Jingfeng
>>>>>>
>>>> --
>>>> Jingfeng
>>>>
>> --
>> Jingfeng
>>
-- 
Jingfeng

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/16 22:34, Alex Deucher wrote:
> On Fri, Jun 16, 2023 at 10:22 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>
>> On 2023/6/16 21:41, Alex Deucher wrote:
>>> On Fri, Jun 16, 2023 at 3:11 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>> Hi,
>>>>
>>>> On 2023/6/16 05:11, Alex Deucher wrote:
>>>>> On Wed, Jun 14, 2023 at 6:50 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2023/6/13 11:01, Sui Jingfeng wrote:
>>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>
>>>>>>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
>>>>>>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
>>>>>>> device(pdev->class != 0x0300) out. There no need to process the non-display
>>>>>>> PCI device.
>>>>>>>
>>>>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>> 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 c1bc6c983932..22a505e877dc 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;
>>>>>>> -
>>>>>> Hi, here is probably a bug fixing.
>>>>>>
>>>>>> For an example, nvidia render only GPU typically has 0x0380.
>>>>>>
>>>>>> as its PCI class number, but render only GPU should not participate in
>>>>>> the arbitration.
>>>>>>
>>>>>> As it shouldn't snoop the legacy fixed VGA address.
>>>>>>
>>>>>> It(render only GPU) can not display anything.
>>>>>>
>>>>>>
>>>>>> But 0x0380 >> 8 = 0x03, the filter  failed.
>>>>>>
>>>>>>
>>>>>>>          /* Allocate structure */
>>>>>>>          vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
>>>>>>>          if (vgadev == NULL) {
>>>>>>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
>>>>>>> +             return 0;
>>>>>> So here we only care 0x0300, my initial intent is to make an optimization,
>>>>>>
>>>>>> nowadays sane display graphic card should all has 0x0300 as its PCI
>>>>>> class number, is this complete right?
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> #define PCI_BASE_CLASS_DISPLAY        0x03
>>>>>> #define PCI_CLASS_DISPLAY_VGA        0x0300
>>>>>> #define PCI_CLASS_DISPLAY_XGA        0x0301
>>>>>> #define PCI_CLASS_DISPLAY_3D        0x0302
>>>>>> #define PCI_CLASS_DISPLAY_OTHER        0x0380
>>>>>>
>>>>>> ```
>>>>>>
>>>>>> Any ideas ?
>>>>> I'm not quite sure what you are asking about here.
>>>> To be honest, I'm worried about the PCI devices which has a
>>>>
>>>> PCI_CLASS_DISPLAY_XGA as its PCI class number.
>>>>
>>>> As those devices are very uncommon in the real world.
>>>>
>>>>
>>>> $ find . -name "*.c" -type f | xargs grep "PCI_CLASS_DISPLAY_XGA"
>>>>
>>>>
>>>> Grep the "PCI_CLASS_DISPLAY_XGA" in the linux kernel tree got ZERO,
>>>>
>>>> there no code reference this macro. So I think it seems safe to ignore
>>>> the XGA ?
>>>>
>>>>
>>>> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate
>>>> the render-only GPU.
>>>>
>>>> And render-only GPU can't decode the fixed VGA address space, it is safe
>>>> to ignore them.
>>>>
>>>>
>>>>>     For vga_arb, we
>>>>> only care about VGA class devices since those should be on the only
>>>>> ones that might have VGA routed to them.
>>>>>     However, as VGA gets deprecated,
>>>> We need the vgaarb for a system with multiple video card.
>>>>
>>>> Not only because some Legacy VGA devices implemented
>>>>
>>>> on PCI will typically have the same "hard-decoded" addresses;
>>>>
>>>> But also these video card need to participate in the arbitration,
>>>>
>>>> determine the default boot device.
>>> But couldn't the boot device be determined via what whatever resources
>>> were used by the pre-OS console?
>> I don't know what you are refer to by saying  pre-OS console, UEFI
>> SHELL,  UEFI GOP  or something like that.
>>
> Right.  Before the OS loads the platform firmware generally sets up
> something for display.  That could be GOP or vesa or some other
> platform specific protocol.
>
>> If you are referring to the framebuffer driver which light up the screen
>> before the Linux kernel is loaded .
>>
>>
>> Then, what you have said is true,  the boot device is determined by the
>> pre-OS console.
>>
>> But the problem is how does the Linux kernel(vgaarb) could know which
>> one is the default boot device
>>
>> on a multiple GPU machine.  Relaying on the firmware fb's address and
>> size is what the mechanism
>>
>> we already in using.
> Right.  It shouldn't need to depend on vgaarb.
>
>>
>>>    I feel like that should be separate from vgaarb.
>> Emm, this really deserved another patch, please ?
>>
>>>    vgaarb should handle PCI VGA routing and some other
>>> mechanism should be used to determine what device provided the pre-OS
>>> console.
>> If the new mechanism need the firmware changed, then this probably break
>> the old machine.
>>
>> Also, this probably will get all arch involved. to get the new mechanism
>> supported.
>>
>> The testing pressure and review power needed is quite large.
>>
>> drm/amdgpu and drm/radeon already being used on X86, ARM64,  Mips and
>> more arch...
>>
>> The reviewing process will became quite difficult then.
>>
>> vgaarb is really what we already in use, and being used more than ten
>> years ...
> Yes, it works for x86 (and a few other platforms) today because of the
> VGA legacy, so we can look at VGA routing to determine this.  But even
> today, we don't need VGA routing to determine what was the primary
> display before starting the OS.  We could probably have a platform
> independent way to handle this by looking at the bread crumbs leftover
> from the pre-OS environment.  E.g., for pre-UEFI platforms, we can
> look at VGA routing.  For UEFI platforms we can look at what GOP left
> us.  For various non-UEFI ARM/PPC/MIPS/etc. platforms we can look at
> whatever breadcrumbs those pre-OS environments left.

Yes, it's good idea. I know what you means.

I'm trying to craft platform-independent way to handle this, and I have 
something in my mind.

I already write a few scratch code to verify the idea.

But when I go deeper, I ask myself frequently, what's the problem you 
are going to solve ?

A platform-independent implement itself is not a good sake (not enough).

To allow the platform device P.K. with the PCI device is a good rationale.

But now,  I don't have such a hardware platform anymore.

Such a hardware platform should have PCIe controller integrated to 
support dedicated GPU.

It should also have integrated GPU(display controller). I should also be 
famous platform (ARM64 for example)

I means that we at least need a bug to push us to do so :-)


Strip it away from vgaarb sound like a challenge to Bjorn, I'm not going 
to do so.

Currently, we are trying to push it to be arch-independent,

which is to make vgaarb more functional(and more useful) on non-x86 arch,

which also help to amdgpu/radeon cooperate with the hardware vendor 
native integrated one harmoniously.

while there are even no enough supporter.

> VGA goes away, we can have a clean break and you won't need vgaarb if
> the platform has no VGA devices.
>
> Alex
>
>>
>>> Alex
>>>
>>>> Nowadays, the 'VGA devices' here is stand for the Graphics card
>>>>
>>>> which is capable of display something on the screen.
>>>>
>>>> We still need vgaarb to select the default boot device.
>>>>
>>>>
>>>>> you'll have more non VGA PCI classes for devices which
>>>>> could be the pre-OS console device.
>>>> Ah, we still want  do this(by applying this patch) first,
>>>>
>>>> and then we will have the opportunity to see who will crying if
>>>> something is broken. Will know more then.
>>>>
>>>> But drop this patch or revise it with more consideration is also
>>>> acceptable.
>>>>
>>>>
>>>> I asking about suggestion and/or review.
>>>>
>>>>> Alex
>>>>>
>>>>>>>          /* 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
>>>>>>> @@ -1510,6 +1508,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;
>>>>>>> @@ -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)
>>>>>>> @@ -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;
>>>>>> --
>>>>>> Jingfeng
>>>>>>
>>>> --
>>>> Jingfeng
>>>>
>> --
>> Jingfeng
>>

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Maciej W. Rozycki]

On Tue, 13 Jun 2023, Sui Jingfeng wrote:

> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the

 Typo here: s/devcie/device/.

> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
> device(pdev->class != 0x0300) out. There no need to process the non-display
> PCI device.

 I've only come across this patch series now.  Without diving into what 
this code actually does I have just one question as a matter of interest.

> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index c1bc6c983932..22a505e877dc 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
> +		return 0;

 Hmm, shouldn't this also handle PCI_CLASS_NOT_DEFINED_VGA?  As far as I 
know it is the equivalent of PCI_CLASS_DISPLAY_VGA for PCI <= 2.0 devices 
that were implemented before the idea of PCI device classes has developed 
into its current form.  I may have such a VGA device somewhere.

  Maciej

Re: [PATCH v7 2/8] PCI/VGA: Deal only with VGA class devices

[Sui Jingfeng]

Hi,

On 2023/6/19 11:02, Maciej W. Rozycki wrote:
> On Tue, 13 Jun 2023, Sui Jingfeng wrote:
>
>> Deal only with the VGA devcie(pdev->class == 0x0300), so replace the
>   Typo here: s/devcie/device/.
Thanks a lot,
>> pci_get_subsys() function with pci_get_class(). Filter the non-PCI display
>> device(pdev->class != 0x0300) out. There no need to process the non-display
>> PCI device.
>   I've only come across this patch series now.  Without diving into what
> this code actually does I have just one question as a matter of interest.
>
>> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
>> index c1bc6c983932..22a505e877dc 100644
>> --- a/drivers/pci/vgaarb.c
>> +++ b/drivers/pci/vgaarb.c
>> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
>> +		return 0;
>   Hmm, shouldn't this also handle PCI_CLASS_NOT_DEFINED_VGA?

If your machine have only one such a VGA card, it probably don't hurt.

But, such a card will also get ignored originally (before applying this 
patch).

>   As far as I
> know it is the equivalent of PCI_CLASS_DISPLAY_VGA for PCI <= 2.0 devices
> that were implemented before the idea of PCI device classes has developed
> into its current form.

If multiple video card problems on your machine is matter,

then I think it do deserve another patch to clarify this issue and to 
explain the rationale.

>   I may have such a VGA device somewhere.
>
>    Maciej

[PATCH v7 3/8] 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.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 drivers/pci/vgaarb.c   | 108 ++++++++++++++++++++++++-----------------
 include/linux/vgaarb.h |   4 +-
 2 files changed, 65 insertions(+), 47 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 22a505e877dc..ceb914245383 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 */
@@ -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);
@@ -860,7 +876,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 	return ret;
 }
 
-/* this is called with the lock */
+/* This is called with the lock */
 static inline void vga_update_device_decodes(struct vga_device *vgadev,
 					     int new_decodes)
 {
@@ -877,7 +893,7 @@ static inline void vga_update_device_decodes(struct vga_device *vgadev,
 		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;
@@ -886,7 +902,7 @@ static inline 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,11 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 	if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
 		return 0;
 
-	/* For now we're only intereted in devices added and removed. I didn't
+	/*
+	 * 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. */
+	 * 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)
@@ -1543,8 +1559,10 @@ 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 */
+	/*
+	 * We add all PCI devices satisfying VGA class in the arbiter by
+	 * default
+	 */
 	while (1) {
 		pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
 		if (!pdev)
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

[PATCH v7 4/8] PCI/VGA: Replace full MIT license text with SPDX identifier

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

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.

Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
---
 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 <benh@kernel.crashing.org>
  * (C) Copyright 2007 Paulo R. Zanoni <przanoni@gmail.com>
  * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti@freedesktop.org>
- *
- * 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

[PATCH v7 5/8] video/aperture: Add a helper to detect if an aperture contains firmware FB

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

This patch adds the aperture_contain_firmware_fb() function to do the
determination. Unfortunately due to the fact that apertures list will be
freed dynamically, the location and size information of the firmware fb
will be lost after dedicated drivers call
aperture_remove_conflicting_devices(),
aperture_remove_conflicting_pci_devices() or
aperture_remove_all_conflicting_devices() functions

We handle this problem by introducing two static variables which record the
firmware framebuffer's start addrness and end addrness. It assumes that the
system has only one active firmware framebuffer driver at a time.

We don't use the global structure screen_info here, because PCI resource
may get reallocated(the VRAM BAR could be moved) at kernel boot stage.

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Helge Deller <deller@gmx.de>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/drm_aperture.c | 16 ++++++++++++++++
 drivers/video/aperture.c       | 29 +++++++++++++++++++++++++++++
 include/drm/drm_aperture.h     |  2 ++
 include/linux/aperture.h       |  7 +++++++
 4 files changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
index 5729f3bb4398..6e5d8a08683c 100644
--- a/drivers/gpu/drm/drm_aperture.c
+++ b/drivers/gpu/drm/drm_aperture.c
@@ -190,3 +190,19 @@ int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
 	return aperture_remove_conflicting_pci_devices(pdev, req_driver->name);
 }
 EXPORT_SYMBOL(drm_aperture_remove_conflicting_pci_framebuffers);
+
+/**
+ * drm_aperture_contain_firmware_fb - Determine if a aperture contains firmware framebuffer
+ *
+ * @base: the aperture's base address in physical memory
+ * @size: aperture size in bytes
+ *
+ * Returns:
+ * true on if there is a firmware framebuffer belong to the aperture passed in,
+ * or false otherwise.
+ */
+bool drm_aperture_contain_firmware_fb(resource_size_t base, resource_size_t size)
+{
+	return aperture_contain_firmware_fb(base, base + size);
+}
+EXPORT_SYMBOL(drm_aperture_contain_firmware_fb);
diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 561be8feca96..5a5422cec669 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -141,6 +141,9 @@ struct aperture_range {
 static LIST_HEAD(apertures);
 static DEFINE_MUTEX(apertures_lock);
 
+static resource_size_t firm_fb_start;
+static resource_size_t firm_fb_end;
+
 static bool overlap(resource_size_t base1, resource_size_t end1,
 		    resource_size_t base2, resource_size_t end2)
 {
@@ -170,6 +173,9 @@ static int devm_aperture_acquire(struct device *dev,
 
 	mutex_lock(&apertures_lock);
 
+	firm_fb_start = base;
+	firm_fb_end = end;
+
 	list_for_each(pos, &apertures) {
 		ap = container_of(pos, struct aperture_range, lh);
 		if (overlap(base, end, ap->base, ap->base + ap->size)) {
@@ -377,3 +383,26 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
 
 }
 EXPORT_SYMBOL(aperture_remove_conflicting_pci_devices);
+
+/**
+ * aperture_contain_firmware_fb - Detect if the firmware framebuffer belong to
+ *                                a aperture.
+ * @ap_start: the aperture's start address in physical memory
+ * @ap_end: the aperture's end address in physical memory
+ *
+ * Returns:
+ * true on if there is a firmware framebuffer belong to the aperture passed in,
+ * or false otherwise.
+ */
+bool aperture_contain_firmware_fb(resource_size_t ap_start, resource_size_t ap_end)
+{
+	/* No firmware framebuffer support */
+	if (!firm_fb_start || !firm_fb_end)
+		return false;
+
+	if (firm_fb_start >= ap_start && firm_fb_end <= ap_end)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(aperture_contain_firmware_fb);
diff --git a/include/drm/drm_aperture.h b/include/drm/drm_aperture.h
index cbe33b49fd5d..6a0b9bacb081 100644
--- a/include/drm/drm_aperture.h
+++ b/include/drm/drm_aperture.h
@@ -35,4 +35,6 @@ drm_aperture_remove_framebuffers(const struct drm_driver *req_driver)
 							    req_driver);
 }
 
+bool drm_aperture_contain_firmware_fb(resource_size_t base, resource_size_t size);
+
 #endif
diff --git a/include/linux/aperture.h b/include/linux/aperture.h
index 1a9a88b11584..d4dc5917c49b 100644
--- a/include/linux/aperture.h
+++ b/include/linux/aperture.h
@@ -19,6 +19,8 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
 int __aperture_remove_legacy_vga_devices(struct pci_dev *pdev);
 
 int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name);
+
+bool aperture_contain_firmware_fb(resource_size_t ap_start, resource_size_t ap_end);
 #else
 static inline int devm_aperture_acquire_for_platform_device(struct platform_device *pdev,
 							    resource_size_t base,
@@ -42,6 +44,11 @@ static inline int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev,
 {
 	return 0;
 }
+
+static inline bool aperture_contain_firmware_fb(resource_size_t ap_start, resource_size_t ap_end)
+{
+	return false;
+}
 #endif
 
 /**
-- 
2.25.1

Re: [PATCH v7 5/8] video/aperture: Add a helper to detect if an aperture contains firmware FB

[Sui Jingfeng]

  PING ?

On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> This patch adds the aperture_contain_firmware_fb() function to do the
> determination. Unfortunately due to the fact that apertures list will be
> freed dynamically, the location and size information of the firmware fb
> will be lost after dedicated drivers call
> aperture_remove_conflicting_devices(),
> aperture_remove_conflicting_pci_devices() or
> aperture_remove_all_conflicting_devices() functions
>
> We handle this problem by introducing two static variables which record the
> firmware framebuffer's start addrness and end addrness. It assumes that the
> system has only one active firmware framebuffer driver at a time.
>
> We don't use the global structure screen_info here, because PCI resource
> may get reallocated(the VRAM BAR could be moved) at kernel boot stage.
>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Helge Deller <deller@gmx.de>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>   drivers/gpu/drm/drm_aperture.c | 16 ++++++++++++++++
>   drivers/video/aperture.c       | 29 +++++++++++++++++++++++++++++
>   include/drm/drm_aperture.h     |  2 ++
>   include/linux/aperture.h       |  7 +++++++
>   4 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
> index 5729f3bb4398..6e5d8a08683c 100644
> --- a/drivers/gpu/drm/drm_aperture.c
> +++ b/drivers/gpu/drm/drm_aperture.c
> @@ -190,3 +190,19 @@ int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>   	return aperture_remove_conflicting_pci_devices(pdev, req_driver->name);
>   }
>   EXPORT_SYMBOL(drm_aperture_remove_conflicting_pci_framebuffers);
> +
> +/**
> + * drm_aperture_contain_firmware_fb - Determine if a aperture contains firmware framebuffer
> + *
> + * @base: the aperture's base address in physical memory
> + * @size: aperture size in bytes
> + *
> + * Returns:
> + * true on if there is a firmware framebuffer belong to the aperture passed in,
> + * or false otherwise.
> + */
> +bool drm_aperture_contain_firmware_fb(resource_size_t base, resource_size_t size)
> +{
> +	return aperture_contain_firmware_fb(base, base + size);
> +}
> +EXPORT_SYMBOL(drm_aperture_contain_firmware_fb);
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index 561be8feca96..5a5422cec669 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -141,6 +141,9 @@ struct aperture_range {
>   static LIST_HEAD(apertures);
>   static DEFINE_MUTEX(apertures_lock);
>   
> +static resource_size_t firm_fb_start;
> +static resource_size_t firm_fb_end;
> +
>   static bool overlap(resource_size_t base1, resource_size_t end1,
>   		    resource_size_t base2, resource_size_t end2)
>   {
> @@ -170,6 +173,9 @@ static int devm_aperture_acquire(struct device *dev,
>   
>   	mutex_lock(&apertures_lock);
>   
> +	firm_fb_start = base;
> +	firm_fb_end = end;
> +
>   	list_for_each(pos, &apertures) {
>   		ap = container_of(pos, struct aperture_range, lh);
>   		if (overlap(base, end, ap->base, ap->base + ap->size)) {
> @@ -377,3 +383,26 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>   
>   }
>   EXPORT_SYMBOL(aperture_remove_conflicting_pci_devices);
> +
> +/**
> + * aperture_contain_firmware_fb - Detect if the firmware framebuffer belong to
> + *                                a aperture.
> + * @ap_start: the aperture's start address in physical memory
> + * @ap_end: the aperture's end address in physical memory
> + *
> + * Returns:
> + * true on if there is a firmware framebuffer belong to the aperture passed in,
> + * or false otherwise.
> + */
> +bool aperture_contain_firmware_fb(resource_size_t ap_start, resource_size_t ap_end)
> +{
> +	/* No firmware framebuffer support */
> +	if (!firm_fb_start || !firm_fb_end)
> +		return false;
> +
> +	if (firm_fb_start >= ap_start && firm_fb_end <= ap_end)
> +		return true;
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(aperture_contain_firmware_fb);
> diff --git a/include/drm/drm_aperture.h b/include/drm/drm_aperture.h
> index cbe33b49fd5d..6a0b9bacb081 100644
> --- a/include/drm/drm_aperture.h
> +++ b/include/drm/drm_aperture.h
> @@ -35,4 +35,6 @@ drm_aperture_remove_framebuffers(const struct drm_driver *req_driver)
>   							    req_driver);
>   }
>   
> +bool drm_aperture_contain_firmware_fb(resource_size_t base, resource_size_t size);
> +
>   #endif
> diff --git a/include/linux/aperture.h b/include/linux/aperture.h
> index 1a9a88b11584..d4dc5917c49b 100644
> --- a/include/linux/aperture.h
> +++ b/include/linux/aperture.h
> @@ -19,6 +19,8 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
>   int __aperture_remove_legacy_vga_devices(struct pci_dev *pdev);
>   
>   int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name);
> +
> +bool aperture_contain_firmware_fb(resource_size_t ap_start, resource_size_t ap_end);
>   #else
>   static inline int devm_aperture_acquire_for_platform_device(struct platform_device *pdev,
>   							    resource_size_t base,
> @@ -42,6 +44,11 @@ static inline int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev,
>   {
>   	return 0;
>   }
> +
> +static inline bool aperture_contain_firmware_fb(resource_size_t ap_start, resource_size_t ap_end)
> +{
> +	return false;
> +}
>   #endif
>   
>   /**

-- 
Jingfeng

[PATCH v7 6/8] 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, it's probably
wrong if we simply remove the arch guard. As the VRAM BAR which contains
firmware framebuffer may move, while the lfb_base and lfb_size members of
the screen_info does not change accordingly. In short, it should take the
re-allocation of the PCI BAR into consideration.

With the observation that device drivers or video aperture helpers may
have better knowledge about which PCI bar contains the firmware fb,
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 not suitable to
make such an optimization since it is loaded too early.

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"

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Pan Xinhui <Xinhui.Pan@amd.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Karol Herbst <kherbst@redhat.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Hawking Zhang <Hawking.Zhang@amd.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: Lijo Lazar <lijo.lazar@amd.com>
Cc: YiPeng Chai <YiPeng.Chai@amd.com>
Cc: Bokun Zhang <Bokun.Zhang@amd.com>
Cc: Likun Gao <Likun.Gao@amd.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
CC: Kevin Tian <kevin.tian@intel.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Yishai Hadas <yishaih@nvidia.com>
Cc: Abhishek Sahu <abhsahu@nvidia.com>
Cc: Yi Liu <yi.l.liu@intel.com>
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                       | 21 ++++++++++++++++++++-
 drivers/vfio/pci/vfio_pci_core.c           |  2 +-
 include/linux/vgaarb.h                     |  8 +++++---
 7 files changed, 30 insertions(+), 10 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 ceb914245383..c574898380f0 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);
@@ -969,6 +970,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
@@ -984,7 +989,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;
@@ -996,6 +1002,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:
@@ -1523,6 +1530,18 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 		notify = vga_arbiter_add_pci_device(pdev);
 	else if (action == BUS_NOTIFY_DEL_DEVICE)
 		notify = vga_arbiter_del_pci_device(pdev);
+	else if (action == BUS_NOTIFY_BOUND_DRIVER) {
+		struct vga_device *vgadev = vgadev_find(pdev);
+		bool boot_dev = false;
+
+		if (vgadev && vgadev->is_boot_device)
+			boot_dev = vgadev->is_boot_device(pdev);
+
+		if (boot_dev) {
+			vgaarb_info(&pdev->dev, "Set as boot device (dictated by driver)\n");
+			vga_set_default_device(pdev);
+		}
+	}
 
 	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
 
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 97129a1bbb7d..dfde5a6ba55a 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -33,7 +33,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)
@@ -59,7 +60,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;
 }
@@ -97,7 +99,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: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Sui Jingfeng]

Hi,


A nouveau developer(Lyude) from redhat send me a R-B,

Thanks for the developers of nouveau project.


Please allow me add a link[1] here.


[1] 
https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.camel@redhat.com/


On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> The vga_is_firmware_default() function is arch-dependent, it's probably
> wrong if we simply remove the arch guard. As the VRAM BAR which contains
> firmware framebuffer may move, while the lfb_base and lfb_size members of
> the screen_info does not change accordingly. In short, it should take the
> re-allocation of the PCI BAR into consideration.
>
> With the observation that device drivers or video aperture helpers may
> have better knowledge about which PCI bar contains the firmware fb,
> 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 not suitable to
> make such an optimization since it is loaded too early.
>
> 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"
>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian Konig <christian.koenig@amd.com>
> Cc: Pan Xinhui <Xinhui.Pan@amd.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Karol Herbst <kherbst@redhat.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Lijo Lazar <lijo.lazar@amd.com>
> Cc: YiPeng Chai <YiPeng.Chai@amd.com>
> Cc: Bokun Zhang <Bokun.Zhang@amd.com>
> Cc: Likun Gao <Likun.Gao@amd.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> CC: Kevin Tian <kevin.tian@intel.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Yishai Hadas <yishaih@nvidia.com>
> Cc: Abhishek Sahu <abhsahu@nvidia.com>
> Cc: Yi Liu <yi.l.liu@intel.com>
> 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                       | 21 ++++++++++++++++++++-
>   drivers/vfio/pci/vfio_pci_core.c           |  2 +-
>   include/linux/vgaarb.h                     |  8 +++++---
>   7 files changed, 30 insertions(+), 10 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 ceb914245383..c574898380f0 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);
> @@ -969,6 +970,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
> @@ -984,7 +989,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;
> @@ -996,6 +1002,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:
> @@ -1523,6 +1530,18 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>   		notify = vga_arbiter_add_pci_device(pdev);
>   	else if (action == BUS_NOTIFY_DEL_DEVICE)
>   		notify = vga_arbiter_del_pci_device(pdev);
> +	else if (action == BUS_NOTIFY_BOUND_DRIVER) {
> +		struct vga_device *vgadev = vgadev_find(pdev);
> +		bool boot_dev = false;
> +
> +		if (vgadev && vgadev->is_boot_device)
> +			boot_dev = vgadev->is_boot_device(pdev);
> +
> +		if (boot_dev) {
> +			vgaarb_info(&pdev->dev, "Set as boot device (dictated by driver)\n");
> +			vga_set_default_device(pdev);
> +		}
> +	}
>   
>   	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
>   
> 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 97129a1bbb7d..dfde5a6ba55a 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -33,7 +33,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)
> @@ -59,7 +60,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;
>   }
> @@ -97,7 +99,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 */

-- 
Jingfeng

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Bjorn Helgaas]

On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote:
> Hi,
> 
> 
> A nouveau developer(Lyude) from redhat send me a R-B,
> 
> Thanks for the developers of nouveau project.
> 
> 
> Please allow me add a link[1] here.
> 
> 
> [1] https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.camel@redhat.com/

1) Thanks for this.  If you post another version of this series,
   please pick up Lyude's Reviewed-by and include it in the relevant
   patches (as long as you haven't made significant changes to the
   code Lyude reviewed).  Whoever applies this should automatically
   pick up Reviewed-by/Ack/etc that are replies to the version being
   applied, but they won't go through previous revisions to find them.

2) Please mention the commit to which the series applies.  I tried to
   apply this on v6.4-rc1, but it doesn't apply cleanly.

3) Thanks for including cover letters in your postings.  Please
   include a little changelog in the cover letter so we know what
   changed between v6 and v7, etc.

4) Right now we're in the middle of the v6.5 merge window, so new
   content, e.g., this series, is too late for v6.5.  Most
   maintainers, including me, wait to merge new content until the
   merge window closes and a new -rc1 is tagged.  This merge window
   should close on July 9, and people will start merging content for
   v6.6, typically based on v6.5-rc1.

Bjorn

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Sui Jingfeng]

Hi,

On 2023/6/29 23:54, Bjorn Helgaas wrote:
> On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>>
>> A nouveau developer(Lyude) from redhat send me a R-B,
>>
>> Thanks for the developers of nouveau project.
>>
>>
>> Please allow me add a link[1] here.
>>
>>
>> [1] https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.camel@redhat.com/
> 1) Thanks for this.  If you post another version of this series,
>     please pick up Lyude's Reviewed-by and include it in the relevant
>     patches (as long as you haven't made significant changes to the
>     code Lyude reviewed).

Yes, no significant changes. Just fix typo.

I also would like to add support for other DRM drivers.

But I think this deserve another patch.

>   Whoever applies this should automatically
>     pick up Reviewed-by/Ack/etc that are replies to the version being
>     applied, but they won't go through previous revisions to find them.
>
> 2) Please mention the commit to which the series applies.  I tried to
>     apply this on v6.4-rc1, but it doesn't apply cleanly.

Since I'm a graphic driver developer, I'm using drm-tip.

I just have already pulled, it still apply cleanly on drm-tip.

> 3) Thanks for including cover letters in your postings.  Please
>     include a little changelog in the cover letter so we know what
>     changed between v6 and v7, etc.

No change between v6 and v7,

it seems that it is because the mailbox don't allow me to sending too 
many mails a day.

so some of the patch is failed to delivery because out of quota.


> 4) Right now we're in the middle of the v6.5 merge window, so new
>     content, e.g., this series, is too late for v6.5.  Most
>     maintainers, including me, wait to merge new content until the
>     merge window closes and a new -rc1 is tagged.  This merge window
>     should close on July 9, and people will start merging content for
>     v6.6, typically based on v6.5-rc1.

I'm wondering

Would you will merge all of the patches in this series (e.g. including 
the patch for drm/amdgpu(7/8) and drm/radeon(8/8)) ?

Or just part of them?

Emm, I don't know because my patch seems across different subsystem of 
Linux kernel.

There is also a developer for AMDGPU (Mario) give me a R-B for the 
patch-0002 of this series.

So, at least, PATCH-0001, PATCH-0002, PATCH-0003, PATCH-0004, PATCH-0006 
are already OK(got reviewed by).

Those 5 patch are already qualified to be merged, I think.

I means that if you could merge those 5 patch first, then there no need 
to send another version again.

I will refine the rest patch with more details and description.

I'm fear of making too much noise.

> Bjorn

RE: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Limonciello, Mario]

[Public]

> -----Original Message-----
> From: 15330273260@189.cn <15330273260@189.cn>
> Sent: Thursday, June 29, 2023 12:00 PM
> To: Bjorn Helgaas <helgaas@kernel.org>; Sui Jingfeng
> <suijingfeng@loongson.cn>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-fbdev@vger.kernel.org;
> Cornelia Huck <cohuck@redhat.com>; Karol Herbst <kherbst@redhat.com>;
> nouveau@lists.freedesktop.org; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; dri-devel@lists.freedesktop.org; Chai,
> Thomas <YiPeng.Chai@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Gao, Likun <Likun.Gao@amd.com>; David
> Airlie <airlied@gmail.com>; Ville Syrjala <ville.syrjala@linux.intel.com>; Yi Liu
> <yi.l.liu@intel.com>; kvm@vger.kernel.org; amd-gfx@lists.freedesktop.org;
> Jason Gunthorpe <jgg@ziepe.ca>; Ben Skeggs <bskeggs@redhat.com>; linux-
> pci@vger.kernel.org; Kevin Tian <kevin.tian@intel.com>; Lazar, Lijo
> <Lijo.Lazar@amd.com>; Thomas Zimmermann <tzimmermann@suse.de>;
> Zhang, Bokun <Bokun.Zhang@amd.com>; intel-gfx@lists.freedesktop.org;
> Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Jani Nikula
> <jani.nikula@linux.intel.com>; Alex Williamson
> <alex.williamson@redhat.com>; Abhishek Sahu <abhsahu@nvidia.com>;
> Maxime Ripard <mripard@kernel.org>; Rodrigo Vivi <rodrigo.vivi@intel.com>;
> Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Yishai Hadas
> <yishaih@nvidia.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; linux-
> kernel@vger.kernel.org; Daniel Vetter <daniel@ffwll.ch>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
> Subject: Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function
> callback to vga_client_register
>
> Hi,
>
> On 2023/6/29 23:54, Bjorn Helgaas wrote:
> > On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote:
> >> Hi,
> >>
> >>
> >> A nouveau developer(Lyude) from redhat send me a R-B,
> >>
> >> Thanks for the developers of nouveau project.
> >>
> >>
> >> Please allow me add a link[1] here.
> >>
> >>
> >> [1]
> https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.ca
> mel@redhat.com/
> > 1) Thanks for this.  If you post another version of this series,
> >     please pick up Lyude's Reviewed-by and include it in the relevant
> >     patches (as long as you haven't made significant changes to the
> >     code Lyude reviewed).
>
> Yes, no significant changes. Just fix typo.
>
> I also would like to add support for other DRM drivers.
>
> But I think this deserve another patch.
>
> >   Whoever applies this should automatically
> >     pick up Reviewed-by/Ack/etc that are replies to the version being
> >     applied, but they won't go through previous revisions to find them.
> >
> > 2) Please mention the commit to which the series applies.  I tried to
> >     apply this on v6.4-rc1, but it doesn't apply cleanly.
>
> Since I'm a graphic driver developer, I'm using drm-tip.
>
> I just have already pulled, it still apply cleanly on drm-tip.
>
> > 3) Thanks for including cover letters in your postings.  Please
> >     include a little changelog in the cover letter so we know what
> >     changed between v6 and v7, etc.
>
> No change between v6 and v7,
>
> it seems that it is because the mailbox don't allow me to sending too
> many mails a day.
>
> so some of the patch is failed to delivery because out of quota.
>
>
> > 4) Right now we're in the middle of the v6.5 merge window, so new
> >     content, e.g., this series, is too late for v6.5.  Most
> >     maintainers, including me, wait to merge new content until the
> >     merge window closes and a new -rc1 is tagged.  This merge window
> >     should close on July 9, and people will start merging content for
> >     v6.6, typically based on v6.5-rc1.
>
> I'm wondering
>
> Would you will merge all of the patches in this series (e.g. including
> the patch for drm/amdgpu(7/8) and drm/radeon(8/8)) ?
>
> Or just part of them?
>
> Emm, I don't know because my patch seems across different subsystem of
> Linux kernel.
>
> There is also a developer for AMDGPU (Mario) give me a R-B for the
> patch-0002 of this series.
>
> So, at least, PATCH-0001, PATCH-0002, PATCH-0003, PATCH-0004, PATCH-
> 0006
> are already OK(got reviewed by).
>
> Those 5 patch are already qualified to be merged, I think.

I think what you can do is pick up all the tags in your next version.  Once the
whole series has tags we can discuss how it merges.

>
> I means that if you could merge those 5 patch first, then there no need
> to send another version again.
>
> I will refine the rest patch with more details and description.
>
> I'm fear of making too much noise.
>
> > Bjorn

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[suijingfeng]

Hi,

On 2023/6/30 01:44, Limonciello, Mario wrote:
> [Public]
>
>> -----Original Message-----
>> From: 15330273260@189.cn <15330273260@189.cn>
>> Sent: Thursday, June 29, 2023 12:00 PM
>> To: Bjorn Helgaas <helgaas@kernel.org>; Sui Jingfeng
>> <suijingfeng@loongson.cn>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-fbdev@vger.kernel.org;
>> Cornelia Huck <cohuck@redhat.com>; Karol Herbst <kherbst@redhat.com>;
>> nouveau@lists.freedesktop.org; Joonas Lahtinen
>> <joonas.lahtinen@linux.intel.com>; dri-devel@lists.freedesktop.org; Chai,
>> Thomas <YiPeng.Chai@amd.com>; Limonciello, Mario
>> <Mario.Limonciello@amd.com>; Gao, Likun <Likun.Gao@amd.com>; David
>> Airlie <airlied@gmail.com>; Ville Syrjala <ville.syrjala@linux.intel.com>; Yi Liu
>> <yi.l.liu@intel.com>; kvm@vger.kernel.org; amd-gfx@lists.freedesktop.org;
>> Jason Gunthorpe <jgg@ziepe.ca>; Ben Skeggs <bskeggs@redhat.com>; linux-
>> pci@vger.kernel.org; Kevin Tian <kevin.tian@intel.com>; Lazar, Lijo
>> <Lijo.Lazar@amd.com>; Thomas Zimmermann <tzimmermann@suse.de>;
>> Zhang, Bokun <Bokun.Zhang@amd.com>; intel-gfx@lists.freedesktop.org;
>> Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Jani Nikula
>> <jani.nikula@linux.intel.com>; Alex Williamson
>> <alex.williamson@redhat.com>; Abhishek Sahu <abhsahu@nvidia.com>;
>> Maxime Ripard <mripard@kernel.org>; Rodrigo Vivi <rodrigo.vivi@intel.com>;
>> Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Yishai Hadas
>> <yishaih@nvidia.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; linux-
>> kernel@vger.kernel.org; Daniel Vetter <daniel@ffwll.ch>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; Koenig, Christian
>> <Christian.Koenig@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
>> Subject: Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function
>> callback to vga_client_register
>>
>> Hi,
>>
>> On 2023/6/29 23:54, Bjorn Helgaas wrote:
>>> On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote:
>>>> Hi,
>>>>
>>>>
>>>> A nouveau developer(Lyude) from redhat send me a R-B,
>>>>
>>>> Thanks for the developers of nouveau project.
>>>>
>>>>
>>>> Please allow me add a link[1] here.
>>>>
>>>>
>>>> [1]
>> https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.ca
>> mel@redhat.com/
>>> 1) Thanks for this.  If you post another version of this series,
>>>      please pick up Lyude's Reviewed-by and include it in the relevant
>>>      patches (as long as you haven't made significant changes to the
>>>      code Lyude reviewed).
>> Yes, no significant changes. Just fix typo.
>>
>> I also would like to add support for other DRM drivers.
>>
>> But I think this deserve another patch.
>>
>>>    Whoever applies this should automatically
>>>      pick up Reviewed-by/Ack/etc that are replies to the version being
>>>      applied, but they won't go through previous revisions to find them.
>>>
>>> 2) Please mention the commit to which the series applies.  I tried to
>>>      apply this on v6.4-rc1, but it doesn't apply cleanly.
>> Since I'm a graphic driver developer, I'm using drm-tip.
>>
>> I just have already pulled, it still apply cleanly on drm-tip.
>>
>>> 3) Thanks for including cover letters in your postings.  Please
>>>      include a little changelog in the cover letter so we know what
>>>      changed between v6 and v7, etc.
>> No change between v6 and v7,
>>
>> it seems that it is because the mailbox don't allow me to sending too
>> many mails a day.
>>
>> so some of the patch is failed to delivery because out of quota.
>>
>>
>>> 4) Right now we're in the middle of the v6.5 merge window, so new
>>>      content, e.g., this series, is too late for v6.5.  Most
>>>      maintainers, including me, wait to merge new content until the
>>>      merge window closes and a new -rc1 is tagged.  This merge window
>>>      should close on July 9, and people will start merging content for
>>>      v6.6, typically based on v6.5-rc1.
>> I'm wondering
>>
>> Would you will merge all of the patches in this series (e.g. including
>> the patch for drm/amdgpu(7/8) and drm/radeon(8/8)) ?
>>
>> Or just part of them?
>>
>> Emm, I don't know because my patch seems across different subsystem of
>> Linux kernel.
>>
>> There is also a developer for AMDGPU (Mario) give me a R-B for the
>> patch-0002 of this series.
>>
>> So, at least, PATCH-0001, PATCH-0002, PATCH-0003, PATCH-0004, PATCH-
>> 0006
>> are already OK(got reviewed by).
>>
>> Those 5 patch are already qualified to be merged, I think.
> I think what you can do is pick up all the tags in your next version.  Once the
> whole series has tags we can discuss how it merges.

Thanks a lot, Mario.


Is it possible to merge the PCI/VGA part as fast as possible, especially the

PATCH-0006 PCI/VGA: Introduce is_boot_device function callback to vga_client_register

As this patch is fundamental, it introduce no functional change, as long as the drm

driver side don't introduce a callback.

I'm not hurry, but drm driver-side's patch have a dependency on this patch,

I think it is better the PCI/VGA-side's patch got merge first.

At least for get the first four cleanup(0001 ~ 0004) patch merged first,

so that I don't have to send so much on the next version on one series.

Being exposed so far, there no obvious objection.

It saying that other people also want it got merged.

Bjorn, is this OK ?

>
>> I means that if you could merge those 5 patch first, then there no need
>> to send another version again.
>>
>> I will refine the rest patch with more details and description.
>>
>> I'm fear of making too much noise.
>>
>>> Bjorn

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Bjorn Helgaas]

On Fri, Jun 30, 2023 at 10:14:11AM +0800, suijingfeng wrote:
> On 2023/6/30 01:44, Limonciello, Mario wrote:
> > > On 2023/6/29 23:54, Bjorn Helgaas wrote:
> > > > On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote:

> > > > 4) Right now we're in the middle of the v6.5 merge window, so new
> > > >      content, e.g., this series, is too late for v6.5.  Most
> > > >      maintainers, including me, wait to merge new content until the
> > > >      merge window closes and a new -rc1 is tagged.  This merge window
> > > >      should close on July 9, and people will start merging content for
> > > >      v6.6, typically based on v6.5-rc1.
> > > 
> > > Would you will merge all of the patches in this series (e.g. including
> > > the patch for drm/amdgpu(7/8) and drm/radeon(8/8)) ?
> > > 
> > > Or just part of them?

The bulk of this series is drivers/pci changes, so typically I would
merge all the patches after getting Acked-by tags from the other
subsystems (DRM and VFIO).

> Is it possible to merge the PCI/VGA part as fast as possible,
> especially the PATCH-0006 PCI/VGA: Introduce is_boot_device function
> callback to vga_client_register

We're in the middle of the v6.5 merge window, so it's too late to add
things to v6.5-rc1.  The most likely path for new material like this
would be to queue it for v6.6, which means I would merge it after
v6.5-rc1 is tagged (that tag will probably happen on July 9).

It would then be in -next until the v6.6 merge window opens (likely in
September), when it would be merged into Linus' tree.

If the series fixes a regression or other major defect, it's
*possible* to merge things earlier, so they appear in v6.5.  But this
series doesn't seem to fall in that category, so I think v6.6 is a
more realistic target.

Merging for v6.6 would include both the PCI parts and the DRM parts at
the same time, so hopefully that addresses your dependency concerns.

I suggest that you wait until v6.5-rc1, rebase your patches so they
apply cleanly on that tag, collect all the Reviewed-by and Acked-by
tags, include them in your commit logs, and then repost them.

Bjorn

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Jani Nikula]

On Fri, 30 Jun 2023, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Jun 30, 2023 at 10:14:11AM +0800, suijingfeng wrote:
>> On 2023/6/30 01:44, Limonciello, Mario wrote:
>> > > On 2023/6/29 23:54, Bjorn Helgaas wrote:
>> > > > On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote:
>
>> > > > 4) Right now we're in the middle of the v6.5 merge window, so new
>> > > >      content, e.g., this series, is too late for v6.5.  Most
>> > > >      maintainers, including me, wait to merge new content until the
>> > > >      merge window closes and a new -rc1 is tagged.  This merge window
>> > > >      should close on July 9, and people will start merging content for
>> > > >      v6.6, typically based on v6.5-rc1.
>> > > 
>> > > Would you will merge all of the patches in this series (e.g. including
>> > > the patch for drm/amdgpu(7/8) and drm/radeon(8/8)) ?
>> > > 
>> > > Or just part of them?
>
> The bulk of this series is drivers/pci changes, so typically I would
> merge all the patches after getting Acked-by tags from the other
> subsystems (DRM and VFIO).

For the (negligible) i915 parts,

Acked-by: Jani Nikula <jani.nikula@intel.com>

>> Is it possible to merge the PCI/VGA part as fast as possible,
>> especially the PATCH-0006 PCI/VGA: Introduce is_boot_device function
>> callback to vga_client_register
>
> We're in the middle of the v6.5 merge window, so it's too late to add
> things to v6.5-rc1.  The most likely path for new material like this
> would be to queue it for v6.6, which means I would merge it after
> v6.5-rc1 is tagged (that tag will probably happen on July 9).

Perhaps the part that causes confusion here is that the drm-misc-next
and drm-intel-next branches, for example, are always open for new
patches; it's just that there's a cutoff at around rc5/rc6 after which
they start targeting the next+1 release. We basically hide the merge
window from a lot of drm developers.

> It would then be in -next until the v6.6 merge window opens (likely in
> September), when it would be merged into Linus' tree.
>
> If the series fixes a regression or other major defect, it's
> *possible* to merge things earlier, so they appear in v6.5.  But this
> series doesn't seem to fall in that category, so I think v6.6 is a
> more realistic target.
>
> Merging for v6.6 would include both the PCI parts and the DRM parts at
> the same time, so hopefully that addresses your dependency concerns.

I guess the main question is whether Sui Jingfeng has follow-up work
planned in drm that depends on these being merged. This would set that
back by a full release. (But it happens.)

BR,
Jani.



>
> I suggest that you wait until v6.5-rc1, rebase your patches so they
> apply cleanly on that tag, collect all the Reviewed-by and Acked-by
> tags, include them in your commit logs, and then repost them.
>
> Bjorn

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Sui Jingfeng]

Hi,

On 2023/6/30 01:44, Limonciello, Mario wrote:
> I think what you can do is pick up all the tags in your next version.  Once the
> whole series has tags we can discuss how it merges.

Yes, you are right.

I will prepare the next version.

But I think, I should only gather the reverent part together.

I means that I probably should divide the 8 patches in V7 into 4 + 4.

The first four patch form a group, and the last four patch form another 
group.


Certainly, I will pick up the precious tags I got in the next version.

Thanks you!

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Sui Jingfeng]

PING ?

On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> The vga_is_firmware_default() function is arch-dependent, it's probably
> wrong if we simply remove the arch guard. As the VRAM BAR which contains
> firmware framebuffer may move, while the lfb_base and lfb_size members of
> the screen_info does not change accordingly. In short, it should take the
> re-allocation of the PCI BAR into consideration.
>
> With the observation that device drivers or video aperture helpers may
> have better knowledge about which PCI bar contains the firmware fb,
> 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 not suitable to
> make such an optimization since it is loaded too early.
>
> 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"
>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian Konig <christian.koenig@amd.com>
> Cc: Pan Xinhui <Xinhui.Pan@amd.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Karol Herbst <kherbst@redhat.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Lijo Lazar <lijo.lazar@amd.com>
> Cc: YiPeng Chai <YiPeng.Chai@amd.com>
> Cc: Bokun Zhang <Bokun.Zhang@amd.com>
> Cc: Likun Gao <Likun.Gao@amd.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> CC: Kevin Tian <kevin.tian@intel.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Yishai Hadas <yishaih@nvidia.com>
> Cc: Abhishek Sahu <abhsahu@nvidia.com>
> Cc: Yi Liu <yi.l.liu@intel.com>
> 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                       | 21 ++++++++++++++++++++-
>   drivers/vfio/pci/vfio_pci_core.c           |  2 +-
>   include/linux/vgaarb.h                     |  8 +++++---
>   7 files changed, 30 insertions(+), 10 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 ceb914245383..c574898380f0 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);
> @@ -969,6 +970,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
> @@ -984,7 +989,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;
> @@ -996,6 +1002,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:
> @@ -1523,6 +1530,18 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>   		notify = vga_arbiter_add_pci_device(pdev);
>   	else if (action == BUS_NOTIFY_DEL_DEVICE)
>   		notify = vga_arbiter_del_pci_device(pdev);
> +	else if (action == BUS_NOTIFY_BOUND_DRIVER) {
> +		struct vga_device *vgadev = vgadev_find(pdev);
> +		bool boot_dev = false;
> +
> +		if (vgadev && vgadev->is_boot_device)
> +			boot_dev = vgadev->is_boot_device(pdev);
> +
> +		if (boot_dev) {
> +			vgaarb_info(&pdev->dev, "Set as boot device (dictated by driver)\n");
> +			vga_set_default_device(pdev);
> +		}
> +	}
>   
>   	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
>   
> 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 97129a1bbb7d..dfde5a6ba55a 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -33,7 +33,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)
> @@ -59,7 +60,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;
>   }
> @@ -97,7 +99,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 */

-- 
Jingfeng

Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register

[Sui Jingfeng]

Hi,


Humble ping !


Please share some bandwidth to help reviewing this series, OK ?


As this series is useful for all architecture, I have tested on my X86, 
mips and LoongArch computer.

Questions and comments is also welcome.

If no one response within three days,

I'm going to send a updated version with another trivial improvement, OK?

On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> The vga_is_firmware_default() function is arch-dependent, it's probably
> wrong if we simply remove the arch guard. As the VRAM BAR which contains
> firmware framebuffer may move, while the lfb_base and lfb_size members of
> the screen_info does not change accordingly. In short, it should take the
> re-allocation of the PCI BAR into consideration.
>
> With the observation that device drivers or video aperture helpers may
> have better knowledge about which PCI bar contains the firmware fb,
> 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 not suitable to
> make such an optimization since it is loaded too early.
>
> 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"
>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian Konig <christian.koenig@amd.com>
> Cc: Pan Xinhui <Xinhui.Pan@amd.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Karol Herbst <kherbst@redhat.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Lijo Lazar <lijo.lazar@amd.com>
> Cc: YiPeng Chai <YiPeng.Chai@amd.com>
> Cc: Bokun Zhang <Bokun.Zhang@amd.com>
> Cc: Likun Gao <Likun.Gao@amd.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> CC: Kevin Tian <kevin.tian@intel.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Yishai Hadas <yishaih@nvidia.com>
> Cc: Abhishek Sahu <abhsahu@nvidia.com>
> Cc: Yi Liu <yi.l.liu@intel.com>
> 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                       | 21 ++++++++++++++++++++-
>   drivers/vfio/pci/vfio_pci_core.c           |  2 +-
>   include/linux/vgaarb.h                     |  8 +++++---
>   7 files changed, 30 insertions(+), 10 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 ceb914245383..c574898380f0 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);
> @@ -969,6 +970,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
> @@ -984,7 +989,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;
> @@ -996,6 +1002,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:
> @@ -1523,6 +1530,18 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
>   		notify = vga_arbiter_add_pci_device(pdev);
>   	else if (action == BUS_NOTIFY_DEL_DEVICE)
>   		notify = vga_arbiter_del_pci_device(pdev);
> +	else if (action == BUS_NOTIFY_BOUND_DRIVER) {
> +		struct vga_device *vgadev = vgadev_find(pdev);
> +		bool boot_dev = false;
> +
> +		if (vgadev && vgadev->is_boot_device)
> +			boot_dev = vgadev->is_boot_device(pdev);
> +
> +		if (boot_dev) {
> +			vgaarb_info(&pdev->dev, "Set as boot device (dictated by driver)\n");
> +			vga_set_default_device(pdev);
> +		}
> +	}
>   
>   	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
>   
> 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 97129a1bbb7d..dfde5a6ba55a 100644
> --- a/include/linux/vgaarb.h
> +++ b/include/linux/vgaarb.h
> @@ -33,7 +33,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)
> @@ -59,7 +60,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;
>   }
> @@ -97,7 +99,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 */

[PATCH v7 7/8] drm/amdgpu: Implement the is_boot_device callback function

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

[why]

The vga_is_firmware_default() defined in drivers/pci/vgaarb.c is
arch-dependent, it's a dummy on non-x86 architectures currently.
This made VGAARB lost an important condition for the arbitration.
It could still be wrong even if we remove the #ifdef and #endif guards.
because the PCI bar will move (resource re-allocation).

[how]

The device that owns the firmware framebuffer should be the default boot
device. This patch adds an arch-independent function to enforce this rule.
The vgaarb subsystem will call back to amdgpu_is_boot_device() function
when drm/amdgpu is successfully bound to an AMDGPU device.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Pan Xinhui <Xinhui.Pan@amd.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Hawking Zhang <Hawking.Zhang@amd.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: Lijo Lazar <lijo.lazar@amd.com>
Cc: YiPeng Chai <YiPeng.Chai@amd.com>
Cc: Bokun Zhang <Bokun.Zhang@amd.com>
CC: Likun Gao <Likun.Gao@amd.com>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 7a096f2d5c16..77624e8062d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3560,6 +3560,15 @@ static const struct attribute *amdgpu_dev_attributes[] = {
 	NULL
 };
 
+static bool amdgpu_is_boot_device(struct pci_dev *pdev)
+{
+	struct drm_device *dev = pci_get_drvdata(pdev);
+	struct amdgpu_device *adev = drm_to_adev(dev);
+	struct amdgpu_gmc *gmc = &adev->gmc;
+
+	return drm_aperture_contain_firmware_fb(gmc->aper_base, gmc->aper_size);
+}
+
 /**
  * amdgpu_device_init - initialize the driver
  *
@@ -3960,7 +3969,8 @@ 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, NULL);
+		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode,
+				    amdgpu_is_boot_device);
 
 	px = amdgpu_device_supports_px(ddev);
 
-- 
2.25.1

Re: [PATCH v7 7/8] drm/amdgpu: Implement the is_boot_device callback function

[Sui Jingfeng]

Hi,


Does anyone has the bandwidth to review this?

I provide more additional information here, hope it helps.


On a non-x86 multiple platform, the discrete AMDGPU fails to override 
the integrated one.

because the PCI BAR 0 of the AMDGPU gets moved.

Below is the log of 'dmesg | grep vgaarb'.

So relaying on screen_info is not always reliable.


[    0.361928] pci 0000:00:06.1: vgaarb: setting as boot VGA device
[    0.361932] pci 0000:00:06.1: vgaarb: bridge control possible
[    0.361933] pci 0000:00:06.1: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none
[    0.361940] pci 0000:05:00.0: vgaarb: bridge control possible
[    0.361941] pci 0000:05:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none
[    0.361943] vgaarb: loaded
[   11.352087] amdgpu 0000:05:00.0: vgaarb: Set as boot device (dictated by driver)
[   11.575505] loongson 0000:00:06.1: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=io+mem
[   11.585100] amdgpu 0000:05:00.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=none


dmesg | grep efifb:


[    0.356355] pci 0000:05:00.0: BAR 0: assigned to efifb
[    0.375793] efifb: probing for efifb
[    0.375795] pci 0000:05:00.0: BAR has moved, updating efifb address
[    0.375803] efifb: framebuffer at 0xe0030000000, using 976k, total 975k
[    0.375805] efifb: mode is 800x600x16, linelength=1664, pages=1
[    0.375806] efifb: scrolling: redraw
[    0.375808] efifb: Truecolor: size=0:5:6:5, shift=0:11:5:0


efifb can also prove that "BAR has been moved"


 From dmesg |  grep "pci 0000:05:00.0":


[    0.356286] pci 0000:05:00.0: [1002:699f] type 00 class 0x030000
[    0.356303] pci 0000:05:00.0: reg 0x10: [mem 
0xe0020000000-0xe002fffffff 64bit pref]
[    0.356315] pci 0000:05:00.0: reg 0x18: [mem 
0xe0030000000-0xe00301fffff 64bit pref]
[    0.356323] pci 0000:05:00.0: reg 0x20: [io  0x40000-0x400ff]
[    0.356331] pci 0000:05:00.0: reg 0x24: [mem 0xe0053100000-0xe005313ffff]
[    0.356339] pci 0000:05:00.0: reg 0x30: [mem 0xfffe0000-0xffffffff pref]
[    0.356346] pci 0000:05:00.0: enabling Extended Tags
[    0.356355] pci 0000:05:00.0: BAR 0: assigned to efifb
[    0.356421] pci 0000:05:00.0: supports D1 D2
[    0.356422] pci 0000:05:00.0: PME# supported from D1 D2 D3hot D3cold
[    0.356858] pci 0000:05:00.0: BAR 0: assigned [mem 
0xe0030000000-0xe003fffffff 64bit pref]
[    0.356866] pci 0000:05:00.0: BAR 2: assigned [mem 
0xe0040000000-0xe00401fffff 64bit pref]
[    0.356875] pci 0000:05:00.0: BAR 5: assigned [mem 
0xe0049000000-0xe004903ffff]
[    0.356878] pci 0000:05:00.0: BAR 6: assigned [mem 
0xe0049040000-0xe004905ffff pref]
[    0.356889] pci 0000:05:00.0: BAR 4: assigned [io 0x4000-0x40ff]
[    0.361940] pci 0000:05:00.0: vgaarb: bridge control possible
[    0.361941] pci 0000:05:00.0: vgaarb: VGA device added: 
decodes=io+mem,owns=none,locks=none
[    0.375795] pci 0000:05:00.0: BAR has moved, updating efifb address

We can see that the Bar 0 of AMDGPU

moved from '0xe0020000000-0xe002fffffff'  to '0xe0030000000-0xe003fffffff'

while the fb location information recorded by the screen_info still 
belong to '0xe0020000000-0xe002fffffff'


I suspect this is also the reason that video/aperture don't relay on the 
information provided by screen_info

in the contrast, it require the firmware framebuffer driver(efifb) to call

devm_aperture_acquire_from_firmware() function, only in this way 
video/aperture

could record the correct information about the aperture being used the 
by the firmware framebuffe.


While vgaarb is loaded too early, even before efifb.

so that we can only relay on the pci_notifier call back to us.


On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> [why]
>
> The vga_is_firmware_default() defined in drivers/pci/vgaarb.c is
> arch-dependent, it's a dummy on non-x86 architectures currently.
> This made VGAARB lost an important condition for the arbitration.
> It could still be wrong even if we remove the #ifdef and #endif guards.
> because the PCI bar will move (resource re-allocation).
>
> [how]
>
> The device that owns the firmware framebuffer should be the default boot
> device. This patch adds an arch-independent function to enforce this rule

-- 
Jingfeng

[PATCH v7 8/8] drm/radeon: Implement the is_boot_device callback function

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

[why]

The vga_is_firmware_default() defined in drivers/pci/vgaarb.c is
arch-dependent, it's a dummy on non-x86 architectures currently.
This made VGAARB lost an important condition for the arbitration.
It could still be wrong even if we remove the #ifdef and #endif guards.
because the PCI bar will move (resource re-allocation).

[how]

The device that owns the firmware framebuffer should be the default boot
device. This patch adds an arch-independent function to enforce this rule.
The vgaarb subsystem will call back to radeon_is_boot_device() function
when drm/radeon is successfully bound to a radeon GPU device.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Pan Xinhui <Xinhui.Pan@amd.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/radeon/radeon_device.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 71f2ff39d6a1..afb49000830c 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -34,6 +34,7 @@
 #include <linux/vga_switcheroo.h>
 #include <linux/vgaarb.h>
 
+#include <drm/drm_aperture.h>
 #include <drm/drm_cache.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_device.h>
@@ -1263,6 +1264,15 @@ static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
 	.can_switch = radeon_switcheroo_can_switch,
 };
 
+static bool radeon_is_boot_device(struct pci_dev *pdev)
+{
+	struct drm_device *dev = pci_get_drvdata(pdev);
+	struct radeon_device *rdev = dev->dev_private;
+	struct radeon_mc *gmc = &rdev->mc;
+
+	return drm_aperture_contain_firmware_fb(gmc->aper_base, gmc->aper_size);
+}
+
 /**
  * radeon_device_init - initialize the driver
  *
@@ -1425,7 +1435,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, NULL);
+	vga_client_register(rdev->pdev, radeon_vga_set_decode, radeon_is_boot_device);
 
 	if (rdev->flags & RADEON_IS_PX)
 		runtime = true;
-- 
2.25.1

Re: [PATCH v7 8/8] drm/radeon: Implement the is_boot_device callback function

[Sui Jingfeng]

PING ?


On 2023/6/13 11:01, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> [why]
>
> The vga_is_firmware_default() defined in drivers/pci/vgaarb.c is
> arch-dependent, it's a dummy on non-x86 architectures currently.
> This made VGAARB lost an important condition for the arbitration.
> It could still be wrong even if we remove the #ifdef and #endif guards.
> because the PCI bar will move (resource re-allocation).
>
> [how]
>
> The device that owns the firmware framebuffer should be the default boot
> device. This patch adds an arch-independent function to enforce this rule

-- 
Jingfeng