Hi Am 03.04.23 um 17:07 schrieb Emil Velikov:
On Mon, 3 Apr 2023 at 15:50, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:Hi Am 03.04.23 um 16:27 schrieb Emil Velikov:On Mon, 3 Apr 2023 at 11:41, Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> --- 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@xxxxxxxxx>
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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
