[v8,3/5] drm: handle HAS_IOPORT dependencies

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

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

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

Could also be a single patch here, of course.

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