| Message ID | 20241007-b4-has_ioport-v6-3-03f7240da6e5@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | treewide: Remove I/O port accessors for HAS_IOPORT=n | expand |
On Mon, Oct 07, 2024 at 01:40:21PM +0200, Niklas Schnelle wrote: >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 +- as an example: $ git grep -lw outb -- drivers/gpu/drm/ drivers/gpu/drm/gma500/cdv_device.c drivers/gpu/drm/i915/display/intel_vga.c drivers/gpu/drm/qxl/qxl_cmd.c drivers/gpu/drm/qxl/qxl_irq.c drivers/gpu/drm/tiny/bochs.c drivers/gpu/drm/tiny/cirrus.c you are adding the dependency on xe, but why are you keeping i915 out? What approach did you use to determine the dependency? Lucas De Marchi
On Mon, Oct 7, 2024, at 14:39, Lucas De Marchi wrote: > as an example: > $ git grep -lw outb -- drivers/gpu/drm/ > drivers/gpu/drm/gma500/cdv_device.c > drivers/gpu/drm/i915/display/intel_vga.c > drivers/gpu/drm/qxl/qxl_cmd.c > drivers/gpu/drm/qxl/qxl_irq.c > drivers/gpu/drm/tiny/bochs.c > drivers/gpu/drm/tiny/cirrus.c > > you are adding the dependency on xe, but why are you keeping i915 out? > What approach did you use to determine the dependency? I did a lot of 'randconfig' build testing on earlier versions (and this version) of the series, which eventually catches all of them. The i915 driver depends on CONfIG_X86 since it is only used in Intel PC chipsets that already rely on PIO. The XE driver is also used for add-on cards, so the drivers can be built on all architectures including those that do not support PCI I/O space access. Adding the dependency on i915 as well wouldn't be wrong, but is not required for correctness. I also sent a patch for vmwgfx, which can be used on arm64. arm64 currently always sets HAS_IOPORT, so my patch is not required as a dependency for [PATCH v6 5/5], but we eventually want this so HAS_IOPORT can become optional on arm64. Arnd
On Mon, Oct 07, 2024 at 02:50:11PM +0000, Arnd Bergmann wrote: >On Mon, Oct 7, 2024, at 14:39, Lucas De Marchi wrote: >> as an example: >> $ git grep -lw outb -- drivers/gpu/drm/ >> drivers/gpu/drm/gma500/cdv_device.c >> drivers/gpu/drm/i915/display/intel_vga.c >> drivers/gpu/drm/qxl/qxl_cmd.c >> drivers/gpu/drm/qxl/qxl_irq.c >> drivers/gpu/drm/tiny/bochs.c >> drivers/gpu/drm/tiny/cirrus.c >> >> you are adding the dependency on xe, but why are you keeping i915 out? >> What approach did you use to determine the dependency? > >I did a lot of 'randconfig' build testing on earlier versions >(and this version) of the series, which eventually catches >all of them. The i915 driver depends on CONfIG_X86 since it >is only used in Intel PC chipsets that already rely on PIO. > >The XE driver is also used for add-on cards, so the drivers >can be built on all architectures including those that do >not support PCI I/O space access. Adding the dependency on >i915 as well wouldn't be wrong, but is not required for >correctness. ok, makes sense. I missed the indirect dependency already present in i915. Thanks. For the xe part, Acked-by: Lucas De Marchi <lucas.demarchi@intel.com> Lucas De Marchi > >I also sent a patch for vmwgfx, which can be used on arm64. >arm64 currently always sets HAS_IOPORT, so my patch is not >required as a dependency for [PATCH v6 5/5], but we eventually >want this so HAS_IOPORT can become optional on arm64. > > Arnd
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