Re: [PATCH v8 3/5] drm: handle HAS_IOPORT dependencies

[Niklas Schnelle <schnelle@linux.ibm.com>]

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