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

[Thomas Zimmermann]

Hi

Am 03.04.23 um 17:07 schrieb 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>

Thanks for reviewing.

Best regards
Thomas

> 
> -Emil

-- 
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/20230404/08f22356/attachment-0001.sig>