[PATCH 0/2] of: Register platform device for each framebuffer

Move the detection of OF framebuffers from fbdev into of platform code
and register a Linux platform device for each framebuffer. Allows for
DRM-based OF drivers and real hot-unplugging of the framebuffer.

This patchset has been tested with qemu's ppc64le emulation, which
provides a framebuffer via OF display node. If someone has an older
32-bit system with BootX available, please test.

Thomas Zimmermann (2):
  of: Create platform devices for OF framebuffers
  fbdev: Remove hot-unplug workaround for framebuffers without device

 drivers/of/platform.c            | 73 ++++++++++++++++++++++--
 drivers/video/fbdev/core/fbmem.c |  9 +--
 drivers/video/fbdev/offb.c       | 98 +++++++++++++++++++++-----------
 3 files changed, 135 insertions(+), 45 deletions(-)


base-commit: 74ee32cc715cd9557c62aba937d6995851c68fe7
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: 6e1032c6302461624f33194c8b8f37103a3fa6ef
prerequisite-patch-id: dbf45768338ff1d944d093dc54bdffb3dc054b44
prerequisite-patch-id: 9c12c87b13a3519a93b81ca18299fae96ae4fefe
prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6
prerequisite-patch-id: ab7611d28d07723ab1dd392dcf9a6345de3b1040
-- 
2.35.1

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/of/platform.c      | 73 ++++++++++++++++++++++++++--
 drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
 2 files changed, 134 insertions(+), 37 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a16b74f32aa9..4c63b9a73587 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c

@@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
 }
 EXPORT_SYMBOL(of_platform_bus_probe);
 
+static int __init of_platform_populate_framebuffers(void)
+{
+	struct device_node *boot_display = NULL;
+	struct device_node *node;
+	struct platform_device *dev;
+	int ret;
+
+	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+	of_platform_device_create(node, NULL, NULL);
+	of_node_put(node);
+
+	/* Check if we have a MacOS display without a node spec */
+	if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
+		/*
+		 * The old code tried to work out which node was the MacOS
+		 * display based on the address. I'm dropping that since the
+		 * lack of a node spec only happens with old BootX versions
+		 * (users can update) and with this code, they'll still get
+		 * a display (just not the palette hacks).
+		 */
+		dev = platform_device_alloc("bootx-noscreen", 0);
+		if (WARN_ON(!dev))
+			return -ENOMEM;
+		ret = platform_device_add(dev);
+		if (WARN_ON(ret)) {
+			platform_device_put(dev);
+			return ret;
+		}
+	}
+
+	/*
+	 * For OF framebuffers, first create the device for the boot display,
+	 * then for the other framebuffers. Only fail for the boot display;
+	 * ignore errors for the rest.
+	 */
+	for_each_node_by_type(node, "display") {
+		if (!of_get_property(node, "linux,opened", NULL) ||
+		    !of_get_property(node, "linux,boot-display", NULL))
+			continue;
+		dev = of_platform_device_create(node, "of-display", NULL);
+		if (WARN_ON(!dev))
+			return -ENOMEM;
+		boot_display = node;
+		break;
+	}
+	for_each_node_by_type(node, "display") {
+		if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+			continue;
+		of_platform_device_create(node, "of-display", NULL);
+	}
+
+	return 0;
+}
+
 /**
  * of_platform_populate() - Populate platform_devices from device tree data
  * @root: parent of the first level to probe or NULL for the root of the tree

@@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void)
 		of_node_put(node);
 	}
 
-	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-	of_platform_device_create(node, NULL, NULL);
-	of_node_put(node);
+	of_platform_populate_framebuffers();
 
 	/* Populate everything else. */
 	of_platform_default_populate(NULL, NULL, NULL);

@@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void)
 	return 0;
 }
 arch_initcall_sync(of_platform_default_populate_init);
