[-next 0/5] Add the pci_is_vga() helper and use it

[-next 0/5] Add the pci_is_vga() helper and use it

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

The PCI code and ID assignment specification defined four types of
display controllers for the display base class(03h), and the devices
with 0x00h sub-class code are VGA devices. VGA devices with programming
interface 0x00 is VGA-compatible, VGA devices with programming interface
0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
is defined to provide backward compatibility for devices that were built
before the class code field was defined. Thus, PCI(e) device with the
PCI_CLASS_NOT_DEFINED_VGA class code should also be handled as the normal
VGA-compatible devices.

Compared with the "if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)" code,
the newly implemented pci_is_vga() is shorter and straightforward. So it
is more easy to use. It is designed as a inline function, the more common
case "if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA))" is put before the
less common case "if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)",
so there should no performance penalty.

Sui Jingfeng (5):
  PCI: Add the pci_is_vga() helper
  PCI/VGA: Deal with VGA devices
  PCI/sysfs: Use pci_is_vga() helper
  drm/virgpu: Switch to pci_is_vga()
  drm/qxl: Switch to pci_is_vga()

 drivers/gpu/drm/qxl/qxl_drv.c        | 11 +++--------
 drivers/gpu/drm/virtio/virtgpu_drv.c |  2 +-
 drivers/pci/pci-sysfs.c              |  6 +++---
 drivers/pci/vgaarb.c                 | 19 +++++++++----------
 include/linux/pci.h                  | 27 +++++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 22 deletions(-)


base-commit: 43cc31da9146f9ce60e4a03d96ef0807c2cdac94
-- 
2.34.1

[-next 1/5] PCI: Add the pci_is_vga() helper

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

The PCI code and ID assignment specification defined four types of
display controllers for the display base class(03h), and the devices
with 0x00h sub-class code are VGA devices. VGA devices with programming
interface 0x00 is VGA-compatible, VGA devices with programming interface
0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
is defined to provide backward compatibility for devices that were built
before the class code field was defined. Hence, introduce the pci_is_vga()
helper, let it handle the details for us. It returns true if the PCI(e)
device being tested belongs to the VGA devices category.

Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 include/linux/pci.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index cf6e0b057752..ace727001911 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -713,6 +713,33 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
 		dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
 }
 
+/**
+ * The PCI code and ID assignment specification defined four types of
+ * display controllers for the display base class(03h), and the devices
+ * with 0x00h sub-class code are VGA devices. VGA devices with programming
+ * interface 0x00 is VGA-compatible, VGA devices with programming interface
+ * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
+ * is defined to provide backward compatibility for devices that were built
+ * before the class code field was defined. This means that it belong to the
+ * VGA devices category also.
+ *
+ * Returns:
+ * true if the PCI device is a VGA device, false otherwise.
+ */
+static inline bool pci_is_vga(struct pci_dev *pdev)
+{
+	if (!pdev)
+		return false;
+
+	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
+		return true;
+
+	if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
+		return true;
+
+	return false;
+}
+
 #define for_each_pci_bridge(dev, bus)				\
 	list_for_each_entry(dev, &bus->devices, bus_list)	\
 		if (!pci_is_bridge(dev)) {} else
-- 
2.34.1

[-next 2/5] PCI/VGA: Deal with VGA devices

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

VGAARB only cares about PCI(e) VGA devices, thus filtering out unqualified
devices as early as possible. This also means that deleting a non-VGA
device snooped won't unnecessarily call into vga_arbiter_del_pci_device()
function. By using the newly implemented pci_is_vga(),
PCI(e) with PCI_CLASS_NOT_DEFINED_VGA class code will also be handled.

Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/vgaarb.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5e6b1eb54c64..ef8fe685de67 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -764,10 +764,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) {
@@ -1503,6 +1499,9 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 
 	vgaarb_dbg(dev, "%s\n", __func__);
 
+	if (!pci_is_vga(pdev))
+		return 0;
+
 	/*
 	 * For now, we're only interested in devices added and removed.
 	 * I didn't test this thing here, so someone needs to double check
@@ -1537,8 +1536,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)
@@ -1547,11 +1546,11 @@ static int __init vga_arb_device_init(void)
 	bus_register_notifier(&pci_bus_type, &pci_notifier);
 
 	/* Add all VGA class PCI devices by default */
-	pdev = NULL;
-	while ((pdev =
-		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
-			       PCI_ANY_ID, pdev)) != NULL)
-		vga_arbiter_add_pci_device(pdev);
+	do {
+		pdev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pdev);
+		if (pci_is_vga(pdev))
+			vga_arbiter_add_pci_device(pdev);
+	} while (pdev);
 
 	pr_info("loaded\n");
 	return rc;
