[PATCH v2 00/10] Set up generic fbdev after registering device

[PATCH v2 00/10] Set up generic fbdev after registering device

[Thomas Zimmermann]

Generic fbdev emulation is a DRM client. If possible, it should behave
like userspace clients. Therefore it should not run before the driver
registered the new DRM device. If the setup function fails, the driver
should not report an error.

This is a follow-up patchset to the discussion at [1].  I went
through all calls to drm_fbdev_generic_setup(), moved them to the
final operation of their driver's probe function, and removed the
return value.

Built on x86-64, aarch64 and arm. Tested with mgag200.

v2:
	* warn in drm_fbdev_generic_setup() if device is unregistered (Jani)
	* document the new behavior (Sam)
	* fix mediatek subject (Noralf)
	* keep kirin patch for now, even though the patched code will
	  probably be removed

Thomas Zimmermann (10):
  drm/ast: Set up fbdev after registering device; remove error checks
  drm/hibmc: Remove error check from fbdev setup
  drm/kirin: Set up fbdev after fully registering device
  drm/ingenic: Remove error check from fbdev setup
  drm/mediatek: Remove error check from fbdev setup
  drm/mgag200: Set up fbdev after registering device; remove error
    checks
  drm/tilcdc: Set up fbdev after fully registering device
  drm/udl: Remove error check from fbdev setup
  drm/vboxvideo: Set up fbdev after registering device; remove error
    checks
  drm/fb-helper: Remove return value from drm_fbdev_generic_setup()

 drivers/gpu/drm/ast/ast_drv.c                 |  3 +++
 drivers/gpu/drm/ast/ast_main.c                |  5 ----
 drivers/gpu/drm/drm_fb_helper.c               | 25 ++++++++++---------
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  6 +----
 .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c   |  4 +--
 drivers/gpu/drm/ingenic/ingenic-drm.c         |  4 +--
 drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  4 +--
 drivers/gpu/drm/mgag200/mgag200_drv.c         |  2 ++
 drivers/gpu/drm/mgag200/mgag200_main.c        |  4 ---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c           |  3 +--
 drivers/gpu/drm/udl/udl_drv.c                 |  6 +----
 drivers/gpu/drm/vboxvideo/vbox_drv.c          |  6 ++---
 include/drm/drm_fb_helper.h                   |  5 ++--
 13 files changed, 30 insertions(+), 47 deletions(-)

--
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 01/10] drm/ast: Set up fbdev after registering device; remove error checks

[Thomas Zimmermann]

Generic fbdev support is a DRM client. Set it up after registering
the new DRM device. Remove the error checks as the driver's probe
function should not depend on a DRM client's state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/ast/ast_drv.c  | 3 +++
 drivers/gpu/drm/ast/ast_main.c | 5 -----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 30aa73a5d9b72..b7ba22dddcad9 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -32,6 +32,7 @@
 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_probe_helper.h>
 
@@ -111,6 +112,8 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto err_ast_driver_unload;
 
+	drm_fbdev_generic_setup(dev, 32);
+
 	return 0;
 
 err_ast_driver_unload:
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 18a0a4ce00f6e..e5398e3dabe70 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -30,7 +30,6 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
-#include <drm/drm_fb_helper.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_vram_helper.h>
@@ -512,10 +511,6 @@ int ast_driver_load(struct drm_device *dev, unsigned long flags)
 
 	drm_mode_config_reset(dev);
 
-	ret = drm_fbdev_generic_setup(dev, 32);
-	if (ret)
-		goto out_free;
-
 	return 0;
 out_free:
 	kfree(ast);
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 02/10] drm/hibmc: Remove error check from fbdev setup

[Thomas Zimmermann]

Generic fbdev support is a DRM client. Remove the error check as the
driver's probe function should not depend on a DRM client's state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 79a180ae4509f..a6fd0c29e5b89 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -307,11 +307,7 @@ static int hibmc_load(struct drm_device *dev)
 	/* reset all the states of crtc/plane/encoder/connector */
 	drm_mode_config_reset(dev);
 
