[PATCH 0/6] drm/omapdrm: Convert fbdev to DRM client

[PATCH 0/6] drm/omapdrm: Convert fbdev to DRM client

[Thomas Zimmermann]

Convert omapdrm's fbdev code to struct drm_client. Replaces the current
ad-hoc integration. The conversion includes a number of cleanups. As
with most other drivers' fbdev emulation, fbdev in omapdrm is now just
another DRM client that runs after the DRM device has been registered.

Once all drivers' fbdev emulation has been converted to struct drm_client,
we can attempt to add additional in-kernel clients. A DRM-based dmesg
log or a bootsplash are commonly mentioned. DRM can then switch easily
among the existing clients if/when required.

I did the conversion from similar experience with other drivers. But I
don't have the hardware to test this. Any testing is welcome.

Thomas Zimmermann (6):
  drm/omapdrm: Include <linux/of.h>
  drm/omapdrm: Remove fb from struct omap_fbdev
  drm/omapdrm: Remove bo from struct omap_fbdev
  drm/omapdrm: Remove fbdev from struct omap_drm_private
  drm/omapdrm: Initialize fbdev DRM client
  drm/omapdrm: Implement fbdev emulation as in-kernel client

 drivers/gpu/drm/omapdrm/omap_debugfs.c |   6 +-
 drivers/gpu/drm/omapdrm/omap_drv.c     |  13 +-
 drivers/gpu/drm/omapdrm/omap_drv.h     |   3 -
 drivers/gpu/drm/omapdrm/omap_fbdev.c   | 165 ++++++++++++++++---------
 drivers/gpu/drm/omapdrm/omap_fbdev.h   |   9 +-
 5 files changed, 114 insertions(+), 82 deletions(-)

-- 
2.40.0

[PATCH 1/6] drm/omapdrm: Include <linux/of.h>

[Thomas Zimmermann]

Include <linux/of.h> to get the contained declarations. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 699ed814e021..fb403b44769c 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -6,6 +6,7 @@
 
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include <linux/sort.h>
 #include <linux/sys_soc.h>
 
-- 
2.40.0

[PATCH 2/6] drm/omapdrm: Remove fb from struct omap_fbdev

[Thomas Zimmermann]

Fbdev's struct fb_helper stores a pointer to the framebuffer. Remove
struct omap_fbdev.fb, which contains the same value. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index a6c8542087ec..b3d57fe4e6ac 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -25,7 +25,6 @@ module_param_named(ywrap, ywrap_enabled, bool, 0644);
 
 struct omap_fbdev {
 	struct drm_fb_helper base;
-	struct drm_framebuffer *fb;
 	struct drm_gem_object *bo;
 	bool ywrap_enabled;
 
@@ -170,7 +169,6 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 
 	DBG("fbi=%p, dev=%p", fbi, dev);
 
-	fbdev->fb = fb;
 	helper->fb = fb;
 
 	fbi->fbops = &omap_fb_ops;
@@ -193,7 +191,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 
 
 	DBG("par=%p, %dx%d", fbi->par, fbi->var.xres, fbi->var.yres);
-	DBG("allocated %dx%d fb", fbdev->fb->width, fbdev->fb->height);
+	DBG("allocated %dx%d fb", fb->width, fb->height);
 
 	return 0;
 
@@ -266,6 +264,7 @@ void omap_fbdev_fini(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct drm_fb_helper *helper = priv->fbdev;
+	struct drm_framebuffer *fb;
 	struct omap_fbdev *fbdev;
 
 	DBG();
@@ -273,6 +272,8 @@ void omap_fbdev_fini(struct drm_device *dev)
 	if (!helper)
 		return;
 
+	fb = helper->fb;
+
 	drm_fb_helper_unregister_info(helper);
 
 	drm_fb_helper_fini(helper);
@@ -284,8 +285,8 @@ void omap_fbdev_fini(struct drm_device *dev)
 		omap_gem_unpin(fbdev->bo);
 
 	/* this will free the backing object */
-	if (fbdev->fb)
-		drm_framebuffer_remove(fbdev->fb);
+	if (fb)
+		drm_framebuffer_remove(fb);
 
 	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
-- 
2.40.0

[PATCH 3/6] drm/omapdrm: Remove bo from struct omap_fbdev

[Thomas Zimmermann]

Fbdev's framebuffer stores a pointer to the GEM object. Remove
struct omap_fbdev.exynos_gem, which contains the same value. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 32 +++++++++++++++++-----------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index b3d57fe4e6ac..d04a20f95e3d 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -10,6 +10,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 
 #include "omap_drv.h"
 
@@ -25,7 +26,6 @@ module_param_named(ywrap, ywrap_enabled, bool, 0644);
 
 struct omap_fbdev {
 	struct drm_fb_helper base;
-	struct drm_gem_object *bo;
 	bool ywrap_enabled;
 
 	/* for deferred dmm roll when getting called in atomic ctx */
@@ -37,12 +37,14 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi);
 static void pan_worker(struct work_struct *work)
 {
 	struct omap_fbdev *fbdev = container_of(work, struct omap_fbdev, work);
-	struct fb_info *fbi = fbdev->base.info;
+	struct drm_fb_helper *helper = &fbdev->base;
+	struct fb_info *fbi = helper->info;
+	struct drm_gem_object *bo = drm_gem_fb_get_obj(helper->fb, 0);
 	int npages;
 
 	/* DMM roll shifts in 4K pages: */
 	npages = fbi->fix.line_length >> PAGE_SHIFT;
-	omap_gem_roll(fbdev->bo, fbi->var.yoffset * npages);
+	omap_gem_roll(bo, fbi->var.yoffset * npages);
 }
 
 static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
@@ -97,6 +99,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	union omap_gem_size gsize;
 	struct fb_info *fbi = NULL;
 	struct drm_mode_fb_cmd2 mode_cmd = {0};
+	struct drm_gem_object *bo;
 	dma_addr_t dma_addr;
 	int ret;
 
@@ -127,20 +130,20 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 		.bytes = PAGE_ALIGN(mode_cmd.pitches[0] * mode_cmd.height),
 	};
 	DBG("allocating %d bytes for fb %d", gsize.bytes, dev->primary->index);
-	fbdev->bo = omap_gem_new(dev, gsize, OMAP_BO_SCANOUT | OMAP_BO_WC);
-	if (!fbdev->bo) {
+	bo = omap_gem_new(dev, gsize, OMAP_BO_SCANOUT | OMAP_BO_WC);
+	if (!bo) {
 		dev_err(dev->dev, "failed to allocate buffer object\n");
 		ret = -ENOMEM;
 		goto fail;
 	}
 
-	fb = omap_framebuffer_init(dev, &mode_cmd, &fbdev->bo);
+	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
 	if (IS_ERR(fb)) {
 		dev_err(dev->dev, "failed to allocate fb\n");
 		/* note: if fb creation failed, we can't rely on fb destroy
 		 * to unref the bo:
 		 */
-		drm_gem_object_put(fbdev->bo);
+		drm_gem_object_put(bo);
 		ret = PTR_ERR(fb);
 		goto fail;
 	}
@@ -153,7 +156,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	 * to it).  Then we just need to be sure that we are able to re-
 	 * pin it in case of an opps.
 	 */
-	ret = omap_gem_pin(fbdev->bo, &dma_addr);
+	ret = omap_gem_pin(bo, &dma_addr);
 	if (ret) {
 		dev_err(dev->dev, "could not pin framebuffer\n");
 		ret = -ENOMEM;
@@ -175,10 +178,10 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 
 	drm_fb_helper_fill_info(fbi, helper, sizes);
 
-	fbi->screen_buffer = omap_gem_vaddr(fbdev->bo);
-	fbi->screen_size = fbdev->bo->size;
+	fbi->screen_buffer = omap_gem_vaddr(bo);
+	fbi->screen_size = bo->size;
 	fbi->fix.smem_start = dma_addr;
-	fbi->fix.smem_len = fbdev->bo->size;
+	fbi->fix.smem_len = bo->size;
 
 	/* if we have DMM, then we can use it for scrolling by just
 	 * shuffling pages around in DMM rather than doing sw blit.
@@ -265,6 +268,7 @@ void omap_fbdev_fini(struct drm_device *dev)
 	struct omap_drm_private *priv = dev->dev_private;
 	struct drm_fb_helper *helper = priv->fbdev;
 	struct drm_framebuffer *fb;
+	struct drm_gem_object *bo;
 	struct omap_fbdev *fbdev;
 
 	DBG();
@@ -280,9 +284,11 @@ void omap_fbdev_fini(struct drm_device *dev)
 
 	fbdev = to_omap_fbdev(helper);
 
+	bo = drm_gem_fb_get_obj(fb, 0);
+
 	/* unpin the GEM object pinned in omap_fbdev_create() */
-	if (fbdev->bo)
-		omap_gem_unpin(fbdev->bo);
+	if (bo)
+		omap_gem_unpin(bo);
 
 	/* this will free the backing object */
 	if (fb)
-- 
2.40.0

[PATCH 4/6] drm/omapdrm: Remove fbdev from struct omap_drm_private

[Thomas Zimmermann]

The DRM device stores a pointer to the fbdev helper. Remove struct
omap_drm_private.fbdev, which contains the same value. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/omapdrm/omap_debugfs.c | 6 +++---
 drivers/gpu/drm/omapdrm/omap_drv.c     | 1 +
 drivers/gpu/drm/omapdrm/omap_drv.h     | 3 ---
 drivers/gpu/drm/omapdrm/omap_fbdev.c   | 7 ++-----
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c b/drivers/gpu/drm/omapdrm/omap_debugfs.c
index bfb2ccb40bd1..a3d470468e5b 100644
--- a/drivers/gpu/drm/omapdrm/omap_debugfs.c
+++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c
@@ -47,15 +47,15 @@ static int fb_show(struct seq_file *m, void *arg)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
-	struct omap_drm_private *priv = dev->dev_private;
+	struct drm_fb_helper *helper = dev->fb_helper;
 	struct drm_framebuffer *fb;
 
 	seq_printf(m, "fbcon ");
-	omap_framebuffer_describe(priv->fbdev->fb, m);
+	omap_framebuffer_describe(helper->fb, m);
 
 	mutex_lock(&dev->mode_config.fb_lock);
 	list_for_each_entry(fb, &dev->mode_config.fb_list, head) {
-		if (fb == priv->fbdev->fb)
+		if (fb == helper->fb)
 			continue;
 
 		seq_printf(m, "user ");
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index fb403b44769c..6a2f446c960f 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -25,6 +25,7 @@
 
 #include "omap_dmm_tiler.h"
 #include "omap_drv.h"
+#include "omap_fbdev.h"
 
 #define DRIVER_NAME		MODULE_NAME
 #define DRIVER_DESC		"OMAP DRM"
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 825960fd3ea9..4c7217b35f6b 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -21,7 +21,6 @@
 #include "omap_crtc.h"
 #include "omap_encoder.h"
 #include "omap_fb.h"
-#include "omap_fbdev.h"
 #include "omap_gem.h"
 #include "omap_irq.h"
 #include "omap_plane.h"
@@ -77,8 +76,6 @@ struct omap_drm_private {
 
 	struct drm_private_obj glob_obj;
 
-	struct drm_fb_helper *fbdev;
-
 	struct workqueue_struct *wq;
 
 	/* lock for obj_list below */
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index d04a20f95e3d..79e417b391bf 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -250,8 +250,6 @@ void omap_fbdev_init(struct drm_device *dev)
 	if (ret)
 		goto fini;
 
-	priv->fbdev = helper;
-
 	return;
 
 fini:
@@ -265,8 +263,7 @@ void omap_fbdev_init(struct drm_device *dev)
 
 void omap_fbdev_fini(struct drm_device *dev)
 {
-	struct omap_drm_private *priv = dev->dev_private;
-	struct drm_fb_helper *helper = priv->fbdev;
+	struct drm_fb_helper *helper = dev->fb_helper;
 	struct drm_framebuffer *fb;
 	struct drm_gem_object *bo;
 	struct omap_fbdev *fbdev;
@@ -297,5 +294,5 @@ void omap_fbdev_fini(struct drm_device *dev)
 	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
 
-	priv->fbdev = NULL;
+	dev->fb_helper = NULL;
 }
-- 
2.40.0

[PATCH 5/6] drm/omapdrm: Initialize fbdev DRM client

[Thomas Zimmermann]

Initialize the fbdev client in the fbdev code with empty helper
functions. Also clean up the client. The helpers will later
implement various functionality of the DRM client. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 33 +++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 79e417b391bf..f0e35f4764a7 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -221,6 +221,30 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi)
 	return fbi->par;
 }
 
+/*
+ * struct drm_client
+ */
+
+static void omap_fbdev_client_unregister(struct drm_client_dev *client)
+{ }
+
+static int omap_fbdev_client_restore(struct drm_client_dev *client)
+{
+	return 0;
+}
+
+static int omap_fbdev_client_hotplug(struct drm_client_dev *client)
+{
+	return 0;
+}
+
+static const struct drm_client_funcs omap_fbdev_client_funcs = {
+	.owner		= THIS_MODULE,
+	.unregister	= omap_fbdev_client_unregister,
+	.restore	= omap_fbdev_client_restore,
+	.hotplug	= omap_fbdev_client_hotplug,
+};
+
 /* initialize fbdev helper */
 void omap_fbdev_init(struct drm_device *dev)
 {
@@ -242,10 +266,14 @@ void omap_fbdev_init(struct drm_device *dev)
 
 	drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
 
-	ret = drm_fb_helper_init(dev, helper);
+	ret = drm_client_init(dev, &helper->client, "fbdev", &omap_fbdev_client_funcs);
 	if (ret)
 		goto fail;
 
+	ret = drm_fb_helper_init(dev, helper);
+	if (ret)
+		goto err_drm_client_release;
+
 	ret = drm_fb_helper_initial_config(helper);
 	if (ret)
 		goto fini;
@@ -254,6 +282,8 @@ void omap_fbdev_init(struct drm_device *dev)
 
 fini:
 	drm_fb_helper_fini(helper);
+err_drm_client_release:
+	drm_client_release(&helper->client);
 fail:
 	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
@@ -291,6 +321,7 @@ void omap_fbdev_fini(struct drm_device *dev)
 	if (fb)
 		drm_framebuffer_remove(fb);
 
+	drm_client_release(&helper->client);
 	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
 
-- 
2.40.0

[PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[Thomas Zimmermann]

Move code from ad-hoc fbdev callbacks into DRM client functions
and remove the old callbacks. The functions instruct the client
to poll for changed output or restore the display. The DRM core
calls both, the old callbacks and the new client helpers, from
the same places. The new functions perform the same operation as
before, so there's no change in functionality.

Replace all code that initializes or releases fbdev emulation
throughout the driver. Instead initialize the fbdev client by a
single call to omapdrm_fbdev_setup() after omapdrm has registered
its DRM device. As in most drivers, omapdrm's fbdev emulation now
acts like a regular DRM client.

The fbdev client setup consists of the initial preparation and the
hot-plugging of the display. The latter creates the fbdev device
and sets up the fbdev framebuffer. The setup performs display
hot-plugging once. If no display can be detected, DRM probe helpers
re-run the detection on each hotplug event.

A call to drm_dev_unregister() releases the client automatically.
No further action is required within omapdrm. If the fbdev
framebuffer has been fully set up, struct fb_ops.fb_destroy
implements the release. For partially initialized emulation, the
fbdev client reverts the initial setup.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/omapdrm/omap_drv.c   |  11 +--
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 136 ++++++++++++++-------------
 drivers/gpu/drm/omapdrm/omap_fbdev.h |   9 +-
 3 files changed, 77 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 6a2f446c960f..5ed549726104 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -15,7 +15,6 @@
 #include <drm/drm_bridge.h>
 #include <drm/drm_bridge_connector.h>
 #include <drm/drm_drv.h>
-#include <drm/drm_fb_helper.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_panel.h>
@@ -221,7 +220,6 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
 
 static const struct drm_mode_config_funcs omap_mode_config_funcs = {
 	.fb_create = omap_framebuffer_create,
-	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = omap_atomic_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
@@ -654,7 +652,6 @@ static const struct drm_driver omap_drm_driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM  |
 		DRIVER_ATOMIC | DRIVER_RENDER,
 	.open = dev_open,
-	.lastclose = drm_fb_helper_lastclose,
 #ifdef CONFIG_DEBUG_FS
 	.debugfs_init = omap_debugfs_init,
 #endif
@@ -743,8 +740,6 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 		goto err_cleanup_modeset;
 	}
 
-	omap_fbdev_init(ddev);
-
 	drm_kms_helper_poll_init(ddev);
 
 	/*
@@ -755,12 +750,12 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	if (ret)
 		goto err_cleanup_helpers;
 
+	omap_fbdev_setup(ddev);
+
 	return 0;
 
 err_cleanup_helpers:
 	drm_kms_helper_poll_fini(ddev);
-
-	omap_fbdev_fini(ddev);
 err_cleanup_modeset:
 	omap_modeset_fini(ddev);
 err_free_overlays:
@@ -786,8 +781,6 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
 
 	drm_kms_helper_poll_fini(ddev);
 
-	omap_fbdev_fini(ddev);
-
 	drm_atomic_helper_shutdown(ddev);
 
 	omap_modeset_fini(ddev);
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index f0e35f4764a7..cd63142a4705 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -4,13 +4,14 @@
  * Author: Rob Clark <rob@ti.com>
  */
 
-#include <drm/drm_crtc.h>
-#include <drm/drm_util.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_file.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_util.h>
 
 #include "omap_drv.h"
 
@@ -72,6 +73,25 @@ static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
 	return drm_fb_helper_pan_display(var, fbi);
 }
 
+static void omap_fbdev_fb_destroy(struct fb_info *info)
+{
+	struct drm_fb_helper *helper = info->par;
+	struct drm_framebuffer *fb = helper->fb;
+	struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
+	struct omap_fbdev *fbdev = to_omap_fbdev(helper);
+
+	DBG();
+
+	drm_fb_helper_fini(helper);
+
+	omap_gem_unpin(bo);
+	drm_framebuffer_remove(fb);
+
+	drm_client_release(&helper->client);
+	drm_fb_helper_unprepare(helper);
+	kfree(fbdev);
+}
+
 static const struct fb_ops omap_fb_ops = {
 	.owner = THIS_MODULE,
 
@@ -87,6 +107,8 @@ static const struct fb_ops omap_fb_ops = {
 	.fb_fillrect = drm_fb_helper_sys_fillrect,
 	.fb_copyarea = drm_fb_helper_sys_copyarea,
 	.fb_imageblit = drm_fb_helper_sys_imageblit,
+
+	.fb_destroy = omap_fbdev_fb_destroy,
 };
 
 static int omap_fbdev_create(struct drm_fb_helper *helper,
@@ -226,16 +248,52 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi)
  */
 
 static void omap_fbdev_client_unregister(struct drm_client_dev *client)
-{ }
+{
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+
+	if (fb_helper->info) {
+		drm_fb_helper_unregister_info(fb_helper);
+	} else {
+		drm_client_release(&fb_helper->client);
+		drm_fb_helper_unprepare(fb_helper);
+		kfree(fb_helper);
+	}
+}
 
 static int omap_fbdev_client_restore(struct drm_client_dev *client)
 {
+	drm_fb_helper_lastclose(client->dev);
+
 	return 0;
 }
 
 static int omap_fbdev_client_hotplug(struct drm_client_dev *client)
 {
+	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
+	struct drm_device *dev = client->dev;
+	int ret;
+
+	if (dev->fb_helper)
+		return drm_fb_helper_hotplug_event(dev->fb_helper);
+
+	ret = drm_fb_helper_init(dev, fb_helper);
+	if (ret)
+		goto err_drm_err;
+
+	if (!drm_drv_uses_atomic_modeset(dev))
+		drm_helper_disable_unused_functions(dev);
+
+	ret = drm_fb_helper_initial_config(fb_helper);
+	if (ret)
+		goto err_drm_fb_helper_fini;
+
 	return 0;
+
+err_drm_fb_helper_fini:
+	drm_fb_helper_fini(fb_helper);
+err_drm_err:
+	drm_err(dev, "Failed to setup fbdev emulation (ret=%d)\n", ret);
+	return ret;
 }
 
 static const struct drm_client_funcs omap_fbdev_client_funcs = {
@@ -245,85 +303,37 @@ static const struct drm_client_funcs omap_fbdev_client_funcs = {
 	.hotplug	= omap_fbdev_client_hotplug,
 };
 
-/* initialize fbdev helper */
-void omap_fbdev_init(struct drm_device *dev)
+void omap_fbdev_setup(struct drm_device *dev)
 {
-	struct omap_drm_private *priv = dev->dev_private;
-	struct omap_fbdev *fbdev = NULL;
+	struct omap_fbdev *fbdev;
 	struct drm_fb_helper *helper;
-	int ret = 0;
+	int ret;
 
-	if (!priv->num_pipes)
-		return;
+	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
+	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
 
 	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
 	if (!fbdev)
 		return;
-
-	INIT_WORK(&fbdev->work, pan_worker);
-
 	helper = &fbdev->base;
 
 	drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
 
 	ret = drm_client_init(dev, &helper->client, "fbdev", &omap_fbdev_client_funcs);
 	if (ret)
-		goto fail;
+		goto err_drm_client_init;
 
-	ret = drm_fb_helper_init(dev, helper);
-	if (ret)
-		goto err_drm_client_release;
+	INIT_WORK(&fbdev->work, pan_worker);
 
-	ret = drm_fb_helper_initial_config(helper);
+	ret = omap_fbdev_client_hotplug(&helper->client);
 	if (ret)
-		goto fini;
-
-	return;
+		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
 
-fini:
-	drm_fb_helper_fini(helper);
-err_drm_client_release:
-	drm_client_release(&helper->client);
-fail:
-	drm_fb_helper_unprepare(helper);
-	kfree(fbdev);
+	drm_client_register(&helper->client);
 
-	dev_warn(dev->dev, "omap_fbdev_init failed\n");
-}
-
-void omap_fbdev_fini(struct drm_device *dev)
-{
-	struct drm_fb_helper *helper = dev->fb_helper;
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *bo;
-	struct omap_fbdev *fbdev;
-
-	DBG();
-
-	if (!helper)
-		return;
-
-	fb = helper->fb;
-
-	drm_fb_helper_unregister_info(helper);
-
-	drm_fb_helper_fini(helper);
-
-	fbdev = to_omap_fbdev(helper);
-
-	bo = drm_gem_fb_get_obj(fb, 0);
-
-	/* unpin the GEM object pinned in omap_fbdev_create() */
-	if (bo)
-		omap_gem_unpin(bo);
-
-	/* this will free the backing object */
-	if (fb)
-		drm_framebuffer_remove(fb);
+	return;
 
-	drm_client_release(&helper->client);
+err_drm_client_init:
 	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
-
-	dev->fb_helper = NULL;
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h b/drivers/gpu/drm/omapdrm/omap_fbdev.h
index 74a68a5a6eab..74c691a8d45f 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.h
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h
@@ -10,16 +10,11 @@
 #define __OMAPDRM_FBDEV_H__
 
 struct drm_device;
-struct drm_fb_helper;
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
-void omap_fbdev_init(struct drm_device *dev);
-void omap_fbdev_fini(struct drm_device *dev);
+void omap_fbdev_setup(struct drm_device *dev);
 #else
-static inline void omap_fbdev_init(struct drm_device *dev)
-{
-}
-static inline void omap_fbdev_fini(struct drm_device *dev)
+static inline void omap_fbdev_setup(struct drm_device *dev)
 {
 }
 #endif
-- 
2.40.0

Re: [PATCH 1/6] drm/omapdrm: Include <linux/of.h>

[Tomi Valkeinen]

On 30/03/2023 11:32, Thomas Zimmermann wrote:
> Include <linux/of.h> to get the contained declarations. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/omapdrm/omap_drv.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 699ed814e021..fb403b44769c 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -6,6 +6,7 @@
>   
>   #include <linux/dma-mapping.h>
>   #include <linux/platform_device.h>
> +#include <linux/of.h>
>   #include <linux/sort.h>
>   #include <linux/sys_soc.h>
>   

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH 2/6] drm/omapdrm: Remove fb from struct omap_fbdev

[Tomi Valkeinen]

On 30/03/2023 11:32, Thomas Zimmermann wrote:
> Fbdev's struct fb_helper stores a pointer to the framebuffer. Remove
> struct omap_fbdev.fb, which contains the same value. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index a6c8542087ec..b3d57fe4e6ac 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -25,7 +25,6 @@ module_param_named(ywrap, ywrap_enabled, bool, 0644);
>   
>   struct omap_fbdev {
>   	struct drm_fb_helper base;
> -	struct drm_framebuffer *fb;
>   	struct drm_gem_object *bo;
>   	bool ywrap_enabled;
>   
> @@ -170,7 +169,6 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   
>   	DBG("fbi=%p, dev=%p", fbi, dev);
>   
> -	fbdev->fb = fb;
>   	helper->fb = fb;
>   
>   	fbi->fbops = &omap_fb_ops;
> @@ -193,7 +191,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   
>   
>   	DBG("par=%p, %dx%d", fbi->par, fbi->var.xres, fbi->var.yres);
> -	DBG("allocated %dx%d fb", fbdev->fb->width, fbdev->fb->height);
> +	DBG("allocated %dx%d fb", fb->width, fb->height);
>   
>   	return 0;
>   
> @@ -266,6 +264,7 @@ void omap_fbdev_fini(struct drm_device *dev)
>   {
>   	struct omap_drm_private *priv = dev->dev_private;
>   	struct drm_fb_helper *helper = priv->fbdev;
> +	struct drm_framebuffer *fb;
>   	struct omap_fbdev *fbdev;
>   
>   	DBG();
> @@ -273,6 +272,8 @@ void omap_fbdev_fini(struct drm_device *dev)
>   	if (!helper)
>   		return;
>   
> +	fb = helper->fb;
> +
>   	drm_fb_helper_unregister_info(helper);
>   
>   	drm_fb_helper_fini(helper);
> @@ -284,8 +285,8 @@ void omap_fbdev_fini(struct drm_device *dev)
>   		omap_gem_unpin(fbdev->bo);
>   
>   	/* this will free the backing object */
> -	if (fbdev->fb)
> -		drm_framebuffer_remove(fbdev->fb);
> +	if (fb)
> +		drm_framebuffer_remove(fb);
>   
>   	drm_fb_helper_unprepare(helper);
>   	kfree(fbdev);


Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH 3/6] drm/omapdrm: Remove bo from struct omap_fbdev

[Tomi Valkeinen]

On 30/03/2023 11:32, Thomas Zimmermann wrote:
> Fbdev's framebuffer stores a pointer to the GEM object. Remove
> struct omap_fbdev.exynos_gem, which contains the same value. No

Copy paste error? =)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 32 +++++++++++++++++-----------
>   1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index b3d57fe4e6ac..d04a20f95e3d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -10,6 +10,7 @@
>   #include <drm/drm_file.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>   
>   #include "omap_drv.h"
>   
> @@ -25,7 +26,6 @@ module_param_named(ywrap, ywrap_enabled, bool, 0644);
>   
>   struct omap_fbdev {
>   	struct drm_fb_helper base;
> -	struct drm_gem_object *bo;
>   	bool ywrap_enabled;
>   
>   	/* for deferred dmm roll when getting called in atomic ctx */
> @@ -37,12 +37,14 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi);
>   static void pan_worker(struct work_struct *work)
>   {
>   	struct omap_fbdev *fbdev = container_of(work, struct omap_fbdev, work);
> -	struct fb_info *fbi = fbdev->base.info;
> +	struct drm_fb_helper *helper = &fbdev->base;
> +	struct fb_info *fbi = helper->info;
> +	struct drm_gem_object *bo = drm_gem_fb_get_obj(helper->fb, 0);
>   	int npages;
>   
>   	/* DMM roll shifts in 4K pages: */
>   	npages = fbi->fix.line_length >> PAGE_SHIFT;
> -	omap_gem_roll(fbdev->bo, fbi->var.yoffset * npages);
> +	omap_gem_roll(bo, fbi->var.yoffset * npages);
>   }
>   
>   static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
> @@ -97,6 +99,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   	union omap_gem_size gsize;
>   	struct fb_info *fbi = NULL;
>   	struct drm_mode_fb_cmd2 mode_cmd = {0};
> +	struct drm_gem_object *bo;
>   	dma_addr_t dma_addr;
>   	int ret;
>   
> @@ -127,20 +130,20 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   		.bytes = PAGE_ALIGN(mode_cmd.pitches[0] * mode_cmd.height),
>   	};
>   	DBG("allocating %d bytes for fb %d", gsize.bytes, dev->primary->index);
> -	fbdev->bo = omap_gem_new(dev, gsize, OMAP_BO_SCANOUT | OMAP_BO_WC);
> -	if (!fbdev->bo) {
> +	bo = omap_gem_new(dev, gsize, OMAP_BO_SCANOUT | OMAP_BO_WC);
> +	if (!bo) {
>   		dev_err(dev->dev, "failed to allocate buffer object\n");
>   		ret = -ENOMEM;
>   		goto fail;
>   	}
>   
> -	fb = omap_framebuffer_init(dev, &mode_cmd, &fbdev->bo);
> +	fb = omap_framebuffer_init(dev, &mode_cmd, &bo);
>   	if (IS_ERR(fb)) {
>   		dev_err(dev->dev, "failed to allocate fb\n");
>   		/* note: if fb creation failed, we can't rely on fb destroy
>   		 * to unref the bo:
>   		 */
> -		drm_gem_object_put(fbdev->bo);
> +		drm_gem_object_put(bo);
>   		ret = PTR_ERR(fb);
>   		goto fail;
>   	}
> @@ -153,7 +156,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   	 * to it).  Then we just need to be sure that we are able to re-
>   	 * pin it in case of an opps.
>   	 */
> -	ret = omap_gem_pin(fbdev->bo, &dma_addr);
> +	ret = omap_gem_pin(bo, &dma_addr);
>   	if (ret) {
>   		dev_err(dev->dev, "could not pin framebuffer\n");
>   		ret = -ENOMEM;
> @@ -175,10 +178,10 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
>   
>   	drm_fb_helper_fill_info(fbi, helper, sizes);
>   
> -	fbi->screen_buffer = omap_gem_vaddr(fbdev->bo);
> -	fbi->screen_size = fbdev->bo->size;
> +	fbi->screen_buffer = omap_gem_vaddr(bo);
> +	fbi->screen_size = bo->size;
>   	fbi->fix.smem_start = dma_addr;
> -	fbi->fix.smem_len = fbdev->bo->size;
> +	fbi->fix.smem_len = bo->size;
>   
>   	/* if we have DMM, then we can use it for scrolling by just
>   	 * shuffling pages around in DMM rather than doing sw blit.
> @@ -265,6 +268,7 @@ void omap_fbdev_fini(struct drm_device *dev)
>   	struct omap_drm_private *priv = dev->dev_private;
>   	struct drm_fb_helper *helper = priv->fbdev;
>   	struct drm_framebuffer *fb;
> +	struct drm_gem_object *bo;
>   	struct omap_fbdev *fbdev;
>   
>   	DBG();
> @@ -280,9 +284,11 @@ void omap_fbdev_fini(struct drm_device *dev)
>   
>   	fbdev = to_omap_fbdev(helper);
>   
> +	bo = drm_gem_fb_get_obj(fb, 0);
> +
>   	/* unpin the GEM object pinned in omap_fbdev_create() */
> -	if (fbdev->bo)
> -		omap_gem_unpin(fbdev->bo);
> +	if (bo)
> +		omap_gem_unpin(bo);
>   
>   	/* this will free the backing object */
>   	if (fb)

Re: [PATCH 4/6] drm/omapdrm: Remove fbdev from struct omap_drm_private

[Tomi Valkeinen]

On 30/03/2023 11:32, Thomas Zimmermann wrote:
> The DRM device stores a pointer to the fbdev helper. Remove struct
> omap_drm_private.fbdev, which contains the same value. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/omapdrm/omap_debugfs.c | 6 +++---
>   drivers/gpu/drm/omapdrm/omap_drv.c     | 1 +
>   drivers/gpu/drm/omapdrm/omap_drv.h     | 3 ---
>   drivers/gpu/drm/omapdrm/omap_fbdev.c   | 7 ++-----
>   4 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c b/drivers/gpu/drm/omapdrm/omap_debugfs.c
> index bfb2ccb40bd1..a3d470468e5b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_debugfs.c
> +++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c
> @@ -47,15 +47,15 @@ static int fb_show(struct seq_file *m, void *arg)
>   {
>   	struct drm_info_node *node = (struct drm_info_node *) m->private;
>   	struct drm_device *dev = node->minor->dev;
> -	struct omap_drm_private *priv = dev->dev_private;
> +	struct drm_fb_helper *helper = dev->fb_helper;
>   	struct drm_framebuffer *fb;
>   
>   	seq_printf(m, "fbcon ");
> -	omap_framebuffer_describe(priv->fbdev->fb, m);
> +	omap_framebuffer_describe(helper->fb, m);
>   
>   	mutex_lock(&dev->mode_config.fb_lock);
>   	list_for_each_entry(fb, &dev->mode_config.fb_list, head) {
> -		if (fb == priv->fbdev->fb)
> +		if (fb == helper->fb)
>   			continue;
>   
>   		seq_printf(m, "user ");
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index fb403b44769c..6a2f446c960f 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -25,6 +25,7 @@
>   
>   #include "omap_dmm_tiler.h"
>   #include "omap_drv.h"
> +#include "omap_fbdev.h"
>   
>   #define DRIVER_NAME		MODULE_NAME
>   #define DRIVER_DESC		"OMAP DRM"
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 825960fd3ea9..4c7217b35f6b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -21,7 +21,6 @@
>   #include "omap_crtc.h"
>   #include "omap_encoder.h"
>   #include "omap_fb.h"
> -#include "omap_fbdev.h"
>   #include "omap_gem.h"
>   #include "omap_irq.h"
>   #include "omap_plane.h"
> @@ -77,8 +76,6 @@ struct omap_drm_private {
>   
>   	struct drm_private_obj glob_obj;
>   
> -	struct drm_fb_helper *fbdev;
> -
>   	struct workqueue_struct *wq;
>   
>   	/* lock for obj_list below */
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index d04a20f95e3d..79e417b391bf 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -250,8 +250,6 @@ void omap_fbdev_init(struct drm_device *dev)
>   	if (ret)
>   		goto fini;
>   
> -	priv->fbdev = helper;
> -
>   	return;
>   
>   fini:
> @@ -265,8 +263,7 @@ void omap_fbdev_init(struct drm_device *dev)
>   
>   void omap_fbdev_fini(struct drm_device *dev)
>   {
> -	struct omap_drm_private *priv = dev->dev_private;
> -	struct drm_fb_helper *helper = priv->fbdev;
> +	struct drm_fb_helper *helper = dev->fb_helper;
>   	struct drm_framebuffer *fb;
>   	struct drm_gem_object *bo;
>   	struct omap_fbdev *fbdev;
> @@ -297,5 +294,5 @@ void omap_fbdev_fini(struct drm_device *dev)
>   	drm_fb_helper_unprepare(helper);
>   	kfree(fbdev);
>   
> -	priv->fbdev = NULL;
> +	dev->fb_helper = NULL;

Is this line needed anymore? As you dropped the priv->fbdev assignment 
in omap_fbdev_init(), it would look symmetrical to also drop this one. 
I'm sure it doesn't hurt, just wondering if this is something drivers 
are supposed to do, or is this just an extra line in the driver.

In either case:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH 5/6] drm/omapdrm: Initialize fbdev DRM client

[Tomi Valkeinen]

On 30/03/2023 11:32, Thomas Zimmermann wrote:
> Initialize the fbdev client in the fbdev code with empty helper
> functions. Also clean up the client. The helpers will later
> implement various functionality of the DRM client. No functional
> changes.

I don't see this doing any cleanups.

I think this could be as well merged to the next patch, as this is such 
a short one.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 33 +++++++++++++++++++++++++++-
>   1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index 79e417b391bf..f0e35f4764a7 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -221,6 +221,30 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi)
>   	return fbi->par;
>   }
>   
> +/*
> + * struct drm_client
> + */
> +
> +static void omap_fbdev_client_unregister(struct drm_client_dev *client)
> +{ }
> +
> +static int omap_fbdev_client_restore(struct drm_client_dev *client)
> +{
> +	return 0;
> +}
> +
> +static int omap_fbdev_client_hotplug(struct drm_client_dev *client)
> +{
> +	return 0;
> +}
> +
> +static const struct drm_client_funcs omap_fbdev_client_funcs = {
> +	.owner		= THIS_MODULE,
> +	.unregister	= omap_fbdev_client_unregister,
> +	.restore	= omap_fbdev_client_restore,
> +	.hotplug	= omap_fbdev_client_hotplug,
> +};
> +
>   /* initialize fbdev helper */
>   void omap_fbdev_init(struct drm_device *dev)
>   {
> @@ -242,10 +266,14 @@ void omap_fbdev_init(struct drm_device *dev)
>   
>   	drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
>   
> -	ret = drm_fb_helper_init(dev, helper);
> +	ret = drm_client_init(dev, &helper->client, "fbdev", &omap_fbdev_client_funcs);
>   	if (ret)
>   		goto fail;
>   
> +	ret = drm_fb_helper_init(dev, helper);
> +	if (ret)
> +		goto err_drm_client_release;
> +
>   	ret = drm_fb_helper_initial_config(helper);
>   	if (ret)
>   		goto fini;
> @@ -254,6 +282,8 @@ void omap_fbdev_init(struct drm_device *dev)
>   
>   fini:
>   	drm_fb_helper_fini(helper);
> +err_drm_client_release:
> +	drm_client_release(&helper->client);
>   fail:
>   	drm_fb_helper_unprepare(helper);
>   	kfree(fbdev);
> @@ -291,6 +321,7 @@ void omap_fbdev_fini(struct drm_device *dev)
>   	if (fb)
>   		drm_framebuffer_remove(fb);
>   
> +	drm_client_release(&helper->client);
>   	drm_fb_helper_unprepare(helper);
>   	kfree(fbdev);
>   

Re: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[kernel test robot]

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230330083205.12621-7-tzimmermann%40suse.de
patch subject: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20230330/202303302056.9US55Dt4-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
        git checkout 5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/drm/omapdrm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303302056.9US55Dt4-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/omapdrm/omap_fbdev.c:306:6: warning: no previous prototype for 'omap_fbdev_setup' [-Wmissing-prototypes]
     306 | void omap_fbdev_setup(struct drm_device *dev)
         |      ^~~~~~~~~~~~~~~~


vim +/omap_fbdev_setup +306 drivers/gpu/drm/omapdrm/omap_fbdev.c

   305	
 > 306	void omap_fbdev_setup(struct drm_device *dev)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[kernel test robot]

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230330083205.12621-7-tzimmermann%40suse.de
patch subject: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20230330/202303302056.9US55Dt4-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
        git checkout 5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/drm/omapdrm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303302056.9US55Dt4-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/omapdrm/omap_fbdev.c:306:6: warning: no previous prototype for 'omap_fbdev_setup' [-Wmissing-prototypes]
     306 | void omap_fbdev_setup(struct drm_device *dev)
         |      ^~~~~~~~~~~~~~~~


vim +/omap_fbdev_setup +306 drivers/gpu/drm/omapdrm/omap_fbdev.c

   305	
 > 306	void omap_fbdev_setup(struct drm_device *dev)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[Tomi Valkeinen]

On 30/03/2023 11:32, Thomas Zimmermann wrote:
> Move code from ad-hoc fbdev callbacks into DRM client functions
> and remove the old callbacks. The functions instruct the client
> to poll for changed output or restore the display. The DRM core
> calls both, the old callbacks and the new client helpers, from
> the same places. The new functions perform the same operation as
> before, so there's no change in functionality.
> 
> Replace all code that initializes or releases fbdev emulation
> throughout the driver. Instead initialize the fbdev client by a
> single call to omapdrm_fbdev_setup() after omapdrm has registered
> its DRM device. As in most drivers, omapdrm's fbdev emulation now
> acts like a regular DRM client.
> 
> The fbdev client setup consists of the initial preparation and the
> hot-plugging of the display. The latter creates the fbdev device
> and sets up the fbdev framebuffer. The setup performs display
> hot-plugging once. If no display can be detected, DRM probe helpers
> re-run the detection on each hotplug event.
> 
> A call to drm_dev_unregister() releases the client automatically.
> No further action is required within omapdrm. If the fbdev
> framebuffer has been fully set up, struct fb_ops.fb_destroy
> implements the release. For partially initialized emulation, the
> fbdev client reverts the initial setup.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/omapdrm/omap_drv.c   |  11 +--
>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 136 ++++++++++++++-------------
>   drivers/gpu/drm/omapdrm/omap_fbdev.h |   9 +-
>   3 files changed, 77 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 6a2f446c960f..5ed549726104 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -15,7 +15,6 @@
>   #include <drm/drm_bridge.h>
>   #include <drm/drm_bridge_connector.h>
>   #include <drm/drm_drv.h>
> -#include <drm/drm_fb_helper.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_ioctl.h>
>   #include <drm/drm_panel.h>
> @@ -221,7 +220,6 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
>   
>   static const struct drm_mode_config_funcs omap_mode_config_funcs = {
>   	.fb_create = omap_framebuffer_create,
> -	.output_poll_changed = drm_fb_helper_output_poll_changed,
>   	.atomic_check = omap_atomic_check,
>   	.atomic_commit = drm_atomic_helper_commit,
>   };
> @@ -654,7 +652,6 @@ static const struct drm_driver omap_drm_driver = {
>   	.driver_features = DRIVER_MODESET | DRIVER_GEM  |
>   		DRIVER_ATOMIC | DRIVER_RENDER,
>   	.open = dev_open,
> -	.lastclose = drm_fb_helper_lastclose,
>   #ifdef CONFIG_DEBUG_FS
>   	.debugfs_init = omap_debugfs_init,
>   #endif
> @@ -743,8 +740,6 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
>   		goto err_cleanup_modeset;
>   	}
>   
> -	omap_fbdev_init(ddev);
> -
>   	drm_kms_helper_poll_init(ddev);
>   
>   	/*
> @@ -755,12 +750,12 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
>   	if (ret)
>   		goto err_cleanup_helpers;
>   
> +	omap_fbdev_setup(ddev);
> +
>   	return 0;
>   
>   err_cleanup_helpers:
>   	drm_kms_helper_poll_fini(ddev);
> -
> -	omap_fbdev_fini(ddev);
>   err_cleanup_modeset:
>   	omap_modeset_fini(ddev);
>   err_free_overlays:
> @@ -786,8 +781,6 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
>   
>   	drm_kms_helper_poll_fini(ddev);
>   
> -	omap_fbdev_fini(ddev);
> -
>   	drm_atomic_helper_shutdown(ddev);
>   
>   	omap_modeset_fini(ddev);
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index f0e35f4764a7..cd63142a4705 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -4,13 +4,14 @@
>    * Author: Rob Clark <rob@ti.com>
>    */
>   
> -#include <drm/drm_crtc.h>
> -#include <drm/drm_util.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_crtc_helper.h>
>   #include <drm/drm_fb_helper.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_util.h>
>   
>   #include "omap_drv.h"
>   
> @@ -72,6 +73,25 @@ static int omap_fbdev_pan_display(struct fb_var_screeninfo *var,
>   	return drm_fb_helper_pan_display(var, fbi);
>   }
>   
> +static void omap_fbdev_fb_destroy(struct fb_info *info)
> +{
> +	struct drm_fb_helper *helper = info->par;
> +	struct drm_framebuffer *fb = helper->fb;
> +	struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
> +	struct omap_fbdev *fbdev = to_omap_fbdev(helper);
> +
> +	DBG();
> +
> +	drm_fb_helper_fini(helper);
> +
> +	omap_gem_unpin(bo);
> +	drm_framebuffer_remove(fb);
> +
> +	drm_client_release(&helper->client);
> +	drm_fb_helper_unprepare(helper);
> +	kfree(fbdev);
> +}
> +
>   static const struct fb_ops omap_fb_ops = {
>   	.owner = THIS_MODULE,
>   
> @@ -87,6 +107,8 @@ static const struct fb_ops omap_fb_ops = {
>   	.fb_fillrect = drm_fb_helper_sys_fillrect,
>   	.fb_copyarea = drm_fb_helper_sys_copyarea,
>   	.fb_imageblit = drm_fb_helper_sys_imageblit,
> +
> +	.fb_destroy = omap_fbdev_fb_destroy,
>   };
>   
>   static int omap_fbdev_create(struct drm_fb_helper *helper,
> @@ -226,16 +248,52 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi)
>    */
>   
>   static void omap_fbdev_client_unregister(struct drm_client_dev *client)
> -{ }
> +{
> +	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
> +
> +	if (fb_helper->info) {
> +		drm_fb_helper_unregister_info(fb_helper);

Just trying to understand the code flow. So this is called if we have 
had a display connected, and have called omap_fbdev_create()? And 
drm_fb_helper_unregister_info() ends up calling omap_fbdev_fb_destroy().

> +	} else {
> +		drm_client_release(&fb_helper->client);
> +		drm_fb_helper_unprepare(fb_helper);
> +		kfree(fb_helper);

And this path is for the case where we haven't created the fbdev.

> +	}
> +}
>   
>   static int omap_fbdev_client_restore(struct drm_client_dev *client)
>   {
> +	drm_fb_helper_lastclose(client->dev);
> +
>   	return 0;
>   }
>   
>   static int omap_fbdev_client_hotplug(struct drm_client_dev *client)
>   {
> +	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
> +	struct drm_device *dev = client->dev;
> +	int ret;
> +
> +	if (dev->fb_helper)

And this tests if this is not the first call to 
omap_fbdev_client_hotplug()?

> +		return drm_fb_helper_hotplug_event(dev->fb_helper);
> +
> +	ret = drm_fb_helper_init(dev, fb_helper);
> +	if (ret)
> +		goto err_drm_err;
> +
> +	if (!drm_drv_uses_atomic_modeset(dev))

Why do we need this? omapdrm supports atomic.

That's my only real comment. Kernel test bot had one comment too. But 
other than that:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

I tested this series on TI DRA76 EVM, and worked fine for me.

  Tomi

> +		drm_helper_disable_unused_functions(dev);
> +
> +	ret = drm_fb_helper_initial_config(fb_helper);
> +	if (ret)
> +		goto err_drm_fb_helper_fini;
> +
>   	return 0;
> +
> +err_drm_fb_helper_fini:
> +	drm_fb_helper_fini(fb_helper);
> +err_drm_err:
> +	drm_err(dev, "Failed to setup fbdev emulation (ret=%d)\n", ret);
> +	return ret;
>   }
>   
>   static const struct drm_client_funcs omap_fbdev_client_funcs = {
> @@ -245,85 +303,37 @@ static const struct drm_client_funcs omap_fbdev_client_funcs = {
>   	.hotplug	= omap_fbdev_client_hotplug,
>   };
>   
> -/* initialize fbdev helper */
> -void omap_fbdev_init(struct drm_device *dev)
> +void omap_fbdev_setup(struct drm_device *dev)
>   {
> -	struct omap_drm_private *priv = dev->dev_private;
> -	struct omap_fbdev *fbdev = NULL;
> +	struct omap_fbdev *fbdev;
>   	struct drm_fb_helper *helper;
> -	int ret = 0;
> +	int ret;
>   
> -	if (!priv->num_pipes)
> -		return;
> +	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
> +	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>   
>   	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>   	if (!fbdev)
>   		return;
> -
> -	INIT_WORK(&fbdev->work, pan_worker);
> -
>   	helper = &fbdev->base;
>   
>   	drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
>   
>   	ret = drm_client_init(dev, &helper->client, "fbdev", &omap_fbdev_client_funcs);
>   	if (ret)
> -		goto fail;
> +		goto err_drm_client_init;
>   
> -	ret = drm_fb_helper_init(dev, helper);
> -	if (ret)
> -		goto err_drm_client_release;
> +	INIT_WORK(&fbdev->work, pan_worker);
>   
> -	ret = drm_fb_helper_initial_config(helper);
> +	ret = omap_fbdev_client_hotplug(&helper->client);
>   	if (ret)
> -		goto fini;
> -
> -	return;
> +		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>   
> -fini:
> -	drm_fb_helper_fini(helper);
> -err_drm_client_release:
> -	drm_client_release(&helper->client);
> -fail:
> -	drm_fb_helper_unprepare(helper);
> -	kfree(fbdev);
> +	drm_client_register(&helper->client);
>   
> -	dev_warn(dev->dev, "omap_fbdev_init failed\n");
> -}
> -
> -void omap_fbdev_fini(struct drm_device *dev)
> -{
> -	struct drm_fb_helper *helper = dev->fb_helper;
> -	struct drm_framebuffer *fb;
> -	struct drm_gem_object *bo;
> -	struct omap_fbdev *fbdev;
> -
> -	DBG();
> -
> -	if (!helper)
> -		return;
> -
> -	fb = helper->fb;
> -
> -	drm_fb_helper_unregister_info(helper);
> -
> -	drm_fb_helper_fini(helper);
> -
> -	fbdev = to_omap_fbdev(helper);
> -
> -	bo = drm_gem_fb_get_obj(fb, 0);
> -
> -	/* unpin the GEM object pinned in omap_fbdev_create() */
> -	if (bo)
> -		omap_gem_unpin(bo);
> -
> -	/* this will free the backing object */
> -	if (fb)
> -		drm_framebuffer_remove(fb);
> +	return;
>   
> -	drm_client_release(&helper->client);
> +err_drm_client_init:
>   	drm_fb_helper_unprepare(helper);
>   	kfree(fbdev);
> -
> -	dev->fb_helper = NULL;
>   }
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h b/drivers/gpu/drm/omapdrm/omap_fbdev.h
> index 74a68a5a6eab..74c691a8d45f 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.h
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h
> @@ -10,16 +10,11 @@
>   #define __OMAPDRM_FBDEV_H__
>   
>   struct drm_device;
> -struct drm_fb_helper;
>   
>   #ifdef CONFIG_DRM_FBDEV_EMULATION
> -void omap_fbdev_init(struct drm_device *dev);
> -void omap_fbdev_fini(struct drm_device *dev);
> +void omap_fbdev_setup(struct drm_device *dev);
>   #else
> -static inline void omap_fbdev_init(struct drm_device *dev)
> -{
> -}
> -static inline void omap_fbdev_fini(struct drm_device *dev)
> +static inline void omap_fbdev_setup(struct drm_device *dev)
>   {
>   }
>   #endif

Re: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[kernel test robot]

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230331]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230330083205.12621-7-tzimmermann%40suse.de
patch subject: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client
config: arm-randconfig-r046-20230401 (https://download.01.org/0day-ci/archive/20230402/202304020128.ePL6D3ZL-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
        git checkout 5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/drm/omapdrm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304020128.ePL6D3ZL-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/omapdrm/omap_fbdev.c:306:6: warning: no previous prototype for function 'omap_fbdev_setup' [-Wmissing-prototypes]
   void omap_fbdev_setup(struct drm_device *dev)
        ^
   drivers/gpu/drm/omapdrm/omap_fbdev.c:306:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void omap_fbdev_setup(struct drm_device *dev)
   ^
   static 
   1 warning generated.


vim +/omap_fbdev_setup +306 drivers/gpu/drm/omapdrm/omap_fbdev.c

   305	
 > 306	void omap_fbdev_setup(struct drm_device *dev)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[kernel test robot]

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230331]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230330083205.12621-7-tzimmermann%40suse.de
patch subject: [PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client
config: arm-randconfig-r046-20230401 (https://download.01.org/0day-ci/archive/20230402/202304020128.ePL6D3ZL-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/drm-omapdrm-Include-linux-of-h/20230330-163453
        git checkout 5b54095ec3eaa71a5cc6b433dfbbf58e26c44e38
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/drm/omapdrm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304020128.ePL6D3ZL-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/omapdrm/omap_fbdev.c:306:6: warning: no previous prototype for function 'omap_fbdev_setup' [-Wmissing-prototypes]
   void omap_fbdev_setup(struct drm_device *dev)
        ^
   drivers/gpu/drm/omapdrm/omap_fbdev.c:306:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void omap_fbdev_setup(struct drm_device *dev)
   ^
   static 
   1 warning generated.


vim +/omap_fbdev_setup +306 drivers/gpu/drm/omapdrm/omap_fbdev.c

   305	
 > 306	void omap_fbdev_setup(struct drm_device *dev)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 4/6] drm/omapdrm: Remove fbdev from struct omap_drm_private

[Thomas Zimmermann]

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

Hi

Am 30.03.23 um 14:08 schrieb Tomi Valkeinen:
> On 30/03/2023 11:32, Thomas Zimmermann wrote:
>> The DRM device stores a pointer to the fbdev helper. Remove struct
>> omap_drm_private.fbdev, which contains the same value. No functional
>> changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_debugfs.c | 6 +++---
>>   drivers/gpu/drm/omapdrm/omap_drv.c     | 1 +
>>   drivers/gpu/drm/omapdrm/omap_drv.h     | 3 ---
>>   drivers/gpu/drm/omapdrm/omap_fbdev.c   | 7 ++-----
>>   4 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c 
>> b/drivers/gpu/drm/omapdrm/omap_debugfs.c
>> index bfb2ccb40bd1..a3d470468e5b 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_debugfs.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c
>> @@ -47,15 +47,15 @@ static int fb_show(struct seq_file *m, void *arg)
>>   {
>>       struct drm_info_node *node = (struct drm_info_node *) m->private;
>>       struct drm_device *dev = node->minor->dev;
>> -    struct omap_drm_private *priv = dev->dev_private;
>> +    struct drm_fb_helper *helper = dev->fb_helper;
>>       struct drm_framebuffer *fb;
>>       seq_printf(m, "fbcon ");
>> -    omap_framebuffer_describe(priv->fbdev->fb, m);
>> +    omap_framebuffer_describe(helper->fb, m);
>>       mutex_lock(&dev->mode_config.fb_lock);
>>       list_for_each_entry(fb, &dev->mode_config.fb_list, head) {
>> -        if (fb == priv->fbdev->fb)
>> +        if (fb == helper->fb)
>>               continue;
>>           seq_printf(m, "user ");
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c 
>> b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index fb403b44769c..6a2f446c960f 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -25,6 +25,7 @@
>>   #include "omap_dmm_tiler.h"
>>   #include "omap_drv.h"
>> +#include "omap_fbdev.h"
>>   #define DRIVER_NAME        MODULE_NAME
>>   #define DRIVER_DESC        "OMAP DRM"
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h 
>> b/drivers/gpu/drm/omapdrm/omap_drv.h
>> index 825960fd3ea9..4c7217b35f6b 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
>> @@ -21,7 +21,6 @@
>>   #include "omap_crtc.h"
>>   #include "omap_encoder.h"
>>   #include "omap_fb.h"
>> -#include "omap_fbdev.h"
>>   #include "omap_gem.h"
>>   #include "omap_irq.h"
>>   #include "omap_plane.h"
>> @@ -77,8 +76,6 @@ struct omap_drm_private {
>>       struct drm_private_obj glob_obj;
>> -    struct drm_fb_helper *fbdev;
>> -
>>       struct workqueue_struct *wq;
>>       /* lock for obj_list below */
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c 
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> index d04a20f95e3d..79e417b391bf 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> @@ -250,8 +250,6 @@ void omap_fbdev_init(struct drm_device *dev)
>>       if (ret)
>>           goto fini;
>> -    priv->fbdev = helper;
>> -
>>       return;
>>   fini:
>> @@ -265,8 +263,7 @@ void omap_fbdev_init(struct drm_device *dev)
>>   void omap_fbdev_fini(struct drm_device *dev)
>>   {
>> -    struct omap_drm_private *priv = dev->dev_private;
>> -    struct drm_fb_helper *helper = priv->fbdev;
>> +    struct drm_fb_helper *helper = dev->fb_helper;
>>       struct drm_framebuffer *fb;
>>       struct drm_gem_object *bo;
>>       struct omap_fbdev *fbdev;
>> @@ -297,5 +294,5 @@ void omap_fbdev_fini(struct drm_device *dev)
>>       drm_fb_helper_unprepare(helper);
>>       kfree(fbdev);
>> -    priv->fbdev = NULL;
>> +    dev->fb_helper = NULL;
> 
> Is this line needed anymore? As you dropped the priv->fbdev assignment 
> in omap_fbdev_init(), it would look symmetrical to also drop this one. 
> I'm sure it doesn't hurt, just wondering if this is something drivers 
> are supposed to do, or is this just an extra line in the driver.

I think I can remove it here. I just didn't want to change too much.

Best regards
Thomas

> 
> In either case:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
>   Tomi
> 

-- 
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 5/6] drm/omapdrm: Initialize fbdev DRM client

[Thomas Zimmermann]

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

Hi

Am 30.03.23 um 14:13 schrieb Tomi Valkeinen:
> On 30/03/2023 11:32, Thomas Zimmermann wrote:
>> Initialize the fbdev client in the fbdev code with empty helper
>> functions. Also clean up the client. The helpers will later
>> implement various functionality of the DRM client. No functional
>> changes.
> 
> I don't see this doing any cleanups.

Yeah, that's really not well phrased in my commit message. I wanted to 
say that we now clean up by calling drm_client_release() in _fini and 
the error-handling code.

> 
> I think this could be as well merged to the next patch, as this is such 
> a short one.

Sure not problem. They both belong together, but I thought it's easier 
for reviewers to see what's happening if they are separate.

Best regards
Thomas

> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
>   Tomi
> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 33 +++++++++++++++++++++++++++-
>>   1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c 
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> index 79e417b391bf..f0e35f4764a7 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> @@ -221,6 +221,30 @@ static struct drm_fb_helper *get_fb(struct 
>> fb_info *fbi)
>>       return fbi->par;
>>   }
>> +/*
>> + * struct drm_client
>> + */
>> +
>> +static void omap_fbdev_client_unregister(struct drm_client_dev *client)
>> +{ }
>> +
>> +static int omap_fbdev_client_restore(struct drm_client_dev *client)
>> +{
>> +    return 0;
>> +}
>> +
>> +static int omap_fbdev_client_hotplug(struct drm_client_dev *client)
>> +{
>> +    return 0;
>> +}
>> +
>> +static const struct drm_client_funcs omap_fbdev_client_funcs = {
>> +    .owner        = THIS_MODULE,
>> +    .unregister    = omap_fbdev_client_unregister,
>> +    .restore    = omap_fbdev_client_restore,
>> +    .hotplug    = omap_fbdev_client_hotplug,
>> +};
>> +
>>   /* initialize fbdev helper */
>>   void omap_fbdev_init(struct drm_device *dev)
>>   {
>> @@ -242,10 +266,14 @@ void omap_fbdev_init(struct drm_device *dev)
>>       drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
>> -    ret = drm_fb_helper_init(dev, helper);
>> +    ret = drm_client_init(dev, &helper->client, "fbdev", 
>> &omap_fbdev_client_funcs);
>>       if (ret)
>>           goto fail;
>> +    ret = drm_fb_helper_init(dev, helper);
>> +    if (ret)
>> +        goto err_drm_client_release;
>> +
>>       ret = drm_fb_helper_initial_config(helper);
>>       if (ret)
>>           goto fini;
>> @@ -254,6 +282,8 @@ void omap_fbdev_init(struct drm_device *dev)
>>   fini:
>>       drm_fb_helper_fini(helper);
>> +err_drm_client_release:
>> +    drm_client_release(&helper->client);
>>   fail:
>>       drm_fb_helper_unprepare(helper);
>>       kfree(fbdev);
>> @@ -291,6 +321,7 @@ void omap_fbdev_fini(struct drm_device *dev)
>>       if (fb)
>>           drm_framebuffer_remove(fb);
>> +    drm_client_release(&helper->client);
>>       drm_fb_helper_unprepare(helper);
>>       kfree(fbdev);
> 

-- 
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 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[Thomas Zimmermann]

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

Hi

Am 30.03.23 um 15:13 schrieb Tomi Valkeinen:
> On 30/03/2023 11:32, Thomas Zimmermann wrote:
>> Move code from ad-hoc fbdev callbacks into DRM client functions
>> and remove the old callbacks. The functions instruct the client
>> to poll for changed output or restore the display. The DRM core
>> calls both, the old callbacks and the new client helpers, from
>> the same places. The new functions perform the same operation as
>> before, so there's no change in functionality.
>>
>> Replace all code that initializes or releases fbdev emulation
>> throughout the driver. Instead initialize the fbdev client by a
>> single call to omapdrm_fbdev_setup() after omapdrm has registered
>> its DRM device. As in most drivers, omapdrm's fbdev emulation now
>> acts like a regular DRM client.
>>
>> The fbdev client setup consists of the initial preparation and the
>> hot-plugging of the display. The latter creates the fbdev device
>> and sets up the fbdev framebuffer. The setup performs display
>> hot-plugging once. If no display can be detected, DRM probe helpers
>> re-run the detection on each hotplug event.
>>
>> A call to drm_dev_unregister() releases the client automatically.
>> No further action is required within omapdrm. If the fbdev
>> framebuffer has been fully set up, struct fb_ops.fb_destroy
>> implements the release. For partially initialized emulation, the
>> fbdev client reverts the initial setup.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_drv.c   |  11 +--
>>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 136 ++++++++++++++-------------
>>   drivers/gpu/drm/omapdrm/omap_fbdev.h |   9 +-
>>   3 files changed, 77 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c 
>> b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index 6a2f446c960f..5ed549726104 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -15,7 +15,6 @@
>>   #include <drm/drm_bridge.h>
>>   #include <drm/drm_bridge_connector.h>
>>   #include <drm/drm_drv.h>
>> -#include <drm/drm_fb_helper.h>
>>   #include <drm/drm_file.h>
>>   #include <drm/drm_ioctl.h>
>>   #include <drm/drm_panel.h>
>> @@ -221,7 +220,6 @@ static const struct drm_mode_config_helper_funcs 
>> omap_mode_config_helper_funcs =
>>   static const struct drm_mode_config_funcs omap_mode_config_funcs = {
>>       .fb_create = omap_framebuffer_create,
>> -    .output_poll_changed = drm_fb_helper_output_poll_changed,
>>       .atomic_check = omap_atomic_check,
>>       .atomic_commit = drm_atomic_helper_commit,
>>   };
>> @@ -654,7 +652,6 @@ static const struct drm_driver omap_drm_driver = {
>>       .driver_features = DRIVER_MODESET | DRIVER_GEM  |
>>           DRIVER_ATOMIC | DRIVER_RENDER,
>>       .open = dev_open,
>> -    .lastclose = drm_fb_helper_lastclose,
>>   #ifdef CONFIG_DEBUG_FS
>>       .debugfs_init = omap_debugfs_init,
>>   #endif
>> @@ -743,8 +740,6 @@ static int omapdrm_init(struct omap_drm_private 
>> *priv, struct device *dev)
>>           goto err_cleanup_modeset;
>>       }
>> -    omap_fbdev_init(ddev);
>> -
>>       drm_kms_helper_poll_init(ddev);
>>       /*
>> @@ -755,12 +750,12 @@ static int omapdrm_init(struct omap_drm_private 
>> *priv, struct device *dev)
>>       if (ret)
>>           goto err_cleanup_helpers;
>> +    omap_fbdev_setup(ddev);
>> +
>>       return 0;
>>   err_cleanup_helpers:
>>       drm_kms_helper_poll_fini(ddev);
>> -
>> -    omap_fbdev_fini(ddev);
>>   err_cleanup_modeset:
>>       omap_modeset_fini(ddev);
>>   err_free_overlays:
>> @@ -786,8 +781,6 @@ static void omapdrm_cleanup(struct 
>> omap_drm_private *priv)
>>       drm_kms_helper_poll_fini(ddev);
>> -    omap_fbdev_fini(ddev);
>> -
>>       drm_atomic_helper_shutdown(ddev);
>>       omap_modeset_fini(ddev);
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c 
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> index f0e35f4764a7..cd63142a4705 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> @@ -4,13 +4,14 @@
>>    * Author: Rob Clark <rob@ti.com>
>>    */
>> -#include <drm/drm_crtc.h>
>> -#include <drm/drm_util.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_fb_helper.h>
>>   #include <drm/drm_file.h>
>>   #include <drm/drm_fourcc.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_util.h>
>>   #include "omap_drv.h"
>> @@ -72,6 +73,25 @@ static int omap_fbdev_pan_display(struct 
>> fb_var_screeninfo *var,
>>       return drm_fb_helper_pan_display(var, fbi);
>>   }
>> +static void omap_fbdev_fb_destroy(struct fb_info *info)
>> +{
>> +    struct drm_fb_helper *helper = info->par;
>> +    struct drm_framebuffer *fb = helper->fb;
>> +    struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
>> +    struct omap_fbdev *fbdev = to_omap_fbdev(helper);
>> +
>> +    DBG();
>> +
>> +    drm_fb_helper_fini(helper);
>> +
>> +    omap_gem_unpin(bo);
>> +    drm_framebuffer_remove(fb);
>> +
>> +    drm_client_release(&helper->client);
>> +    drm_fb_helper_unprepare(helper);
>> +    kfree(fbdev);
>> +}
>> +
>>   static const struct fb_ops omap_fb_ops = {
>>       .owner = THIS_MODULE,
>> @@ -87,6 +107,8 @@ static const struct fb_ops omap_fb_ops = {
>>       .fb_fillrect = drm_fb_helper_sys_fillrect,
>>       .fb_copyarea = drm_fb_helper_sys_copyarea,
>>       .fb_imageblit = drm_fb_helper_sys_imageblit,
>> +
>> +    .fb_destroy = omap_fbdev_fb_destroy,
>>   };
>>   static int omap_fbdev_create(struct drm_fb_helper *helper,
>> @@ -226,16 +248,52 @@ static struct drm_fb_helper *get_fb(struct 
>> fb_info *fbi)
>>    */
>>   static void omap_fbdev_client_unregister(struct drm_client_dev *client)
>> -{ }
>> +{
>> +    struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>> +
>> +    if (fb_helper->info) {
>> +        drm_fb_helper_unregister_info(fb_helper);
> 
> Just trying to understand the code flow. So this is called if we have 
> had a display connected, and have called omap_fbdev_create()? And 
> drm_fb_helper_unregister_info() ends up calling omap_fbdev_fb_destroy().

Exactly.

> 
>> +    } else {
>> +        drm_client_release(&fb_helper->client);
>> +        drm_fb_helper_unprepare(fb_helper);
>> +        kfree(fb_helper);
> 
> And this path is for the case where we haven't created the fbdev.

Exactly.

When you look at other fbdev code, you'll find the same pattern. We 
either clean up everything; or just the client and the minimal fbdev 
state that has been prepared by the _setup function.

There's been talk about sharing some of this in helpers among drivers. 
And I hope to do this, but I first want to convert older fbdev code to 
drm_client. I first wanted a clear idea of which code can actually be 
shared.

> 
>> +    }
>> +}
>>   static int omap_fbdev_client_restore(struct drm_client_dev *client)
>>   {
>> +    drm_fb_helper_lastclose(client->dev);
>> +
>>       return 0;
>>   }
>>   static int omap_fbdev_client_hotplug(struct drm_client_dev *client)
>>   {
>> +    struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>> +    struct drm_device *dev = client->dev;
>> +    int ret;
>> +
>> +    if (dev->fb_helper)
> 
> And this tests if this is not the first call to 
> omap_fbdev_client_hotplug()?

Exactly. Here, we already have a fully initialized fbdev device with 
framebuffer and videomode.

> 
>> +        return drm_fb_helper_hotplug_event(dev->fb_helper);
>> +
>> +    ret = drm_fb_helper_init(dev, fb_helper);
>> +    if (ret)
>> +        goto err_drm_err;
>> +
>> +    if (!drm_drv_uses_atomic_modeset(dev))
> 
> Why do we need this? omapdrm supports atomic.

Even better. I'm going to remove it.

> 
> That's my only real comment. Kernel test bot had one comment too. But 
> other than that:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> I tested this series on TI DRA76 EVM, and worked fine for me.

That you so much.

I'm going to send out an update with the fixed patches. Is there an omap 
tree or does it later go through drm-misc?

Best regards
Thomas

> 
>   Tomi
> 
>> +        drm_helper_disable_unused_functions(dev);
>> +
>> +    ret = drm_fb_helper_initial_config(fb_helper);
>> +    if (ret)
>> +        goto err_drm_fb_helper_fini;
>> +
>>       return 0;
>> +
>> +err_drm_fb_helper_fini:
>> +    drm_fb_helper_fini(fb_helper);
>> +err_drm_err:
>> +    drm_err(dev, "Failed to setup fbdev emulation (ret=%d)\n", ret);
>> +    return ret;
>>   }
>>   static const struct drm_client_funcs omap_fbdev_client_funcs = {
>> @@ -245,85 +303,37 @@ static const struct drm_client_funcs 
>> omap_fbdev_client_funcs = {
>>       .hotplug    = omap_fbdev_client_hotplug,
>>   };
>> -/* initialize fbdev helper */
>> -void omap_fbdev_init(struct drm_device *dev)
>> +void omap_fbdev_setup(struct drm_device *dev)
>>   {
>> -    struct omap_drm_private *priv = dev->dev_private;
>> -    struct omap_fbdev *fbdev = NULL;
>> +    struct omap_fbdev *fbdev;
>>       struct drm_fb_helper *helper;
>> -    int ret = 0;
>> +    int ret;
>> -    if (!priv->num_pipes)
>> -        return;
>> +    drm_WARN(dev, !dev->registered, "Device has not been 
>> registered.\n");
>> +    drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>>       fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>>       if (!fbdev)
>>           return;
>> -
>> -    INIT_WORK(&fbdev->work, pan_worker);
>> -
>>       helper = &fbdev->base;
>>       drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
>>       ret = drm_client_init(dev, &helper->client, "fbdev", 
>> &omap_fbdev_client_funcs);
>>       if (ret)
>> -        goto fail;
>> +        goto err_drm_client_init;
>> -    ret = drm_fb_helper_init(dev, helper);
>> -    if (ret)
>> -        goto err_drm_client_release;
>> +    INIT_WORK(&fbdev->work, pan_worker);
>> -    ret = drm_fb_helper_initial_config(helper);
>> +    ret = omap_fbdev_client_hotplug(&helper->client);
>>       if (ret)
>> -        goto fini;
>> -
>> -    return;
>> +        drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>> -fini:
>> -    drm_fb_helper_fini(helper);
>> -err_drm_client_release:
>> -    drm_client_release(&helper->client);
>> -fail:
>> -    drm_fb_helper_unprepare(helper);
>> -    kfree(fbdev);
>> +    drm_client_register(&helper->client);
>> -    dev_warn(dev->dev, "omap_fbdev_init failed\n");
>> -}
>> -
>> -void omap_fbdev_fini(struct drm_device *dev)
>> -{
>> -    struct drm_fb_helper *helper = dev->fb_helper;
>> -    struct drm_framebuffer *fb;
>> -    struct drm_gem_object *bo;
>> -    struct omap_fbdev *fbdev;
>> -
>> -    DBG();
>> -
>> -    if (!helper)
>> -        return;
>> -
>> -    fb = helper->fb;
>> -
>> -    drm_fb_helper_unregister_info(helper);
>> -
>> -    drm_fb_helper_fini(helper);
>> -
>> -    fbdev = to_omap_fbdev(helper);
>> -
>> -    bo = drm_gem_fb_get_obj(fb, 0);
>> -
>> -    /* unpin the GEM object pinned in omap_fbdev_create() */
>> -    if (bo)
>> -        omap_gem_unpin(bo);
>> -
>> -    /* this will free the backing object */
>> -    if (fb)
>> -        drm_framebuffer_remove(fb);
>> +    return;
>> -    drm_client_release(&helper->client);
>> +err_drm_client_init:
>>       drm_fb_helper_unprepare(helper);
>>       kfree(fbdev);
>> -
>> -    dev->fb_helper = NULL;
>>   }
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h 
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.h
>> index 74a68a5a6eab..74c691a8d45f 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h
>> @@ -10,16 +10,11 @@
>>   #define __OMAPDRM_FBDEV_H__
>>   struct drm_device;
>> -struct drm_fb_helper;
>>   #ifdef CONFIG_DRM_FBDEV_EMULATION
>> -void omap_fbdev_init(struct drm_device *dev);
>> -void omap_fbdev_fini(struct drm_device *dev);
>> +void omap_fbdev_setup(struct drm_device *dev);
>>   #else
>> -static inline void omap_fbdev_init(struct drm_device *dev)
>> -{
>> -}
>> -static inline void omap_fbdev_fini(struct drm_device *dev)
>> +static inline void omap_fbdev_setup(struct drm_device *dev)
>>   {
>>   }
>>   #endif
> 

-- 
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 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client

[Tomi Valkeinen]

Hi Thomas,

On 03/04/2023 11:24, Thomas Zimmermann wrote:

>> That's my only real comment. Kernel test bot had one comment too. But 
>> other than that:
>>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>
>> I tested this series on TI DRA76 EVM, and worked fine for me.
> 
> That you so much.
> 
> I'm going to send out an update with the fixed patches. Is there an omap 
> tree or does it later go through drm-misc?

drm-misc. I can push the v2 after testing it.

  Tomi