[PATCH 00/11] fbdev: Maintain device ownership with aperture helpers

[PATCH 00/11] fbdev: Maintain device ownership with aperture helpers

[Thomas Zimmermann]

Fbdev firmware drivers acquire ownership of framebuffer I/O ranges and
hand them over to native drivers during the boot process. Re-implement
this mechanism with aperture helpers and remove the respective fbdev
code.

This change allows to perform hand-over from DRM firmware drivers. In a
later patchset, device ownership can be moved from DRM and fbdev entirely
into aperture helpers.

Patches 1 and 4 are cleanups.

Patches 2 and 3 integrate EGA/VGA support into sysfb, although it's not
clear if the x86 architecture code actually still supports VGA graphics
mode.

Patches 5 to 10 replace fbdev's ownership management with aperture
helpers. This includes removal of conflicting framebuffer drivers,
removal of conflicting VGA drivers and registration of fbdev firmware
devices. Notably, many PCI-based fbdev drivers failed to remove firmware
devices until now; and therefore probably haven't worked correctly for
some time.

Patch 11 removes the implementation of fbdev ownership management.

The patchset has been tested by handing over device ownership between
firmware and native drivers of DRM and fbdev in various combinations.

Thomas Zimmermann (11):
  fbdev: Remove trailing whitespaces
  fbdev/vga16fb: Create EGA/VGA devices in sysfb code
  fbdev/vga16fb: Auto-generate module init/exit code
  fbdev/core: Remove remove_conflicting_pci_framebuffers()
  fbdev: Convert drivers to aperture helpers
  fbdev: Remove conflicting devices on PCI bus
  video/aperture: Disable and unregister sysfb devices via aperture
    helpers
  video: Provide constants for VGA I/O range
  video/aperture: Remove conflicting VGA devices, if any
  fbdev: Acquire framebuffer apertures for firmware devices
  fbdev: Remove conflict-handling code

 drivers/firmware/sysfb.c                     |   4 +
 drivers/staging/sm750fb/sm750.c              |  15 +-
 drivers/video/aperture.c                     |  69 ++--
 drivers/video/fbdev/arkfb.c                  |   5 +
 drivers/video/fbdev/asiliantfb.c             |   5 +
 drivers/video/fbdev/aty/aty128fb.c           |  57 ++--
 drivers/video/fbdev/aty/atyfb_base.c         |   7 +-
 drivers/video/fbdev/aty/radeon_base.c        |  83 +++--
 drivers/video/fbdev/carminefb.c              |   5 +
 drivers/video/fbdev/chipsfb.c                |  13 +-
 drivers/video/fbdev/cirrusfb.c               |   5 +
 drivers/video/fbdev/core/fbmem.c             | 176 ++---------
 drivers/video/fbdev/cyber2000fb.c            |   5 +
 drivers/video/fbdev/geode/gx1fb_core.c       |   5 +
 drivers/video/fbdev/geode/gxfb_core.c        |   5 +
 drivers/video/fbdev/geode/lxfb_core.c        |   5 +
 drivers/video/fbdev/gxt4500.c                |   5 +
 drivers/video/fbdev/hyperv_fb.c              |   6 +-
 drivers/video/fbdev/i740fb.c                 |   5 +
 drivers/video/fbdev/i810/i810_main.c         | 315 ++++++++++---------
 drivers/video/fbdev/imsttfb.c                |  36 ++-
 drivers/video/fbdev/intelfb/intelfbdrv.c     |   5 +
 drivers/video/fbdev/kyro/fbdev.c             |   5 +
 drivers/video/fbdev/matrox/matroxfb_base.c   |   5 +
 drivers/video/fbdev/mb862xx/mb862xxfbdrv.c   |   5 +
 drivers/video/fbdev/neofb.c                  |  41 +--
 drivers/video/fbdev/nvidia/nvidia.c          |   7 +-
 drivers/video/fbdev/pm2fb.c                  |   5 +
 drivers/video/fbdev/pm3fb.c                  |   5 +
 drivers/video/fbdev/pvr2fb.c                 |   5 +
 drivers/video/fbdev/riva/fbdev.c             |  67 ++--
 drivers/video/fbdev/s3fb.c                   |   5 +
 drivers/video/fbdev/savage/savagefb_driver.c |   5 +
 drivers/video/fbdev/sis/sis_main.c           |   5 +
 drivers/video/fbdev/skeletonfb.c             | 210 +++++++------
 drivers/video/fbdev/sm712fb.c                |   5 +
 drivers/video/fbdev/sstfb.c                  |  43 +--
 drivers/video/fbdev/sunxvr2500.c             |   5 +
 drivers/video/fbdev/sunxvr500.c              |   5 +
 drivers/video/fbdev/tdfxfb.c                 |   5 +
 drivers/video/fbdev/tgafb.c                  |  17 +-
 drivers/video/fbdev/tridentfb.c              |   5 +
 drivers/video/fbdev/vermilion/vermilion.c    |   7 +-
 drivers/video/fbdev/vga16fb.c                | 191 +++++------
 drivers/video/fbdev/via/via-core.c           |   5 +
 drivers/video/fbdev/vt8623fb.c               |   5 +
 include/linux/fb.h                           |   4 -
 include/video/vga.h                          |  20 +-
 48 files changed, 788 insertions(+), 735 deletions(-)


base-commit: 11d480026e922adacd274306728adb6df6dd262a
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
-- 
2.36.1

[PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

[Thomas Zimmermann]

Move the device-creation from vga16fb to sysfb code. Move the few
extra videomode checks into vga16fb's probe function.

The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC
or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except
for some MIPS systems. It's not clear if the vga16fb driver actually
works in practice.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/firmware/sysfb.c      |  4 +++
 drivers/video/fbdev/vga16fb.c | 59 +++++++++++++++++------------------
 2 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 1f276f108cc9..3fd3563d962b 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -94,6 +94,10 @@ static __init int sysfb_init(void)
 		name = "efi-framebuffer";
 	else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
 		name = "vesa-framebuffer";
+	else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
+		name = "vga-framebuffer";
+	else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
+		name = "ega-framebuffer";
 	else
 		name = "platform-framebuffer";
 
diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
index faf76972114d..f7c1bb018843 100644
--- a/drivers/video/fbdev/vga16fb.c
+++ b/drivers/video/fbdev/vga16fb.c
@@ -185,19 +185,19 @@ static inline void setindex(int index)
 }
 
 /* Check if the video mode is supported by the driver */
-static inline int check_mode_supported(void)
+static inline int check_mode_supported(const struct screen_info *si)
 {
 	/* non-x86 architectures treat orig_video_isVGA as a boolean flag */
 #if defined(CONFIG_X86)
 	/* only EGA and VGA in 16 color graphic mode are supported */
-	if (screen_info.orig_video_isVGA != VIDEO_TYPE_EGAC &&
-	    screen_info.orig_video_isVGA != VIDEO_TYPE_VGAC)
+	if (si->orig_video_isVGA != VIDEO_TYPE_EGAC &&
+	    si->orig_video_isVGA != VIDEO_TYPE_VGAC)
 		return -ENODEV;
 
-	if (screen_info.orig_video_mode != 0x0D &&	/* 320x200/4 (EGA) */
-	    screen_info.orig_video_mode != 0x0E &&	/* 640x200/4 (EGA) */
-	    screen_info.orig_video_mode != 0x10 &&	/* 640x350/4 (EGA) */
-	    screen_info.orig_video_mode != 0x12)	/* 640x480/4 (VGA) */
+	if (si->orig_video_mode != 0x0D &&	/* 320x200/4 (EGA) */
+	    si->orig_video_mode != 0x0E &&	/* 640x200/4 (EGA) */
+	    si->orig_video_mode != 0x10 &&	/* 640x350/4 (EGA) */
+	    si->orig_video_mode != 0x12)	/* 640x480/4 (VGA) */
 		return -ENODEV;
 #endif
 	return 0;
@@ -1321,11 +1321,20 @@ static int __init vga16fb_setup(char *options)
 
 static int vga16fb_probe(struct platform_device *dev)
 {
+	struct screen_info *si;
 	struct fb_info *info;
 	struct vga16fb_par *par;
 	int i;
 	int ret = 0;
 
+	si = dev_get_platdata(&dev->dev);
+	if (!si)
+		return -ENODEV;
+
+	ret = check_mode_supported(si);
+	if (ret)
+		return ret;
+
 	printk(KERN_DEBUG "vga16fb: initializing\n");
 	info = framebuffer_alloc(sizeof(struct vga16fb_par), &dev->dev);
 
@@ -1352,10 +1361,10 @@ static int vga16fb_probe(struct platform_device *dev)
 	par = info->par;
 
 #if defined(CONFIG_X86)
-	par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC;
+	par->isVGA = si->orig_video_isVGA == VIDEO_TYPE_VGAC;
 #else
 	/* non-x86 architectures treat orig_video_isVGA as a boolean flag */
-	par->isVGA = screen_info.orig_video_isVGA;
+	par->isVGA = si->orig_video_isVGA;
 #endif
 	par->palette_blanked = 0;
 	par->vesa_blanked = 0;
@@ -1425,16 +1434,21 @@ static int vga16fb_remove(struct platform_device *dev)
 	return 0;
 }
 
+static const struct platform_device_id vga16fb_driver_id_table[] = {
+	{"ega-framebuffer", 0},
+	{"vga-framebuffer", 0},
+	{ }
+};
+
 static struct platform_driver vga16fb_driver = {
 	.probe = vga16fb_probe,
 	.remove = vga16fb_remove,
 	.driver = {
-		.name = "vga16fb",
+		.name = "vga-framebuffer",
 	},
+	.id_table = vga16fb_driver_id_table,
 };
 
-static struct platform_device *vga16fb_device;
-
 static int __init vga16fb_init(void)
 {
 	int ret;
@@ -1447,32 +1461,15 @@ static int __init vga16fb_init(void)
 	vga16fb_setup(option);
 #endif
 
-	ret = check_mode_supported();
+	ret = platform_driver_register(&vga16fb_driver);
 	if (ret)
 		return ret;
 
-	ret = platform_driver_register(&vga16fb_driver);
-
-	if (!ret) {
-		vga16fb_device = platform_device_alloc("vga16fb", 0);
-
-		if (vga16fb_device)
-			ret = platform_device_add(vga16fb_device);
-		else
-			ret = -ENOMEM;
-
-		if (ret) {
-			platform_device_put(vga16fb_device);
-			platform_driver_unregister(&vga16fb_driver);
-		}
-	}
-
-	return ret;
+	return 0;
 }
 
 static void __exit vga16fb_exit(void)
 {
-	platform_device_unregister(vga16fb_device);
 	platform_driver_unregister(&vga16fb_driver);
 }
 
-- 
2.36.1

[PATCH 03/11] fbdev/vga16fb: Auto-generate module init/exit code

[Thomas Zimmermann]

Move vgag16fb's option parsing into the driver's probe function and
generate the rest of the module's init/exit functions from macros.
Keep the options code, although there are no options defined.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/vga16fb.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
index f7c1bb018843..e7767ed50c5b 100644
--- a/drivers/video/fbdev/vga16fb.c
+++ b/drivers/video/fbdev/vga16fb.c
@@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options)
 
 static int vga16fb_probe(struct platform_device *dev)
 {
+#ifndef MODULE
+	char *option = NULL;
+#endif
 	struct screen_info *si;
 	struct fb_info *info;
 	struct vga16fb_par *par;
 	int i;
 	int ret = 0;
 
+#ifndef MODULE
+	if (fb_get_options("vga16fb", &option))
+		return -ENODEV;
+	vga16fb_setup(option);
+#endif
+
 	si = dev_get_platdata(&dev->dev);
 	if (!si)
 		return -ENODEV;
@@ -1449,31 +1458,7 @@ static struct platform_driver vga16fb_driver = {
 	.id_table = vga16fb_driver_id_table,
 };
 
-static int __init vga16fb_init(void)
-{
-	int ret;
-#ifndef MODULE
-	char *option = NULL;
-
-	if (fb_get_options("vga16fb", &option))
-		return -ENODEV;
-
-	vga16fb_setup(option);
-#endif
-
-	ret = platform_driver_register(&vga16fb_driver);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
-static void __exit vga16fb_exit(void)
-{
-	platform_driver_unregister(&vga16fb_driver);
-}
+module_platform_driver(vga16fb_driver);
 
 MODULE_DESCRIPTION("Legacy VGA framebuffer device driver");
 MODULE_LICENSE("GPL");
-module_init(vga16fb_init);
-module_exit(vga16fb_exit);
-- 
2.36.1

[PATCH 04/11] fbdev/core: Remove remove_conflicting_pci_framebuffers()

[Thomas Zimmermann]

Remove remove_conflicting_pci_framebuffers() and implement similar
functionality in aperture_remove_conflicting_pci_device(), which was
the only caller. Removes an otherwise unused interface and streamlines
the aperture helper. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/aperture.c         | 30 ++++++++++++--------
 drivers/video/fbdev/core/fbmem.c | 48 --------------------------------
 include/linux/fb.h               |  2 --
 3 files changed, 18 insertions(+), 62 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 538f2d40acda..f42a0d8bc211 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -321,30 +321,36 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
  */
 int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
 {
+	bool primary = false;
 	resource_size_t base, size;
 	int bar, ret;
 
-	/*
-	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
-	 * otherwise the vga fbdev driver falls over.
-	 */
-#if IS_REACHABLE(CONFIG_FB)
-	ret = remove_conflicting_pci_framebuffers(pdev, name);
-	if (ret)
-		return ret;
+#ifdef CONFIG_X86
+	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
 #endif
-	ret = vga_remove_vgacon(pdev);
-	if (ret)
-		return ret;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
 		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
 			continue;
+
 		base = pci_resource_start(pdev, bar);
 		size = pci_resource_len(pdev, bar);
-		aperture_detach_devices(base, size);
+		ret = aperture_remove_conflicting_devices(base, size, primary, name);
+		if (ret)
+			break;
 	}
 
+	if (ret)
+		return ret;
+
+	/*
+	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
+	 * otherwise the vga fbdev driver falls over.
+	 */
+	ret = vga_remove_vgacon(pdev);
+	if (ret)
+		return ret;
+
 	return 0;
 
 }
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index c4a18322dee9..877de85309f3 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1787,54 +1787,6 @@ int remove_conflicting_framebuffers(struct apertures_struct *a,
 }
 EXPORT_SYMBOL(remove_conflicting_framebuffers);
 
-/**
- * remove_conflicting_pci_framebuffers - remove firmware-configured framebuffers for PCI devices
- * @pdev: PCI device
- * @name: requesting driver name
- *
- * This function removes framebuffer devices (eg. initialized by firmware)
- * using memory range configured for any of @pdev's memory bars.
- *
- * The function assumes that PCI device with shadowed ROM drives a primary
- * display and so kicks out vga16fb.
- */
-int remove_conflicting_pci_framebuffers(struct pci_dev *pdev, const char *name)
-{
-	struct apertures_struct *ap;
-	bool primary = false;
-	int err, idx, bar;
-
-	for (idx = 0, bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
-			continue;
-		idx++;
-	}
-
-	ap = alloc_apertures(idx);
-	if (!ap)
-		return -ENOMEM;
-
-	for (idx = 0, bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
-			continue;
-		ap->ranges[idx].base = pci_resource_start(pdev, bar);
-		ap->ranges[idx].size = pci_resource_len(pdev, bar);
-		pci_dbg(pdev, "%s: bar %d: 0x%lx -> 0x%lx\n", __func__, bar,
-			(unsigned long)pci_resource_start(pdev, bar),
-			(unsigned long)pci_resource_end(pdev, bar));
-		idx++;
-	}
-
-#ifdef CONFIG_X86
-	primary = pdev->resource[PCI_ROM_RESOURCE].flags &
-					IORESOURCE_ROM_SHADOW;
-#endif
-	err = remove_conflicting_framebuffers(ap, name, primary);
-	kfree(ap);
-	return err;
-}
-EXPORT_SYMBOL(remove_conflicting_pci_framebuffers);
-
 /**
  *	register_framebuffer - registers a frame buffer device
  *	@fb_info: frame buffer info structure
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 07fcd0e56682..b91c77016560 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -615,8 +615,6 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 /* drivers/video/fbmem.c */
 extern int register_framebuffer(struct fb_info *fb_info);
 extern void unregister_framebuffer(struct fb_info *fb_info);
-extern int remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
-					       const char *name);
 extern int remove_conflicting_framebuffers(struct apertures_struct *a,
 					   const char *name, bool primary);
 extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);