-	ret = drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth);
-	if (ret) {
-		DRM_ERROR("failed to initialize fbdev: %d\n", ret);
-		goto err;
-	}
+	drm_fbdev_generic_setup(dev, dev->mode_config.preferred_depth);
 
 	return 0;
 
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 03/10] drm/kirin: Set up fbdev after fully registering device

[Thomas Zimmermann]

Generic fbdev support is a DRM client. Set it up after fully registering
the new DRM device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index d3145ae877d74..981858cc8d2b5 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -277,8 +277,6 @@ static int kirin_drm_bind(struct device *dev)
 	if (ret)
 		goto err_kms_cleanup;
 
-	drm_fbdev_generic_setup(drm_dev, 32);
-
 	/* connectors should be registered after drm device register */
 	if (driver_data->register_connects) {
 		ret = kirin_drm_connectors_register(drm_dev);
@@ -286,6 +284,8 @@ static int kirin_drm_bind(struct device *dev)
 			goto err_drm_dev_unregister;
 	}
 
+	drm_fbdev_generic_setup(drm_dev, 32);
+
 	return 0;
 
 err_drm_dev_unregister:
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 04/10] drm/ingenic: Remove error check from fbdev setup

[Thomas Zimmermann]

Remove the error check from the fbdev setup function. The function
will print a warning.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/ingenic/ingenic-drm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.c b/drivers/gpu/drm/ingenic/ingenic-drm.c
index 7f3f869f57b3f..d938f2b1a96f1 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm.c
@@ -783,9 +783,7 @@ static int ingenic_drm_probe(struct platform_device *pdev)
 		goto err_devclk_disable;
 	}
 
-	ret = drm_fbdev_generic_setup(drm, 32);
-	if (ret)
-		dev_warn(dev, "Unable to start fbdev emulation: %i", ret);
+	drm_fbdev_generic_setup(drm, 32);
 
 	return 0;
 
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 05/10] drm/mediatek: Remove error check from fbdev setup

[Thomas Zimmermann]

Remove the error check from the fbdev setup function. The function
will print a warning.

v2:
	* fix subject line

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 2eaa9080d2505..ce570283b55f7 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -347,9 +347,7 @@ static int mtk_drm_bind(struct device *dev)
 	if (ret < 0)
 		goto err_deinit;
 
-	ret = drm_fbdev_generic_setup(drm, 32);
-	if (ret)
-		DRM_ERROR("Failed to initialize fbdev: %d\n", ret);
+	drm_fbdev_generic_setup(drm, 32);
 
 	return 0;
 
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 06/10] drm/mgag200: Set up fbdev after registering device; remove error checks

[Thomas Zimmermann]

Generic fbdev support is a DRM client. Set it up after registering
the new DRM device. Remove the error checks as the driver's probe
function should not depend on a DRM client's state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 2 ++
 drivers/gpu/drm/mgag200/mgag200_main.c | 4 ----
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 7a5bad2f57d70..3298b7ef18b03 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -77,6 +77,8 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto err_mgag200_driver_unload;
 
+	drm_fbdev_generic_setup(dev, 0);
+
 	return 0;
 
 err_mgag200_driver_unload:
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index e278b6a547bde..b680cf47cbb94 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -181,10 +181,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 		dev_warn(&dev->pdev->dev,
 			"Could not initialize cursors. Not doing hardware cursors.\n");
 
-	r = drm_fbdev_generic_setup(mdev->dev, 0);
-	if (r)
-		goto err_modeset;
-
 	return 0;
 
 err_modeset:
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 07/10] drm/tilcdc: Set up fbdev after fully registering device

[Thomas Zimmermann]

Generic fbdev support is a DRM client. Set it up after fully registering
the new DRM device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 78c1877d13a83..a5e9ee4c7fbf4 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -390,10 +390,9 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 	ret = drm_dev_register(ddev, 0);
 	if (ret)
 		goto init_failed;
+	priv->is_registered = true;
 
 	drm_fbdev_generic_setup(ddev, bpp);
