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

[Emil Velikov]

On Mon, 3 Apr 2023 at 15:50, Thomas Zimmermann <tzimmermann at suse.de> wrote:
>
> Hi
>
> Am 03.04.23 um 16:27 schrieb Emil Velikov:
> > On Mon, 3 Apr 2023 at 11:41, Thomas Zimmermann <tzimmermann at suse.de> 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.
> >>
> >> v2:
> >>          * init drm_client in this patch (Tomi)
> >>          * don't handle non-atomic modesetting (Tomi)
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> >> Reviewed-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >> ---
> >>   drivers/gpu/drm/omapdrm/omap_drv.c   |  11 +--
> >>   drivers/gpu/drm/omapdrm/omap_fbdev.c | 132 +++++++++++++++++----------
> >>   drivers/gpu/drm/omapdrm/omap_fbdev.h |   9 +-
> >>   3 files changed, 90 insertions(+), 62 deletions(-)
> >>
> >
> >> +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();
> >> +
> >
> > What a lovely little surprise. It's a pre-existing "feature", so let's
> > ignore that for now ;-)
>
> I have no idea what you're talking about. This code was in the original
> clean-up function. If it is not supposed to be here, I can remove it.
>

Precisely - the original code had the DBG() and your change preserves
it. Based on my reading, it shouldn't be there ... then again don't
read too much into that.

> >
> >> +       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);
> >> +}
> >
> >
> >> -void omap_fbdev_fini(struct drm_device *dev)
> >> +static const struct drm_client_funcs omap_fbdev_client_funcs = {
> >> +       .owner          = THIS_MODULE,
> >> +       .unregister     = omap_fbdev_client_unregister,
> >> +       .restore        = omap_fbdev_client_restore,
> >> +       .hotplug        = omap_fbdev_client_hotplug,
> >
> > AFAICT the fbdev client helpers above are identical to the generic
> > ones in drm_fbdev_generic.c. Why aren't we reusing those but
> > copy/pasting them in the driver?
>
> The code in drm_fbdev_generic.c (and other fbdev files) might be
> shareable at some point when I know what exactly I need.

> I already plan
> to move some of the damage handling from drm_fb_helper.c to
> drm_fbdev_generic.c and that will affect the helpers that are currently
> identical.

Now that's the piece that I was missing.

> There's little point in code sharing right now.
>
> I know that the fbdev emulation is convoluted and confusing at times.
> It's the result of various redesigns and false starts. Things are
> getting better, though.
>

Been keeping an eye on your work and it's shaping up great. It's
deeply appreciated.

Fwiw the series is:
Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>

-Emil