-- 
2.34.1

[-next 3/5] PCI/sysfs: Use pci_is_vga() helper

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Instead of accessing the PCI_CLASS_DISPLAY_VGA and pdev->class directly.
The PCI_CLASS_NOT_DEFINED_VGA is defined to provide backward compatibility
for devices that were built before the class code field was defined. It
should be visiable via sysfs(boot_vga) as the normal VGA-compatible devices.

Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/pci/pci-sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d9eede2dbc0e..522708938563 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1552,10 +1552,10 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 	struct pci_dev *pdev = to_pci_dev(dev);
 
 	if (a == &dev_attr_boot_vga.attr)
-		if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
-			return 0;
+		if (pci_is_vga(pdev))
+			return a->mode;
 
-	return a->mode;
+	return 0;
 }
 
 static struct attribute *pci_dev_hp_attrs[] = {
-- 
2.34.1

[-next 4/5] drm/virgpu: Switch to pci_is_vga()

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Should be no functional change, just for cleanup purpose.

Cc: David Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Gurchetan Singh <gurchetansingh@chromium.org>
Cc: Chia-I Wu <olvaffe@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index add075681e18..3a368304475a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	const char *pname = dev_name(&pdev->dev);
-	bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
+	bool vga = pci_is_vga(pdev);
 	int ret;
 
 	DRM_INFO("pci: %s detected at %s\n",
-- 
2.34.1

[-next 5/5] drm/qxl: Switch to pci_is_vga()

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Should be no functional change, just for cleanup purpose.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/qxl/qxl_drv.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index a3b83f89e061..08586bd2448f 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -68,11 +68,6 @@ module_param_named(num_heads, qxl_num_crtc, int, 0400);
 static struct drm_driver qxl_driver;
 static struct pci_driver qxl_pci_driver;
 
-static bool is_vga(struct pci_dev *pdev)
-{
-	return pdev->class == PCI_CLASS_DISPLAY_VGA << 8;
-}
-
 static int
 qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
@@ -100,7 +95,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto disable_pci;
 
-	if (is_vga(pdev) && pdev->revision < 5) {
+	if (pci_is_vga(pdev) && pdev->revision < 5) {
 		ret = vga_get_interruptible(pdev, VGA_RSRC_LEGACY_IO);
 		if (ret) {
 			DRM_ERROR("can't get legacy vga ioports\n");
@@ -131,7 +126,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 unload:
 	qxl_device_fini(qdev);
 put_vga:
-	if (is_vga(pdev) && pdev->revision < 5)
+	if (pci_is_vga(pdev) && pdev->revision < 5)
 		vga_put(pdev, VGA_RSRC_LEGACY_IO);
 disable_pci:
 	pci_disable_device(pdev);
@@ -159,7 +154,7 @@ qxl_pci_remove(struct pci_dev *pdev)
 
 	drm_dev_unregister(dev);
 	drm_atomic_helper_shutdown(dev);
-	if (is_vga(pdev) && pdev->revision < 5)
+	if (pci_is_vga(pdev) && pdev->revision < 5)
 		vga_put(pdev, VGA_RSRC_LEGACY_IO);
 }
 
-- 
2.34.1

Re: [-next 4/5] drm/virgpu: Switch to pci_is_vga()

[Bjorn Helgaas]

In subject: "drm/virtio" to match previous history.

On Wed, Aug 30, 2023 at 07:15:31PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> Should be no functional change, just for cleanup purpose.
> 
> Cc: David Airlie <airlied@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Gurchetan Singh <gurchetansingh@chromium.org>
> Cc: Chia-I Wu <olvaffe@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index add075681e18..3a368304475a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  	const char *pname = dev_name(&pdev->dev);
> -	bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> +	bool vga = pci_is_vga(pdev);

This *is* a functional change: Previously "vga" was only true for
PCI_CLASS_DISPLAY_VGA (0x0300).  Now it will be true for both
PCI_CLASS_DISPLAY_VGA (0x0300) and PCI_CLASS_DISPLAY_OTHER (0x0380).

Is that desirable?  I can't tell.  Maybe the GPU folks will chime in.

>  	int ret;
>  
>  	DRM_INFO("pci: %s detected at %s\n",
> -- 
> 2.34.1
> 

Re: [-next 4/5] drm/virgpu: Switch to pci_is_vga()

[Bjorn Helgaas]

On Thu, Oct 05, 2023 at 04:57:14PM -0500, Bjorn Helgaas wrote:
> In subject: "drm/virtio" to match previous history.
> 
> On Wed, Aug 30, 2023 at 07:15:31PM +0800, Sui Jingfeng wrote:
> > From: Sui Jingfeng <suijingfeng@loongson.cn>
> > 
> > Should be no functional change, just for cleanup purpose.
> > 
> > Cc: David Airlie <airlied@redhat.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Gurchetan Singh <gurchetansingh@chromium.org>
> > Cc: Chia-I Wu <olvaffe@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> > ---
> >  drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > index add075681e18..3a368304475a 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > @@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev->dev);
> >  	const char *pname = dev_name(&pdev->dev);
> > -	bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> > +	bool vga = pci_is_vga(pdev);
> 
> This *is* a functional change: Previously "vga" was only true for
> PCI_CLASS_DISPLAY_VGA (0x0300).  Now it will be true for both
> PCI_CLASS_DISPLAY_VGA (0x0300) and PCI_CLASS_DISPLAY_OTHER (0x0380).

Oops, sorry, my mistake here.  I meant PCI_CLASS_NOT_DEFINED_VGA, not
PCI_CLASS_DISPLAY_OTHER.  pci_is_vga() is true for either of:

  PCI_CLASS_DISPLAY_VGA       0x0300
  PCI_CLASS_NOT_DEFINED_VGA   0x0001

(PCI_CLASS_NOT_DEFINED_VGA is defined in the PCI Code and Assignment
spec r1.15, sec 1.1; PCI_CLASS_DISPLAY_VGA is sec 1.4.)

> Is that desirable?  I can't tell.  Maybe the GPU folks will chime in.
> 
> >  	int ret;
> >  
> >  	DRM_INFO("pci: %s detected at %s\n",
> > -- 
> > 2.34.1
> > 

Re: [-next 1/5] PCI: Add the pci_is_vga() helper

[Bjorn Helgaas]

On Wed, Aug 30, 2023 at 07:15:28PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> The PCI code and ID assignment specification defined four types of
> display controllers for the display base class(03h), and the devices
> with 0x00h sub-class code are VGA devices. VGA devices with programming

I can update this with the spec details (PCI Code and Assignment spec
r1.15, secs 1.1 and 1.4).

> interface 0x00 is VGA-compatible, VGA devices with programming interface
> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> is defined to provide backward compatibility for devices that were built
> before the class code field was defined. Hence, introduce the pci_is_vga()
> helper, let it handle the details for us. It returns true if the PCI(e)
> device being tested belongs to the VGA devices category.
>
> Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  include/linux/pci.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cf6e0b057752..ace727001911 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -713,6 +713,33 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
>  		dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
>  }
>  
> +/**
> + * The PCI code and ID assignment specification defined four types of
> + * display controllers for the display base class(03h), and the devices
> + * with 0x00h sub-class code are VGA devices. VGA devices with programming
> + * interface 0x00 is VGA-compatible, VGA devices with programming interface
> + * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> + * is defined to provide backward compatibility for devices that were built
> + * before the class code field was defined. This means that it belong to the
> + * VGA devices category also.
> + *
> + * Returns:
> + * true if the PCI device is a VGA device, false otherwise.
> + */
> +static inline bool pci_is_vga(struct pci_dev *pdev)
> +{
> +	if (!pdev)
> +		return false;
> +
> +	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> +		return true;
> +
> +	if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
> +		return true;

Are you seeing a problem that will be fixed by this series, i.e., a
PCI_CLASS_NOT_DEFINED_VGA device that we currently don't handle
correctly?

I think this makes sense per the spec, but there's always a risk of
breaking something, so it's nice if the change actually *fixes*
something to make that risk worthwhile.

> +	return false;
> +}
> +
>  #define for_each_pci_bridge(dev, bus)				\
>  	list_for_each_entry(dev, &bus->devices, bus_list)	\
>  		if (!pci_is_bridge(dev)) {} else
> -- 
> 2.34.1
> 

Re: [-next 4/5] drm/virgpu: Switch to pci_is_vga()

[Sui Jingfeng]

Hi,


On 2023/10/6 05:57, Bjorn Helgaas wrote:
> In subject: "drm/virtio" to match previous history.
>
> On Wed, Aug 30, 2023 at 07:15:31PM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Should be no functional change, just for cleanup purpose.
>>
>> Cc: David Airlie <airlied@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Gurchetan Singh <gurchetansingh@chromium.org>
>> Cc: Chia-I Wu <olvaffe@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> index add075681e18..3a368304475a 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> @@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
>>   {
>>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>>   	const char *pname = dev_name(&pdev->dev);
>> -	bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
>> +	bool vga = pci_is_vga(pdev);
> This *is* a functional change: Previously "vga" was only true for
> PCI_CLASS_DISPLAY_VGA (0x0300).  Now it will be true for both
> PCI_CLASS_DISPLAY_VGA (0x0300) and PCI_CLASS_DISPLAY_OTHER (0x0380).
>
> Is that desirable?  I can't tell.  Maybe the GPU folks will chime in.
>

Yes, the vga variable still will be "true" for the PCI_CLASS_DISPLAY_VGA (0x0300) class code,
and this is the major case. But the devices with PCI_CLASS_NOT_DEFINED_VGA class code are quite
uncommon, and virtio gpu is virtual GPU driver, It is unlikely that the QEMU to emulate a
old device with PCI_CLASS_NOT_DEFINED_VGA class code. I means that there no reason to do so.
Am I correct? Is there anyone know more?

For virtio virtual GPU driver, I would like to drop this patch, if no one response.

We probably only need to consider PCI_CLASS_NOT_DEFINED_VGA case for the real (probably old) hardware device.
It do exists, as Maciej mention at [1].

[1] https://lkml.org/lkml/2023/6/18/315


>>   	int ret;
>>   
>>   	DRM_INFO("pci: %s detected at %s\n",
>> -- 
>> 2.34.1
>>

Re: [-next 1/5] PCI: Add the pci_is_vga() helper

[Sui Jingfeng]

Hi,


On 2023/10/6 06:51, Bjorn Helgaas wrote:
> On Wed, Aug 30, 2023 at 07:15:28PM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> The PCI code and ID assignment specification defined four types of
>> display controllers for the display base class(03h), and the devices
>> with 0x00h sub-class code are VGA devices. VGA devices with programming
> I can update this with the spec details (PCI Code and Assignment spec
> r1.15, secs 1.1 and 1.4).
>
>> interface 0x00 is VGA-compatible, VGA devices with programming interface
>> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
>> is defined to provide backward compatibility for devices that were built
>> before the class code field was defined. Hence, introduce the pci_is_vga()
>> helper, let it handle the details for us. It returns true if the PCI(e)
>> device being tested belongs to the VGA devices category.
>>
>> Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   include/linux/pci.h | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index cf6e0b057752..ace727001911 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -713,6 +713,33 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
>>   		dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
>>   }
>>   
>> +/**
>> + * The PCI code and ID assignment specification defined four types of
>> + * display controllers for the display base class(03h), and the devices
>> + * with 0x00h sub-class code are VGA devices. VGA devices with programming
>> + * interface 0x00 is VGA-compatible, VGA devices with programming interface
>> + * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
>> + * is defined to provide backward compatibility for devices that were built
>> + * before the class code field was defined. This means that it belong to the
>> + * VGA devices category also.
>> + *
>> + * Returns:
>> + * true if the PCI device is a VGA device, false otherwise.
>> + */
>> +static inline bool pci_is_vga(struct pci_dev *pdev)
>> +{
>> +	if (!pdev)
>> +		return false;
>> +
>> +	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>> +		return true;
>> +
>> +	if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
>> +		return true;
> Are you seeing a problem that will be fixed by this series, i.e., a
> PCI_CLASS_NOT_DEFINED_VGA device that we currently don't handle
> correctly?

No, I write it according to the spec.
But I'm sure that the boot_vga will not be shown at sysfs for a PCI_CLASS_NOT_DEFINED_VGA device.


> I think this makes sense per the spec, but there's always a risk of
> breaking something, so it's nice if the change actually *fixes*
> something to make that risk worthwhile.


Maciej mentioned that PCI_CLASS_NOT_DEFINED_VGA device should also be handled in the past.
see [1]. But if no one interested in PCI_CLASS_NOT_DEFINED_VGA nowaday, then I guess
the gains of this patch may not deserve the time and risk. But I don't mind if someone
would like pick it up for other purpose.

Thanks for the reviewing. :-)

[1] https://lkml.org/lkml/2023/6/18/315


>> +	return false;
>> +}
>> +
>>   #define for_each_pci_bridge(dev, bus)				\
>>   	list_for_each_entry(dev, &bus->devices, bus_list)	\
>>   		if (!pci_is_bridge(dev)) {} else
>> -- 
>> 2.34.1
>>

Re: [-next 1/5] PCI: Add the pci_is_vga() helper

[Maciej W. Rozycki]

On Fri, 6 Oct 2023, Sui Jingfeng wrote:

> > I think this makes sense per the spec, but there's always a risk of
> > breaking something, so it's nice if the change actually *fixes*
> > something to make that risk worthwhile.
> 
> 
> Maciej mentioned that PCI_CLASS_NOT_DEFINED_VGA device should also be handled
> in the past.
> see [1]. But if no one interested in PCI_CLASS_NOT_DEFINED_VGA nowaday, then I
> guess
> the gains of this patch may not deserve the time and risk. But I don't mind if
> someone
> would like pick it up for other purpose.

 Well, if we need to determine for whatever purpose whether a PCI/e device 
presents a VGA programming interface, then I think we ought to do this in 
a complete manner.  I'm not sure offhand what could possibly break if we 
write our code according to specs and include PCI_CLASS_NOT_DEFINED_VGA 
devices in the class.

 Yes, I'm aware they won't be the latest and greatest, but they may still 
be there out there in service.  For one I continue using my 30 years old 
Trident 8900C ISA VGA adapter with the most recent Linux kernel.  The card 
serves its purpose, mostly as a glass TTY, so why should I replace it?

 Of course there are broken devices out there regardless, which won't work 
as we expect without special handling or sometimes at all even.  It does 
not mean we should refrain from making the best effort for good compliant 
devices and assume in advance that something will break even if we write 
our code according to the relevant specs.  I'd say do write according to 
specs and only try to sort out the situation somehow if something actually 
does break.

 In any case if we actually do choose to ignore PCI_CLASS_NOT_DEFINED_VGA 
devices, then I wanted to make sure we do it deliberately rather than from 
the lack of awareness of the existence of such devices.

  Maciej

Re: [-next 0/5] Add the pci_is_vga() helper and use it

[Bjorn Helgaas]

On Wed, Aug 30, 2023 at 07:15:27PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> The PCI code and ID assignment specification defined four types of
> display controllers for the display base class(03h), and the devices
> with 0x00h sub-class code are VGA devices. VGA devices with programming
> interface 0x00 is VGA-compatible, VGA devices with programming interface
> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> is defined to provide backward compatibility for devices that were built
> before the class code field was defined. Thus, PCI(e) device with the
> PCI_CLASS_NOT_DEFINED_VGA class code should also be handled as the normal
> VGA-compatible devices.
> 
> Compared with the "if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)" code,
> the newly implemented pci_is_vga() is shorter and straightforward. So it
> is more easy to use. It is designed as a inline function, the more common
> case "if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA))" is put before the
> less common case "if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)",
> so there should no performance penalty.
> 
> Sui Jingfeng (5):
>   PCI: Add the pci_is_vga() helper
>   PCI/VGA: Deal with VGA devices
>   PCI/sysfs: Use pci_is_vga() helper
>   drm/virgpu: Switch to pci_is_vga()
>   drm/qxl: Switch to pci_is_vga()
> 
>  drivers/gpu/drm/qxl/qxl_drv.c        | 11 +++--------
>  drivers/gpu/drm/virtio/virtgpu_drv.c |  2 +-
>  drivers/pci/pci-sysfs.c              |  6 +++---
>  drivers/pci/vgaarb.c                 | 19 +++++++++----------
>  include/linux/pci.h                  | 27 +++++++++++++++++++++++++++
>  5 files changed, 43 insertions(+), 22 deletions(-)

I applied these on pci/vga for v6.7, thanks!

Please take a look at
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=vga
and let me know if I broke anything because I changed a few things;
interdiff below:

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 522708938563..252ee29fd697 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1551,9 +1551,8 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (a == &dev_attr_boot_vga.attr)
-		if (pci_is_vga(pdev))
-			return a->mode;
+	if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev))
+		return a->mode;
 
 	return 0;
 }
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index ef8fe685de67..feca96fc4255 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1499,6 +1499,7 @@ static int pci_notify(struct notifier_block *nb, unsigned long action,
 
 	vgaarb_dbg(dev, "%s\n", __func__);
 
+	/* Only deal with VGA class devices */
 	if (!pci_is_vga(pdev))
 		return 0;
 
@@ -1536,8 +1537,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)
@@ -1546,11 +1547,13 @@ static int __init vga_arb_device_init(void)
 	bus_register_notifier(&pci_bus_type, &pci_notifier);
 
 	/* Add all VGA class PCI devices by default */
-	do {
-		pdev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, pdev);
+	pdev = NULL;
+	while ((pdev =
+		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
+			       PCI_ANY_ID, pdev)) != NULL) {
 		if (pci_is_vga(pdev))
 			vga_arbiter_add_pci_device(pdev);
-	} while (pdev);
+	}
 
 	pr_info("loaded\n");
 	return rc;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 53e24e31c27e..7bab234391cb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -714,23 +714,20 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
 }
 
 /**
- * The PCI code and ID assignment specification defined four types of
- * display controllers for the display base class(03h), and the devices
- * with 0x00h sub-class code are VGA devices. VGA devices with programming
- * interface 0x00 is VGA-compatible, VGA devices with programming interface
- * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
- * is defined to provide backward compatibility for devices that were built
- * before the class code field was defined. This means that it belong to the
- * VGA devices category also.
+ * pci_is_vga - check if the PCI device is a VGA device
  *
- * Returns:
- * true if the PCI device is a VGA device, false otherwise.
+ * The PCI Code and ID Assignment spec, r1.15, secs 1.4 and 1.1, define
+ * VGA Base Class and Sub-Classes:
+ *
+ *   03 00  PCI_CLASS_DISPLAY_VGA      VGA-compatible or 8514-compatible
+ *   00 01  PCI_CLASS_NOT_DEFINED_VGA  VGA-compatible (before Class Code)
+ *
+ * Return true if the PCI device is a VGA device and uses the legacy VGA
+ * resources ([mem 0xa0000-0xbffff], [io 0x3b0-0x3bb], [io 0x3c0-0x3df] and
+ * aliases).
  */
 static inline bool pci_is_vga(struct pci_dev *pdev)
 {
-	if (!pdev)
-		return false;
-
 	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
 		return true;