-
-	priv->is_registered = true;
 	return 0;
 
 init_failed:
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 08/10] drm/udl: Remove error check from fbdev setup

[Thomas Zimmermann]

Remove the error check from the fbdev setup function. The driver's
probe function should not depend on a DRM client's state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/udl/udl_drv.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 1ce2d865c36dc..9cc6d075cb402 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -97,14 +97,10 @@ static int udl_usb_probe(struct usb_interface *interface,
 
 	DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index);
 
-	r = drm_fbdev_generic_setup(&udl->drm, 0);
-	if (r)
-		goto err_drm_dev_unregister;
+	drm_fbdev_generic_setup(&udl->drm, 0);
 
 	return 0;
 
-err_drm_dev_unregister:
-	drm_dev_unregister(&udl->drm);
 err_free:
 	drm_dev_put(&udl->drm);
 	return r;
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 09/10] drm/vboxvideo: Set up fbdev after registering device; remove error checks

[Thomas Zimmermann]

Generic fbdev support is a DRM client. Set it up after registering
the new DRM device. Remove the error checks as the driver's probe
function should not depend on a DRM client's state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/vboxvideo/vbox_drv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
index d685ec197fa05..282348e071fe3 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -82,14 +82,12 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto err_mode_fini;
 
-	ret = drm_fbdev_generic_setup(&vbox->ddev, 32);
-	if (ret)
-		goto err_irq_fini;
-
 	ret = drm_dev_register(&vbox->ddev, 0);
 	if (ret)
 		goto err_irq_fini;
 
+	drm_fbdev_generic_setup(&vbox->ddev, 32);
+
 	return 0;
 
 err_irq_fini:
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 10/10] drm/fb-helper: Remove return value from drm_fbdev_generic_setup()

[Thomas Zimmermann]

Generic fbdev emulation is a DRM client. Drivers should invoke the
setup function, but not depend on its success. Hence remove the return
value.

v2:
	* warn if fbdev device has not been registered yet
	* document the new behavior
	* convert the existing warning to the new dev_ interface

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++------------
 include/drm/drm_fb_helper.h     |  5 +++--
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 165c8dab50797..97f5e43837486 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2169,7 +2169,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  *                 @dev->mode_config.preferred_depth is used if this is zero.
  *
  * This function sets up generic fbdev emulation for drivers that supports
- * dumb buffers with a virtual address and that can be mmap'ed.
+ * dumb buffers with a virtual address and that can be mmap'ed. It's supposed
+ * to run after the DRM driver registered the new DRM device with
+ * drm_dev_register().
  *
  * Restore, hotplug events and teardown are all taken care of. Drivers that do
  * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
@@ -2186,29 +2188,30 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  * Setup will be retried on the next hotplug event.
  *
  * The fbdev is destroyed by drm_dev_unregister().
- *
- * Returns:
- * Zero on success or negative error code on failure.
  */
-int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
+void drm_fbdev_generic_setup(struct drm_device *dev,
+			     unsigned int preferred_bpp)
 {
 	struct drm_fb_helper *fb_helper;
 	int ret;
 
-	WARN(dev->fb_helper, "fb_helper is already set!\n");
+	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
+	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
 
 	if (!drm_fbdev_emulation)
-		return 0;
+		return;
 
 	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
-	if (!fb_helper)
-		return -ENOMEM;
+	if (!fb_helper) {
+		drm_err(dev, "Failed to allocate fb_helper\n");
+		return;
+	}
 
 	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
 	if (ret) {
 		kfree(fb_helper);
 		drm_err(dev, "Failed to register client: %d\n", ret);
-		return ret;
+		return;
 	}
 
 	if (!preferred_bpp)
@@ -2222,8 +2225,6 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
 
 	drm_client_register(&fb_helper->client);
-
-	return 0;
 }
 EXPORT_SYMBOL(drm_fbdev_generic_setup);
 
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 208dbf87afa3e..fb037be83997d 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -269,7 +269,8 @@ int drm_fb_helper_debug_leave(struct fb_info *info);
 void drm_fb_helper_lastclose(struct drm_device *dev);
 void drm_fb_helper_output_poll_changed(struct drm_device *dev);
 
-int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp);
+void drm_fbdev_generic_setup(struct drm_device *dev,
+			     unsigned int preferred_bpp);
 #else
 static inline void drm_fb_helper_prepare(struct drm_device *dev,
 					struct drm_fb_helper *helper,
@@ -443,7 +444,7 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
 {
 }
 
-static inline int
+static inline void
 drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 {
 	return 0;
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 10/10] drm/fb-helper: Remove return value from drm_fbdev_generic_setup()

[Sam Ravnborg]

Hi Thomas.

You missed my ack on the first 9 patches:
https://lore.kernel.org/dri-devel/20200407101354.GA12686@ravnborg.org/
Not that it matters as others have acked/reviewed them.

On Wed, Apr 08, 2020 at 10:26:41AM +0200, Thomas Zimmermann wrote:
> Generic fbdev emulation is a DRM client. Drivers should invoke the
> setup function, but not depend on its success. Hence remove the return
> value.
> 
> v2:
> 	* warn if fbdev device has not been registered yet
> 	* document the new behavior
> 	* convert the existing warning to the new dev_ interface
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++------------
>  include/drm/drm_fb_helper.h     |  5 +++--
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 165c8dab50797..97f5e43837486 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2169,7 +2169,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   *                 @dev->mode_config.preferred_depth is used if this is zero.
>   *
>   * This function sets up generic fbdev emulation for drivers that supports
> - * dumb buffers with a virtual address and that can be mmap'ed.
> + * dumb buffers with a virtual address and that can be mmap'ed. It's supposed
> + * to run after the DRM driver registered the new DRM device with
> + * drm_dev_register().
OR maybe be more explicit - something like:
drm_fbdev_generic_setup() shall be called after the DRM is registered
with drm_dev_register().

Either way is fine.

	Sam

>   *
>   * Restore, hotplug events and teardown are all taken care of. Drivers that do
>   * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
> @@ -2186,29 +2188,30 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   * Setup will be retried on the next hotplug event.
>   *
>   * The fbdev is destroyed by drm_dev_unregister().
> - *
> - * Returns:
> - * Zero on success or negative error code on failure.
>   */
> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
> +void drm_fbdev_generic_setup(struct drm_device *dev,
> +			     unsigned int preferred_bpp)
>  {
>  	struct drm_fb_helper *fb_helper;
>  	int ret;
>  
> -	WARN(dev->fb_helper, "fb_helper is already set!\n");
> +	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
> +	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>  
>  	if (!drm_fbdev_emulation)
> -		return 0;
> +		return;
>  
>  	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> -	if (!fb_helper)
> -		return -ENOMEM;
> +	if (!fb_helper) {
> +		drm_err(dev, "Failed to allocate fb_helper\n");
> +		return;
> +	}
>  
>  	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
>  	if (ret) {
>  		kfree(fb_helper);
>  		drm_err(dev, "Failed to register client: %d\n", ret);
> -		return ret;
> +		return;
>  	}
>  
>  	if (!preferred_bpp)
> @@ -2222,8 +2225,6 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>  
>  	drm_client_register(&fb_helper->client);
> -
> -	return 0;
>  }
>  EXPORT_SYMBOL(drm_fbdev_generic_setup);
>  
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 208dbf87afa3e..fb037be83997d 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -269,7 +269,8 @@ int drm_fb_helper_debug_leave(struct fb_info *info);
>  void drm_fb_helper_lastclose(struct drm_device *dev);
>  void drm_fb_helper_output_poll_changed(struct drm_device *dev);
>  
> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp);
> +void drm_fbdev_generic_setup(struct drm_device *dev,
> +			     unsigned int preferred_bpp);
>  #else
>  static inline void drm_fb_helper_prepare(struct drm_device *dev,
>  					struct drm_fb_helper *helper,
> @@ -443,7 +444,7 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
>  {
>  }
>  
> -static inline int
> +static inline void
>  drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  {
>  	return 0;
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 10/10] drm/fb-helper: Remove return value from drm_fbdev_generic_setup()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5099 bytes --]

Hi Sam

Am 08.04.20 um 10:50 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> You missed my ack on the first 9 patches:
> https://lore.kernel.org/dri-devel/20200407101354.GA12686@ravnborg.org/
> Not that it matters as others have acked/reviewed them.

This got lost. I'll add you acks. Thanks for taking another look at the
patches.

> 
> On Wed, Apr 08, 2020 at 10:26:41AM +0200, Thomas Zimmermann wrote:
>> Generic fbdev emulation is a DRM client. Drivers should invoke the
>> setup function, but not depend on its success. Hence remove the return
>> value.
>>
>> v2:
>> 	* warn if fbdev device has not been registered yet
>> 	* document the new behavior
>> 	* convert the existing warning to the new dev_ interface
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++------------
>>  include/drm/drm_fb_helper.h     |  5 +++--
>>  2 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 165c8dab50797..97f5e43837486 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2169,7 +2169,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>>   *                 @dev->mode_config.preferred_depth is used if this is zero.
>>   *
>>   * This function sets up generic fbdev emulation for drivers that supports
>> - * dumb buffers with a virtual address and that can be mmap'ed.
>> + * dumb buffers with a virtual address and that can be mmap'ed. It's supposed
>> + * to run after the DRM driver registered the new DRM device with
>> + * drm_dev_register().
> OR maybe be more explicit - something like:
> drm_fbdev_generic_setup() shall be called after the DRM is registered
> with drm_dev_register().

I think this one is even better.

Best regards
Thomas

> 
> Either way is fine.
> 
> 	Sam
> 
>>   *
>>   * Restore, hotplug events and teardown are all taken care of. Drivers that do
>>   * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
>> @@ -2186,29 +2188,30 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>>   * Setup will be retried on the next hotplug event.
>>   *
>>   * The fbdev is destroyed by drm_dev_unregister().
>> - *
>> - * Returns:
>> - * Zero on success or negative error code on failure.
>>   */
>> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>> +void drm_fbdev_generic_setup(struct drm_device *dev,
>> +			     unsigned int preferred_bpp)
>>  {
>>  	struct drm_fb_helper *fb_helper;
>>  	int ret;
>>  
>> -	WARN(dev->fb_helper, "fb_helper is already set!\n");
>> +	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
>> +	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>>  
>>  	if (!drm_fbdev_emulation)
>> -		return 0;
>> +		return;
>>  
>>  	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
>> -	if (!fb_helper)
>> -		return -ENOMEM;
>> +	if (!fb_helper) {
>> +		drm_err(dev, "Failed to allocate fb_helper\n");
>> +		return;
>> +	}
>>  
>>  	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
>>  	if (ret) {
>>  		kfree(fb_helper);
>>  		drm_err(dev, "Failed to register client: %d\n", ret);
>> -		return ret;
>> +		return;
>>  	}
>>  
>>  	if (!preferred_bpp)
>> @@ -2222,8 +2225,6 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>>  		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>>  
>>  	drm_client_register(&fb_helper->client);
>> -
>> -	return 0;
>>  }
>>  EXPORT_SYMBOL(drm_fbdev_generic_setup);
>>  
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 208dbf87afa3e..fb037be83997d 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -269,7 +269,8 @@ int drm_fb_helper_debug_leave(struct fb_info *info);
>>  void drm_fb_helper_lastclose(struct drm_device *dev);
>>  void drm_fb_helper_output_poll_changed(struct drm_device *dev);
>>  
>> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp);
>> +void drm_fbdev_generic_setup(struct drm_device *dev,
>> +			     unsigned int preferred_bpp);
>>  #else
>>  static inline void drm_fb_helper_prepare(struct drm_device *dev,
>>  					struct drm_fb_helper *helper,
>> @@ -443,7 +444,7 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
>>  {
>>  }
>>  
>> -static inline int
>> +static inline void
>>  drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>>  {
>>  	return 0;
>> -- 
>> 2.26.0

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


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel