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@ffw...
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
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);
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;
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:
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
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
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;
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
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;
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/
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:
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:
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:
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;
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:
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;
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
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@ffw...
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
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@ffw...
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
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@ffw...
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
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@ffw...
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
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@ffw...
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
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
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@ffw...
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.
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@ffw...
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.
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@tronn...
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
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@tronn...
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....
Sam
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Gerd Hoffmann -
Jani Nikula -
Jyri Sarha -
Noralf Trønnes -
Paul Cercueil -
Sam Ravnborg -
Thomas Zimmermann