+#else
+static int __init of_platform_default_populate_init(void)
+{
+	device_links_supplier_sync_state_pause();
+
+	if (!of_have_populated_dt())
+		return -ENODEV;
+
+	of_platform_populate_framebuffers();
+
+	return 0;
+}
+arch_initcall_sync(of_platform_default_populate_init);
+#endif
 
 static int __init of_platform_sync_state_init(void)
 {

@@ -558,7 +624,6 @@ static int __init of_platform_sync_state_init(void)
 	return 0;
 }
 late_initcall_sync(of_platform_sync_state_init);
-#endif
 
 int of_platform_device_destroy(struct device *dev, void *data)
 {

diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
index afdb6aa48add..b1acb1ebebe9 100644
--- a/drivers/video/fbdev/offb.c
+++ b/drivers/video/fbdev/offb.c

@@ -386,10 +386,10 @@ static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp
 		FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_STATIC_PSEUDOCOLOR;
 }
 
-static void __init offb_init_fb(const char *name,
-				int width, int height, int depth,
-				int pitch, unsigned long address,
-				int foreign_endian, struct device_node *dp)
+static void offb_init_fb(struct platform_device *parent, const char *name,
+			 int width, int height, int depth,
+			 int pitch, unsigned long address,
+			 int foreign_endian, struct device_node *dp)
 {
 	unsigned long res_size = pitch * height;
 	struct offb_par *par = &default_par;

@@ -410,12 +410,13 @@ static void __init offb_init_fb(const char *name,
 		return;
 	}
 
-	info = framebuffer_alloc(sizeof(u32) * 16, NULL);
+	info = framebuffer_alloc(sizeof(u32) * 16, &parent->dev);
 
 	if (!info) {
 		release_mem_region(res_start, res_size);
 		return;
 	}
+	platform_set_drvdata(parent, info);
 
 	fix = &info->fix;
 	var = &info->var;

@@ -535,7 +536,8 @@ static void __init offb_init_fb(const char *name,
 }
 
 
-static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
+static void offb_init_nodriver(struct platform_device *parent, struct device_node *dp,
+			       int no_real_node)
 {
 	unsigned int len;
 	int i, width = 640, height = 480, depth = 8, pitch = 640;

@@ -650,46 +652,76 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
 		/* kludge for valkyrie */
 		if (of_node_name_eq(dp, "valkyrie"))
 			address += 0x1000;
-		offb_init_fb(no_real_node ? "bootx" : NULL,
+		offb_init_fb(parent, no_real_node ? "bootx" : NULL,
 			     width, height, depth, pitch, address,
 			     foreign_endian, no_real_node ? NULL : dp);
 	}
 }
 
-static int __init offb_init(void)
+static int offb_remove(struct platform_device *pdev)
 {
-	struct device_node *dp = NULL, *boot_disp = NULL;
+	struct fb_info *info = platform_get_drvdata(pdev);
 
-	if (fb_get_options("offb", NULL))
-		return -ENODEV;
+	if (info)
+		unregister_framebuffer(info);
 
-	/* Check if we have a MacOS display without a node spec */
-	if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL) != NULL) {
-		/* The old code tried to work out which node was the MacOS
-		 * display based on the address. I'm dropping that since the
-		 * lack of a node spec only happens with old BootX versions
-		 * (users can update) and with this code, they'll still get
-		 * a display (just not the palette hacks).
-		 */
-		offb_init_nodriver(of_chosen, 1);
-	}
+	return 0;
+}
 
-	for_each_node_by_type(dp, "display") {
-		if (of_get_property(dp, "linux,opened", NULL) &&
-		    of_get_property(dp, "linux,boot-display", NULL)) {
-			boot_disp = dp;
-			offb_init_nodriver(dp, 0);
-		}
-	}
-	for_each_node_by_type(dp, "display") {
-		if (of_get_property(dp, "linux,opened", NULL) &&
-		    dp != boot_disp)
-			offb_init_nodriver(dp, 0);
-	}
+static int offb_probe_bootx_noscreen(struct platform_device *pdev)
+{
+	offb_init_nodriver(pdev, of_chosen, 1);
 
 	return 0;
 }
 
