[PATCH 6/6] drm/omapdrm: Implement fbdev emulation as in-kernel client
Thomas Zimmermann
tzimmermann at suse.de
Mon Apr 3 08:24:26 UTC 2023
Hi
Am 30.03.23 um 15:13 schrieb Tomi Valkeinen:
> On 30/03/2023 11:32, Thomas Zimmermann wrote:
>> Move code from ad-hoc fbdev callbacks into DRM client functions
>> and remove the old callbacks. The functions instruct the client
>> to poll for changed output or restore the display. The DRM core
>> calls both, the old callbacks and the new client helpers, from
>> the same places. The new functions perform the same operation as
>> before, so there's no change in functionality.
>>
>> Replace all code that initializes or releases fbdev emulation
>> throughout the driver. Instead initialize the fbdev client by a
>> single call to omapdrm_fbdev_setup() after omapdrm has registered
>> its DRM device. As in most drivers, omapdrm's fbdev emulation now
>> acts like a regular DRM client.
>>
>> The fbdev client setup consists of the initial preparation and the
>> hot-plugging of the display. The latter creates the fbdev device
>> and sets up the fbdev framebuffer. The setup performs display
>> hot-plugging once. If no display can be detected, DRM probe helpers
>> re-run the detection on each hotplug event.
>>
>> A call to drm_dev_unregister() releases the client automatically.
>> No further action is required within omapdrm. If the fbdev
>> framebuffer has been fully set up, struct fb_ops.fb_destroy
>> implements the release. For partially initialized emulation, the
>> fbdev client reverts the initial setup.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/omapdrm/omap_drv.c | 11 +--
>> drivers/gpu/drm/omapdrm/omap_fbdev.c | 136 ++++++++++++++-------------
>> drivers/gpu/drm/omapdrm/omap_fbdev.h | 9 +-
>> 3 files changed, 77 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>> b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index 6a2f446c960f..5ed549726104 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -15,7 +15,6 @@
>> #include <drm/drm_bridge.h>
>> #include <drm/drm_bridge_connector.h>
>> #include <drm/drm_drv.h>
>> -#include <drm/drm_fb_helper.h>
>> #include <drm/drm_file.h>
>> #include <drm/drm_ioctl.h>
>> #include <drm/drm_panel.h>
>> @@ -221,7 +220,6 @@ static const struct drm_mode_config_helper_funcs
>> omap_mode_config_helper_funcs =
>> static const struct drm_mode_config_funcs omap_mode_config_funcs = {
>> .fb_create = omap_framebuffer_create,
>> - .output_poll_changed = drm_fb_helper_output_poll_changed,
>> .atomic_check = omap_atomic_check,
>> .atomic_commit = drm_atomic_helper_commit,
>> };
>> @@ -654,7 +652,6 @@ static const struct drm_driver omap_drm_driver = {
>> .driver_features = DRIVER_MODESET | DRIVER_GEM |
>> DRIVER_ATOMIC | DRIVER_RENDER,
>> .open = dev_open,
>> - .lastclose = drm_fb_helper_lastclose,
>> #ifdef CONFIG_DEBUG_FS
>> .debugfs_init = omap_debugfs_init,
>> #endif
>> @@ -743,8 +740,6 @@ static int omapdrm_init(struct omap_drm_private
>> *priv, struct device *dev)
>> goto err_cleanup_modeset;
>> }
>> - omap_fbdev_init(ddev);
>> -
>> drm_kms_helper_poll_init(ddev);
>> /*
>> @@ -755,12 +750,12 @@ static int omapdrm_init(struct omap_drm_private
>> *priv, struct device *dev)
>> if (ret)
>> goto err_cleanup_helpers;
>> + omap_fbdev_setup(ddev);
>> +
>> return 0;
>> err_cleanup_helpers:
>> drm_kms_helper_poll_fini(ddev);
>> -
>> - omap_fbdev_fini(ddev);
>> err_cleanup_modeset:
>> omap_modeset_fini(ddev);
>> err_free_overlays:
>> @@ -786,8 +781,6 @@ static void omapdrm_cleanup(struct
>> omap_drm_private *priv)
>> drm_kms_helper_poll_fini(ddev);
>> - omap_fbdev_fini(ddev);
>> -
>> drm_atomic_helper_shutdown(ddev);
>> omap_modeset_fini(ddev);
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> index f0e35f4764a7..cd63142a4705 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> @@ -4,13 +4,14 @@
>> * Author: Rob Clark <rob at ti.com>
>> */
>> -#include <drm/drm_crtc.h>
>> -#include <drm/drm_util.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_crtc_helper.h>
>> #include <drm/drm_fb_helper.h>
>> #include <drm/drm_file.h>
>> #include <drm/drm_fourcc.h>
>> #include <drm/drm_framebuffer.h>
>> #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_util.h>
>> #include "omap_drv.h"
>> @@ -72,6 +73,25 @@ static int omap_fbdev_pan_display(struct
>> fb_var_screeninfo *var,
>> return drm_fb_helper_pan_display(var, fbi);
>> }
>> +static void omap_fbdev_fb_destroy(struct fb_info *info)
>> +{
>> + struct drm_fb_helper *helper = info->par;
>> + struct drm_framebuffer *fb = helper->fb;
>> + struct drm_gem_object *bo = drm_gem_fb_get_obj(fb, 0);
>> + struct omap_fbdev *fbdev = to_omap_fbdev(helper);
>> +
>> + DBG();
>> +
>> + drm_fb_helper_fini(helper);
>> +
>> + omap_gem_unpin(bo);
>> + drm_framebuffer_remove(fb);
>> +
>> + drm_client_release(&helper->client);
>> + drm_fb_helper_unprepare(helper);
>> + kfree(fbdev);
>> +}
>> +
>> static const struct fb_ops omap_fb_ops = {
>> .owner = THIS_MODULE,
>> @@ -87,6 +107,8 @@ static const struct fb_ops omap_fb_ops = {
>> .fb_fillrect = drm_fb_helper_sys_fillrect,
>> .fb_copyarea = drm_fb_helper_sys_copyarea,
>> .fb_imageblit = drm_fb_helper_sys_imageblit,
>> +
>> + .fb_destroy = omap_fbdev_fb_destroy,
>> };
>> static int omap_fbdev_create(struct drm_fb_helper *helper,
>> @@ -226,16 +248,52 @@ static struct drm_fb_helper *get_fb(struct
>> fb_info *fbi)
>> */
>> static void omap_fbdev_client_unregister(struct drm_client_dev *client)
>> -{ }
>> +{
>> + struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>> +
>> + if (fb_helper->info) {
>> + drm_fb_helper_unregister_info(fb_helper);
>
> Just trying to understand the code flow. So this is called if we have
> had a display connected, and have called omap_fbdev_create()? And
> drm_fb_helper_unregister_info() ends up calling omap_fbdev_fb_destroy().
Exactly.
>
>> + } else {
>> + drm_client_release(&fb_helper->client);
>> + drm_fb_helper_unprepare(fb_helper);
>> + kfree(fb_helper);
>
> And this path is for the case where we haven't created the fbdev.
Exactly.
When you look at other fbdev code, you'll find the same pattern. We
either clean up everything; or just the client and the minimal fbdev
state that has been prepared by the _setup function.
There's been talk about sharing some of this in helpers among drivers.
And I hope to do this, but I first want to convert older fbdev code to
drm_client. I first wanted a clear idea of which code can actually be
shared.
>
>> + }
>> +}
>> static int omap_fbdev_client_restore(struct drm_client_dev *client)
>> {
>> + drm_fb_helper_lastclose(client->dev);
>> +
>> return 0;
>> }
>> static int omap_fbdev_client_hotplug(struct drm_client_dev *client)
>> {
>> + struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>> + struct drm_device *dev = client->dev;
>> + int ret;
>> +
>> + if (dev->fb_helper)
>
> And this tests if this is not the first call to
> omap_fbdev_client_hotplug()?
Exactly. Here, we already have a fully initialized fbdev device with
framebuffer and videomode.
>
>> + return drm_fb_helper_hotplug_event(dev->fb_helper);
>> +
>> + ret = drm_fb_helper_init(dev, fb_helper);
>> + if (ret)
>> + goto err_drm_err;
>> +
>> + if (!drm_drv_uses_atomic_modeset(dev))
>
> Why do we need this? omapdrm supports atomic.
Even better. I'm going to remove it.
>
> That's my only real comment. Kernel test bot had one comment too. But
> other than that:
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>
> I tested this series on TI DRA76 EVM, and worked fine for me.
That you so much.
I'm going to send out an update with the fixed patches. Is there an omap
tree or does it later go through drm-misc?
Best regards
Thomas
>
> Tomi
>
>> + drm_helper_disable_unused_functions(dev);
>> +
>> + ret = drm_fb_helper_initial_config(fb_helper);
>> + if (ret)
>> + goto err_drm_fb_helper_fini;
>> +
>> return 0;
>> +
>> +err_drm_fb_helper_fini:
>> + drm_fb_helper_fini(fb_helper);
>> +err_drm_err:
>> + drm_err(dev, "Failed to setup fbdev emulation (ret=%d)\n", ret);
>> + return ret;
>> }
>> static const struct drm_client_funcs omap_fbdev_client_funcs = {
>> @@ -245,85 +303,37 @@ static const struct drm_client_funcs
>> omap_fbdev_client_funcs = {
>> .hotplug = omap_fbdev_client_hotplug,
>> };
>> -/* initialize fbdev helper */
>> -void omap_fbdev_init(struct drm_device *dev)
>> +void omap_fbdev_setup(struct drm_device *dev)
>> {
>> - struct omap_drm_private *priv = dev->dev_private;
>> - struct omap_fbdev *fbdev = NULL;
>> + struct omap_fbdev *fbdev;
>> struct drm_fb_helper *helper;
>> - int ret = 0;
>> + int ret;
>> - if (!priv->num_pipes)
>> - return;
>> + drm_WARN(dev, !dev->registered, "Device has not been
>> registered.\n");
>> + drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>> fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>> if (!fbdev)
>> return;
>> -
>> - INIT_WORK(&fbdev->work, pan_worker);
>> -
>> helper = &fbdev->base;
>> drm_fb_helper_prepare(dev, helper, 32, &omap_fb_helper_funcs);
>> ret = drm_client_init(dev, &helper->client, "fbdev",
>> &omap_fbdev_client_funcs);
>> if (ret)
>> - goto fail;
>> + goto err_drm_client_init;
>> - ret = drm_fb_helper_init(dev, helper);
>> - if (ret)
>> - goto err_drm_client_release;
>> + INIT_WORK(&fbdev->work, pan_worker);
>> - ret = drm_fb_helper_initial_config(helper);
>> + ret = omap_fbdev_client_hotplug(&helper->client);
>> if (ret)
>> - goto fini;
>> -
>> - return;
>> + drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>> -fini:
>> - drm_fb_helper_fini(helper);
>> -err_drm_client_release:
>> - drm_client_release(&helper->client);
>> -fail:
>> - drm_fb_helper_unprepare(helper);
>> - kfree(fbdev);
>> + drm_client_register(&helper->client);
>> - dev_warn(dev->dev, "omap_fbdev_init failed\n");
>> -}
>> -
>> -void omap_fbdev_fini(struct drm_device *dev)
>> -{
>> - struct drm_fb_helper *helper = dev->fb_helper;
>> - struct drm_framebuffer *fb;
>> - struct drm_gem_object *bo;
>> - struct omap_fbdev *fbdev;
>> -
>> - DBG();
>> -
>> - if (!helper)
>> - return;
>> -
>> - fb = helper->fb;
>> -
>> - drm_fb_helper_unregister_info(helper);
>> -
>> - drm_fb_helper_fini(helper);
>> -
>> - fbdev = to_omap_fbdev(helper);
>> -
>> - bo = drm_gem_fb_get_obj(fb, 0);
>> -
>> - /* unpin the GEM object pinned in omap_fbdev_create() */
>> - if (bo)
>> - omap_gem_unpin(bo);
>> -
>> - /* this will free the backing object */
>> - if (fb)
>> - drm_framebuffer_remove(fb);
>> + return;
>> - drm_client_release(&helper->client);
>> +err_drm_client_init:
>> drm_fb_helper_unprepare(helper);
>> kfree(fbdev);
>> -
>> - dev->fb_helper = NULL;
>> }
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h
>> b/drivers/gpu/drm/omapdrm/omap_fbdev.h
>> index 74a68a5a6eab..74c691a8d45f 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h
>> @@ -10,16 +10,11 @@
>> #define __OMAPDRM_FBDEV_H__
>> struct drm_device;
>> -struct drm_fb_helper;
>> #ifdef CONFIG_DRM_FBDEV_EMULATION
>> -void omap_fbdev_init(struct drm_device *dev);
>> -void omap_fbdev_fini(struct drm_device *dev);
>> +void omap_fbdev_setup(struct drm_device *dev);
>> #else
>> -static inline void omap_fbdev_init(struct drm_device *dev)
>> -{
>> -}
>> -static inline void omap_fbdev_fini(struct drm_device *dev)
>> +static inline void omap_fbdev_setup(struct drm_device *dev)
>> {
>> }
>> #endif
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230403/05d6b1f9/attachment-0001.sig>
More information about the dri-devel
mailing list