diff mbox series

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

Message ID 20241007-b4-has_ioport-v6-3-03f7240da6e5@linux.ibm.com (mailing list archive)
State Superseded
Headers show
Series treewide: Remove I/O port accessors for HAS_IOPORT=n | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: drivers/gpu/drm/tiny/cirrus.c is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please. WARNING: drivers/gpu/drm/tiny/cirrus.c is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please. total: 0 errors, 2 warnings, 92 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13824501.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix fail "Bluetooth: " prefix is not specified in the subject

Commit Message

Niklas Schnelle Oct. 7, 2024, 11:40 a.m. UTC 
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(-)

Comments

Lucas De Marchi Oct. 7, 2024, 2:39 p.m. UTC | #1 
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
Arnd Bergmann Oct. 7, 2024, 2:50 p.m. UTC | #2 
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
Lucas De Marchi Oct. 7, 2024, 4:49 p.m. UTC | #3 
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 mbox series

Patch

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