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

[PATCH 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-tested on x86-64, aarch64 and arm.

[1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867

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/mediathek: 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                | 18 ++++++++----------
 .../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, 25 insertions(+), 45 deletions(-)

--
2.26.0

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

[PATCH 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>
---
 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 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>
---
 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 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>
---
 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 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>
---
 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 05/10] drm/mediathek: 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>
---
 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 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>
---
 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 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>
---
 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 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>
---
 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 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>
---
 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 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.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 18 ++++++++----------
 include/drm/drm_fb_helper.h     |  5 +++--
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 165c8dab50797..24db97eee53d4 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2186,11 +2186,9 @@ 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;
@@ -2198,17 +2196,19 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 	WARN(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 +2222,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 00/10] Set up generic fbdev after registering device

[Sam Ravnborg]

Hi Thomas.

On Mon, Apr 06, 2020 at 03:43:55PM +0200, Thomas Zimmermann wrote:
> 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.

Thanks for taking the time to refactor all the relevant drivers.

I have received some push-back in the past when suggesting this,
but cannot remember from who.
Let's see what review comments you get.

As the rule is that the fbdev setup shall be setup after registering
the DRM device - it would be nice to have this included in the
documentation of drm_fbdev_generic_setup

Could you try to to update the documentation to cover this?

I will get back to the patches later this week.

	Sam

> 
> 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-tested on x86-64, aarch64 and arm.
> 
> [1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867
> 
> 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/mediathek: 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                | 18 ++++++++----------
>  .../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, 25 insertions(+), 45 deletions(-)
> 
> --
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Paul Cercueil]

Hi Thomas,

Le lun. 6 avril 2020 à 15:43, Thomas Zimmermann <tzimmermann@suse.de> 
a écrit :
> 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>

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

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

[Thomas Zimmermann]

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

Hi Sam

Am 06.04.20 um 22:00 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Mon, Apr 06, 2020 at 03:43:55PM +0200, Thomas Zimmermann wrote:
>> 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.
> 
> Thanks for taking the time to refactor all the relevant drivers.
> 
> I have received some push-back in the past when suggesting this,
> but cannot remember from who.
> Let's see what review comments you get.
> 
> As the rule is that the fbdev setup shall be setup after registering
> the DRM device - it would be nice to have this included in the
> documentation of drm_fbdev_generic_setup
> 
> Could you try to to update the documentation to cover this?

Good idea. I'll add this to patchset's next iteration.

Best regards
Thomas

> 
> I will get back to the patches later this week.
> 
> 	Sam
> 
>>
>> 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-tested on x86-64, aarch64 and arm.
>>
>> [1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867
>>
>> 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/mediathek: 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                | 18 ++++++++----------
>>  .../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, 25 insertions(+), 45 deletions(-)
>>
>> --
>> 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

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

[Gerd Hoffmann]

On Mon, Apr 06, 2020 at 03:43:55PM +0200, Thomas Zimmermann wrote:
> 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 always has been my expectation, as you might have concluded from
the absence of any qemu guest driver patches in this series ;)

The patches look sane too, so for the series:
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

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

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

[Jani Nikula]

On Tue, 07 Apr 2020, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi Sam
>
> Am 06.04.20 um 22:00 schrieb Sam Ravnborg:
>> Hi Thomas.
>> 
>> On Mon, Apr 06, 2020 at 03:43:55PM +0200, Thomas Zimmermann wrote:
>>> 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.
>> 
>> Thanks for taking the time to refactor all the relevant drivers.
>> 
>> I have received some push-back in the past when suggesting this,
>> but cannot remember from who.
>> Let's see what review comments you get.
>> 
>> As the rule is that the fbdev setup shall be setup after registering
>> the DRM device - it would be nice to have this included in the
>> documentation of drm_fbdev_generic_setup
>> 
>> Could you try to to update the documentation to cover this?
>
> Good idea. I'll add this to patchset's next iteration.

How about something like:

	drm_WARN_ON(dev, !dev->registered);

(Not sure if that needs to be !dev->driver->load && !dev->registered).

This can be a follow-up patch later too.

BR,
Jani.


>
> Best regards
> Thomas
>
>> 
>> I will get back to the patches later this week.
>> 
>> 	Sam
>> 
>>>
>>> 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-tested on x86-64, aarch64 and arm.
>>>
>>> [1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867
>>>
>>> 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/mediathek: 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                | 18 ++++++++----------
>>>  .../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, 25 insertions(+), 45 deletions(-)
>>>
>>> --
>>> 2.26.0

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Mon, Apr 06, 2020 at 03:43:58PM +0200, Thomas Zimmermann wrote:
> 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>
> ---
>  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);

The code you jump over is nonsense and should be reverted. I replied to
the patch that landed this.
-Daniel

> +
>  	return 0;
>  
>  err_drm_dev_unregister:
> -- 
> 2.26.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sam Ravnborg]

Hi Thomas.

On Tue, Apr 07, 2020 at 08:28:59AM +0200, Thomas Zimmermann wrote:
> Hi Sam
> 
> Am 06.04.20 um 22:00 schrieb Sam Ravnborg:
> > Hi Thomas.
> > 
> > On Mon, Apr 06, 2020 at 03:43:55PM +0200, Thomas Zimmermann wrote:
> >> 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.
> > 
> > Thanks for taking the time to refactor all the relevant drivers.
> > 
> > I have received some push-back in the past when suggesting this,
> > but cannot remember from who.
> > Let's see what review comments you get.
> > 
> > As the rule is that the fbdev setup shall be setup after registering
> > the DRM device - it would be nice to have this included in the
> > documentation of drm_fbdev_generic_setup
> > 
> > Could you try to to update the documentation to cover this?
> 
> Good idea. I'll add this to patchset's next iteration.

Thanks

Patch 1 to 9 are all:
Acked-by: Sam Ravnborg <sam@ravnborg.org>


This patch "drm/tilcdc: Set up fbdev after fully registering device"
looks a little point less, but I see from a consistency point of view
why you did it.
So therefore it is also acked.


	Sam


> 
> Best regards
> Thomas
> 
> > 
> > I will get back to the patches later this week.
> > 
> > 	Sam
> > 
> >>
> >> 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-tested on x86-64, aarch64 and arm.
> >>
> >> [1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867
> >>
> >> 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/mediathek: 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                | 18 ++++++++----------
> >>  .../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, 25 insertions(+), 45 deletions(-)
> >>
> >> --
> >> 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
> 



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

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

[Sam Ravnborg]

Hi Thomas.

On Mon, Apr 06, 2020 at 03:44:05PM +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.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

If this goes in as-is then it is:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

You could apply the series now to avoid letting a doc update
postponse the others.
And then make the doc update a follow-up patch.

	Sam


> ---
>  drivers/gpu/drm/drm_fb_helper.c | 18 ++++++++----------
>  include/drm/drm_fb_helper.h     |  5 +++--
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 165c8dab50797..24db97eee53d4 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2186,11 +2186,9 @@ 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;
> @@ -2198,17 +2196,19 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  	WARN(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 +2222,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 00/10] Set up generic fbdev after registering device

[Noralf Trønnes]

Den 06.04.2020 15.43, skrev 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-tested on x86-64, aarch64 and arm.
> 
> [1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867
> 

Thanks for doing this, series is:

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

With so many drivers using generic fbdev I wondered if we could make it
the default and let DRM core handle it (would pull drm_fb_helper into
drm.ko).

Something like this at the end of drm_dev_register():

if (!dev->fb_helper)
	drm_fbdev_generic_setup(dev, 0);

AFAICS all drivers that don't use generic fbdev, do fbdev setup before
calling drm_dev_register() except msm so that would need some
adjustment, calling drm_fb_helper_init() before drm_dev_register() would do.

One missing piece is for drivers (like vc4) that uses 16 bpp on fbdev to
save on memory, not sure how that should be handled, an optional
drm_mode_config->fbdev_bpp maybe.

Having DRM core take care of fbdev emulation was an idea Laurent had
which was the spark that set me off making the generic fbdev emulation.

Maybe it's still too early to make this move, I don't know.

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

Re: [PATCH 05/10] drm/mediathek: Remove error check from fbdev setup

[Noralf Trønnes]

Den 06.04.2020 15.44, skrev 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>
> ---

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

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

[Jyri Sarha]

On 06/04/2020 16:44, Thomas Zimmermann wrote:
> 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>

However, this change hardly makes any difference, as the only place
where "is_registere"'s value is checked is in tilcdc_fini() which is
also called to cleanup the resources if tilcdc_init() fails.

Best regards,
Jyri

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


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

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

Hi Noralf

Am 07.04.20 um 13:02 schrieb Noralf Trønnes:
> 
> 
> Den 06.04.2020 15.43, skrev 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-tested on x86-64, aarch64 and arm.
>>
>> [1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867
>>
> 
> Thanks for doing this, series is:
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> 
> With so many drivers using generic fbdev I wondered if we could make it
> the default and let DRM core handle it (would pull drm_fb_helper into
> drm.ko).
> 
> Something like this at the end of drm_dev_register():
> 
> if (!dev->fb_helper)
> 	drm_fbdev_generic_setup(dev, 0);
> 
> AFAICS all drivers that don't use generic fbdev, do fbdev setup before
> calling drm_dev_register() except msm so that would need some
> adjustment, calling drm_fb_helper_init() before drm_dev_register() would do.
> 
> One missing piece is for drivers (like vc4) that uses 16 bpp on fbdev to
> save on memory, not sure how that should be handled, an optional
> drm_mode_config->fbdev_bpp maybe.
> 
> Having DRM core take care of fbdev emulation was an idea Laurent had
> which was the spark that set me off making the generic fbdev emulation.
> 
> Maybe it's still too early to make this move, I don't know.

I think we should wait a bit. As you mentioned, there are drivers that
have an fbdev bpp that differs from the preferred one. There might also
be a chicken-and-egg problem with core and fb-helper modules.

Additionally, we should think about possible other in-kernel clients
(e.g., the bootsplash) and how they would interact with such defaults.

At some later point, we could introduce an interface that sets up all
available in-kernel clients.

Best regards
Thomas

> 
> Noralf.
> 

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

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

[Thomas Zimmermann]

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

Hi

Am 07.04.20 um 10:04 schrieb Daniel Vetter:
> On Mon, Apr 06, 2020 at 03:43:58PM +0200, Thomas Zimmermann wrote:
>> 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>
>> ---
>>  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);
> 
> The code you jump over is nonsense and should be reverted. I replied to
> the patch that landed this.

What did they respond?

When I read this code, I wondered why it might be there.

Best regards
Thomas

> -Daniel
> 
>> +
>>  	return 0;
>>  
>>  err_drm_dev_unregister:
>> -- 
>> 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

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

[Sam Ravnborg]

Hi Thomas/Noralf.

> > Having DRM core take care of fbdev emulation was an idea Laurent had
> > which was the spark that set me off making the generic fbdev emulation.
> > 
> > Maybe it's still too early to make this move, I don't know.
> 
> I think we should wait a bit. As you mentioned, there are drivers that
> have an fbdev bpp that differs from the preferred one. There might also
> be a chicken-and-egg problem with core and fb-helper modules.

Noralf - you had analyzed what drivers are (yet) to migrate to
to the common fbdev emulation.
Link: https://lore.kernel.org/dri-devel/34e654ae-0cc9-e393-ac02-e4ac6eda60f6@tronnes.org/

rmada
gma500
amd
omapdrm
nouveau
i915
msm
tegra
exynos
radeon
rockchip

Maybe add this list to todo.rst - and if someone knows about it
we could have a small description what is required before a
driver can migrate.
I can cook up the patch if anyone thinks this is useful.

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

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

[Thomas Zimmermann]

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

Hi Sam

Am 07.04.20 um 18:50 schrieb Sam Ravnborg:
> Hi Thomas/Noralf.
> 
>>> Having DRM core take care of fbdev emulation was an idea Laurent had
>>> which was the spark that set me off making the generic fbdev emulation.
>>>
>>> Maybe it's still too early to make this move, I don't know.
>>
>> I think we should wait a bit. As you mentioned, there are drivers that
>> have an fbdev bpp that differs from the preferred one. There might also
>> be a chicken-and-egg problem with core and fb-helper modules.
> 
> Noralf - you had analyzed what drivers are (yet) to migrate to
> to the common fbdev emulation.
> Link: https://lore.kernel.org/dri-devel/34e654ae-0cc9-e393-ac02-e4ac6eda60f6@tronnes.org/
> 
> armada

This one looks like its convertible to common infrastructure. Not just
the fbdev console, but quite a bit of the framebuffer and GEM code. But
I don't have the HW to test any patches.

> gma500

I have a patch for replacing some of the gma500 code with common
infrastructure. It currently fails and I haven't found time to look closer.

> amd
> omapdrm
> nouveau
> i915
> msm
> tegra
> exynos
> radeon
> rockchip
> 
> Maybe add this list to todo.rst - and if someone knows about it
> we could have a small description what is required before a
> driver can migrate.
> I can cook up the patch if anyone thinks this is useful.

I thought there already was such an entry in the TODO list. Some of
these drivers have HW acceleration of some kind; although the benefits
might be debatable. And there where 2 or 3 drivers with a design that is
incompatible with generic fbdev. There's also some information at [1].

In any case, we should consider initializing each driver's fb console
after registering the device. All console implementations would than act
like DRM clients and less like driver components.

Best regards
Thomas

[1]
https://lore.kernel.org/dri-devel/7016126a-f498-1a4a-4721-c6305a961457@suse.de/

> 
> 	Sam
> 

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

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

[Thomas Zimmermann]

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

Hi Jani

Am 07.04.20 um 09:24 schrieb Jani Nikula:
> On Tue, 07 Apr 2020, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi Sam
>>
>> Am 06.04.20 um 22:00 schrieb Sam Ravnborg:
>>> Hi Thomas.
>>>
>>> On Mon, Apr 06, 2020 at 03:43:55PM +0200, Thomas Zimmermann wrote:
>>>> 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.
>>>
>>> Thanks for taking the time to refactor all the relevant drivers.
>>>
>>> I have received some push-back in the past when suggesting this,
>>> but cannot remember from who.
>>> Let's see what review comments you get.
>>>
>>> As the rule is that the fbdev setup shall be setup after registering
>>> the DRM device - it would be nice to have this included in the
>>> documentation of drm_fbdev_generic_setup
>>>
>>> Could you try to to update the documentation to cover this?
>>
>> Good idea. I'll add this to patchset's next iteration.
> 
> How about something like:
> 
> 	drm_WARN_ON(dev, !dev->registered);
> 
> (Not sure if that needs to be !dev->driver->load && !dev->registered).

I added such a warning to the patch. Only radeon uses load(), but it has
its own fbdev code. So the original test should be fine.

Best regards
Thomas

> 
> This can be a follow-up patch later too.
> 
> BR,
> Jani.
> 
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> I will get back to the patches later this week.
>>>
>>> 	Sam
>>>
>>>>
>>>> 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-tested on x86-64, aarch64 and arm.
>>>>
>>>> [1] https://lore.kernel.org/dri-devel/20200403135828.2542770-1-daniel.vetter@ffwll.ch/T/#m216b5b37aeeb7b28d55ad73b7a702b3d1d7bf867
>>>>
>>>> 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/mediathek: 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                | 18 ++++++++----------
>>>>  .../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, 25 insertions(+), 45 deletions(-)
>>>>
>>>> --
>>>> 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