+static struct platform_driver offb_driver_bootx_noscreen = {
+	.driver = {
+		.name = "bootx-noscreen",
+	},
+	.probe = offb_probe_bootx_noscreen,
+	.remove = offb_remove,
+};
+
+static int offb_probe_display(struct platform_device *pdev)
+{
+	offb_init_nodriver(pdev, pdev->dev.of_node, 0);
+
+	return 0;
+}
 
+static const struct of_device_id offb_of_match_display[] = {
+	{ .compatible = "display", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, offb_of_match_display);
+
+static struct platform_driver offb_driver_display = {
+	.driver = {
+		.name = "of-display",
+		.of_match_table = offb_of_match_display,
+	},
+	.probe = offb_probe_display,
+	.remove = offb_remove,
+};
+
+static int __init offb_init(void)
+{
+	if (fb_get_options("offb", NULL))
+		return -ENODEV;
+
+	platform_driver_register(&offb_driver_bootx_noscreen);
+	platform_driver_register(&offb_driver_display);
+
+	return 0;
+}
 module_init(offb_init);
+
+static void __exit offb_exit(void)
+{
+	platform_driver_unregister(&offb_driver_display);
+	platform_driver_unregister(&offb_driver_bootx_noscreen);
+}
+module_exit(offb_exit);
+
 MODULE_LICENSE("GPL");

-- 
2.35.1

Hello Thomas,

Thanks for working on this.

On 4/13/22 11:24, Thomas Zimmermann wrote:

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---

[snip]

+	for_each_node_by_type(node, "display") {
+		if (!of_get_property(node, "linux,opened", NULL) ||
+		    !of_get_property(node, "linux,boot-display", NULL))
+			continue;
+		dev = of_platform_device_create(node, "of-display", NULL);
+		if (WARN_ON(!dev))
+			return -ENOMEM;
+		boot_display = node;
+		break;
+	}
+	for_each_node_by_type(node, "display") {
+		if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+			continue;
+		of_platform_device_create(node, "of-display", NULL);

Shouldn't check for the return value here too ?

Other than this small nit, it looks good to me.

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

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Hi

Am 13.04.22 um 12:45 schrieb Javier Martinez Canillas:

Hello Thomas,

Thanks for working on this.

On 4/13/22 11:24, Thomas Zimmermann wrote:

quoted

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---

[snip]

quoted

+	for_each_node_by_type(node, "display") {
+		if (!of_get_property(node, "linux,opened", NULL) ||
+		    !of_get_property(node, "linux,boot-display", NULL))
+			continue;
+		dev = of_platform_device_create(node, "of-display", NULL);
+		if (WARN_ON(!dev))
+			return -ENOMEM;
+		boot_display = node;
+		break;
+	}
+	for_each_node_by_type(node, "display") {
+		if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+			continue;
+		of_platform_device_create(node, "of-display", NULL);

Shouldn't check for the return value here too ?

Failing is probably not useful, as it's not the main FB used for 
booting. Printing an error message wouldn't hurt, I guess. I'll also 
update the new driver registration in offb with an error message.

Other than this small nit, it looks good to me.

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

Thanks.

Best regards
Thomas

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

On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann [off-list ref] wrote:

The rest is PPC only, so bail out here if !PPC.

I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).

Hi

Am 13.04.22 um 14:51 schrieb Rob Herring:

On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann [off-list ref] wrote:

quoted

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
  drivers/of/platform.c      | 73 ++++++++++++++++++++++++++--
  drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
  2 files changed, 134 insertions(+), 37 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a16b74f32aa9..4c63b9a73587 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c

@@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
  }
  EXPORT_SYMBOL(of_platform_bus_probe);

+static int __init of_platform_populate_framebuffers(void)
+{
+       struct device_node *boot_display = NULL;
+       struct device_node *node;
+       struct platform_device *dev;
+       int ret;
+
+       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+       of_platform_device_create(node, NULL, NULL);
+       of_node_put(node);
+

The rest is PPC only, so bail out here if !PPC.

quoted

+       /* Check if we have a MacOS display without a node spec */
+       if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
+               /*
+                * The old code tried to work out which node was the MacOS
+                * display based on the address. I'm dropping that since the
+                * lack of a node spec only happens with old BootX versions
+                * (users can update) and with this code, they'll still get
+                * a display (just not the palette hacks).
+                */
+               dev = platform_device_alloc("bootx-noscreen", 0);
+               if (WARN_ON(!dev))
+                       return -ENOMEM;
+               ret = platform_device_add(dev);
+               if (WARN_ON(ret)) {
+                       platform_device_put(dev);
+                       return ret;
+               }
+       }
+
+       /*
+        * For OF framebuffers, first create the device for the boot display,
+        * then for the other framebuffers. Only fail for the boot display;
+        * ignore errors for the rest.
+        */
+       for_each_node_by_type(node, "display") {
+               if (!of_get_property(node, "linux,opened", NULL) ||
+                   !of_get_property(node, "linux,boot-display", NULL))
+                       continue;
+               dev = of_platform_device_create(node, "of-display", NULL);
+               if (WARN_ON(!dev))
+                       return -ENOMEM;
+               boot_display = node;
+               break;
+       }
+       for_each_node_by_type(node, "display") {
+               if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+                       continue;
+               of_platform_device_create(node, "of-display", NULL);
+       }
+
+       return 0;
+}
+
  /**
   * of_platform_populate() - Populate platform_devices from device tree data
   * @root: parent of the first level to probe or NULL for the root of the tree

@@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void)
                 of_node_put(node);
         }

-       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-       of_platform_device_create(node, NULL, NULL);
-       of_node_put(node);
+       of_platform_populate_framebuffers();

         /* Populate everything else. */
         of_platform_default_populate(NULL, NULL, NULL);

I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).

Together with the changes in of_platform_populate_framebuffers(), the 
code is more or less an "if-else" depending on PPC. I'll drop 
of_platform_populate_framebuffers() from the patch and make a separate 
implementation of of_platform_default_populate_init for PPC. Seems like 
the easiest solution to me.

Best regards
Thomas

quoted

@@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void)
         return 0;
  }
  arch_initcall_sync(of_platform_default_populate_init);
+#else
+static int __init of_platform_default_populate_init(void)
+{
+       device_links_supplier_sync_state_pause();
+
+       if (!of_have_populated_dt())
+               return -ENODEV;
+
+       of_platform_populate_framebuffers();
+
+       return 0;
+}
+arch_initcall_sync(of_platform_default_populate_init);
+#endif

  static int __init of_platform_sync_state_init(void)
  {

@@ -558,7 +624,6 @@ static int __init of_platform_sync_state_init(void)
         return 0;
  }
  late_initcall_sync(of_platform_sync_state_init);
-#endif

  int of_platform_device_destroy(struct device *dev, void *data)
  {

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

On 4/13/22 19:58, Thomas Zimmermann wrote:

Hi

[snip]

quoted

quoted

         /* Populate everything else. */
         of_platform_default_populate(NULL, NULL, NULL);

I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).

Together with the changes in of_platform_populate_framebuffers(), the 
code is more or less an "if-else" depending on PPC. I'll drop 
of_platform_populate_framebuffers() from the patch and make a separate 
implementation of of_platform_default_populate_init for PPC. Seems like 
the easiest solution to me.

That sounds reasonable to me as well. Feel free to retain my R-B tag
when posting v2.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

On Wed, Apr 13, 2022 at 12:58 PM Thomas Zimmermann [off-list ref] wrote:

Hi

Am 13.04.22 um 14:51 schrieb Rob Herring:

quoted

On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann [off-list ref] wrote:

quoted

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
  drivers/of/platform.c      | 73 ++++++++++++++++++++++++++--
  drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
  2 files changed, 134 insertions(+), 37 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a16b74f32aa9..4c63b9a73587 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c

@@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
  }
  EXPORT_SYMBOL(of_platform_bus_probe);

+static int __init of_platform_populate_framebuffers(void)
+{
+       struct device_node *boot_display = NULL;
+       struct device_node *node;
+       struct platform_device *dev;
+       int ret;
+
+       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+       of_platform_device_create(node, NULL, NULL);
+       of_node_put(node);
+

The rest is PPC only, so bail out here if !PPC.

quoted

+       /* Check if we have a MacOS display without a node spec */
+       if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
+               /*
+                * The old code tried to work out which node was the MacOS
+                * display based on the address. I'm dropping that since the
+                * lack of a node spec only happens with old BootX versions
+                * (users can update) and with this code, they'll still get
+                * a display (just not the palette hacks).
+                */
+               dev = platform_device_alloc("bootx-noscreen", 0);
+               if (WARN_ON(!dev))
+                       return -ENOMEM;
+               ret = platform_device_add(dev);
+               if (WARN_ON(ret)) {
+                       platform_device_put(dev);
+                       return ret;
+               }
+       }
+
+       /*
+        * For OF framebuffers, first create the device for the boot display,
+        * then for the other framebuffers. Only fail for the boot display;
+        * ignore errors for the rest.
+        */
+       for_each_node_by_type(node, "display") {
+               if (!of_get_property(node, "linux,opened", NULL) ||
+                   !of_get_property(node, "linux,boot-display", NULL))
+                       continue;
+               dev = of_platform_device_create(node, "of-display", NULL);
+               if (WARN_ON(!dev))
+                       return -ENOMEM;
+               boot_display = node;
+               break;
+       }
+       for_each_node_by_type(node, "display") {
+               if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+                       continue;
+               of_platform_device_create(node, "of-display", NULL);
+       }
+
+       return 0;
+}
+
  /**
   * of_platform_populate() - Populate platform_devices from device tree data
   * @root: parent of the first level to probe or NULL for the root of the tree

@@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void)
                 of_node_put(node);
         }

-       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-       of_platform_device_create(node, NULL, NULL);
-       of_node_put(node);
+       of_platform_populate_framebuffers();

         /* Populate everything else. */
         of_platform_default_populate(NULL, NULL, NULL);

I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).

Together with the changes in of_platform_populate_framebuffers(), the
code is more or less an "if-else" depending on PPC. I'll drop
of_platform_populate_framebuffers() from the patch and make a separate
implementation of of_platform_default_populate_init for PPC. Seems like
the easiest solution to me.

That just moves us farther from PPC ever using
of_platform_default_populate_init(). But I don't know that anyone in
PPC cares about that, so fine I guess.

Rob

 eOn Wed, Apr 13, 2022 at 1:46 PM Rob Herring [off-list ref] wrote:

On Wed, Apr 13, 2022 at 12:58 PM Thomas Zimmermann [off-list ref] wrote:

quoted

Hi

Am 13.04.22 um 14:51 schrieb Rob Herring:

quoted

On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann [off-list ref] wrote:

quoted

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
  drivers/of/platform.c      | 73 ++++++++++++++++++++++++++--
  drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
  2 files changed, 134 insertions(+), 37 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a16b74f32aa9..4c63b9a73587 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c

@@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
  }
  EXPORT_SYMBOL(of_platform_bus_probe);

+static int __init of_platform_populate_framebuffers(void)
+{
+       struct device_node *boot_display = NULL;
+       struct device_node *node;
+       struct platform_device *dev;
+       int ret;
+
+       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+       of_platform_device_create(node, NULL, NULL);
+       of_node_put(node);
+

The rest is PPC only, so bail out here if !PPC.

quoted

+       /* Check if we have a MacOS display without a node spec */
+       if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
+               /*
+                * The old code tried to work out which node was the MacOS
+                * display based on the address. I'm dropping that since the
+                * lack of a node spec only happens with old BootX versions
+                * (users can update) and with this code, they'll still get
+                * a display (just not the palette hacks).
+                */
+               dev = platform_device_alloc("bootx-noscreen", 0);
+               if (WARN_ON(!dev))
+                       return -ENOMEM;
+               ret = platform_device_add(dev);
+               if (WARN_ON(ret)) {
+                       platform_device_put(dev);
+                       return ret;
+               }
+       }
+
+       /*
+        * For OF framebuffers, first create the device for the boot display,
+        * then for the other framebuffers. Only fail for the boot display;
+        * ignore errors for the rest.
+        */
+       for_each_node_by_type(node, "display") {
+               if (!of_get_property(node, "linux,opened", NULL) ||
+                   !of_get_property(node, "linux,boot-display", NULL))
+                       continue;
+               dev = of_platform_device_create(node, "of-display", NULL);
+               if (WARN_ON(!dev))
+                       return -ENOMEM;
+               boot_display = node;
+               break;
+       }
+       for_each_node_by_type(node, "display") {
+               if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+                       continue;
+               of_platform_device_create(node, "of-display", NULL);
+       }
+
+       return 0;
+}
+
  /**
   * of_platform_populate() - Populate platform_devices from device tree data
   * @root: parent of the first level to probe or NULL for the root of the tree

@@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void)
                 of_node_put(node);
         }

-       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-       of_platform_device_create(node, NULL, NULL);
-       of_node_put(node);
+       of_platform_populate_framebuffers();

         /* Populate everything else. */
         of_platform_default_populate(NULL, NULL, NULL);

I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).

Together with the changes in of_platform_populate_framebuffers(), the
code is more or less an "if-else" depending on PPC. I'll drop
of_platform_populate_framebuffers() from the patch and make a separate
implementation of of_platform_default_populate_init for PPC. Seems like
the easiest solution to me.

That just moves us farther from PPC ever using
of_platform_default_populate_init(). But I don't know that anyone in
PPC cares about that, so fine I guess.

Actually, no. Make it work with IS_ENABLED(CONFIG_PPC) rather than an
#ifdef. Currently, I don't have to build this (or any of drivers/of/)
for PPC because it is IS_ENABLED(CONFIG_PPC) everywhere. Yes, there's
an #ifdef already, but there's not an #else and no PPC only code
compiled.

Rob

Hi

Am 13.04.22 um 14:51 schrieb Rob Herring:
...

quoted

+
  /**
   * of_platform_populate() - Populate platform_devices from device tree data
   * @root: parent of the first level to probe or NULL for the root of the tree

@@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void)
                 of_node_put(node);
         }

-       node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-       of_platform_device_create(node, NULL, NULL);
-       of_node_put(node);
+       of_platform_populate_framebuffers();

         /* Populate everything else. */
         of_platform_default_populate(NULL, NULL, NULL);

I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).

That didn't work. The boot process stops at some point. I'll send you an 
updated patch that covers most of the function with IS_ENABLED(CONFIG_PPC)

Best regards
Thomas

quoted

@@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void)
         return 0;
  }
  arch_initcall_sync(of_platform_default_populate_init);
+#else
+static int __init of_platform_default_populate_init(void)
+{
+       device_links_supplier_sync_state_pause();
+
+       if (!of_have_populated_dt())
+               return -ENODEV;
+
+       of_platform_populate_framebuffers();
+
+       return 0;
+}
+arch_initcall_sync(of_platform_default_populate_init);
+#endif

  static int __init of_platform_sync_state_init(void)
  {

@@ -558,7 +624,6 @@ static int __init of_platform_sync_state_init(void)
         return 0;
  }
  late_initcall_sync(of_platform_sync_state_init);
-#endif

  int of_platform_device_destroy(struct device *dev, void *data)
  {

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

A workaround makes fbdev hot-unplugging work for framebuffers without
device. The only user for this feature was offb. As each OF framebuffer
now has an associated platform device, the workaround is no longer
needed. Remove it. Effectively reverts commit 0f525289ff0d ("fbdev: Fix
unregistering of framebuffers without device").

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

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index bc6ed750e915..bdd00d381bbc 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c

@@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 			 * If it's not a platform device, at least print a warning. A
 			 * fix would add code to remove the device from the system.
 			 */
-			if (!device) {
-				/* TODO: Represent each OF framebuffer as its own
-				 * device in the device hierarchy. For now, offb
-				 * doesn't have such a device, so unregister the
-				 * framebuffer as before without warning.
-				 */
-				do_unregister_framebuffer(registered_fb[i]);
-			} else if (dev_is_platform(device)) {
+			if (dev_is_platform(device)) {
 				registered_fb[i]->forced_out = true;
 				platform_device_unregister(to_platform_device(device));
 			} else {

-- 
2.35.1

On 4/13/22 11:24, Thomas Zimmermann wrote:

Maybe we could still keep this for a couple of releases but with a big
warning that's not supported in case there are out-of-tree drivers out
there that still do this ?

Or at least a warning if the do_unregister_framebuffer() call is removed.

Regardless of what you chose to do, the patch looks good to me.

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

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

On Wed, Apr 13, 2022 at 12:50:50PM +0200, Javier Martinez Canillas wrote:

On 4/13/22 11:24, Thomas Zimmermann wrote:

quoted

A workaround makes fbdev hot-unplugging work for framebuffers without
device. The only user for this feature was offb. As each OF framebuffer
now has an associated platform device, the workaround is no longer
needed. Remove it. Effectively reverts commit 0f525289ff0d ("fbdev: Fix
unregistering of framebuffers without device").

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

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index bc6ed750e915..bdd00d381bbc 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c

@@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 			 * If it's not a platform device, at least print a warning. A
 			 * fix would add code to remove the device from the system.
 			 */
-			if (!device) {
-				/* TODO: Represent each OF framebuffer as its own
-				 * device in the device hierarchy. For now, offb
-				 * doesn't have such a device, so unregister the
-				 * framebuffer as before without warning.
-				 */
-				do_unregister_framebuffer(registered_fb[i]);

Maybe we could still keep this for a couple of releases but with a big
warning that's not supported in case there are out-of-tree drivers out
there that still do this ?

Or at least a warning if the do_unregister_framebuffer() call is removed.

Yeah dying while holding console_lock isn't fun, and not having a WARN_ON
+ bail-out code pretty much forces bug reporters to do a bisect here to
give us something more than "machine dies at boot with no messages".

I'd just outright keep the WARN_ON here for 1-2 years even to really make
sure we got all the bug reports, since often these older machines only
update onto LTS releases.

And it needs to be a WARN_ON + bail out since BUG_ON is as bad as just
oopsing.
-Daniel

Regardless of what you chose to do, the patch looks good to me.

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

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Hi

Am 13.04.22 um 18:05 schrieb Daniel Vetter:

On Wed, Apr 13, 2022 at 12:50:50PM +0200, Javier Martinez Canillas wrote:

quoted

On 4/13/22 11:24, Thomas Zimmermann wrote:

quoted

A workaround makes fbdev hot-unplugging work for framebuffers without
device. The only user for this feature was offb. As each OF framebuffer
now has an associated platform device, the workaround is no longer
needed. Remove it. Effectively reverts commit 0f525289ff0d ("fbdev: Fix
unregistering of framebuffers without device").

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

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index bc6ed750e915..bdd00d381bbc 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c

@@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
  			 * If it's not a platform device, at least print a warning. A
  			 * fix would add code to remove the device from the system.
  			 */
-			if (!device) {
-				/* TODO: Represent each OF framebuffer as its own
-				 * device in the device hierarchy. For now, offb
-				 * doesn't have such a device, so unregister the
-				 * framebuffer as before without warning.
-				 */
-				do_unregister_framebuffer(registered_fb[i]);

Maybe we could still keep this for a couple of releases but with a big
warning that's not supported in case there are out-of-tree drivers out
there that still do this ?

Or at least a warning if the do_unregister_framebuffer() call is removed.

Yeah dying while holding console_lock isn't fun, and not having a WARN_ON
+ bail-out code pretty much forces bug reporters to do a bisect here to
give us something more than "machine dies at boot with no messages".

I'd just outright keep the WARN_ON here for 1-2 years even to really make
sure we got all the bug reports, since often these older machines only
update onto LTS releases.

If that's what the consent is, I'll go with it.

I'm just not sure if we talk about the same problem. offb didn't have a 
platform device, so we recently added this workaround with 'if 
(!device)'.  All the other fbdev drivers have a platform device; and 
anything else that could fail is out-of-tree. We don't really care about 
those AFAIK.

With offb converted, we could practically remove all of the checks here 
and call platform_device_unregister() unconditionally.

Best regards
Thomas

And it needs to be a WARN_ON + bail out since BUG_ON is as bad as just
oopsing.
-Daniel

quoted

Regardless of what you chose to do, the patch looks good to me.

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

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

Hello Thomas,

On 4/13/22 20:09, Thomas Zimmermann wrote:

[snip]

quoted

quoted

quoted

index bc6ed750e915..bdd00d381bbc 100644

--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c

@@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
  			 * If it's not a platform device, at least print a warning. A
  			 * fix would add code to remove the device from the system.
  			 */
-			if (!device) {
-				/* TODO: Represent each OF framebuffer as its own
-				 * device in the device hierarchy. For now, offb
-				 * doesn't have such a device, so unregister the
-				 * framebuffer as before without warning.
-				 */
-				do_unregister_framebuffer(registered_fb[i]);

Maybe we could still keep this for a couple of releases but with a big
warning that's not supported in case there are out-of-tree drivers out
there that still do this ?

Or at least a warning if the do_unregister_framebuffer() call is removed.

Yeah dying while holding console_lock isn't fun, and not having a WARN_ON
+ bail-out code pretty much forces bug reporters to do a bisect here to
give us something more than "machine dies at boot with no messages".

I'd just outright keep the WARN_ON here for 1-2 years even to really make
sure we got all the bug reports, since often these older machines only
update onto LTS releases.

If that's what the consent is, I'll go with it.

I'm just not sure if we talk about the same problem. offb didn't have a 
platform device, so we recently added this workaround with 'if 
(!device)'.  All the other fbdev drivers have a platform device; and 
anything else that could fail is out-of-tree. We don't really care about 
those AFAIK.

Yes, agreed on the offb change but I'm not really sure if we don't care
about out-of-tree modules. I mean, you are right in theory but I still
feel that we are changing a core behavior without giving people time to
sort out if needed.

Since before commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices
on forced removal") registered FBs didn't need to have a device, but now
that will lead to a NULL pointer dereference in dev_is_platform(device).

And that change only landed in v5.18-rc1, so it is fairly recent.

I know that we follow https://www.kernel.org/doc/Documentation/process/stable-api-nonsense.rst
but still my opinion is that having a warning for a couple of releases
if registered_fb[i]->device is NULL, instead of just crashing would be
a better way to handle this.
 

With offb converted, we could practically remove all of the checks here 
and call platform_device_unregister() unconditionally.

Yes for mainline, but as mentioned I thought mostly about out-of-tree. If
folks agree that we shouldn't care about these, I'm Ok with that as well.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat