In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
compile time. We thus need to add HAS_IOPORT as dependency for those
drivers using them. In the bochs driver there is optional MMIO support
detected at runtime, warn if this isn't taken when HAS_IOPORT is not
defined.

There is also a direct and hard coded use in cirrus.c which according to
the comment is only necessary during resume.  Let's just skip this as
for example s390 which doesn't have I/O port support also doesen't
support suspend/resume.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/gpu/drm/gma500/Kconfig |  2 +-
 drivers/gpu/drm/qxl/Kconfig    |  1 +
 drivers/gpu/drm/tiny/bochs.c   | 17 +++++++++++++++++
 drivers/gpu/drm/tiny/cirrus.c  |  2 ++
 drivers/gpu/drm/xe/Kconfig     |  2 +-
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index efb4a2dd2f80885cb59c925d09401002278d7d61..23b7c14de5e29238ece939d5822d8a9ffc4675cc 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config DRM_GMA500
 	tristate "Intel GMA500/600/3600/3650 KMS Framebuffer"
-	depends on DRM && PCI && X86 && MMU
+	depends on DRM && PCI && X86 && MMU && HAS_IOPORT
 	select DRM_KMS_HELPER
 	select FB_IOMEM_HELPERS if DRM_FBDEV_EMULATION
 	select I2C
diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig
index ca3f51c2a8fe1a383f8a2479f04b5c0b3fb14e44..d0e0d440c8d96564cb7b8ffd2385c44fc43f873d 100644
--- a/drivers/gpu/drm/qxl/Kconfig
+++ b/drivers/gpu/drm/qxl/Kconfig
@@ -2,6 +2,7 @@
 config DRM_QXL
 	tristate "QXL virtual GPU"
 	depends on DRM && PCI && MMU
+	depends on HAS_IOPORT
 	select DRM_KMS_HELPER
 	select DRM_TTM
 	select DRM_TTM_HELPER
diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 31fc5d839e106ea4d5c8fe42d1bfc3c70291e3fb..0ed78d3d5774778f91de972ac27056938036e722 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -2,6 +2,7 @@
 
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/bug.h>
 
 #include <drm/drm_aperture.h>
 #include <drm/drm_atomic_helper.h>
@@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
 
 		writeb(val, bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		outb(val, ioport);
+#endif
 	}
 }
 
@@ -119,7 +122,11 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
 
 		return readb(bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		return inb(ioport);
+#else
+		return 0xff;
+#endif
 	}
 }
 
@@ -132,8 +139,12 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
 
 		ret = readw(bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		ret = inw(VBE_DISPI_IOPORT_DATA);
+#else
+		ret = 0xffff;
+#endif
 	}
 	return ret;
 }
@@ -145,8 +156,10 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
 
 		writew(val, bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		outw(val, VBE_DISPI_IOPORT_DATA);
+#endif
 	}
 }
 
@@ -229,6 +242,10 @@ static int bochs_hw_init(struct drm_device *dev)
 			return -ENOMEM;
 		}
 	} else {
+		if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
+			DRM_ERROR("I/O ports are not supported\n");
+			return -EIO;
+		}
 		ioaddr = VBE_DISPI_IOPORT_INDEX;
 		iosize = 2;
 		if (!request_region(ioaddr, iosize, "bochs-drm")) {
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index 751326e3d9c374baf72115492aeefff2b73869f0..e31e1df029ab0272c4a1ff0ab3eb026ca679b560 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -509,8 +509,10 @@ static void cirrus_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	cirrus_mode_set(cirrus, &crtc_state->mode);
 
+#ifdef CONFIG_HAS_IOPORT
 	/* Unblank (needed on S3 resume, vgabios doesn't do it then) */
 	outb(VGA_AR_ENABLE_DISPLAY, VGA_ATT_W);
+#endif
 
 	drm_dev_exit(idx);
 }
diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 7bbe46a98ff1f449bc2af30686585a00e9e8af93..116f58774135fc3a9f37d6d72d41340f5c812297 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -49,7 +49,7 @@ config DRM_XE
 
 config DRM_XE_DISPLAY
 	bool "Enable display support"
-	depends on DRM_XE && DRM_XE=m
+	depends on DRM_XE && DRM_XE=m && HAS_IOPORT
 	select FB_IOMEM_HELPERS
 	select I2C
 	select I2C_ALGOBIT

-- 
2.43.0

Hi

Am 08.10.24 um 14:39 schrieb Niklas Schnelle:
> In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
> compile time. We thus need to add HAS_IOPORT as dependency for those
> drivers using them. In the bochs driver there is optional MMIO support
> detected at runtime, warn if this isn't taken when HAS_IOPORT is not
> defined.
>
> There is also a direct and hard coded use in cirrus.c which according to
> the comment is only necessary during resume.  Let's just skip this as
> for example s390 which doesn't have I/O port support also doesen't
> support suspend/resume.
>
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

I feel like I reviewed this before, but can't find it.

> ---
>   drivers/gpu/drm/gma500/Kconfig |  2 +-
>   drivers/gpu/drm/qxl/Kconfig    |  1 +
>   drivers/gpu/drm/tiny/bochs.c   | 17 +++++++++++++++++
>   drivers/gpu/drm/tiny/cirrus.c  |  2 ++
>   drivers/gpu/drm/xe/Kconfig     |  2 +-
>   5 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
> index efb4a2dd2f80885cb59c925d09401002278d7d61..23b7c14de5e29238ece939d5822d8a9ffc4675cc 100644
> --- a/drivers/gpu/drm/gma500/Kconfig
> +++ b/drivers/gpu/drm/gma500/Kconfig
> @@ -1,7 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   config DRM_GMA500
>   	tristate "Intel GMA500/600/3600/3650 KMS Framebuffer"
> -	depends on DRM && PCI && X86 && MMU
> +	depends on DRM && PCI && X86 && MMU && HAS_IOPORT
>   	select DRM_KMS_HELPER
>   	select FB_IOMEM_HELPERS if DRM_FBDEV_EMULATION
>   	select I2C
> diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig
> index ca3f51c2a8fe1a383f8a2479f04b5c0b3fb14e44..d0e0d440c8d96564cb7b8ffd2385c44fc43f873d 100644
> --- a/drivers/gpu/drm/qxl/Kconfig
> +++ b/drivers/gpu/drm/qxl/Kconfig
> @@ -2,6 +2,7 @@
>   config DRM_QXL
>   	tristate "QXL virtual GPU"
>   	depends on DRM && PCI && MMU
> +	depends on HAS_IOPORT

Is there a difference between this style (multiple 'depends on') and the 
one used for gma500 (&& && &&)?

>   	select DRM_KMS_HELPER
>   	select DRM_TTM
>   	select DRM_TTM_HELPER
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 31fc5d839e106ea4d5c8fe42d1bfc3c70291e3fb..0ed78d3d5774778f91de972ac27056938036e722 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -2,6 +2,7 @@
>   
>   #include <linux/module.h>
>   #include <linux/pci.h>
> +#include <linux/bug.h>

Alphabetic sorting please.

>   
>   #include <drm/drm_aperture.h>
>   #include <drm/drm_atomic_helper.h>
> @@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
>   
>   		writeb(val, bochs->mmio + offset);
>   	} else {
> +#ifdef CONFIG_HAS_IOPORT
>   		outb(val, ioport);
> +#endif

Could you provide empty defines for the out() interfaces at the top of 
the file?

>   	}
>   }
>   
> @@ -119,7 +122,11 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
>   
>   		return readb(bochs->mmio + offset);
>   	} else {
> +#ifdef CONFIG_HAS_IOPORT
>   		return inb(ioport);
> +#else
> +		return 0xff;
> +#endif

And the in() interfaces could be defined to 0xff[ff].

I assume that you don't want to provide such empty macros in the 
kernel's io.h header?

>   	}
>   }
>   
> @@ -132,8 +139,12 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
>   
>   		ret = readw(bochs->mmio + offset);
>   	} else {
> +#ifdef CONFIG_HAS_IOPORT
>   		outw(reg, VBE_DISPI_IOPORT_INDEX);
>   		ret = inw(VBE_DISPI_IOPORT_DATA);
> +#else
> +		ret = 0xffff;
> +#endif
>   	}
>   	return ret;
>   }
> @@ -145,8 +156,10 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
>   
>   		writew(val, bochs->mmio + offset);
>   	} else {
> +#ifdef CONFIG_HAS_IOPORT
>   		outw(reg, VBE_DISPI_IOPORT_INDEX);
>   		outw(val, VBE_DISPI_IOPORT_DATA);
> +#endif
>   	}
>   }
>   
> @@ -229,6 +242,10 @@ static int bochs_hw_init(struct drm_device *dev)
>   			return -ENOMEM;
>   		}
>   	} else {
> +		if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
> +			DRM_ERROR("I/O ports are not supported\n");
> +			return -EIO;
> +		}

It would be nicer to use an "} else if(IOPORT) {" here and put the 
"return -EIO" into a trailing else branch.

If you want to add an error message, please don't use DRM_ERROR(). In 
this case, dev_err(dev->dev, "...\n") seems appropriate.

Best regards
Thomas

>   		ioaddr = VBE_DISPI_IOPORT_INDEX;
>   		iosize = 2;
>   		if (!request_region(ioaddr, iosize, "bochs-drm")) {
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index 751326e3d9c374baf72115492aeefff2b73869f0..e31e1df029ab0272c4a1ff0ab3eb026ca679b560 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -509,8 +509,10 @@ static void cirrus_crtc_helper_atomic_enable(struct drm_crtc *crtc,
>   
>   	cirrus_mode_set(cirrus, &crtc_state->mode);
>   
> +#ifdef CONFIG_HAS_IOPORT
>   	/* Unblank (needed on S3 resume, vgabios doesn't do it then) */
>   	outb(VGA_AR_ENABLE_DISPLAY, VGA_ATT_W);
> +#endif
>   
>   	drm_dev_exit(idx);
>   }
> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> index 7bbe46a98ff1f449bc2af30686585a00e9e8af93..116f58774135fc3a9f37d6d72d41340f5c812297 100644
> --- a/drivers/gpu/drm/xe/Kconfig
> +++ b/drivers/gpu/drm/xe/Kconfig
> @@ -49,7 +49,7 @@ config DRM_XE
>   
>   config DRM_XE_DISPLAY
>   	bool "Enable display support"
> -	depends on DRM_XE && DRM_XE=m
> +	depends on DRM_XE && DRM_XE=m && HAS_IOPORT
>   	select FB_IOMEM_HELPERS
>   	select I2C
>   	select I2C_ALGOBIT
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Mon, Oct 21, 2024, at 07:52, Thomas Zimmermann wrote:
> Am 08.10.24 um 14:39 schrieb Niklas Schnelle:
d 100644
>> --- a/drivers/gpu/drm/qxl/Kconfig
>> +++ b/drivers/gpu/drm/qxl/Kconfig
>> @@ -2,6 +2,7 @@
>>   config DRM_QXL
>>   	tristate "QXL virtual GPU"
>>   	depends on DRM && PCI && MMU
>> +	depends on HAS_IOPORT
>
> Is there a difference between this style (multiple 'depends on') and the 
> one used for gma500 (&& && &&)?

No, it's the same. Doing it in one line is mainly useful
if you have some '||' as well.

>> @@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
>>   
>>   		writeb(val, bochs->mmio + offset);
>>   	} else {
>> +#ifdef CONFIG_HAS_IOPORT
>>   		outb(val, ioport);
>> +#endif
>
> Could you provide empty defines for the out() interfaces at the top of 
> the file?

That no longer works since there are now __compiletime_error()
versions of these funcitons. However we can do it more nicely like:

diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 9b337f948434..034af6e32200 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -112,14 +112,12 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
 	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
 		return;
 
-	if (bochs->mmio) {
+	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
 		int offset = ioport - 0x3c0 + 0x400;
 
 		writeb(val, bochs->mmio + offset);
 	} else {
-#ifdef CONFIG_HAS_IOPORT
 		outb(val, ioport);
-#endif
 	}
 }
 
@@ -128,16 +126,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
 	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
 		return 0xff;
 
-	if (bochs->mmio) {
+	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
 		int offset = ioport - 0x3c0 + 0x400;
 
 		return readb(bochs->mmio + offset);
 	} else {
-#ifdef CONFIG_HAS_IOPORT
 		return inb(ioport);
-#else
-		return 0xff;
-#endif
 	}
 }
 
@@ -145,32 +139,26 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
 {
 	u16 ret = 0;
 
-	if (bochs->mmio) {
+	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
 		int offset = 0x500 + (reg << 1);
 
 		ret = readw(bochs->mmio + offset);
 	} else {
-#ifdef CONFIG_HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		ret = inw(VBE_DISPI_IOPORT_DATA);
-#else
-		ret = 0xffff;
-#endif
 	}
 	return ret;
 }
 
 static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
 {
-	if (bochs->mmio) {
+	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
 		int offset = 0x500 + (reg << 1);
 
 		writew(val, bochs->mmio + offset);
 	} else {
-#ifdef CONFIG_HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		outw(val, VBE_DISPI_IOPORT_DATA);
-#endif
 	}
 }
 
> And the in() interfaces could be defined to 0xff[ff].
>
> I assume that you don't want to provide such empty macros in the 
> kernel's io.h header?

That was the original idea many years ago, but Linus rejected
my pull request for it, so Niklas worked through all drivers
individually to add the dependencies instead.

     Arnd

Hi

Am 21.10.24 um 12:08 schrieb Arnd Bergmann:
> On Mon, Oct 21, 2024, at 07:52, Thomas Zimmermann wrote:
>> Am 08.10.24 um 14:39 schrieb Niklas Schnelle:
> d 100644
>>> --- a/drivers/gpu/drm/qxl/Kconfig
>>> +++ b/drivers/gpu/drm/qxl/Kconfig
>>> @@ -2,6 +2,7 @@
>>>    config DRM_QXL
>>>    	tristate "QXL virtual GPU"
>>>    	depends on DRM && PCI && MMU
>>> +	depends on HAS_IOPORT
>> Is there a difference between this style (multiple 'depends on') and the
>> one used for gma500 (&& && &&)?
> No, it's the same. Doing it in one line is mainly useful
> if you have some '||' as well.
>
>>> @@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
>>>    
>>>    		writeb(val, bochs->mmio + offset);
>>>    	} else {
>>> +#ifdef CONFIG_HAS_IOPORT
>>>    		outb(val, ioport);
>>> +#endif
>> Could you provide empty defines for the out() interfaces at the top of
>> the file?
> That no longer works since there are now __compiletime_error()
> versions of these funcitons. However we can do it more nicely like:
>
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 9b337f948434..034af6e32200 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -112,14 +112,12 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
>   	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
>   		return;
>   
> -	if (bochs->mmio) {
> +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
>   		int offset = ioport - 0x3c0 + 0x400;
>   
>   		writeb(val, bochs->mmio + offset);
>   	} else {
> -#ifdef CONFIG_HAS_IOPORT
>   		outb(val, ioport);
> -#endif
>   	}

For all functions with such a pattern, could we use:

bool bochs_uses_mmio(bochs)
{
     return !IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio
}

void writeb_func()
{
     if (bochs_uses_mmio()) {
       writeb()
#if CONFIG_HAS_IOPORT
     } else {
       outb()
#endif
     }
}

u8 readb_func()
{
     u8 ret = 0xff
     if (bochs_uses_mmio()) {
       ret = readb()
#if CONFIG_HAS_IOPORT
     } else {
       ret = inb()
#endif
     }
     return ret;
}

?

The code in bochs_uses_mmio() could then also print a drm_warn_once if 
we have neither MMIO nor I/O ports.

I'd review a separate series for such a change.

>   }
>   
> @@ -128,16 +126,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
>   	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
>   		return 0xff;
>   
> -	if (bochs->mmio) {
> +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
>   		int offset = ioport - 0x3c0 + 0x400;
>   
>   		return readb(bochs->mmio + offset);
>   	} else {
> -#ifdef CONFIG_HAS_IOPORT
>   		return inb(ioport);
> -#else
> -		return 0xff;
> -#endif
>   	}
>   }
>   
> @@ -145,32 +139,26 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
>   {
>   	u16 ret = 0;
>   
> -	if (bochs->mmio) {
> +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
>   		int offset = 0x500 + (reg << 1);
>   
>   		ret = readw(bochs->mmio + offset);
>   	} else {
> -#ifdef CONFIG_HAS_IOPORT
>   		outw(reg, VBE_DISPI_IOPORT_INDEX);
>   		ret = inw(VBE_DISPI_IOPORT_DATA);
> -#else
> -		ret = 0xffff;
> -#endif
>   	}
>   	return ret;
>   }
>   
>   static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
>   {
> -	if (bochs->mmio) {
> +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
>   		int offset = 0x500 + (reg << 1);
>   
>   		writew(val, bochs->mmio + offset);
>   	} else {
> -#ifdef CONFIG_HAS_IOPORT
>   		outw(reg, VBE_DISPI_IOPORT_INDEX);
>   		outw(val, VBE_DISPI_IOPORT_DATA);
> -#endif
>   	}
>   }
>   
>> And the in() interfaces could be defined to 0xff[ff].
>>
>> I assume that you don't want to provide such empty macros in the
>> kernel's io.h header?
> That was the original idea many years ago, but Linus rejected
> my pull request for it, so Niklas worked through all drivers
> individually to add the dependencies instead.