-- 
2.36.1

[PATCH 05/11] fbdev: Convert drivers to aperture helpers

[Thomas Zimmermann]

Convert fbdev drivers from fbdev's remove_conflicting_framebuffers() to
the framework-independent aperture_remove_conflicting_devices(). Calling
this function will also remove conflicting DRM drivers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/staging/sm750fb/sm750.c       | 15 +++++----------
 drivers/video/fbdev/aty/radeon_base.c | 17 ++++-------------
 drivers/video/fbdev/hyperv_fb.c       |  6 ++++--
 3 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index dbd1159a2ef0..ce04c38f6afd 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <linux/aperture.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/errno.h>
@@ -987,22 +988,16 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
 
 static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev)
 {
-	struct apertures_struct *ap;
+	resource_size_t base = pci_resource_start(pdev, 0);
+	resource_size_t size = pci_resource_len(pdev, 0);
 	bool primary = false;
 
-	ap = alloc_apertures(1);
-	if (!ap)
-		return -ENOMEM;
-
-	ap->ranges[0].base = pci_resource_start(pdev, 0);
-	ap->ranges[0].size = pci_resource_len(pdev, 0);
 #ifdef CONFIG_X86
 	primary = pdev->resource[PCI_ROM_RESOURCE].flags &
 					IORESOURCE_ROM_SHADOW;
 #endif
-	remove_conflicting_framebuffers(ap, "sm750_fb1", primary);
-	kfree(ap);
-	return 0;
+
+	return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");
 }
 
 static int lynxfb_pci_probe(struct pci_dev *pdev,
diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
index b311c07fe66d..e5e362b8c9da 100644
--- a/drivers/video/fbdev/aty/radeon_base.c
+++ b/drivers/video/fbdev/aty/radeon_base.c
@@ -54,6 +54,7 @@
 
 #include "radeonfb.h"
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
@@ -2239,20 +2240,10 @@ static const struct bin_attribute edid2_attr = {
 
 static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
 {
-	struct apertures_struct *ap;
+	resource_size_t base = pci_resource_start(pdev, 0);
+	resource_size_t size = pci_resource_len(pdev, 0);
 
-	ap = alloc_apertures(1);
-	if (!ap)
-		return -ENOMEM;
-
-	ap->ranges[0].base = pci_resource_start(pdev, 0);
-	ap->ranges[0].size = pci_resource_len(pdev, 0);
-
-	remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
-
-	kfree(ap);
-
-	return 0;
+	return aperture_remove_conflicting_devices(base, size, KBUILD_MODNAME, false);
 }
 
 static int radeonfb_pci_register(struct pci_dev *pdev,
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 886c564787f1..a944a6620527 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -45,6 +45,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/vmalloc.h>
@@ -1074,8 +1075,9 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 	info->screen_size = dio_fb_size;
 
 getmem_done:
-	remove_conflicting_framebuffers(info->apertures,
-					KBUILD_MODNAME, false);
+	aperture_remove_conflicting_devices(info->apertures->ranges[0].base,
+					    info->apertures->ranges[0].size,
+					    KBUILD_MODNAME, false);
 
 	if (gen2vm) {
 		/* framebuffer is reallocated, clear screen_info to avoid misuse from kexec */
-- 
2.36.1

[PATCH 06/11] fbdev: Remove conflicting devices on PCI bus

[Thomas Zimmermann]

Remove firmware devices on the PCI bus, by calling
aperture_remove_conflicting_pci_devices() in the probe function of
each related fbdev driver. iSo far, most of these drivers did not
remove conflicting VESA or EFI devices, or outride failed for
resource conflicts (i.e., matroxfb.) This must have been broken
for quite some time.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/arkfb.c                  | 5 +++++
 drivers/video/fbdev/asiliantfb.c             | 5 +++++
 drivers/video/fbdev/aty/aty128fb.c           | 5 +++++
 drivers/video/fbdev/aty/atyfb_base.c         | 7 ++++++-
 drivers/video/fbdev/carminefb.c              | 5 +++++
 drivers/video/fbdev/chipsfb.c                | 7 ++++++-
 drivers/video/fbdev/cirrusfb.c               | 5 +++++
 drivers/video/fbdev/cyber2000fb.c            | 5 +++++
 drivers/video/fbdev/geode/gx1fb_core.c       | 5 +++++
 drivers/video/fbdev/geode/gxfb_core.c        | 5 +++++
 drivers/video/fbdev/geode/lxfb_core.c        | 5 +++++
 drivers/video/fbdev/gxt4500.c                | 5 +++++
 drivers/video/fbdev/i740fb.c                 | 5 +++++
 drivers/video/fbdev/i810/i810_main.c         | 5 +++++
 drivers/video/fbdev/imsttfb.c                | 8 +++++++-
 drivers/video/fbdev/intelfb/intelfbdrv.c     | 5 +++++
 drivers/video/fbdev/kyro/fbdev.c             | 5 +++++
 drivers/video/fbdev/matrox/matroxfb_base.c   | 5 +++++
 drivers/video/fbdev/mb862xx/mb862xxfbdrv.c   | 5 +++++
 drivers/video/fbdev/neofb.c                  | 5 +++++
 drivers/video/fbdev/nvidia/nvidia.c          | 7 ++++++-
 drivers/video/fbdev/pm2fb.c                  | 5 +++++
 drivers/video/fbdev/pm3fb.c                  | 5 +++++
 drivers/video/fbdev/pvr2fb.c                 | 5 +++++
 drivers/video/fbdev/riva/fbdev.c             | 5 +++++
 drivers/video/fbdev/s3fb.c                   | 5 +++++
 drivers/video/fbdev/savage/savagefb_driver.c | 5 +++++
 drivers/video/fbdev/sis/sis_main.c           | 5 +++++
 drivers/video/fbdev/skeletonfb.c             | 8 ++++++++
 drivers/video/fbdev/sm712fb.c                | 5 +++++
 drivers/video/fbdev/sstfb.c                  | 5 +++++
 drivers/video/fbdev/sunxvr2500.c             | 5 +++++
 drivers/video/fbdev/sunxvr500.c              | 5 +++++
 drivers/video/fbdev/tdfxfb.c                 | 5 +++++
 drivers/video/fbdev/tgafb.c                  | 7 +++++++
 drivers/video/fbdev/tridentfb.c              | 5 +++++
 drivers/video/fbdev/vermilion/vermilion.c    | 7 ++++++-
 drivers/video/fbdev/via/via-core.c           | 5 +++++
 drivers/video/fbdev/vt8623fb.c               | 5 +++++
 39 files changed, 206 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
index eb3e47c58c5f..453daa072f53 100644
--- a/drivers/video/fbdev/arkfb.c
+++ b/drivers/video/fbdev/arkfb.c
@@ -11,6 +11,7 @@
  *  Code is based on s3fb
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -949,6 +950,10 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	int rc;
 	u8 regval;
 
+	rc = aperture_remove_conflicting_pci_devices(dev, "arkfb");
+	if (rc < 0)
+		return rc;
+
 	/* Ignore secondary VGA device because there is no VGA arbitration */
 	if (! svga_primary_device(dev)) {
 		dev_info(&(dev->dev), "ignoring secondary device\n");
diff --git a/drivers/video/fbdev/asiliantfb.c b/drivers/video/fbdev/asiliantfb.c
index f8ef62542f7f..3818437a8f69 100644
--- a/drivers/video/fbdev/asiliantfb.c
+++ b/drivers/video/fbdev/asiliantfb.c
@@ -29,6 +29,7 @@
  *  more details.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -545,6 +546,10 @@ static int asiliantfb_pci_init(struct pci_dev *dp,
 	struct fb_info *p;
 	int err;
 
+	err = aperture_remove_conflicting_pci_devices(dp, "asiliantfb");
+	if (err)
+		return err;
+
 	if ((dp->resource[0].flags & IORESOURCE_MEM) == 0)
 		return -ENODEV;
 	addr = pci_resource_start(dp, 0);
diff --git a/drivers/video/fbdev/aty/aty128fb.c b/drivers/video/fbdev/aty/aty128fb.c
index 5cdbbba2a013..57e398fe7a81 100644
--- a/drivers/video/fbdev/aty/aty128fb.c
+++ b/drivers/video/fbdev/aty/aty128fb.c
@@ -47,6 +47,7 @@
  */
 
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
@@ -2055,6 +2056,10 @@ static int aty128_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	void __iomem *bios = NULL;
 #endif
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "aty128fb");
+	if (err)
+		return err;
+
 	/* Enable device in PCI config */
 	if ((err = pci_enable_device(pdev))) {
 		printk(KERN_ERR "aty128fb: Cannot enable PCI device: %d\n",
diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
index a3e6faed7745..4804b6e9f3f4 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -48,6 +48,7 @@
 
 ******************************************************************************/
 
+#include <linux/aperture.h>
 #include <linux/compat.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -3533,7 +3534,11 @@ static int atyfb_pci_probe(struct pci_dev *pdev,
 	struct fb_info *info;
 	struct resource *rp;
 	struct atyfb_par *par;
-	int rc = -ENOMEM;
+	int rc;
+
+	rc = aperture_remove_conflicting_pci_devices(pdev, "atyfb");
+	if (rc)
+		return rc;
 
 	/* Enable device in PCI config */
 	if (pci_enable_device(pdev)) {
diff --git a/drivers/video/fbdev/carminefb.c b/drivers/video/fbdev/carminefb.c
index 3a1c2e0739a1..4651b48a87f9 100644
--- a/drivers/video/fbdev/carminefb.c
+++ b/drivers/video/fbdev/carminefb.c
@@ -7,6 +7,7 @@
  * - FB1 is display 1 with unique memory area
  * - both display use 32 bit colors
  */
+#include <linux/aperture.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/fb.h>
@@ -614,6 +615,10 @@ static int carminefb_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 	struct fb_info *info;
 	int ret;
 
+	ret = aperture_remove_conflicting_pci_devices(dev, "carminefb");
+	if (ret)
+		return ret;
+
 	ret = pci_enable_device(dev);
 	if (ret)
 		return ret;
diff --git a/drivers/video/fbdev/chipsfb.c b/drivers/video/fbdev/chipsfb.c
index 618fb6dbbedb..5ad64714d39e 100644
--- a/drivers/video/fbdev/chipsfb.c
+++ b/drivers/video/fbdev/chipsfb.c
@@ -14,6 +14,7 @@
  *  more details.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -351,7 +352,11 @@ static int chipsfb_pci_init(struct pci_dev *dp, const struct pci_device_id *ent)
 	struct fb_info *p;
 	unsigned long addr;
 	unsigned short cmd;
-	int rc = -ENODEV;
+	int rc;
+
+	rc = aperture_remove_conflicting_pci_devices(dp, "chipsfb");
+	if (rc)
+		return rc;
 
 	if (pci_enable_device(dp) < 0) {
 		dev_err(&dp->dev, "Cannot enable PCI device\n");
diff --git a/drivers/video/fbdev/cirrusfb.c b/drivers/video/fbdev/cirrusfb.c
index 51e072c03e1c..4ff6f624f912 100644
--- a/drivers/video/fbdev/cirrusfb.c
+++ b/drivers/video/fbdev/cirrusfb.c
@@ -34,6 +34,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -2085,6 +2086,10 @@ static int cirrusfb_pci_register(struct pci_dev *pdev,
 	unsigned long board_addr, board_size;
 	int ret;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "cirrusfb");
+	if (ret)
+		return ret;
+
 	ret = pci_enable_device(pdev);
 	if (ret < 0) {
 		printk(KERN_ERR "cirrusfb: Cannot enable PCI device\n");
diff --git a/drivers/video/fbdev/cyber2000fb.c b/drivers/video/fbdev/cyber2000fb.c
index d45355b9a58c..be7bcf95c96a 100644
--- a/drivers/video/fbdev/cyber2000fb.c
+++ b/drivers/video/fbdev/cyber2000fb.c
@@ -33,6 +33,7 @@
  * (which, incidentally, is about the same saving as a 2.5in hard disk
  * entering standby mode.)
  */
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1720,6 +1721,10 @@ static int cyberpro_pci_probe(struct pci_dev *dev,
 
 	sprintf(name, "CyberPro%4X", id->device);
 
+	err = aperture_remove_conflicting_pci_devices(dev, name);
+	if (err)
+		return err;
+
 	err = pci_enable_device(dev);
 	if (err)
 		return err;
diff --git a/drivers/video/fbdev/geode/gx1fb_core.c b/drivers/video/fbdev/geode/gx1fb_core.c
index 5d34d89fb665..4cac7e3bb1a0 100644
--- a/drivers/video/fbdev/geode/gx1fb_core.c
+++ b/drivers/video/fbdev/geode/gx1fb_core.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2005 Arcom Control Systems Ltd.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -320,6 +321,10 @@ static int gx1fb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct fb_info *info;
 	int ret;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "gx1fb");
+	if (ret)
+		return ret;
+
 	info = gx1fb_init_fbinfo(&pdev->dev);
 	if (!info)
 		return -ENOMEM;
diff --git a/drivers/video/fbdev/geode/gxfb_core.c b/drivers/video/fbdev/geode/gxfb_core.c
index 44089b331f91..2527bd80ec5f 100644
--- a/drivers/video/fbdev/geode/gxfb_core.c
+++ b/drivers/video/fbdev/geode/gxfb_core.c
@@ -15,6 +15,7 @@
  *
  * 16 MiB of framebuffer memory is assumed to be available.
  */
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -364,6 +365,10 @@ static int gxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct fb_videomode *modedb_ptr;
 	unsigned int modedb_size;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "gxfb");
+	if (ret)
+		return ret;
+
 	info = gxfb_init_fbinfo(&pdev->dev);
 	if (!info)
 		return -ENOMEM;
diff --git a/drivers/video/fbdev/geode/lxfb_core.c b/drivers/video/fbdev/geode/lxfb_core.c
index 66c81262d18f..9d26592dbfce 100644
--- a/drivers/video/fbdev/geode/lxfb_core.c
+++ b/drivers/video/fbdev/geode/lxfb_core.c
@@ -6,6 +6,7 @@
  * Built from gxfb (which is Copyright (C) 2006 Arcom Control Systems Ltd.)
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -484,6 +485,10 @@ static int lxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct fb_videomode *modedb_ptr;
 	unsigned int modedb_size;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "lxfb");
+	if (ret)
+		return ret;
+
 	info = lxfb_init_fbinfo(&pdev->dev);
 
 	if (info == NULL)
diff --git a/drivers/video/fbdev/gxt4500.c b/drivers/video/fbdev/gxt4500.c
index e5475ae1e158..f0d36a4fdaef 100644
--- a/drivers/video/fbdev/gxt4500.c
+++ b/drivers/video/fbdev/gxt4500.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2006 Paul Mackerras, IBM Corp. <paulus@samba.org>
  */
 
+#include <linux/aperture.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/fb.h>
@@ -621,6 +622,10 @@ static int gxt4500_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct fb_var_screeninfo var;
 	enum gxt_cards cardtype;
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "gxt4500fb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(pdev);
 	if (err) {
 		dev_err(&pdev->dev, "gxt4500: cannot enable PCI device: %d\n",
diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c
index 09dd85553d4f..23329de28e77 100644
--- a/drivers/video/fbdev/i740fb.c
+++ b/drivers/video/fbdev/i740fb.c
@@ -12,6 +12,7 @@
  *  i740fb by Patrick LERDA, v0.9
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1008,6 +1009,10 @@ static int i740fb_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 	bool found = false;
 	u8 *edid;
 
+	ret = aperture_remove_conflicting_pci_devices(dev, "i740fb");
+	if (ret)
+		return ret;
+
 	info = framebuffer_alloc(sizeof(struct i740fb_par), &(dev->dev));
 	if (!info)
 		return -ENOMEM;
diff --git a/drivers/video/fbdev/i810/i810_main.c b/drivers/video/fbdev/i810/i810_main.c
index 41a86efca516..ff09f8c20bfc 100644
--- a/drivers/video/fbdev/i810/i810_main.c
+++ b/drivers/video/fbdev/i810/i810_main.c
@@ -28,6 +28,7 @@
  *  more details.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -2016,6 +2017,10 @@ static int i810fb_init_pci(struct pci_dev *dev,
 	struct fb_videomode mode;
 	int err = -1, vfreq, hfreq, pixclock;
 
+	err = aperture_remove_conflicting_pci_devices(dev, "i810fb");
+	if (err)
+		return err;
+
 	info = framebuffer_alloc(sizeof(struct i810fb_par), &dev->dev);
 	if (!info)
 		return -ENOMEM;
diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index f489386855c0..d7edb9c5d3a3 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -16,6 +16,7 @@
  *  more details.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1469,7 +1470,12 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct imstt_par *par;
 	struct fb_info *info;
 	struct device_node *dp;
-	int ret = -ENOMEM;
+	int ret;
+
+	ret = aperture_remove_conflicting_pci_devices(pdev, "imsttfb");
+	if (ret)
+		return ret;
+	ret = -ENOMEM;
 
 	dp = pci_device_to_OF_node(pdev);
 	if(dp)
diff --git a/drivers/video/fbdev/intelfb/intelfbdrv.c b/drivers/video/fbdev/intelfb/intelfbdrv.c
index 5647fca8c49a..d4a2891a9a7a 100644
--- a/drivers/video/fbdev/intelfb/intelfbdrv.c
+++ b/drivers/video/fbdev/intelfb/intelfbdrv.c
@@ -107,6 +107,7 @@
  *              Add support for 945GME. (Phil Endecott <spam_from_intelfb@chezphil.org>)
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -483,6 +484,10 @@ static int intelfb_pci_register(struct pci_dev *pdev,
 
 	DBG_MSG("intelfb_pci_register\n");
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "intelfb");
+	if (err)
+		return err;
+
 	num_registered++;
 	if (num_registered != 1) {
 		ERR_MSG("Attempted to register %d devices "
diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
index d57772f96ad2..b4b93054c520 100644
--- a/drivers/video/fbdev/kyro/fbdev.c
+++ b/drivers/video/fbdev/kyro/fbdev.c
@@ -9,6 +9,7 @@
  * for more details.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -676,6 +677,10 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	unsigned long size;
 	int err;
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "kyrofb");
+	if (err)
+		return err;
+
 	if ((err = pci_enable_device(pdev))) {
 		printk(KERN_WARNING "kyrofb: Can't enable pdev: %d\n", err);
 		return err;
diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c
index 236521b19daf..3e26346c05a2 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.c
+++ b/drivers/video/fbdev/matrox/matroxfb_base.c
@@ -100,6 +100,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/version.h>
 
 #include "matroxfb_base.h"
@@ -2044,6 +2045,10 @@ static int matroxfb_probe(struct pci_dev* pdev, const struct pci_device_id* dumm
 	u_int32_t cmd;
 	DBG(__func__)
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "matroxfb");
+	if (err)
+		return err;
+
 	svid = pdev->subsystem_vendor;
 	sid = pdev->subsystem_device;
 	for (b = dev_list; b->vendor; b++) {
diff --git a/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c b/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
index a7508f5be343..96800c9c9cd9 100644
--- a/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
+++ b/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
@@ -10,6 +10,7 @@
 
 #undef DEBUG
 
+#include <linux/aperture.h>
 #include <linux/fb.h>
 #include <linux/delay.h>
 #include <linux/uaccess.h>
@@ -999,6 +1000,10 @@ static int mb862xx_pci_probe(struct pci_dev *pdev,
 	struct device *dev = &pdev->dev;
 	int ret;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "mb862xxfb");
+	if (ret)
+		return ret;
+
 	ret = pci_enable_device(pdev);
 	if (ret < 0) {
 		dev_err(dev, "Cannot enable PCI device\n");
diff --git a/drivers/video/fbdev/neofb.c b/drivers/video/fbdev/neofb.c
index 8ad2a79623af..93a2d2d1abe8 100644
--- a/drivers/video/fbdev/neofb.c
+++ b/drivers/video/fbdev/neofb.c
@@ -54,6 +54,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -2029,6 +2030,10 @@ static int neofb_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	DBG("neofb_probe");
 
+	err = aperture_remove_conflicting_pci_devices(dev, "neofb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(dev);
 	if (err)
 		return err;
diff --git a/drivers/video/fbdev/nvidia/nvidia.c b/drivers/video/fbdev/nvidia/nvidia.c
index a372a183c1f0..329e2e8133c6 100644
--- a/drivers/video/fbdev/nvidia/nvidia.c
+++ b/drivers/video/fbdev/nvidia/nvidia.c
@@ -9,6 +9,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1276,11 +1277,15 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
 	struct nvidia_par *par;
 	struct fb_info *info;
 	unsigned short cmd;
-
+	int ret;
 
 	NVTRACE_ENTER();
 	assert(pd != NULL);
 
+	ret = aperture_remove_conflicting_pci_devices(pd, "nvidiafb");
+	if (ret)
+		return ret;
+
 	info = framebuffer_alloc(sizeof(struct nvidia_par), &pd->dev);
 
 	if (!info)
diff --git a/drivers/video/fbdev/pm2fb.c b/drivers/video/fbdev/pm2fb.c
index d3be2c64f1c0..bc80d8498aeb 100644
--- a/drivers/video/fbdev/pm2fb.c
+++ b/drivers/video/fbdev/pm2fb.c
@@ -27,6 +27,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
@@ -1516,6 +1517,10 @@ static int pm2fb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	int err;
 	int retval = -ENXIO;
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "pm2fb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(pdev);
 	if (err) {
 		printk(KERN_WARNING "pm2fb: Can't enable pdev: %d\n", err);
diff --git a/drivers/video/fbdev/pm3fb.c b/drivers/video/fbdev/pm3fb.c
index a8faf46adeb1..ba69846d444f 100644
--- a/drivers/video/fbdev/pm3fb.c
+++ b/drivers/video/fbdev/pm3fb.c
@@ -22,6 +22,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1315,6 +1316,10 @@ static int pm3fb_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 	int err;
 	int retval = -ENXIO;
 
+	err = aperture_remove_conflicting_pci_devices(dev, "pm3fb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(dev);
 	if (err) {
 		printk(KERN_WARNING "pm3fb: Can't enable PCI dev: %d\n", err);
diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c
index f4add36cb5f4..b73ad14efa20 100644
--- a/drivers/video/fbdev/pvr2fb.c
+++ b/drivers/video/fbdev/pvr2fb.c
@@ -45,6 +45,7 @@
 
 #undef DEBUG
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -942,6 +943,10 @@ static int pvr2fb_pci_probe(struct pci_dev *pdev,
 {
 	int ret;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "pvrfb");
+	if (ret)
+		return ret;
+
 	ret = pci_enable_device(pdev);
 	if (ret) {
 		printk(KERN_ERR "pvr2fb: PCI enable failed\n");
diff --git a/drivers/video/fbdev/riva/fbdev.c b/drivers/video/fbdev/riva/fbdev.c
index 5bafc44c591b..0ea74e28f915 100644
--- a/drivers/video/fbdev/riva/fbdev.c
+++ b/drivers/video/fbdev/riva/fbdev.c
@@ -29,6 +29,7 @@
  *	doublescan modes are broken
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1898,6 +1899,10 @@ static int rivafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
 	NVTRACE_ENTER();
 	assert(pd != NULL);
 
+	ret = aperture_remove_conflicting_pci_devices(pd, "rivafb");
+	if (ret)
+		return ret;
+
 	info = framebuffer_alloc(sizeof(struct riva_par), &pd->dev);
 	if (!info) {
 		ret = -ENOMEM;
diff --git a/drivers/video/fbdev/s3fb.c b/drivers/video/fbdev/s3fb.c
index b93c8eb02336..f66c4de0e188 100644
--- a/drivers/video/fbdev/s3fb.c
+++ b/drivers/video/fbdev/s3fb.c
@@ -11,6 +11,7 @@
  * which is based on the code of neofb.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1129,6 +1130,10 @@ static int s3_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 	}
 
+	rc = aperture_remove_conflicting_pci_devices(dev, "s3fb");
+	if (rc)
+		return rc;
+
 	/* Allocate and fill driver data structure */
 	info = framebuffer_alloc(sizeof(struct s3fb_info), &(dev->dev));
 	if (!info)
diff --git a/drivers/video/fbdev/savage/savagefb_driver.c b/drivers/video/fbdev/savage/savagefb_driver.c
index 8114c921ceb8..b7818b652698 100644
--- a/drivers/video/fbdev/savage/savagefb_driver.c
+++ b/drivers/video/fbdev/savage/savagefb_driver.c
@@ -41,6 +41,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -2176,6 +2177,10 @@ static int savagefb_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	DBG("savagefb_probe");
 
+	err = aperture_remove_conflicting_pci_devices(dev, "savagefb");
+	if (err)
+		return err;
+
 	info = framebuffer_alloc(sizeof(struct savagefb_par), &dev->dev);
 	if (!info)
 		return -ENOMEM;
diff --git a/drivers/video/fbdev/sis/sis_main.c b/drivers/video/fbdev/sis/sis_main.c
index f28fd69d5eb7..7114c5c17c91 100644
--- a/drivers/video/fbdev/sis/sis_main.c
+++ b/drivers/video/fbdev/sis/sis_main.c
@@ -19,6 +19,7 @@
  * which is (c) 1998 Gerd Knorr <kraxel@goldbach.in-berlin.de>
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
@@ -5849,6 +5850,10 @@ static int sisfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if(sisfb_off)
 		return -ENXIO;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "sisfb");
+	if (ret)
+		return ret;
+
 	sis_fb_info = framebuffer_alloc(sizeof(*ivideo), &pdev->dev);
 	if(!sis_fb_info)
 		return -ENOMEM;
diff --git a/drivers/video/fbdev/skeletonfb.c b/drivers/video/fbdev/skeletonfb.c
index 304320ce6c6f..125df366e23a 100644
--- a/drivers/video/fbdev/skeletonfb.c
+++ b/drivers/video/fbdev/skeletonfb.c
@@ -42,6 +42,7 @@
  *  more details.
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -668,6 +669,13 @@ static int xxxfb_probe(struct pci_dev *dev, const struct pci_device_id *ent)
     struct device *device = &dev->dev; /* or &pdev->dev */
     int cmap_len, retval;
 
+    /*
+     * Remove firmware-based drivers that create resource conflicts.
+     */
+    retval = aperture_remove_conflicting_pci_devices(pdev, "xxxfb");
+    if (retval)
+	    return retval;
+
     /*
      * Dynamically allocate info and par
      */
diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c
index 092a1caa1208..3baf33635e65 100644
--- a/drivers/video/fbdev/sm712fb.c
+++ b/drivers/video/fbdev/sm712fb.c
@@ -18,6 +18,7 @@
  * Framebuffer driver for Silicon Motion SM710, SM712, SM721 and SM722 chips
  */
 
+#include <linux/aperture.h>
 #include <linux/io.h>
 #include <linux/fb.h>
 #include <linux/pci.h>
@@ -1502,6 +1503,10 @@ static int smtcfb_pci_probe(struct pci_dev *pdev,
 
 	dev_info(&pdev->dev, "Silicon Motion display driver.\n");
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "smtcfb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(pdev);	/* enable SMTC chip */
 	if (err)
 		return err;
diff --git a/drivers/video/fbdev/sstfb.c b/drivers/video/fbdev/sstfb.c
index 535ef4693cd4..73ca2782ebfc 100644
--- a/drivers/video/fbdev/sstfb.c
+++ b/drivers/video/fbdev/sstfb.c
@@ -80,6 +80,7 @@
  * Includes
  */
 
+#include <linux/aperture.h>
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -1326,6 +1327,10 @@ static int sstfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct sst_spec *spec;
 	int err;
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "sstfb");
+	if (err)
+		return err;
+
 	/* Enable device in PCI config. */
 	if ((err=pci_enable_device(pdev))) {
 		printk(KERN_ERR "cannot enable device\n");
diff --git a/drivers/video/fbdev/sunxvr2500.c b/drivers/video/fbdev/sunxvr2500.c
index 1d3bacd9d5ac..81d59613ea1f 100644
--- a/drivers/video/fbdev/sunxvr2500.c
+++ b/drivers/video/fbdev/sunxvr2500.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2007 David S. Miller (davem@davemloft.net)
  */
 
+#include <linux/aperture.h>
 #include <linux/kernel.h>
 #include <linux/fb.h>
 #include <linux/pci.h>
@@ -123,6 +124,10 @@ static int s3d_pci_register(struct pci_dev *pdev,
 	struct s3d_info *sp;
 	int err;
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "s3dfb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(pdev);
 	if (err < 0) {
 		printk(KERN_ERR "s3d: Cannot enable PCI device %s\n",
diff --git a/drivers/video/fbdev/sunxvr500.c b/drivers/video/fbdev/sunxvr500.c
index 9daf17b11106..3a51b2a1480c 100644
--- a/drivers/video/fbdev/sunxvr500.c
+++ b/drivers/video/fbdev/sunxvr500.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2007 David S. Miller (davem@davemloft.net)
  */
 
+#include <linux/aperture.h>
 #include <linux/kernel.h>
 #include <linux/fb.h>
 #include <linux/pci.h>
@@ -249,6 +250,10 @@ static int e3d_pci_register(struct pci_dev *pdev,
 	unsigned int line_length;
 	int err;
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "e3dfb");
+	if (err)
+		return err;
+
 	of_node = pci_device_to_OF_node(pdev);
 	if (!of_node) {
 		printk(KERN_ERR "e3d: Cannot find OF node of %s\n",
diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index 67e37a62b07c..059e0174e139 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -64,6 +64,7 @@
  *
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -1376,6 +1377,10 @@ static int tdfxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct fb_monspecs *specs;
 	bool found;
 
+	err = aperture_remove_conflicting_pci_devices(pdev, "tdfxfb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(pdev);
 	if (err) {
 		printk(KERN_ERR "tdfxfb: Can't enable pdev: %d\n", err);
diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
index 9266c76783cc..4600138e3bef 100644
--- a/drivers/video/fbdev/tgafb.c
+++ b/drivers/video/fbdev/tgafb.c
@@ -12,6 +12,7 @@
  *  more details.
  */
 
+#include <linux/aperture.h>
 #include <linux/bitrev.h>
 #include <linux/compiler.h>
 #include <linux/delay.h>
@@ -106,6 +107,12 @@ static struct pci_driver tgafb_pci_driver = {
 static int tgafb_pci_register(struct pci_dev *pdev,
 			      const struct pci_device_id *ent)
 {
+	int ret;
+
+	ret = aperture_remove_conflicting_pci_devices(pdev, "tgafb");
+	if (ret)
+		return ret;
+
 	return tgafb_register(&pdev->dev);
 }
 
diff --git a/drivers/video/fbdev/tridentfb.c b/drivers/video/fbdev/tridentfb.c
index 319131bd72cf..6813df793c49 100644
--- a/drivers/video/fbdev/tridentfb.c
+++ b/drivers/video/fbdev/tridentfb.c
@@ -16,6 +16,7 @@
  *	timing value tweaking so it looks good on every monitor in every mode
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/fb.h>
 #include <linux/init.h>
@@ -1470,6 +1471,10 @@ static int trident_pci_probe(struct pci_dev *dev,
 	int chip_id;
 	bool found = false;
 
+	err = aperture_remove_conflicting_pci_devices(dev, "tridentfb");
+	if (err)
+		return err;
+
 	err = pci_enable_device(dev);
 	if (err)
 		return err;
diff --git a/drivers/video/fbdev/vermilion/vermilion.c b/drivers/video/fbdev/vermilion/vermilion.c
index ff61605b8764..82b36dbb5b1a 100644
--- a/drivers/video/fbdev/vermilion/vermilion.c
+++ b/drivers/video/fbdev/vermilion/vermilion.c
@@ -14,6 +14,7 @@
  *   Alan Hourihane <alanh-at-tungstengraphics-dot-com>
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -442,7 +443,11 @@ static int vml_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	struct vml_info *vinfo;
 	struct fb_info *info;
 	struct vml_par *par;
-	int err = 0;
+	int err;
+
+	err = aperture_remove_conflicting_pci_devices(dev, "vmlfb");
+	if (err)
+		return err;
 
 	par = kzalloc(sizeof(*par), GFP_KERNEL);
 	if (par == NULL)
diff --git a/drivers/video/fbdev/via/via-core.c b/drivers/video/fbdev/via/via-core.c
index 89d75079b730..2ee8fcae08df 100644
--- a/drivers/video/fbdev/via/via-core.c
+++ b/drivers/video/fbdev/via/via-core.c
@@ -8,6 +8,7 @@
 /*
  * Core code for the Via multifunction framebuffer device.
  */
+#include <linux/aperture.h>
 #include <linux/via-core.h>
 #include <linux/via_i2c.h>
 #include <linux/via-gpio.h>
@@ -617,6 +618,10 @@ static int via_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	int ret;
 
+	ret = aperture_remove_conflicting_pci_devices(pdev, "viafb");
+	if (ret)
+		return ret;
+
 	ret = pci_enable_device(pdev);
 	if (ret)
 		return ret;
diff --git a/drivers/video/fbdev/vt8623fb.c b/drivers/video/fbdev/vt8623fb.c
index a92a8c670cf0..62318cef5f8c 100644
--- a/drivers/video/fbdev/vt8623fb.c
+++ b/drivers/video/fbdev/vt8623fb.c
@@ -12,6 +12,7 @@
  * (http://davesdomain.org.uk/viafb/)
  */
 
+#include <linux/aperture.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -670,6 +671,10 @@ static int vt8623_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 	}
 
+	rc = aperture_remove_conflicting_pci_devices(dev, "vt8623fb");
+	if (rc)
+		return rc;
+
 	/* Allocate and fill driver data structure */
 	info = framebuffer_alloc(sizeof(struct vt8623fb_info), &(dev->dev));
 	if (!info)
-- 
2.36.1

[PATCH 07/11] video/aperture: Disable and unregister sysfb devices via aperture helpers

[Thomas Zimmermann]

Call sysfb_disable() before removing conflicting devices in aperture
helpers. Fixes sysfb state if fbdev has been disabled.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: fb84efa28a48 ("drm/aperture: Run fbdev removal before internal helpers")
Cc: Zack Rusin <zackr@vmware.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Helge Deller <deller@gmx.de>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Changcheng Deng <deng.changcheng@zte.com.cn>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/video/aperture.c         | 14 ++++++++++++++
 drivers/video/fbdev/core/fbmem.c | 12 ------------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index f42a0d8bc211..101e13c2cf41 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -8,6 +8,7 @@
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/sysfb.h>
 #include <linux/types.h>
 #include <linux/vgaarb.h>
 
@@ -286,7 +287,20 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
 #if IS_REACHABLE(CONFIG_FB)
 	struct apertures_struct *a;
 	int ret;
+#endif
+
+	/*
+	 * If a driver asked to unregister a platform device registered by
+	 * sysfb, then can be assumed that this is a driver for a display
+	 * that is set up by the system firmware and has a generic driver.
+	 *
+	 * Drivers for devices that don't have a generic driver will never
+	 * ask for this, so let's assume that a real driver for the display
+	 * was already probed and prevent sysfb to register devices later.
+	 */
+	sysfb_disable();
 
+#if IS_REACHABLE(CONFIG_FB)
 	a = alloc_apertures(1);
 	if (!a)
 		return -ENOMEM;
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 877de85309f3..3ee3ea018245 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -19,7 +19,6 @@
 #include <linux/kernel.h>
 #include <linux/major.h>
 #include <linux/slab.h>
-#include <linux/sysfb.h>
 #include <linux/mm.h>
 #include <linux/mman.h>
 #include <linux/vt.h>
@@ -1765,17 +1764,6 @@ int remove_conflicting_framebuffers(struct apertures_struct *a,
 		do_free = true;
 	}
 
-	/*
-	 * If a driver asked to unregister a platform device registered by
-	 * sysfb, then can be assumed that this is a driver for a display
-	 * that is set up by the system firmware and has a generic driver.
-	 *
-	 * Drivers for devices that don't have a generic driver will never
-	 * ask for this, so let's assume that a real driver for the display
-	 * was already probed and prevent sysfb to register devices later.
-	 */
-	sysfb_disable();
-
 	mutex_lock(&registration_lock);
 	do_remove_conflicting_framebuffers(a, name, primary);
 	mutex_unlock(&registration_lock);
-- 
2.36.1

[PATCH 08/11] video: Provide constants for VGA I/O range

[Thomas Zimmermann]

Provide VGA_FB_ constants for the VGA framebuffer I/O range and convert
fbdev code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fbmem.c |  4 ++--
 drivers/video/fbdev/vga16fb.c    | 15 ++++++---------
 include/video/vga.h              |  2 ++
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3ee3ea018245..2237049327db 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -39,6 +39,7 @@
 
 #include <asm/fb.h>
 
+#include <video/vga.h>
 
     /*
      *  Frame buffer device initialization and setup routines
@@ -1549,7 +1550,6 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
 
 static void do_unregister_framebuffer(struct fb_info *fb_info);
 
-#define VGA_FB_PHYS 0xA0000
 static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 					       const char *name, bool primary)
 {
@@ -1568,7 +1568,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 		device = registered_fb[i]->device;
 		if (fb_do_apertures_overlap(gen_aper, a) ||
 			(primary && gen_aper && gen_aper->count &&
-			 gen_aper->ranges[0].base == VGA_FB_PHYS)) {
+			 gen_aper->ranges[0].base == VGA_FB_PHYS_BASE)) {
 
 			printk(KERN_INFO "fb%d: switching to %s from %s\n",
 			       i, name, registered_fb[i]->fix.id);
diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
index e7767ed50c5b..595a69634082 100644
--- a/drivers/video/fbdev/vga16fb.c
+++ b/drivers/video/fbdev/vga16fb.c
@@ -25,9 +25,6 @@
 #include <asm/io.h>
 #include <video/vga.h>
 
-#define VGA_FB_PHYS 0xA0000
-#define VGA_FB_PHYS_LEN 65536
-
 #define MODE_SKIP4	1
 #define MODE_8BPP	2
 #define MODE_CFB	4
@@ -87,8 +84,8 @@ static struct fb_var_screeninfo vga16fb_defined = {
 /* name should not depend on EGA/VGA */
 static const struct fb_fix_screeninfo vga16fb_fix = {
 	.id		= "VGA16 VGA",
-	.smem_start	= VGA_FB_PHYS,
-	.smem_len	= VGA_FB_PHYS_LEN,
+	.smem_start	= VGA_FB_PHYS_BASE,
+	.smem_len	= VGA_FB_PHYS_SIZE,
 	.type		= FB_TYPE_VGA_PLANES,
 	.type_aux	= FB_AUX_VGA_PLANES_VGA4,
 	.visual		= FB_VISUAL_PSEUDOCOLOR,
@@ -1357,8 +1354,8 @@ static int vga16fb_probe(struct platform_device *dev)
 		goto err_ioremap;
 	}
 
-	/* XXX share VGA_FB_PHYS and I/O region with vgacon and others */
-	info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS, 0);
+	/* XXX share VGA_FB_PHYS_BASE and I/O region with vgacon and others */
+	info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS_BASE, 0);
 
 	if (!info->screen_base) {
 		printk(KERN_ERR "vga16fb: unable to map device\n");
@@ -1409,8 +1406,8 @@ static int vga16fb_probe(struct platform_device *dev)
 
 	vga16fb_update_fix(info);
 
-	info->apertures->ranges[0].base = VGA_FB_PHYS;
-	info->apertures->ranges[0].size = VGA_FB_PHYS_LEN;
+	info->apertures->ranges[0].base = VGA_FB_PHYS_BASE;
+	info->apertures->ranges[0].size = VGA_FB_PHYS_SIZE;
 
 	if (register_framebuffer(info) < 0) {
 		printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
diff --git a/include/video/vga.h b/include/video/vga.h
index ef8e9fa9b9bd..947c0abd04ef 100644
--- a/include/video/vga.h
+++ b/include/video/vga.h
@@ -22,6 +22,8 @@
 #include <asm/vga.h>
 #include <asm/byteorder.h>
 
+#define VGA_FB_PHYS_BASE	0xA0000 /* VGA framebuffer I/O base */
+#define VGA_FB_PHYS_SIZE	65536	/* VGA framebuffer I/O size */
 
 /* Some of the code below is taken from SVGAlib.  The original,
    unmodified copyright notice for that code is below. */
-- 
2.36.1

[PATCH 09/11] video/aperture: Remove conflicting VGA devices, if any

[Thomas Zimmermann]

On the primary graphics adapter, a driver might conflict with a VGA
driver that controls the VGA framebuffer I/O range. Remove the VGA
driver from the aperture helpers. Until now, this case has been
hendled by fbdev, but it should work even with fbdev disabled.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/aperture.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 101e13c2cf41..abc691284a77 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -12,6 +12,8 @@
 #include <linux/types.h>
 #include <linux/vgaarb.h>
 
+#include <video/vga.h>
+
 /**
  * DOC: overview
  *
@@ -300,6 +302,16 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
 	 */
 	sysfb_disable();
 
+	aperture_detach_devices(base, size);
+
+	/*
+	 * If this is the primary adapter, there could be a VGA device
+	 * that consumes the VGA framebuffer I/O range. Remove this device
+	 * as well.
+	 */
+	if (primary)
+		aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
+
 #if IS_REACHABLE(CONFIG_FB)
 	a = alloc_apertures(1);
 	if (!a)
@@ -315,8 +327,6 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
 		return ret;
 #endif
 
-	aperture_detach_devices(base, size);
-
 	return 0;
 }
 EXPORT_SYMBOL(aperture_remove_conflicting_devices);
-- 
2.36.1

[PATCH 10/11] fbdev: Acquire framebuffer apertures for firmware devices

[Thomas Zimmermann]

When registering a generic framebuffer, automatically acquire ownership
of the framebuffer's I/O range. The device will now be handled by the
aperture helpers. Fbdev-based conflict handling is no longer required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fbmem.c | 33 ++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 2237049327db..e556ad69f48f 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -13,6 +13,7 @@
 
 #include <linux/module.h>
 
+#include <linux/aperture.h>
 #include <linux/compat.h>
 #include <linux/types.h>
 #include <linux/errno.h>
@@ -1739,6 +1740,32 @@ static void do_unregister_framebuffer(struct fb_info *fb_info)
 	put_fb_info(fb_info);
 }
 
+static int fbm_aperture_acquire_for_platform_device(struct fb_info *fb_info)
+{
+	struct apertures_struct *ap = fb_info->apertures;
+	struct device *dev = fb_info->device;
+	struct platform_device *pdev;
+	unsigned int i;
+	int ret;
+
+	if (!ap)
+		return 0;
+
+	if (!dev_is_platform(dev))
+		return 0;
+
+	pdev = to_platform_device(dev);
+
+	for (ret = 0, i = 0; i < ap->count; ++i) {
+		ret = devm_aperture_acquire_for_platform_device(pdev, ap->ranges[i].base,
+								ap->ranges[i].size);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 /**
  * remove_conflicting_framebuffers - remove firmware-configured framebuffers
  * @a: memory range, users of which are to be removed
@@ -1789,6 +1816,12 @@ register_framebuffer(struct fb_info *fb_info)
 {
 	int ret;
 
+	if (fb_info->flags & FBINFO_MISC_FIRMWARE) {
+		ret = fbm_aperture_acquire_for_platform_device(fb_info);
+		if (ret)
+			return ret;
+	}
+
 	mutex_lock(&registration_lock);
 	ret = do_register_framebuffer(fb_info);
 	mutex_unlock(&registration_lock);
-- 
2.36.1

[PATCH 11/11] fbdev: Remove conflict-handling code

[Thomas Zimmermann]

Remove the call to do_remove_conflicting_framebuffers() from the
framebuffer registration. Aperture helpers take care of removing
conflicting devices. With all ownership information stored in the
aperture datastrcutures, remove remove_conflicting_framebuffers()
entirely.

This change also rectifies DRM generic-framebuffer registration, which
tried to unregister conflicting framebuffers, even though it's entirely
build on top of DRM.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/aperture.c         | 21 -------
 drivers/video/fbdev/core/fbmem.c | 99 --------------------------------
 include/linux/fb.h               |  2 -
 3 files changed, 122 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index abc691284a77..9e6bcc03a1a4 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -2,7 +2,6 @@
 
 #include <linux/aperture.h>
 #include <linux/device.h>
-#include <linux/fb.h> /* for old fbdev helpers */
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/pci.h>
@@ -286,11 +285,6 @@ static void aperture_detach_devices(resource_size_t base, resource_size_t size)
 int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t size,
 					bool primary, const char *name)
 {
-#if IS_REACHABLE(CONFIG_FB)
-	struct apertures_struct *a;
-	int ret;
-#endif
-
 	/*
 	 * If a driver asked to unregister a platform device registered by
 	 * sysfb, then can be assumed that this is a driver for a display
@@ -312,21 +306,6 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
 	if (primary)
 		aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
 
-#if IS_REACHABLE(CONFIG_FB)
-	a = alloc_apertures(1);
-	if (!a)
-		return -ENOMEM;
-
-	a->ranges[0].base = base;
-	a->ranges[0].size = size;
-
-	ret = remove_conflicting_framebuffers(a, name, primary);
-	kfree(a);
-
-	if (ret)
-		return ret;
-#endif
-
 	return 0;
 }
 EXPORT_SYMBOL(aperture_remove_conflicting_devices);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index e556ad69f48f..9d3d0ed479d2 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1551,65 +1551,6 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
 
 static void do_unregister_framebuffer(struct fb_info *fb_info);
 
-static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
-					       const char *name, bool primary)
-{
-	int i;
-
-restart_removal:
-	/* check all firmware fbs and kick off if the base addr overlaps */
-	for_each_registered_fb(i) {
-		struct apertures_struct *gen_aper;
-		struct device *device;
-
-		if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE))
-			continue;
-
-		gen_aper = registered_fb[i]->apertures;
-		device = registered_fb[i]->device;
-		if (fb_do_apertures_overlap(gen_aper, a) ||
-			(primary && gen_aper && gen_aper->count &&
-			 gen_aper->ranges[0].base == VGA_FB_PHYS_BASE)) {
-
-			printk(KERN_INFO "fb%d: switching to %s from %s\n",
-			       i, name, registered_fb[i]->fix.id);
-
-			/*
-			 * If we kick-out a firmware driver, we also want to remove
-			 * the underlying platform device, such as simple-framebuffer,
-			 * VESA, EFI, etc. A native driver will then be able to
-			 * allocate the memory range.
-			 *
-			 * If it's not a platform device, at least print a warning. A
-			 * fix would add code to remove the device from the system. For
-			 * framebuffers without any Linux device, print a warning as
-			 * well.
-			 */
-			if (!device) {
-				pr_warn("fb%d: no device set\n", i);
-				do_unregister_framebuffer(registered_fb[i]);
-			} else if (dev_is_platform(device)) {
-				/*
-				 * Drop the lock because if the device is unregistered, its
-				 * driver will call to unregister_framebuffer(), that takes
-				 * this lock.
-				 */
-				mutex_unlock(&registration_lock);
-				platform_device_unregister(to_platform_device(device));
-				mutex_lock(&registration_lock);
-			} else {
-				pr_warn("fb%d: cannot remove device\n", i);
-				do_unregister_framebuffer(registered_fb[i]);
-			}
-			/*
-			 * Restart the removal loop now that the device has been
-			 * unregistered and its associated framebuffer gone.
-			 */
-			goto restart_removal;
-		}
-	}
-}
-
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i;
@@ -1618,10 +1559,6 @@ static int do_register_framebuffer(struct fb_info *fb_info)
 	if (fb_check_foreignness(fb_info))
 		return -ENOSYS;
 
-	do_remove_conflicting_framebuffers(fb_info->apertures,
-					   fb_info->fix.id,
-					   fb_is_primary_device(fb_info));
-
 	if (num_registered_fb == FB_MAX)
 		return -ENXIO;
 
@@ -1766,42 +1703,6 @@ static int fbm_aperture_acquire_for_platform_device(struct fb_info *fb_info)
 	return ret;
 }
 
-/**
- * remove_conflicting_framebuffers - remove firmware-configured framebuffers
- * @a: memory range, users of which are to be removed
- * @name: requesting driver name
- * @primary: also kick vga16fb if present
- *
- * This function removes framebuffer devices (initialized by firmware/bootloader)
- * which use memory range described by @a. If @a is NULL all such devices are
- * removed.
- */
-int remove_conflicting_framebuffers(struct apertures_struct *a,
-				    const char *name, bool primary)
-{
-	bool do_free = false;
-
-	if (!a) {
-		a = alloc_apertures(1);
-		if (!a)
-			return -ENOMEM;
-
-		a->ranges[0].base = 0;
-		a->ranges[0].size = ~0;
-		do_free = true;
-	}
-
-	mutex_lock(&registration_lock);
-	do_remove_conflicting_framebuffers(a, name, primary);
-	mutex_unlock(&registration_lock);
-
-	if (do_free)
-		kfree(a);
-
-	return 0;
-}
-EXPORT_SYMBOL(remove_conflicting_framebuffers);
-
 /**
  *	register_framebuffer - registers a frame buffer device
  *	@fb_info: frame buffer info structure
diff --git a/include/linux/fb.h b/include/linux/fb.h
index b91c77016560..453c3b2b6b8e 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -615,8 +615,6 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 /* drivers/video/fbmem.c */
 extern int register_framebuffer(struct fb_info *fb_info);
 extern void unregister_framebuffer(struct fb_info *fb_info);
-extern int remove_conflicting_framebuffers(struct apertures_struct *a,
-					   const char *name, bool primary);
 extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);
 extern int fb_show_logo(struct fb_info *fb_info, int rotate);
 extern char* fb_get_buffer_offset(struct fb_info *info, struct fb_pixmap *buf, u32 size);
-- 
2.36.1

Re: [PATCH 01/11] fbdev: Remove trailing whitespaces

[Javier Martinez Canillas]

Hello Thomas,

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Fix coding style. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

[Javier Martinez Canillas]

Hello Thomas,

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Move the device-creation from vga16fb to sysfb code. Move the few
> extra videomode checks into vga16fb's probe function.
> 
> The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC
> or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except
> for some MIPS systems. It's not clear if the vga16fb driver actually
> works in practice.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/firmware/sysfb.c      |  4 +++
>  drivers/video/fbdev/vga16fb.c | 59 +++++++++++++++++------------------
>  2 files changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index 1f276f108cc9..3fd3563d962b 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -94,6 +94,10 @@ static __init int sysfb_init(void)
>  		name = "efi-framebuffer";
>  	else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
>  		name = "vesa-framebuffer";
> +	else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
> +		name = "vga-framebuffer";
> +	else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
> +

I wonder if we really need to do this distinction or could just have a single
platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the
same fbdev driver is bound against both platform devices.

[...]

>  static int vga16fb_probe(struct platform_device *dev)
>  {
> +	struct screen_info *si;
>  	struct fb_info *info;
>  	struct vga16fb_par *par;
>  	int i;
>  	int ret = 0;
>  
> +	si = dev_get_platdata(&dev->dev);
> +	if (!si)
> +		return -ENODEV;
> +
> +	ret = check_mode_supported(si);
> +	if (ret)
> +		return ret;
> +

What do you see as the advantage of moving the check to the driver's probe?

Because after this change the platform driver will be registered but for no
reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC.

[...]

> +static const struct platform_device_id vga16fb_driver_id_table[] = {
> +	{"ega-framebuffer", 0},
> +	{"vga-framebuffer", 0},
> +	{ }
> +};
> +

The fact that the two entries don't have a platform data is an indication for
me that we could just consolidate in a single "vga16-framebuffer" or smt. I
know that this won't be consistent with efi, vesa, etc but I don't think is
that important and also quite likely we will get rid of this driver and the
platform device registration soon. Since as you said, it's unclear that is
even used.

>  static struct platform_driver vga16fb_driver = {
>  	.probe = vga16fb_probe,
>  	.remove = vga16fb_remove,
>  	.driver = {
> -		.name = "vga16fb",
> +		.name = "vga-framebuffer",
>  	},

Maybe "vga16-framebuffer" instead? Since for example VESA is also VGA AFAIK.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 03/11] fbdev/vga16fb: Auto-generate module init/exit code

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Move vgag16fb's option parsing into the driver's probe function and
> generate the rest of the module's init/exit functions from macros.
> Keep the options code, although there are no options defined.
>

Ah, I see now why you wanted to move the check to the probe function. If
is to allow this cleanup then discard that comment from previous patch
and I'm OK with the move.

Maybe you could comment in patch 02/11 commit message that the check is
moved to the probe handler to allow this cleanup as a follow-up patch ?

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/video/fbdev/vga16fb.c | 35 ++++++++++-------------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
> index f7c1bb018843..e7767ed50c5b 100644
> --- a/drivers/video/fbdev/vga16fb.c
> +++ b/drivers/video/fbdev/vga16fb.c
> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options)
>  
>  static int vga16fb_probe(struct platform_device *dev)
>  {
> +#ifndef MODULE
> +	char *option = NULL;
> +#endif
>  	struct screen_info *si;
>  	struct fb_info *info;
>  	struct vga16fb_par *par;
>  	int i;
>  	int ret = 0;
>  
> +#ifndef MODULE
> +	if (fb_get_options("vga16fb", &option))
> +		return -ENODEV;
> +	vga16fb_setup(option);
> +#endif
> +

I would just drop these ifdefery and have the option unconditionally.
It seems that's what most fbdev drivers do AFAICT.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4819 bytes --]

Hi Javier,

I'll try to give the motivation of this patch below. I known it's rather 
hypothetical as the VGA driver is probably not used much.

Am 08.07.22 um 15:09 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 7/7/22 17:39, Thomas Zimmermann wrote:
>> Move the device-creation from vga16fb to sysfb code. Move the few
>> extra videomode checks into vga16fb's probe function.
>>
>> The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC
>> or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except
>> for some MIPS systems. It's not clear if the vga16fb driver actually
>> works in practice.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/firmware/sysfb.c      |  4 +++
>>   drivers/video/fbdev/vga16fb.c | 59 +++++++++++++++++------------------
>>   2 files changed, 32 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
>> index 1f276f108cc9..3fd3563d962b 100644
>> --- a/drivers/firmware/sysfb.c
>> +++ b/drivers/firmware/sysfb.c
>> @@ -94,6 +94,10 @@ static __init int sysfb_init(void)
>>   		name = "efi-framebuffer";
>>   	else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
>>   		name = "vesa-framebuffer";
>> +	else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
>> +		name = "vga-framebuffer";
>> +	else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
>> +
> 
> I wonder if we really need to do this distinction or could just have a single
> platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the
> same fbdev driver is bound against both platform devices.

With the current driver, we don't strictly need to distinguish. But the 
sysfb code is the one we care about. So I wanted it to be clear and 
nicely looking. All the mode tests, etc depend on the driver (which no 
one cares about).

There's also a difference in hardware features between EGA and VGA. Most 
notably, VGA has much better color support.

> 
> [...]
> 
>>   static int vga16fb_probe(struct platform_device *dev)
>>   {
>> +	struct screen_info *si;
>>   	struct fb_info *info;
>>   	struct vga16fb_par *par;
>>   	int i;
>>   	int ret = 0;
>>   
>> +	si = dev_get_platdata(&dev->dev);
>> +	if (!si)
>> +		return -ENODEV;
>> +
>> +	ret = check_mode_supported(si);
>> +	if (ret)
>> +		return ret;
>> +
> 
> What do you see as the advantage of moving the check to the driver's probe?
> 
> Because after this change the platform driver will be registered but for no
> reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC.

The driver only supports very specific modes, which may not be what 
screen_info detected. Note that VGAC/EGAC can also refer to text-mode 
buffers. The current vgacon could be turned into a platform driver that 
adopts such a text buffer and integrates it with aperture helpers.

> 
> [...]
> 
>> +static const struct platform_device_id vga16fb_driver_id_table[] = {
>> +	{"ega-framebuffer", 0},
>> +	{"vga-framebuffer", 0},
>> +	{ }
>> +};
>> +
> 
> The fact that the two entries don't have a platform data is an indication for

The name is the indication. I know that vga16 doesn't treat them 
differently.

> me that we could just consolidate in a single "vga16-framebuffer" or smt. I
> know that this won't be consistent with efi, vesa, etc but I don't think is
> that important and also quite likely we will get rid of this driver and the
> platform device registration soon. Since as you said, it's unclear that is
> even used.

There's mips code in the arch/ directory that appears to setup 
screen_info in the correct way. I can't say whether that's still useful 
to anyone. On x86, I could set a VGA mode on the kernel command line, 
but screen_info's isVGA only contained '1'. It might be possible to fix 
this easily by setting the right values in vga_probe(). [1] I simply 
don't have the time to provide a patch and deal with the potential 
fallout of such a change.

> 
>>   static struct platform_driver vga16fb_driver = {
>>   	.probe = vga16fb_probe,
>>   	.remove = vga16fb_remove,
>>   	.driver = {
>> -		.name = "vga16fb",
>> +		.name = "vga-framebuffer",
>>   	},
> 
> Maybe "vga16-framebuffer" instead? Since for example VESA is also VGA AFAIK.

VESA is something else than VGA. Setting a VESA mode (done via INT 10h 
IIRC) and then fiddling with VGA state will likely produce broken output 
on the screen.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.18.10/source/arch/x86/boot/video-vga.c#L231

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 03/11] fbdev/vga16fb: Auto-generate module init/exit code

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2123 bytes --]

Hi

Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas:
> On 7/7/22 17:39, Thomas Zimmermann wrote:
>> Move vgag16fb's option parsing into the driver's probe function and
>> generate the rest of the module's init/exit functions from macros.
>> Keep the options code, although there are no options defined.
>>
> 
> Ah, I see now why you wanted to move the check to the probe function. If
> is to allow this cleanup then discard that comment from previous patch
> and I'm OK with the move.
> 
> Maybe you could comment in patch 02/11 commit message that the check is
> moved to the probe handler to allow this cleanup as a follow-up patch ?

Sure.

I mostly wanted to use module_platform_driver(). The options handling is 
in the way.

> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/video/fbdev/vga16fb.c | 35 ++++++++++-------------------------
>>   1 file changed, 10 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
>> index f7c1bb018843..e7767ed50c5b 100644
>> --- a/drivers/video/fbdev/vga16fb.c
>> +++ b/drivers/video/fbdev/vga16fb.c
>> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options)
>>   
>>   static int vga16fb_probe(struct platform_device *dev)
>>   {
>> +#ifndef MODULE
>> +	char *option = NULL;
>> +#endif
>>   	struct screen_info *si;
>>   	struct fb_info *info;
>>   	struct vga16fb_par *par;
>>   	int i;
>>   	int ret = 0;
>>   
>> +#ifndef MODULE
>> +	if (fb_get_options("vga16fb", &option))
>> +		return -ENODEV;
>> +	vga16fb_setup(option);
>> +#endif
>> +
> 
> I would just drop these ifdefery and have the option unconditionally.
> It seems that's what most fbdev drivers do AFAICT.

Or can we kill it entirely? There are no actual options.

Best regards
Thomas

> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

[Javier Martinez Canillas]

Hello Thomas,

On 7/11/22 09:58, Thomas Zimmermann wrote:
> Hi Javier,
> 
> I'll try to give the motivation of this patch below. I known it's rather 
> hypothetical as the VGA driver is probably not used much.
> 
> Am 08.07.22 um 15:09 schrieb Javier Martinez Canillas:
>> Hello Thomas,
>>
>> On 7/7/22 17:39, Thomas Zimmermann wrote:
>>> Move the device-creation from vga16fb to sysfb code. Move the few
>>> extra videomode checks into vga16fb's probe function.
>>>
>>> The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC
>>> or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except
>>> for some MIPS systems. It's not clear if the vga16fb driver actually
>>> works in practice.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/firmware/sysfb.c      |  4 +++
>>>   drivers/video/fbdev/vga16fb.c | 59 +++++++++++++++++------------------
>>>   2 files changed, 32 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
>>> index 1f276f108cc9..3fd3563d962b 100644
>>> --- a/drivers/firmware/sysfb.c
>>> +++ b/drivers/firmware/sysfb.c
>>> @@ -94,6 +94,10 @@ static __init int sysfb_init(void)
>>>   		name = "efi-framebuffer";
>>>   	else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
>>>   		name = "vesa-framebuffer";
>>> +	else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
>>> +		name = "vga-framebuffer";
>>> +	else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
>>> +
>>
>> I wonder if we really need to do this distinction or could just have a single
>> platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the
>> same fbdev driver is bound against both platform devices.
> 
> With the current driver, we don't strictly need to distinguish. But the 
> sysfb code is the one we care about. So I wanted it to be clear and 
> nicely looking. All the mode tests, etc depend on the driver (which no 
> one cares about).
>

Right. That is a good point. We don't want to leak a driver implementation
detail in the sysfb code. And in theory there could be for example a DRM
driver for EGA and one for VGA.
 
> There's also a difference in hardware features between EGA and VGA. Most 
> notably, VGA has much better color support.
>

Yes, I know the differences. My point was that the orig_video_isVGA was
used to make the distinction and that the same driver supported both, but
as you said that may not be the case and separate drivers could be used.

>>
>> [...]
>>
>>>   static int vga16fb_probe(struct platform_device *dev)
>>>   {
>>> +	struct screen_info *si;
>>>   	struct fb_info *info;
>>>   	struct vga16fb_par *par;
>>>   	int i;
>>>   	int ret = 0;
>>>   
>>> +	si = dev_get_platdata(&dev->dev);
>>> +	if (!si)
>>> +		return -ENODEV;
>>> +
>>> +	ret = check_mode_supported(si);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>
>> What do you see as the advantage of moving the check to the driver's probe?
>>
>> Because after this change the platform driver will be registered but for no
>> reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC.
> 
> The driver only supports very specific modes, which may not be what 
> screen_info detected. Note that VGAC/EGAC can also refer to text-mode 
> buffers. The current vgacon could be turned into a platform driver that 
> adopts such a text buffer and integrates it with aperture helpers.
>

Yes, and also there's also the monochrome variant of VGA and EGA (VGAM/EGAM).

Should we also make that distinction or for example "vga-framebuffer" should
handle both color and monochrome variants if at some point vgacon is turned
into a fbdev or DRM driver ?

>>
>> [...]
>>
>>> +static const struct platform_device_id vga16fb_driver_id_table[] = {
>>> +	{"ega-framebuffer", 0},
>>> +	{"vga-framebuffer", 0},
>>> +	{ }
>>> +};
>>> +
>>
>> The fact that the two entries don't have a platform data is an indication for
> 
> The name is the indication. I know that vga16 doesn't treat them 
> differently.
>

Yes, my point was that the platform device name used to bind is an internal
Linux interface that could be changed later if needed. But I understand your
point and since the platform device names are exposed to user-space, makes
more sense for them to reflect what devices are bound even when the existing
driver doesn't treat them differently.
 
>> me that we could just consolidate in a single "vga16-framebuffer" or smt. I
>> know that this won't be consistent with efi, vesa, etc but I don't think is
>> that important and also quite likely we will get rid of this driver and the
>> platform device registration soon. Since as you said, it's unclear that is
>> even used.
> 
> There's mips code in the arch/ directory that appears to setup 
> screen_info in the correct way. I can't say whether that's still useful 
> to anyone. On x86, I could set a VGA mode on the kernel command line, 
> but screen_info's isVGA only contained '1'. It might be possible to fix 
> this easily by setting the right values in vga_probe(). [1] I simply 
> don't have the time to provide a patch and deal with the potential 
> fallout of such a change.
>

Indeed. This seems to be a remnant from the time when isVGA was just a bool
and then at some point the field semantics was extended to denote the mode
but the existing users weren't fixed (nor the field named to reflect this).

Probably should be cleaned up at some point but unsure if the churn would
be worth it.
 
>>
>>>   static struct platform_driver vga16fb_driver = {
>>>   	.probe = vga16fb_probe,
>>>   	.remove = vga16fb_remove,
>>>   	.driver = {
>>> -		.name = "vga16fb",
>>> +		.name = "vga-framebuffer",
>>>   	},
>>
>> Maybe "vga16-framebuffer" instead? Since for example VESA is also VGA AFAIK.
> 
> VESA is something else than VGA. Setting a VESA mode (done via INT 10h 
> IIRC) and then fiddling with VGA state will likely produce broken output 
> on the screen.
>

Technically it is something else but Linux conflates them in many places. For
example, as you mentioned one can change the VESA modes using the "vga" param
(which confusingly leads to the use of vesafb+fbcon driver instead of vgacon).

That's why I think that "vga-framebuffer" as driver name would be misleading.
Specially since it would also bind to the "ega-framebuffer" platform device.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 03/11] fbdev/vga16fb: Auto-generate module init/exit code

[Javier Martinez Canillas]

On 7/11/22 10:01, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas:
>> On 7/7/22 17:39, Thomas Zimmermann wrote:
>>> Move vgag16fb's option parsing into the driver's probe function and
>>> generate the rest of the module's init/exit functions from macros.
>>> Keep the options code, although there are no options defined.
>>>
>>
>> Ah, I see now why you wanted to move the check to the probe function. If
>> is to allow this cleanup then discard that comment from previous patch
>> and I'm OK with the move.
>>
>> Maybe you could comment in patch 02/11 commit message that the check is
>> moved to the probe handler to allow this cleanup as a follow-up patch ?
> 
> Sure.
> 
> I mostly wanted to use module_platform_driver(). The options handling is 
> in the way.
>

Yes, I got it when looked at this patch.
 
>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/video/fbdev/vga16fb.c | 35 ++++++++++-------------------------
>>>   1 file changed, 10 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
>>> index f7c1bb018843..e7767ed50c5b 100644
>>> --- a/drivers/video/fbdev/vga16fb.c
>>> +++ b/drivers/video/fbdev/vga16fb.c
>>> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options)
>>>   
>>>   static int vga16fb_probe(struct platform_device *dev)
>>>   {
>>> +#ifndef MODULE
>>> +	char *option = NULL;
>>> +#endif
>>>   	struct screen_info *si;
>>>   	struct fb_info *info;
>>>   	struct vga16fb_par *par;
>>>   	int i;
>>>   	int ret = 0;
>>>   
>>> +#ifndef MODULE
>>> +	if (fb_get_options("vga16fb", &option))
>>> +		return -ENODEV;
>>> +	vga16fb_setup(option);
>>> +#endif
>>> +
>>
>> I would just drop these ifdefery and have the option unconditionally.
>> It seems that's what most fbdev drivers do AFAICT.
> 
> Or can we kill it entirely? There are no actual options.
>

That sounds good to me as well.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7356 bytes --]

Hi Javier

Am 11.07.22 um 11:54 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 7/11/22 09:58, Thomas Zimmermann wrote:
>> Hi Javier,
>>
>> I'll try to give the motivation of this patch below. I known it's rather
>> hypothetical as the VGA driver is probably not used much.
>>
>> Am 08.07.22 um 15:09 schrieb Javier Martinez Canillas:
>>> Hello Thomas,
>>>
>>> On 7/7/22 17:39, Thomas Zimmermann wrote:
>>>> Move the device-creation from vga16fb to sysfb code. Move the few
>>>> extra videomode checks into vga16fb's probe function.
>>>>
>>>> The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC
>>>> or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except
>>>> for some MIPS systems. It's not clear if the vga16fb driver actually
>>>> works in practice.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>    drivers/firmware/sysfb.c      |  4 +++
>>>>    drivers/video/fbdev/vga16fb.c | 59 +++++++++++++++++------------------
>>>>    2 files changed, 32 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
>>>> index 1f276f108cc9..3fd3563d962b 100644
>>>> --- a/drivers/firmware/sysfb.c
>>>> +++ b/drivers/firmware/sysfb.c
>>>> @@ -94,6 +94,10 @@ static __init int sysfb_init(void)
>>>>    		name = "efi-framebuffer";
>>>>    	else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
>>>>    		name = "vesa-framebuffer";
>>>> +	else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
>>>> +		name = "vga-framebuffer";
>>>> +	else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
>>>> +
>>>
>>> I wonder if we really need to do this distinction or could just have a single
>>> platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the
>>> same fbdev driver is bound against both platform devices.
>>
>> With the current driver, we don't strictly need to distinguish. But the
>> sysfb code is the one we care about. So I wanted it to be clear and
>> nicely looking. All the mode tests, etc depend on the driver (which no
>> one cares about).
>>
> 
> Right. That is a good point. We don't want to leak a driver implementation
> detail in the sysfb code. And in theory there could be for example a DRM
> driver for EGA and one for VGA.
>   
>> There's also a difference in hardware features between EGA and VGA. Most
>> notably, VGA has much better color support.
>>
> 
> Yes, I know the differences. My point was that the orig_video_isVGA was
> used to make the distinction and that the same driver supported both, but
> as you said that may not be the case and separate drivers could be used.
> 
>>>
>>> [...]
>>>
>>>>    static int vga16fb_probe(struct platform_device *dev)
>>>>    {
>>>> +	struct screen_info *si;
>>>>    	struct fb_info *info;
>>>>    	struct vga16fb_par *par;
>>>>    	int i;
>>>>    	int ret = 0;
>>>>    
>>>> +	si = dev_get_platdata(&dev->dev);
>>>> +	if (!si)
>>>> +		return -ENODEV;
>>>> +
>>>> +	ret = check_mode_supported(si);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>
>>> What do you see as the advantage of moving the check to the driver's probe?
>>>
>>> Because after this change the platform driver will be registered but for no
>>> reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC.
>>
>> The driver only supports very specific modes, which may not be what
>> screen_info detected. Note that VGAC/EGAC can also refer to text-mode
>> buffers. The current vgacon could be turned into a platform driver that
>> adopts such a text buffer and integrates it with aperture helpers.
>>
> 
> Yes, and also there's also the monochrome variant of VGA and EGA (VGAM/EGAM).
> 
> Should we also make that distinction or for example "vga-framebuffer" should
> handle both color and monochrome variants if at some point vgacon is turned
> into a fbdev or DRM driver ?
> 
>>>
>>> [...]
>>>
>>>> +static const struct platform_device_id vga16fb_driver_id_table[] = {
>>>> +	{"ega-framebuffer", 0},
>>>> +	{"vga-framebuffer", 0},
>>>> +	{ }
>>>> +};
>>>> +
>>>
>>> The fact that the two entries don't have a platform data is an indication for
>>
>> The name is the indication. I know that vga16 doesn't treat them
>> differently.
>>
> 
> Yes, my point was that the platform device name used to bind is an internal
> Linux interface that could be changed later if needed. But I understand your
> point and since the platform device names are exposed to user-space, makes
> more sense for them to reflect what devices are bound even when the existing
> driver doesn't treat them differently.
>   
>>> me that we could just consolidate in a single "vga16-framebuffer" or smt. I
>>> know that this won't be consistent with efi, vesa, etc but I don't think is
>>> that important and also quite likely we will get rid of this driver and the
>>> platform device registration soon. Since as you said, it's unclear that is
>>> even used.
>>
>> There's mips code in the arch/ directory that appears to setup
>> screen_info in the correct way. I can't say whether that's still useful
>> to anyone. On x86, I could set a VGA mode on the kernel command line,
>> but screen_info's isVGA only contained '1'. It might be possible to fix
>> this easily by setting the right values in vga_probe(). [1] I simply
>> don't have the time to provide a patch and deal with the potential
>> fallout of such a change.
>>
> 
> Indeed. This seems to be a remnant from the time when isVGA was just a bool
> and then at some point the field semantics was extended to denote the mode
> but the existing users weren't fixed (nor the field named to reflect this).
> 
> Probably should be cleaned up at some point but unsure if the churn would
> be worth it.
>   
>>>
>>>>    static struct platform_driver vga16fb_driver = {
>>>>    	.probe = vga16fb_probe,
>>>>    	.remove = vga16fb_remove,
>>>>    	.driver = {
>>>> -		.name = "vga16fb",
>>>> +		.name = "vga-framebuffer",
>>>>    	},
>>>
>>> Maybe "vga16-framebuffer" instead? Since for example VESA is also VGA AFAIK.
>>
>> VESA is something else than VGA. Setting a VESA mode (done via INT 10h
>> IIRC) and then fiddling with VGA state will likely produce broken output
>> on the screen.
>>
> 
> Technically it is something else but Linux conflates them in many places. For
> example, as you mentioned one can change the VESA modes using the "vga" param
> (which confusingly leads to the use of vesafb+fbcon driver instead of vgacon).
> 
> That's why I think that "vga-framebuffer" as driver name would be misleading.
> Specially since it would also bind to the "ega-framebuffer" platform device.

I messed up device and driver name, such that misunderstood your remark.

As we use the id_table field for matching devices, the driver name 
doesn't matter. [1] So let's keep the driver name as vga16fb. The change 
above must have been left over from an earlier prototype patch, I guess.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.18.10/source/drivers/base/platform.c#L1365

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

[Javier Martinez Canillas]

On 7/11/22 12:42, Thomas Zimmermann wrote:
> Hi Javier

[...]

>> That's why I think that "vga-framebuffer" as driver name would be misleading.
>> Specially since it would also bind to the "ega-framebuffer" platform device.
> 
> I messed up device and driver name, such that misunderstood your remark.
> 
> As we use the id_table field for matching devices, the driver name 
> doesn't matter. [1] So let's keep the driver name as vga16fb. The change 
> above must have been left over from an earlier prototype patch, I guess.
> 

Agreed. The driver name is used as the last resort to match a device
only if there isn't any device ID table (ACPI, OF, platform, etc) but
that's discouraged. Specially when the same driver supports different
devices as it's the case for this driver.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 04/11] fbdev/core: Remove remove_conflicting_pci_framebuffers()

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Remove remove_conflicting_pci_framebuffers() and implement similar
> functionality in aperture_remove_conflicting_pci_device(), which was
> the only caller. Removes an otherwise unused interface and streamlines
> the aperture helper. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 05/11] fbdev: Convert drivers to aperture helpers

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Convert fbdev drivers from fbdev's remove_conflicting_framebuffers() to
> the framework-independent aperture_remove_conflicting_devices(). Calling
> this function will also remove conflicting DRM drivers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

>  static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev)
>  {
> -	struct apertures_struct *ap;
> +	resource_size_t base = pci_resource_start(pdev, 0);
> +	resource_size_t size = pci_resource_len(pdev, 0);
>  	bool primary = false;
>  
> -	ap = alloc_apertures(1);
> -	if (!ap)
> -		return -ENOMEM;
> -
> -	ap->ranges[0].base = pci_resource_start(pdev, 0);
> -	ap->ranges[0].size = pci_resource_len(pdev, 0);
>  #ifdef CONFIG_X86
>  	primary = pdev->resource[PCI_ROM_RESOURCE].flags &
>  					IORESOURCE_ROM_SHADOW;
>  #endif
> -	remove_conflicting_framebuffers(ap, "sm750_fb1", primary);
> -	kfree(ap);
> -	return 0;
> +
> +	return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");

Do you know why this can't just use aperture_remove_conflicting_pci_devices() ?

It seems that the driver is open coding part of the logic already in that helper.
For example, figuring out if is a primary by checking the IORESOURCE_ROM_SHADOW
flag in the PCI_ROM_RESOURCE.

But also getting the base and size for PCI BAR 0, since the loop in that helper
would already take care of that (it also starts at BAR 0).

>  }
>  
>  static int lynxfb_pci_probe(struct pci_dev *pdev,
> diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
> index b311c07fe66d..e5e362b8c9da 100644
> --- a/drivers/video/fbdev/aty/radeon_base.c
> +++ b/drivers/video/fbdev/aty/radeon_base.c
> @@ -54,6 +54,7 @@
>  
>  #include "radeonfb.h"
>  
> +#include <linux/aperture.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/kernel.h>
> @@ -2239,20 +2240,10 @@ static const struct bin_attribute edid2_attr = {
>  
>  static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
>  {
> -	struct apertures_struct *ap;
> +	resource_size_t base = pci_resource_start(pdev, 0);
> +	resource_size_t size = pci_resource_len(pdev, 0);
>  
> -	ap = alloc_apertures(1);
> -	if (!ap)
> -		return -ENOMEM;
> -
> -	ap->ranges[0].base = pci_resource_start(pdev, 0);
> -	ap->ranges[0].size = pci_resource_len(pdev, 0);
> -
> -	remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
> -
> -	kfree(ap);
> -
> -	return 0;
> +	return aperture_remove_conflicting_devices(base, size, KBUILD_MODNAME, false);

Same for this.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 06/11] fbdev: Remove conflicting devices on PCI bus

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Remove firmware devices on the PCI bus, by calling
> aperture_remove_conflicting_pci_devices() in the probe function of
> each related fbdev driver. iSo far, most of these drivers did not
> remove conflicting VESA or EFI devices, or outride failed for
> resource conflicts (i.e., matroxfb.) This must have been broken
> for quite some time.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

> @@ -949,6 +950,10 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	int rc;
>  	u8 regval;
>  
> +	rc = aperture_remove_conflicting_pci_devices(dev, "arkfb");
> +	if (rc < 0)
> +		return rc;
> +

I wonder if we could think of a trick to avoid open coding the same check in
all drivers. Maybe a combination of using KBUILD_MODNAME for the name and
a probe callback wrapper or something ?

But probably not worth to invest more in the fbdev drivers and could be done
as a follow-up anyways if someone feels like it.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 07/11] video/aperture: Disable and unregister sysfb devices via aperture helpers

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Call sysfb_disable() before removing conflicting devices in aperture
> helpers. Fixes sysfb state if fbdev has been disabled.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: fb84efa28a48 ("drm/aperture: Run fbdev removal before internal helpers")
> Cc: Zack Rusin <zackr@vmware.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Zhen Lei <thunder.leizhen@huawei.com>
> Cc: Changcheng Deng <deng.changcheng@zte.com.cn>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 08/11] video: Provide constants for VGA I/O range

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Provide VGA_FB_ constants for the VGA framebuffer I/O range and convert
> fbdev code.
>

I would probably mention in the commit message to that you are renaming
s/VGA_FB_PHYS/VGA_FB_PHYS_BASE and s/VGA_FB_PHYS_LEN/VGA_FB_PHYS_SIZE.

I do agree with the rename and think that makes it much more readable,
but think that's worth to mention the rationale for that change too.
 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 09/11] video/aperture: Remove conflicting VGA devices, if any

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> On the primary graphics adapter, a driver might conflict with a VGA
> driver that controls the VGA framebuffer I/O range. Remove the VGA
> driver from the aperture helpers. Until now, this case has been
> hendled by fbdev, but it should work even with fbdev disabled.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 10/11] fbdev: Acquire framebuffer apertures for firmware devices

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> When registering a generic framebuffer, automatically acquire ownership
> of the framebuffer's I/O range. The device will now be handled by the
> aperture helpers. Fbdev-based conflict handling is no longer required.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/video/fbdev/core/fbmem.c | 33 ++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 2237049327db..e556ad69f48f 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -13,6 +13,7 @@
>  
>  #include <linux/module.h>
>  
> +#include <linux/aperture.h>
>  #include <linux/compat.h>
>  #include <linux/types.h>
>  #include <linux/errno.h>
> @@ -1739,6 +1740,32 @@ static void do_unregister_framebuffer(struct fb_info *fb_info)
>  	put_fb_info(fb_info);
>  }
>  
> +static int fbm_aperture_acquire_for_platform_device(struct fb_info *fb_info)
> +{

What's the meaning of 'm' here ? Misc, memory ? I would just call it 'fb_'.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 11/11] fbdev: Remove conflict-handling code

[Javier Martinez Canillas]

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Remove the call to do_remove_conflicting_framebuffers() from the
> framebuffer registration. Aperture helpers take care of removing
> conflicting devices. With all ownership information stored in the
> aperture datastrcutures, remove remove_conflicting_framebuffers()

data structures.

> entirely.
> 
> This change also rectifies DRM generic-framebuffer registration, which
> tried to unregister conflicting framebuffers, even though it's entirely
> build on top of DRM.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Amazing patch! The whole series really. Thanks a lot for working on this.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 05/11] fbdev: Convert drivers to aperture helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3452 bytes --]

Hi

Am 11.07.22 um 13:01 schrieb Javier Martinez Canillas:
> On 7/7/22 17:39, Thomas Zimmermann wrote:
>> Convert fbdev drivers from fbdev's remove_conflicting_framebuffers() to
>> the framework-independent aperture_remove_conflicting_devices(). Calling
>> this function will also remove conflicting DRM drivers.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>>   static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev)
>>   {
>> -	struct apertures_struct *ap;
>> +	resource_size_t base = pci_resource_start(pdev, 0);
>> +	resource_size_t size = pci_resource_len(pdev, 0);
>>   	bool primary = false;
>>   
>> -	ap = alloc_apertures(1);
>> -	if (!ap)
>> -		return -ENOMEM;
>> -
>> -	ap->ranges[0].base = pci_resource_start(pdev, 0);
>> -	ap->ranges[0].size = pci_resource_len(pdev, 0);
>>   #ifdef CONFIG_X86
>>   	primary = pdev->resource[PCI_ROM_RESOURCE].flags &
>>   					IORESOURCE_ROM_SHADOW;
>>   #endif
>> -	remove_conflicting_framebuffers(ap, "sm750_fb1", primary);
>> -	kfree(ap);
>> -	return 0;
>> +
>> +	return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");
> 
> Do you know why this can't just use aperture_remove_conflicting_pci_devices() ?

I simply don't want change too much at once, because there was this 
problem with the PCI helper on ast.

At some point we can make a push to really fix this throughout the code 
base. And that would include an update to fb_is_primary_device(), [1] 
which doesn't fill well into the new interfaces.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/arch/x86/video/fbdev.c#L14

> 
> It seems that the driver is open coding part of the logic already in that helper.
> For example, figuring out if is a primary by checking the IORESOURCE_ROM_SHADOW
> flag in the PCI_ROM_RESOURCE.
> 
> But also getting the base and size for PCI BAR 0, since the loop in that helper
> would already take care of that (it also starts at BAR 0).
> 
>>   }
>>   
>>   static int lynxfb_pci_probe(struct pci_dev *pdev,
>> diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
>> index b311c07fe66d..e5e362b8c9da 100644
>> --- a/drivers/video/fbdev/aty/radeon_base.c
>> +++ b/drivers/video/fbdev/aty/radeon_base.c
>> @@ -54,6 +54,7 @@
>>   
>>   #include "radeonfb.h"
>>   
>> +#include <linux/aperture.h>
>>   #include <linux/module.h>
>>   #include <linux/moduleparam.h>
>>   #include <linux/kernel.h>
>> @@ -2239,20 +2240,10 @@ static const struct bin_attribute edid2_attr = {
>>   
>>   static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
>>   {
>> -	struct apertures_struct *ap;
>> +	resource_size_t base = pci_resource_start(pdev, 0);
>> +	resource_size_t size = pci_resource_len(pdev, 0);
>>   
>> -	ap = alloc_apertures(1);
>> -	if (!ap)
>> -		return -ENOMEM;
>> -
>> -	ap->ranges[0].base = pci_resource_start(pdev, 0);
>> -	ap->ranges[0].size = pci_resource_len(pdev, 0);
>> -
>> -	remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
>> -
>> -	kfree(ap);
>> -
>> -	return 0;
>> +	return aperture_remove_conflicting_devices(base, size, KBUILD_MODNAME, false);
> 
> Same for this.
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 06/11] fbdev: Remove conflicting devices on PCI bus

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1850 bytes --]

Hi

Am 11.07.22 um 13:13 schrieb Javier Martinez Canillas:
> On 7/7/22 17:39, Thomas Zimmermann wrote:
>> Remove firmware devices on the PCI bus, by calling
>> aperture_remove_conflicting_pci_devices() in the probe function of
>> each related fbdev driver. iSo far, most of these drivers did not
>> remove conflicting VESA or EFI devices, or outride failed for
>> resource conflicts (i.e., matroxfb.) This must have been broken
>> for quite some time.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> @@ -949,6 +950,10 @@ static int ark_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>   	int rc;
>>   	u8 regval;
>>   
>> +	rc = aperture_remove_conflicting_pci_devices(dev, "arkfb");
>> +	if (rc < 0)
>> +		return rc;
>> +
> 
> I wonder if we could think of a trick to avoid open coding the same check in
> all drivers. Maybe a combination of using KBUILD_MODNAME for the name and
> a probe callback wrapper or something ?

Originally, I tried to hack this into register_framebuffer(), where the 
removal was originally located. But that's too late as there is 
conflicting resource access in most driver's probe function. The top of 
the probe function is the only place where this call can realistically 
be located. It's the same rule as with DRM drivers: the firmware driver 
needs to be gone before the native driver touches hardware.

Best regards
Thomas

> 
> But probably not worth to invest more in the fbdev drivers and could be done
> as a follow-up anyways if someone feels like it.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 05/11] fbdev: Convert drivers to aperture helpers

[Javier Martinez Canillas]

Hello Thomas,

On 7/15/22 13:48, Thomas Zimmermann wrote:

[...]

>>> +
>>> +	return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");
>>
>> Do you know why this can't just use aperture_remove_conflicting_pci_devices() ?
> 
> I simply don't want change too much at once, because there was this 
> problem with the PCI helper on ast.
>

Makes sense. Feel free to add:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

> At some point we can make a push to really fix this throughout the code 
> base. And that would include an update to fb_is_primary_device(), [1] 
> which doesn't fill well into the new interfaces.
>

Agreed.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 10/11] fbdev: Acquire framebuffer apertures for firmware devices

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1621 bytes --]

Hi

Am 11.07.22 um 13:29 schrieb Javier Martinez Canillas:
> On 7/7/22 17:39, Thomas Zimmermann wrote:
>> When registering a generic framebuffer, automatically acquire ownership
>> of the framebuffer's I/O range. The device will now be handled by the
>> aperture helpers. Fbdev-based conflict handling is no longer required.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/video/fbdev/core/fbmem.c | 33 ++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 2237049327db..e556ad69f48f 100644
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -13,6 +13,7 @@
>>   
>>   #include <linux/module.h>
>>   
>> +#include <linux/aperture.h>
>>   #include <linux/compat.h>
>>   #include <linux/types.h>
>>   #include <linux/errno.h>
>> @@ -1739,6 +1740,32 @@ static void do_unregister_framebuffer(struct fb_info *fb_info)
>>   	put_fb_info(fb_info);
>>   }
>>   
>> +static int fbm_aperture_acquire_for_platform_device(struct fb_info *fb_info)
>> +{
> 
> What's the meaning of 'm' here ? Misc, memory ? I would just call it 'fb_'.

'managed' as in drmm_ But using fb_ is also good. I actually wasn't sure 
about this naming.

Best regards
Thomas

> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]