I see.

Best regards
Thomas

>
>       Arnd

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Mon, Oct 21, 2024, at 10:58, Thomas Zimmermann wrote:
> Am 21.10.24 um 12:08 schrieb Arnd Bergmann:
>> On Mon, Oct 21, 2024, at 07:52, Thomas Zimmermann wrote:
>> --- a/drivers/gpu/drm/tiny/bochs.c
>> +++ b/drivers/gpu/drm/tiny/bochs.c
>> @@ -112,14 +112,12 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
>>   	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
>>   		return;
>>   
>> -	if (bochs->mmio) {
>> +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {

I meant IS_ENABLED() of course.

> For all functions with such a pattern, could we use:
>
> bool bochs_uses_mmio(bochs)
> {
>      return !IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio
> }
>
> void writeb_func()
> {
>      if (bochs_uses_mmio()) {
>        writeb()
> #if CONFIG_HAS_IOPORT
>      } else {
>        outb()
> #endif
>      }

Yes, that helper function look fine, but it should then
be either __always_inline or a macro. With that, the
#ifdef is not needed since gcc only warns if there is
a path that leads to outb() actually getting called.

      Arnd

On Mon, 2024-10-21 at 12:58 +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 21.10.24 um 12:08 schrieb Arnd Bergmann:
> > On Mon, Oct 21, 2024, at 07:52, Thomas Zimmermann wrote:
> > > Am 08.10.24 um 14:39 schrieb Niklas Schnelle:
> > d 100644
> > > > --- a/drivers/gpu/drm/qxl/Kconfig
> > > > +++ b/drivers/gpu/drm/qxl/Kconfig
> > > > @@ -2,6 +2,7 @@
> > > >    config DRM_QXL
> > > >    	tristate "QXL virtual GPU"
> > > >    	depends on DRM && PCI && MMU
> > > > +	depends on HAS_IOPORT
> > > Is there a difference between this style (multiple 'depends on') and the
> > > one used for gma500 (&& && &&)?
> > No, it's the same. Doing it in one line is mainly useful
> > if you have some '||' as well.
> > 
> > > > @@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
> > > >    
> > > >    		writeb(val, bochs->mmio + offset);
> > > >    	} else {
> > > > +#ifdef CONFIG_HAS_IOPORT
> > > >    		outb(val, ioport);
> > > > +#endif
> > > Could you provide empty defines for the out() interfaces at the top of
> > > the file?
> > That no longer works since there are now __compiletime_error()
> > versions of these funcitons. However we can do it more nicely like:
> > 
> > diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> > index 9b337f948434..034af6e32200 100644
> > --- a/drivers/gpu/drm/tiny/bochs.c
> > +++ b/drivers/gpu/drm/tiny/bochs.c
> > @@ -112,14 +112,12 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
> >   	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
> >   		return;
> >   
> > -	if (bochs->mmio) {
> > +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
> >   		int offset = ioport - 0x3c0 + 0x400;
> >   
> >   		writeb(val, bochs->mmio + offset);
> >   	} else {
> > -#ifdef CONFIG_HAS_IOPORT
> >   		outb(val, ioport);
> > -#endif
> >   	}
> 
> For all functions with such a pattern, could we use:
> 
> bool bochs_uses_mmio(bochs)
> {
>      return !IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio
> }
> 
> void writeb_func()
> {
>      if (bochs_uses_mmio()) {
>        writeb()
> #if CONFIG_HAS_IOPORT
>      } else {
>        outb()
> #endif
>      }
> }
> 

I think if the helper were __always_inline we could still take
advantage of the dead code elimination and combine this with Arnd's
approach. Though I feel like it is a bit odd to try to do the MMIO
approach despite bochs->mmio being false on !HAS_IOPORT systems.
Is that what you wanted to correct by keeping the #ifdef
CONFIG_HAS_IOPORT around the else? And yes the warning makes sense to
me too.

Thanks,
Niklas

On Mon, Oct 21, 2024 at 01:18:20PM +0200, Niklas Schnelle wrote:
> On Mon, 2024-10-21 at 12:58 +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 21.10.24 um 12:08 schrieb Arnd Bergmann:
> > > On Mon, Oct 21, 2024, at 07:52, Thomas Zimmermann wrote:
> > > > Am 08.10.24 um 14:39 schrieb Niklas Schnelle:
> > > d 100644
> > > > > --- a/drivers/gpu/drm/qxl/Kconfig
> > > > > +++ b/drivers/gpu/drm/qxl/Kconfig
> > > > > @@ -2,6 +2,7 @@
> > > > >    config DRM_QXL
> > > > >    	tristate "QXL virtual GPU"
> > > > >    	depends on DRM && PCI && MMU
> > > > > +	depends on HAS_IOPORT
> > > > Is there a difference between this style (multiple 'depends on') and the
> > > > one used for gma500 (&& && &&)?
> > > No, it's the same. Doing it in one line is mainly useful
> > > if you have some '||' as well.
> > > 
> > > > > @@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
> > > > >    
> > > > >    		writeb(val, bochs->mmio + offset);
> > > > >    	} else {
> > > > > +#ifdef CONFIG_HAS_IOPORT
> > > > >    		outb(val, ioport);
> > > > > +#endif
> > > > Could you provide empty defines for the out() interfaces at the top of
> > > > the file?
> > > That no longer works since there are now __compiletime_error()
> > > versions of these funcitons. However we can do it more nicely like:
> > > 
> > > diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> > > index 9b337f948434..034af6e32200 100644
> > > --- a/drivers/gpu/drm/tiny/bochs.c
> > > +++ b/drivers/gpu/drm/tiny/bochs.c
> > > @@ -112,14 +112,12 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
> > >   	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
> > >   		return;
> > >   
> > > -	if (bochs->mmio) {
> > > +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
> > >   		int offset = ioport - 0x3c0 + 0x400;
> > >   
> > >   		writeb(val, bochs->mmio + offset);
> > >   	} else {
> > > -#ifdef CONFIG_HAS_IOPORT
> > >   		outb(val, ioport);
> > > -#endif
> > >   	}
> > 
> > For all functions with such a pattern, could we use:
> > 
> > bool bochs_uses_mmio(bochs)
> > {
> >      return !IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio
> > }
> > 
> > void writeb_func()
> > {
> >      if (bochs_uses_mmio()) {
> >        writeb()
> > #if CONFIG_HAS_IOPORT
> >      } else {
> >        outb()
> > #endif
> >      }
> > }
> > 
> 
> I think if the helper were __always_inline we could still take
> advantage of the dead code elimination and combine this with Arnd's
> approach. Though I feel like it is a bit odd to try to do the MMIO
> approach despite bochs->mmio being false on !HAS_IOPORT systems.
> Is that what you wanted to correct by keeping the #ifdef
> CONFIG_HAS_IOPORT around the else? And yes the warning makes sense to
> me too.

Working on this now, I think we don't need a warning in the bochs_uses_mmio()
helper because we should never get here with !IS_ENABLED(CONFIG_HAS_IOPORT)
at runtime thanks to the check added in bochs_hw_init(). This also takes
care of my original worry that we might try writeb()/readb() with an invalid
bochs->mmio value. I'll sent a v9 with the helper added and #ifdefs's removed.

Thanks,
Niklas

>
> I'd review a separate series for such a change.

Could also be a single patch here, of course.



-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)