Hello,

On Tuesday, 29 August 2017 00:34:57 EEST Daniel Vetter wrote:

On Mon, Aug 28, 2017 at 07:17:43PM +0200, Noralf Trønnes wrote:

...

drm_fb_helper_resume_worker() uses fb_helper->fbdev to call fb_set_suspend() which dereferences the pointer. Move sync-canceling of the resume worker in drm_fb_helper_fini() before setting fb_helper->fbdev to NULL.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

drivers/gpu/drm/drm_fb_helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1b8f013..2e33467 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -910,6 +910,8 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)> if (!drm_fbdev_emulation || !fb_helper)

return;

• cancel_work_sync(&fb_helper->resume_work);

• info = fb_helper->fbdev; if (info) {

  if (info->cmap.len)

@@ -918,7 +920,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)> } fb_helper->fbdev = NULL;

• cancel_work_sync(&fb_helper->resume_work);

  cancel_work_sync(&fb_helper->dirty_work);

Hm, I would have moved both up, just for safety. Either way:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

I was going to mention the same, let's move both. With this changed,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

...

mutex_lock(&kernel_fb_helper_lock);

Den 28.08.2017 23.41, skrev Daniel Vetter:

On Mon, Aug 28, 2017 at 07:17:44PM +0200, Noralf Trønnes wrote:

...

Support device unplug by introducing a new initialization function: drm_fb_helper_simple_init() which together with drm_fb_helper_dev_unplug() and drm_fb_helper_fb_destroy() handles open fbdev fd's after device unplug. There's also a drm_fb_helper_simple_fini() for drivers who's device won't be removed.

It turns out that fbdev has the necessary ref counting, so unregister_framebuffer() together with fb_ops->destroy handles unplug with open fd's. The ref counting doesn't apply to the fb_info structure itself, but to use of the fbdev framebuffer.

Analysis of entry points after unregister_framebuffer():

• fbcon has been unbound (through notifier)
• sysfs files removed

First we look at possible operations on open fbdev file handles:

static const struct file_operations fb_fops = { .read = fb_read, .write = fb_write, .unlocked_ioctl = fb_ioctl, .compat_ioctl = fb_compat_ioctl, .mmap = fb_mmap, .open = fb_open, .release = fb_release, .get_unmapped_area = get_fb_unmapped_area, .fsync = fb_deferred_io_fsync, };

Not possible after unregister: fb_open() -> fb_ops.fb_open

Protected by file_fb_info() (-ENODEV): fb_read() -> fb_ops.fb_read : drm_fb_helper_sys_read() fb_write() -> fb_ops.fb_write : drm_fb_helper_sys_write() fb_ioctl() -> fb_ops.fb_ioctl : drm_fb_helper_ioctl() fb_compat_ioctl() -> fb_ops.fb_compat_ioctl fb_mmap() -> fb_ops.fb_mmap

Safe: fb_release() -> fb_ops.fb_release get_fb_unmapped_area() : info->screen_base fb_deferred_io_fsync() : if (info->fbdefio) &info->deferred_work

Active mmap's will need the backing buffer to be present. If deferred IO is used, mmap writes will via a worker generate calls to drm_fb_helper_deferred_io() which in turn via a worker calls into drm_fb_helper_dirty_work().

Secondly we look at the remaining struct fb_ops operations:

Called by fbcon:

• fb_ops.fb_fillrect : drm_fb_helper_{sys,cfb}_fillrect()
• fb_ops.fb_copyarea : drm_fb_helper_{sys,cfb}_copyarea()
• fb_ops.fb_imageblit : drm_fb_helper_{sys,cfb}_imageblit()

Called in fb_set_var() which is called from ioctl, sysfs and fbcon:

• fb_ops.fb_check_var : drm_fb_helper_check_var()
• fb_ops.fb_set_par : drm_fb_helper_set_par()

drm_fb_helper_set_par() is also called from drm_fb_helper_hotplug_event().

Called in fb_set_cmap() which is called from fb_set_var(), ioctl and fbcon:

• fb_ops.fb_setcolreg
• fb_ops.fb_setcmap : drm_fb_helper_setcmap()

Called in fb_blank() which is called from ioctl and fbcon:

• fb_ops.fb_blank : drm_fb_helper_blank()

Called in fb_pan_display() which is called from fb_set_var(), ioctl, sysfs and fbcon:

• fb_ops.fb_pan_display : drm_fb_helper_pan_display()

Called by fbcon:

• fb_ops.fb_cursor

Called in fb_read(), fb_write(), and fb_get_buffer_offset() which is called by fbcon:

• fb_ops.fb_sync

Called by fb_set_var():

• fb_ops.fb_get_caps

Called by fbcon:

• fb_ops.fb_debug_enter : drm_fb_helper_debug_enter()
• fb_ops.fb_debug_leave : drm_fb_helper_debug_leave()

Destroy is safe

• fb_ops.fb_destroy

Finally we look at other call paths:

drm_fb_helper_set_suspend{_unlocked}() and drm_fb_helper_resume_worker(): Calls into fb_set_suspend(), but that's fine since it just uses the fbdev notifier.

drm_fb_helper_restore_fbdev_mode_unlocked(): Called from drm_driver.last_close, possibly after drm_fb_helper_fini().

drm_fb_helper_force_kernel_mode(): Triggered by sysrq, possibly after unplug, but before drm_fb_helper_fini().

drm_fb_helper_hotplug_event(): Called by drm_kms_helper_hotplug_event(). I don't know if this can be called after drm_dev_unregister(), so add a check to be safe.

Based on this analysis the following functions get a drm_dev_is_unplugged() check:

• drm_fb_helper_restore_fbdev_mode_unlocked()
• drm_fb_helper_force_kernel_mode()
• drm_fb_helper_hotplug_event()
• drm_fb_helper_dirty_work()

Signed-off-by: Noralf Trønnes noralf@tronnes.org

You're mixing in the new simple fbdev helper helpers as a bonus, that's two things in one patch and makes it harder to review what's really going on.

My gut feeling is that when we need a helper for the helper the original helper needs to be improved, but I think that's much easier to decide when it's a separate patch. Splitting things out would definitely make this patch much smaller and easier to understand and review.

The only reason for the simple helpers that I could find is the drm_dev_ref/unref, we should be able to do that in one of the existing callbacks.

The reason I did that is because I couldn't put it in the existing helpers since the framebuffer is removed after drm_fb_helper_fini() and I can't drop the ref on drm_device before the framebuffer is gone.

I can split the patch in two: 1. Add drm_dev_is_unplugged() checks 2. Add drm_fb_helper_simple_init/fini() and drm_fb_helper_fb_destroy()

Maybe there's a way to remove the framebuffer in drm_fb_helper_fini() so we can drop the ref there, I don't know.

Here's how drivers destroy fbdev:

static int amdgpu_fbdev_destroy(struct drm_device *dev, struct amdgpu_fbdev *rfbdev) {     struct amdgpu_framebuffer *rfb = &rfbdev->rfb;

drm_fb_helper_unregister_fbi(&rfbdev->helper);

if (rfb->obj) {         amdgpufb_destroy_pinned_object(rfb->obj);         rfb->obj = NULL;     }     drm_fb_helper_fini(&rfbdev->helper);     drm_framebuffer_unregister_private(&rfb->base);     drm_framebuffer_cleanup(&rfb->base);

return 0; }

void armada_fbdev_fini(struct drm_device *dev) {     struct armada_private *priv = dev->dev_private;     struct drm_fb_helper *fbh = priv->fbdev;

if (fbh) {         drm_fb_helper_unregister_fbi(fbh);

drm_fb_helper_fini(fbh);

if (fbh->fb)             fbh->fb->funcs->destroy(fbh->fb);

priv->fbdev = NULL;     } }

static void ast_fbdev_destroy(struct drm_device *dev,                   struct ast_fbdev *afbdev) {     struct ast_framebuffer *afb = &afbdev->afb;

drm_fb_helper_unregister_fbi(&afbdev->helper);

if (afb->obj) {         drm_gem_object_unreference_unlocked(afb->obj);         afb->obj = NULL;     }     drm_fb_helper_fini(&afbdev->helper);

vfree(afbdev->sysram);     drm_framebuffer_unregister_private(&afb->base);     drm_framebuffer_cleanup(&afb->base); }

static int bochs_fbdev_destroy(struct bochs_device *bochs) {     struct bochs_framebuffer *gfb = &bochs->fb.gfb;

DRM_DEBUG_DRIVER("\n");

drm_fb_helper_unregister_fbi(&bochs->fb.helper);

if (gfb->obj) {         drm_gem_object_unreference_unlocked(gfb->obj);         gfb->obj = NULL;     }

drm_framebuffer_unregister_private(&gfb->base);     drm_framebuffer_cleanup(&gfb->base);

return 0; }

static int cirrus_fbdev_destroy(struct drm_device *dev,                 struct cirrus_fbdev *gfbdev) {     struct cirrus_framebuffer *gfb = &gfbdev->gfb;

drm_fb_helper_unregister_fbi(&gfbdev->helper);

if (gfb->obj) {         drm_gem_object_unreference_unlocked(gfb->obj);         gfb->obj = NULL;     }

vfree(gfbdev->sysram);     drm_fb_helper_fini(&gfbdev->helper);     drm_framebuffer_unregister_private(&gfb->base);     drm_framebuffer_cleanup(&gfb->base);

return 0; }

void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) {     drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);     if (fbdev_cma->fb_helper.fbdev)         drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);

if (fbdev_cma->fb)         drm_framebuffer_remove(&fbdev_cma->fb->fb);

drm_fb_helper_fini(&fbdev_cma->fb_helper);     kfree(fbdev_cma); }

static void exynos_drm_fbdev_destroy(struct drm_device *dev,                       struct drm_fb_helper *fb_helper) {     struct exynos_drm_fbdev *exynos_fbd = to_exynos_fbdev(fb_helper);     struct exynos_drm_gem *exynos_gem = exynos_fbd->exynos_gem;     struct drm_framebuffer *fb;

vunmap(exynos_gem->kvaddr);

/* release drm framebuffer and real buffer */     if (fb_helper->fb && fb_helper->fb->funcs) {         fb = fb_helper->fb;         if (fb)             drm_framebuffer_remove(fb);     }

drm_fb_helper_unregister_fbi(fb_helper);

drm_fb_helper_fini(fb_helper); }

static int psb_fbdev_destroy(struct drm_device *dev, struct psb_fbdev *fbdev) {     struct psb_framebuffer *psbfb = &fbdev->pfb;

drm_fb_helper_unregister_fbi(&fbdev->psb_fb_helper);

drm_fb_helper_fini(&fbdev->psb_fb_helper);     drm_framebuffer_unregister_private(&psbfb->base);     drm_framebuffer_cleanup(&psbfb->base);

if (psbfb->gtt) drm_gem_object_unreference_unlocked(&psbfb->gtt->gem);     return 0; }

static void hibmc_fbdev_destroy(struct hibmc_fbdev *fbdev) {     struct hibmc_framebuffer *gfb = fbdev->fb;     struct drm_fb_helper *fbh = &fbdev->helper;

drm_fb_helper_unregister_fbi(fbh);

drm_fb_helper_fini(fbh);

if (gfb)         drm_framebuffer_unreference(&gfb->fb); }

static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) {     /* We rely on the object-free to release the VMA pinning for      * the info->screen_base mmaping. Leaking the VMA is simpler than      * trying to rectify all the possible error paths leading here.      */

drm_fb_helper_unregister_fbi(&ifbdev->helper);

drm_fb_helper_fini(&ifbdev->helper);

if (ifbdev->vma) {         mutex_lock(&ifbdev->helper.dev->struct_mutex);         intel_unpin_fb_vma(ifbdev->vma);         mutex_unlock(&ifbdev->helper.dev->struct_mutex);     }

if (ifbdev->fb)         drm_framebuffer_remove(&ifbdev->fb->base);

kfree(ifbdev); }

static int mga_fbdev_destroy(struct drm_device *dev,                 struct mga_fbdev *mfbdev) {     struct mga_framebuffer *mfb = &mfbdev->mfb;

drm_fb_helper_unregister_fbi(&mfbdev->helper);

if (mfb->obj) {         drm_gem_object_unreference_unlocked(mfb->obj);         mfb->obj = NULL;     }     drm_fb_helper_fini(&mfbdev->helper);     vfree(mfbdev->sysram);     drm_framebuffer_unregister_private(&mfb->base);     drm_framebuffer_cleanup(&mfb->base);

return 0; }

void msm_fbdev_free(struct drm_device *dev) {     struct msm_drm_private *priv = dev->dev_private;     struct drm_fb_helper *helper = priv->fbdev;     struct msm_fbdev *fbdev;

DBG();

drm_fb_helper_unregister_fbi(helper);

drm_fb_helper_fini(helper);

fbdev = to_msm_fbdev(priv->fbdev);

/* this will free the backing object */     if (fbdev->fb) {         msm_gem_put_vaddr(fbdev->bo);         drm_framebuffer_remove(fbdev->fb);     }

kfree(fbdev);

priv->fbdev = NULL; }

static int nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) {     struct nouveau_framebuffer *nouveau_fb = nouveau_framebuffer(fbcon->helper.fb);

drm_fb_helper_unregister_fbi(&fbcon->helper);     drm_fb_helper_fini(&fbcon->helper);

if (nouveau_fb->nvbo) {         nouveau_bo_vma_del(nouveau_fb->nvbo, &nouveau_fb->vma);         nouveau_bo_unmap(nouveau_fb->nvbo);         nouveau_bo_unpin(nouveau_fb->nvbo);         drm_framebuffer_unreference(&nouveau_fb->base);     }

return 0; }

void omap_fbdev_free(struct drm_device *dev) {     struct omap_drm_private *priv = dev->dev_private;     struct drm_fb_helper *helper = priv->fbdev;     struct omap_fbdev *fbdev;

DBG();

drm_fb_helper_unregister_fbi(helper);

drm_fb_helper_fini(helper);

fbdev = to_omap_fbdev(priv->fbdev);

/* release the ref taken in omap_fbdev_create() */     omap_gem_put_paddr(fbdev->bo);

/* this will free the backing object */     if (fbdev->fb)         drm_framebuffer_remove(fbdev->fb);

kfree(fbdev);

priv->fbdev = NULL; }

static int qxl_fbdev_destroy(struct drm_device *dev, struct qxl_fbdev *qfbdev) {     struct qxl_framebuffer *qfb = &qfbdev->qfb;

drm_fb_helper_unregister_fbi(&qfbdev->helper);

if (qfb->obj) {         qxlfb_destroy_pinned_object(qfb->obj);         qfb->obj = NULL;     }     drm_fb_helper_fini(&qfbdev->helper);     vfree(qfbdev->shadow);     drm_framebuffer_cleanup(&qfb->base);

return 0; }

static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfbdev) {     struct radeon_framebuffer *rfb = &rfbdev->rfb;

drm_fb_helper_unregister_fbi(&rfbdev->helper);

if (rfb->obj) {         radeonfb_destroy_pinned_object(rfb->obj);         rfb->obj = NULL;     }     drm_fb_helper_fini(&rfbdev->helper);     drm_framebuffer_unregister_private(&rfb->base);     drm_framebuffer_cleanup(&rfb->base);

return 0; }

void rockchip_drm_fbdev_fini(struct drm_device *dev) {     struct rockchip_drm_private *private = dev->dev_private;     struct drm_fb_helper *helper;

helper = &private->fbdev_helper;

drm_fb_helper_unregister_fbi(helper);

if (helper->fb)         drm_framebuffer_unreference(helper->fb);

drm_fb_helper_fini(helper); }

static void tegra_fbdev_exit(struct tegra_fbdev *fbdev) {     drm_fb_helper_unregister_fbi(&fbdev->base);

if (fbdev->fb)         drm_framebuffer_remove(&fbdev->fb->base);

drm_fb_helper_fini(&fbdev->base);     tegra_fbdev_free(fbdev); }

static void udl_fbdev_destroy(struct drm_device *dev,                   struct udl_fbdev *ufbdev) {     drm_fb_helper_unregister_fbi(&ufbdev->helper);     drm_fb_helper_fini(&ufbdev->helper);     drm_framebuffer_unregister_private(&ufbdev->ufb.base);     drm_framebuffer_cleanup(&ufbdev->ufb.base); drm_gem_object_unreference_unlocked(&ufbdev->ufb.obj->base); }

static int virtio_gpu_fbdev_destroy(struct drm_device *dev,                     struct virtio_gpu_fbdev *vgfbdev) {     struct virtio_gpu_framebuffer *vgfb = &vgfbdev->vgfb;

drm_fb_helper_unregister_fbi(&vgfbdev->helper);

if (vgfb->obj)         vgfb->obj = NULL;     drm_fb_helper_fini(&vgfbdev->helper);     drm_framebuffer_cleanup(&vgfb->base);

return 0; }

Noralf.

...

---

drivers/gpu/drm/drm_fb_helper.c | 204 +++++++++++++++++++++++++++++++++++++++- include/drm/drm_fb_helper.h | 35 +++++++ 2 files changed, 237 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 2e33467..fc898db 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -105,6 +105,158 @@ static DEFINE_MUTEX(kernel_fb_helper_lock);

• mmap page writes.

*/

+/**

• • drm_fb_helper_simple_init - Simple fbdev emulation initialization
• • @dev: drm device
• • @fb_helper: driver-allocated fbdev helper structure to initialize
• • @bpp_sel: bpp value to use for the framebuffer configuration
• • @max_conn_count: max connector count
• • @funcs: pointer to structure of functions associate with this helper
• • Simple fbdev emulation initialization which calls the following functions:
• • drm_fb_helper_prepare(), drm_fb_helper_init(),
• • drm_fb_helper_single_add_all_connectors() and
• • drm_fb_helper_initial_config().
• • This function takes a ref on &drm_device and must be used together with
• • drm_fb_helper_simple_fini() or drm_fb_helper_dev_unplug().
• • fbdev deferred I/O users must use drm_fb_helper_defio_init().
• • Returns:
• • 0 on success or a negative error code on failure.
• */

+int drm_fb_helper_simple_init(struct drm_device *dev,

• 	      struct drm_fb_helper *fb_helper, int bpp_sel,
  

• 	      int max_conn_count,
  

• 	      const struct drm_fb_helper_funcs *funcs)
  

+{

• int ret;
• drm_fb_helper_prepare(dev, fb_helper, funcs);
• ret = drm_fb_helper_init(dev, fb_helper, max_conn_count);
• if (ret < 0) {

• DRM_DEV_ERROR(dev->dev, "Failed to initialize fb helper.\n");
  

• return ret;
  

• }
• drm_dev_ref(dev);
• ret = drm_fb_helper_single_add_all_connectors(fb_helper);
• if (ret < 0) {

• DRM_DEV_ERROR(dev->dev, "Failed to add connectors.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• ret = drm_fb_helper_initial_config(fb_helper, bpp_sel);
• if (ret < 0) {

• DRM_DEV_ERROR(dev->dev, "Failed to set initial hw config.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• return 0;

+err_drm_fb_helper_fini:

• drm_fb_helper_fini(fb_helper);
• drm_dev_unref(dev);
• return ret;

+} +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_init);

+static void drm_fb_helper_simple_fini_cleanup(struct drm_fb_helper *fb_helper) +{

• struct fb_info *info = fb_helper->fbdev;
• struct fb_ops *fbops = NULL;
• if (info && info->fbdefio) {

• fb_deferred_io_cleanup(info);
  

• kfree(info->fbdefio);
  

• info->fbdefio = NULL;
  

• fbops = info->fbops;
  

• }
• drm_fb_helper_fini(fb_helper);
• kfree(fbops);
• if (fb_helper->fb)

• drm_framebuffer_remove(fb_helper->fb);
  

• drm_dev_unref(fb_helper->dev);

+}

+/**

• • drm_fb_helper_simple_fini - Simple fbdev cleanup
• • @fb_helper: fbdev emulation structure, can be NULL
• • Simple fbdev emulation cleanup. This function unregisters fbdev, cleans up
• • deferred I/O if necessary, finalises @fb_helper and removes the framebuffer.
• • The driver if responsible for freeing the @fb_helper structure.
• • Don't use this function if you use drm_fb_helper_dev_unplug().
• */

+void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper) +{

• struct fb_info *info;
• if (!fb_helper)

• return;
  

• info = fb_helper->fbdev;
• /* Has drm_fb_helper_dev_unplug() been used? */
• if (info && info->dev)

• drm_fb_helper_unregister_fbi(fb_helper);
  

• if (!(info && info->fbops && info->fbops->fb_destroy))

• drm_fb_helper_simple_fini_cleanup(fb_helper);
  

+} +EXPORT_SYMBOL_GPL(drm_fb_helper_simple_fini);

+/**

• • drm_fb_helper_dev_unplug - unplug an fbdev device
• • @fb_helper: driver-allocated fbdev helper, can be NULL
• • This unplugs the fbdev emulation for a hotpluggable DRM device, which makes
• • fbdev inaccessible to userspace operations. This essentially unregisters
• • fbdev and can be called while there are still open users of @fb_helper.
• • Entry-points from fbdev into drm core/helpers are protected by the fbdev
• • &fb_info ref count and drm_dev_is_unplugged(). This means that the driver
• • also has to call drm_dev_unplug() to complete the unplugging.
• • Drivers must use drm_fb_helper_fb_destroy() as their &fb_ops.fb_destroy
• • callback and call drm_mode_config_cleanup() and free @fb_helper in their
• • &drm_driver->release callback.
• • @fb_helper is finalized by this function unless there are open fbdev fd's
• • in case this is postponed to the closing of the last fd. Finalizing includes
• • dropping the ref taken on &drm_device in drm_fb_helper_simple_init().
• */

+void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper) +{

• drm_fb_helper_unregister_fbi(fb_helper);

I don't see the value of this wrapper. If you want to explain how to best unplug the fbdev emulation I thinkt that's better done in some dedicated DOC section (referenced by drm_dev_unplug() maybe), than by providing a wrapper that only gives you a different name.

_unplug is also a bit misleading, since _unplug is about yanking the backing device. This just unregisters the the fbdev interface.

...

+} +EXPORT_SYMBOL(drm_fb_helper_dev_unplug);

+/**

• • drm_fb_helper_fb_destroy - implementation for &fb_ops.fb_destroy
• • @info: fbdev registered by the helper
• • This function does the same as drm_fb_helper_simple_fini() except
• • unregistering fbdev which is already done.
• • &fb_ops.fb_destroy is called during unregister_framebuffer() or the last
• • fb_release() which ever comes last.
• */

+void drm_fb_helper_fb_destroy(struct fb_info *info) +{

• struct drm_fb_helper *helper = info->par;
• DRM_DEBUG("\n");
• drm_fb_helper_simple_fini_cleanup(helper);

+} +EXPORT_SYMBOL(drm_fb_helper_fb_destroy);

• #define drm_fb_helper_for_each_connector(fbh, i__) \ for (({ lockdep_assert_held(&(fbh)->lock); }), \ i__ = 0; i__ < (fbh)->connector_count; i__++)

@@ -498,7 +650,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) bool do_delayed; int ret;

• if (!drm_fbdev_emulation)

• if (!drm_fbdev_emulation || drm_dev_is_unplugged(fb_helper->dev)) return -ENODEV;

  if (READ_ONCE(fb_helper->deferred_setup))

@@ -563,6 +715,9 @@ static bool drm_fb_helper_force_kernel_mode(void) list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) { struct drm_device *dev = helper->dev;

• if (drm_dev_is_unplugged(dev))
  

• 	continue;
  

• if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) continue;

@@ -735,6 +890,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) struct drm_clip_rect clip_copy; unsigned long flags;

• if (drm_dev_is_unplugged(helper->dev))

• return;
  

• spin_lock_irqsave(&helper->dirty_lock, flags); clip_copy = *clip; clip->x1 = clip->y1 = ~0;

@@ -949,6 +1107,48 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper) } EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);

+/**

• • drm_fb_helper_defio_init - fbdev deferred I/O initialization
• • @fb_helper: driver-allocated fbdev helper
• • This function allocates &fb_deferred_io, sets callback to
• • drm_fb_helper_deferred_io(), delay to 50ms and calls fb_deferred_io_init().
• • NOTE: A copy of &fb_ops is made and assigned to &info->fbops. This is done
• • because fb_deferred_io_cleanup() clears &fbops->fb_mmap and would thereby
• • affect other instances of that &fb_ops. This copy is freed by the helper
• • during cleanup.
• • Returns:
• • 0 on success or a negative error code on failure.
• */

+int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper) +{

• struct fb_info *info = fb_helper->fbdev;
• struct fb_deferred_io *fbdefio;
• struct fb_ops *fbops;
• fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
• fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
• if (!fbdefio || !fbops) {

• kfree(fbdefio);
  

• kfree(fbops);
  

• return -ENOMEM;
  

• }
• info->fbdefio = fbdefio;
• fbdefio->delay = msecs_to_jiffies(50);
• fbdefio->deferred_io = drm_fb_helper_deferred_io;
• *fbops = *info->fbops;
• info->fbops = fbops;
• fb_deferred_io_init(info);
• return 0;

+} +EXPORT_SYMBOL(drm_fb_helper_defio_init);

• static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y, u32 width, u32 height) {

@@ -2591,7 +2791,7 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { int err = 0;

• if (!drm_fbdev_emulation)

• if (!drm_fbdev_emulation || drm_dev_is_unplugged(fb_helper->dev)) return 0;

  mutex_lock(&fb_helper->lock);

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 33fe959..1c5a648 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -242,6 +242,14 @@ struct drm_fb_helper { .fb_ioctl = drm_fb_helper_ioctl

#ifdef CONFIG_DRM_FBDEV_EMULATION +int drm_fb_helper_simple_init(struct drm_device *dev,

• 	      struct drm_fb_helper *fb_helper, int bpp_sel,
  

• 	      int max_conn_count,
  

• 	      const struct drm_fb_helper_funcs *funcs);
  

+void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper); +void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper); +void drm_fb_helper_fb_destroy(struct fb_info *info);

• void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, const struct drm_fb_helper_funcs *funcs); int drm_fb_helper_init(struct drm_device *dev,

@@ -265,6 +273,7 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,

void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);

+int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper); void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagelist);

@@ -311,6 +320,27 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, struct drm_connector *connector); #else +static inline int +drm_fb_helper_simple_init(struct drm_device *dev,

• 	  struct drm_fb_helper *fb_helper, int bpp_sel,
  

• 	  int max_conn_count,
  

• 	  const struct drm_fb_helper_funcs *funcs)
  

+{

• return 0;

+}

+static inline void drm_fb_helper_simple_fini(struct drm_fb_helper *fb_helper) +{ +}

+static inline void drm_fb_helper_dev_unplug(struct drm_fb_helper *fb_helper) +{ +}

+static inline void drm_fb_helper_fb_destroy(struct fb_info *info) +{ +}

• static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, const struct drm_fb_helper_funcs *funcs)

@@ -393,6 +423,11 @@ static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper) { }

+static inline int drm_fb_helper_defio_init(struct drm_fb_helper *fb_helper) +{

• return 0;

+}

• static inline void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagelist) {

-- 2.7.4

On Mon, Aug 28, 2017 at 07:17:45PM +0200, Noralf Trønnes wrote:

Add drm_fbdev_cma_dev_unplug() and use the drm_fb_helper device unplug support. Pin driver module on fb_open().

Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/drm_fb_cma_helper.c | 139 +++++++++++++++++------------------- include/drm/drm_fb_cma_helper.h | 1 + 2 files changed, 68 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index f2ee883..2b044be 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -25,8 +25,6 @@ #include <drm/drm_fb_cma_helper.h> #include <linux/module.h>

-#define DEFAULT_FBDEFIO_DELAY_MS 50

struct drm_fbdev_cma { struct drm_fb_helper fb_helper; const struct drm_framebuffer_funcs *fb_funcs; @@ -238,6 +236,34 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg) EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show); #endif

+static int drm_fbdev_cma_fb_open(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct drm_device *dev = fb_helper->dev;
• /*

• * The fb_ops definition resides in this library, meaning fb_open()
  

• * will take a ref on the library instead of the driver. Make sure the
  

• * driver module is pinned. Skip fbcon (user==0) since it can detach
  

• * itself on unregister_framebuffer().
  

• */
  

• if (user && !try_module_get(dev->driver->fops->owner))

• return -ENODEV;
  

I thought we've fixed this by redoing the fb_ops as a macro? Maybe tinydrm isn't fixed yet, but then we need to fix up tinydrm. This here otoh kinda smells a bit like a hack ...

If we need it, then I'd vote to put it into the fbdev helpers, not the cma specific parts.

• return 0;

+}

+static int drm_fbdev_cma_fb_release(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct drm_device *dev = fb_helper->dev;
• if (user)

• module_put(dev->driver->fops->owner);
  

• return 0;

+}

static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) { return dma_mmap_writecombine(info->device, vma, info->screen_base, @@ -247,10 +273,13 @@ static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) static struct fb_ops drm_fbdev_cma_ops = { .owner = THIS_MODULE, DRM_FB_HELPER_DEFAULT_OPS,

• .fb_open = drm_fbdev_cma_fb_open,
• .fb_release = drm_fbdev_cma_fb_release, .fb_fillrect = drm_fb_helper_sys_fillrect, .fb_copyarea = drm_fb_helper_sys_copyarea, .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_mmap = drm_fb_cma_mmap,
• .fb_destroy = drm_fb_helper_fb_destroy,

};

static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, @@ -262,52 +291,26 @@ static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, return 0; }

-static int drm_fbdev_cma_defio_init(struct fb_info *fbi, +static int drm_fbdev_cma_defio_init(struct drm_fb_helper *helper, struct drm_gem_cma_object *cma_obj) {

• struct fb_deferred_io *fbdefio;
• struct fb_ops *fbops;

• struct fb_info *fbi = helper->fbdev;
• int ret;

• /*

• * Per device structures are needed because:
  

• * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
  

• * fbdefio: individual delays
  

• */
  

• fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
• fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
• if (!fbdefio || !fbops) {

• kfree(fbdefio);
  

• kfree(fbops);
  

• return -ENOMEM;
  

• }

• ret = drm_fb_helper_defio_init(helper);

• if (ret)

• return ret;
  
  

  /* can't be offset from vaddr since dirty() uses cma_obj */ fbi->screen_buffer = cma_obj->vaddr; /* fb_deferred_io_fault() needs a physical address */ fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));

• *fbops = *fbi->fbops;

• fbi->fbops = fbops;

• fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);

• fbdefio->deferred_io = drm_fb_helper_deferred_io;

• fbi->fbdefio = fbdefio;

• fb_deferred_io_init(fbi); fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;

  return 0;

}

-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi) -{

• if (!fbi->fbdefio)

• return;
  

• fb_deferred_io_cleanup(fbi);
• kfree(fbi->fbdefio);
• kfree(fbi->fbops);

-}

Above changes look like unrelated cleanup? Should be a separate patch I think.

static int drm_fbdev_cma_create(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes) @@ -365,7 +368,7 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper, fbi->fix.smem_len = size;

if (fbdev_cma->fb_funcs->dirty) {

• ret = drm_fbdev_cma_defio_init(fbi, obj);
  

• ret = drm_fbdev_cma_defio_init(helper, obj);
  

  if (ret) goto err_cma_destroy; }

@@ -399,7 +402,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, const struct drm_framebuffer_funcs *funcs) { struct drm_fbdev_cma *fbdev_cma;

• struct drm_fb_helper *helper; int ret;

  fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);

@@ -409,37 +411,15 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, } fbdev_cma->fb_funcs = funcs;

• helper = &fbdev_cma->fb_helper;
• drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
• ret = drm_fb_helper_init(dev, helper, max_conn_count);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to initialize drm fb helper.\n");
  

• goto err_free;
  

• }
• ret = drm_fb_helper_single_add_all_connectors(helper);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to add connectors.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• ret = drm_fb_helper_initial_config(helper, preferred_bpp);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to set initial hw configuration.\n");
  

• goto err_drm_fb_helper_fini;
  

• ret = drm_fb_helper_simple_init(dev, &fbdev_cma->fb_helper,

• 			preferred_bpp, max_conn_count,
  

• 			&drm_fb_cma_helper_funcs);
  

Yeah the conversion to simple* should be split out I think. But then I thought more of this will be unified with the overall fbdev helpers?

• if (ret) {

• kfree(fbdev_cma);
  

• return ERR_PTR(ret);
  

  }

  return fbdev_cma;

-err_drm_fb_helper_fini:

• drm_fb_helper_fini(helper);

-err_free:

• kfree(fbdev_cma);
• return ERR_PTR(ret);

} EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);

@@ -468,17 +448,15 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init); /**

• drm_fbdev_cma_fini() - Free drm_fbdev_cma struct
• @fbdev_cma: The drm_fbdev_cma struct

• • This function calls drm_fb_helper_simple_fini() and frees @fbdev_cma.
• • Don't use this function together with drm_fbdev_cma_dev_unplug().
  */

void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) {

• drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);
• if (fbdev_cma->fb_helper.fbdev)

• drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
  

• if (fbdev_cma->fb_helper.fb)

• drm_framebuffer_remove(fbdev_cma->fb_helper.fb);
  

• drm_fb_helper_fini(&fbdev_cma->fb_helper);

• if (fbdev_cma)

• drm_fb_helper_simple_fini(&fbdev_cma->fb_helper);
  

  kfree(fbdev_cma);

} EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); @@ -542,3 +520,20 @@ void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma, state); } EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);

+/**

• • drm_fbdev_cma_dev_unplug - wrapper around drm_fb_helper_dev_unplug
• • @fbdev_cma: The drm_fbdev_cma struct, may be NULL
• • This unplugs the fbdev emulation for a hotpluggable DRM device. See
• • drm_fb_helper_dev_unplug() for details.
• • Drivers must call drm_mode_config_cleanup() and free @fbdev_cma in their
• • &drm_driver->release callback.
• */

+void drm_fbdev_cma_dev_unplug(struct drm_fbdev_cma *fbdev_cma) +{

• if (fbdev_cma)

• drm_fb_helper_dev_unplug(&fbdev_cma->fb_helper);
  

+}

I thought we're trying to phase out the cma helpers, why do we need a wrapper for a wrapper?

+EXPORT_SYMBOL(drm_fbdev_cma_dev_unplug); diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h index a323781..a6aafae 100644 --- a/include/drm/drm_fb_cma_helper.h +++ b/include/drm/drm_fb_cma_helper.h @@ -27,6 +27,7 @@ void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma); void drm_fbdev_cma_set_suspend(struct drm_fbdev_cma *fbdev_cma, bool state); void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma, bool state); +void drm_fbdev_cma_dev_unplug(struct drm_fbdev_cma *fbdev_cma);

void drm_fb_cma_destroy(struct drm_framebuffer *fb); int drm_fb_cma_create_handle(struct drm_framebuffer *fb, -- 2.7.4

On Tue, Aug 29, 2017 at 07:23:14PM +0200, Noralf Trønnes wrote:

Den 28.08.2017 23.46, skrev Daniel Vetter:

...

On Mon, Aug 28, 2017 at 07:17:45PM +0200, Noralf Trønnes wrote:

...

Add drm_fbdev_cma_dev_unplug() and use the drm_fb_helper device unplug support. Pin driver module on fb_open().

Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/drm_fb_cma_helper.c | 139 +++++++++++++++++------------------- include/drm/drm_fb_cma_helper.h | 1 + 2 files changed, 68 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index f2ee883..2b044be 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -25,8 +25,6 @@ #include <drm/drm_fb_cma_helper.h> #include <linux/module.h> -#define DEFAULT_FBDEFIO_DELAY_MS 50

• struct drm_fbdev_cma { struct drm_fb_helper fb_helper; const struct drm_framebuffer_funcs *fb_funcs;

@@ -238,6 +236,34 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg) EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show); #endif +static int drm_fbdev_cma_fb_open(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct drm_device *dev = fb_helper->dev;
• /*

• * The fb_ops definition resides in this library, meaning fb_open()
  

• * will take a ref on the library instead of the driver. Make sure the
  

• * driver module is pinned. Skip fbcon (user==0) since it can detach
  

• * itself on unregister_framebuffer().
  

• */
  

• if (user && !try_module_get(dev->driver->fops->owner))

• return -ENODEV;
  

I thought we've fixed this by redoing the fb_ops as a macro? Maybe tinydrm isn't fixed yet, but then we need to fix up tinydrm. This here otoh kinda smells a bit like a hack ...

If we need it, then I'd vote to put it into the fbdev helpers, not the cma specific parts.

This is only necessary in this library atm because it contains this:

static struct fb_ops drm_fbdev_cma_ops = {     .owner        = THIS_MODULE,

Other drivers have this definition in their own module, so fb_open() takes a ref on the right module.

My shmem gem library will also wrap struct fb_ops and needs to do this. I can add them to drm_fb_helper with a note that they should only be used by libraries.

Yes it's kind of a hack, but I find the cma library is very nice in this respect, that it makes fbdev emulation very easy for drivers. It wraps up basic fbdev in 3 function calls.

If we were to do this strictly by the book, we would have to add something like this to every cma driver:

DEFINE_DRM_FBDEV_CMA_FB_OPS(driver_fb_ops);

static int driver_fb_probe(struct drm_fb_helper *helper,             struct drm_fb_helper_surface_size *sizes) {     return drm_fbdev_cma_create(helper, sizes, &driver_fb_ops); }

static const struct drm_fb_helper_funcs driver_helper_funcs = {     .fb_probe = driver_fb_probe, };

static int driver_fbdev_init() { -    fbdev_cma = drm_fbdev_cma_init(dev, preferred_bpp, max_conn_count); +    fbdev_cma = drm_fbdev_cma_init(dev, preferred_bpp, max_conn_count, +                    &driver_helper_funcs); }

Well, maybe not so bad after all :-) Maybe I should do it like this in the shmem gem library.

Hm, this does add considerable amounts of boilerplate everywhere, so not sure it's a good idea.

I can't do refactoring like this, this time around. I need to do that shmem gem library before I run out of steam. A refactoring like this will have to wait to later.

I think your fb_open/close hooks are just fine, but instead of putting them into cma helpers only let's just put them into the core helpers (and add them to DRM_FB_HELPER_DEFAULT_OPS). Them being a special case for cma only was what really irked me, not so much what they do. And doing the get/put module by default shouldn't really hurt (for drivers where the fb_ops and the drm_driver ops match it'll mean double refcounting, but that doesn't matter really).

...

...

• return 0;

+}

+static int drm_fbdev_cma_fb_release(struct fb_info *info, int user) +{

• struct drm_fb_helper *fb_helper = info->par;
• struct drm_device *dev = fb_helper->dev;
• if (user)

• module_put(dev->driver->fops->owner);
  

• return 0;

+}

• static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) { return dma_mmap_writecombine(info->device, vma, info->screen_base,

@@ -247,10 +273,13 @@ static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma) static struct fb_ops drm_fbdev_cma_ops = { .owner = THIS_MODULE, DRM_FB_HELPER_DEFAULT_OPS,

• .fb_open = drm_fbdev_cma_fb_open,
• .fb_release = drm_fbdev_cma_fb_release, .fb_fillrect = drm_fb_helper_sys_fillrect, .fb_copyarea = drm_fb_helper_sys_copyarea, .fb_imageblit = drm_fb_helper_sys_imageblit, .fb_mmap = drm_fb_cma_mmap,
• .fb_destroy = drm_fb_helper_fb_destroy, }; static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,

@@ -262,52 +291,26 @@ static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, return 0; } -static int drm_fbdev_cma_defio_init(struct fb_info *fbi, +static int drm_fbdev_cma_defio_init(struct drm_fb_helper *helper, struct drm_gem_cma_object *cma_obj) {

• struct fb_deferred_io *fbdefio;
• struct fb_ops *fbops;

• struct fb_info *fbi = helper->fbdev;
• int ret;

• /*

• * Per device structures are needed because:
  

• * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
  

• * fbdefio: individual delays
  

• */
  

• fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
• fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
• if (!fbdefio || !fbops) {

• kfree(fbdefio);
  

• kfree(fbops);
  

• return -ENOMEM;
  

• }

• ret = drm_fb_helper_defio_init(helper);
• if (ret)

• return ret;
  

  /* can't be offset from vaddr since dirty() uses cma_obj */ fbi->screen_buffer = cma_obj->vaddr; /* fb_deferred_io_fault() needs a physical address */ fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));

• *fbops = *fbi->fbops;
• fbi->fbops = fbops;
• fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
• fbdefio->deferred_io = drm_fb_helper_deferred_io;
• fbi->fbdefio = fbdefio;
• fb_deferred_io_init(fbi); fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; return 0; }

-static void drm_fbdev_cma_defio_fini(struct fb_info *fbi) -{

• if (!fbi->fbdefio)

• return;
  

• fb_deferred_io_cleanup(fbi);
• kfree(fbi->fbdefio);
• kfree(fbi->fbops);

-}

Above changes look like unrelated cleanup? Should be a separate patch I think.

...

static int drm_fbdev_cma_create(struct drm_fb_helper *helper, struct drm_fb_helper_surface_size *sizes) @@ -365,7 +368,7 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper, fbi->fix.smem_len = size; if (fbdev_cma->fb_funcs->dirty) {

• ret = drm_fbdev_cma_defio_init(fbi, obj);
  

• ret = drm_fbdev_cma_defio_init(helper, obj);
  

  if (ret) goto err_cma_destroy; }

@@ -399,7 +402,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, const struct drm_framebuffer_funcs *funcs) { struct drm_fbdev_cma *fbdev_cma;

• struct drm_fb_helper *helper; int ret; fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);

@@ -409,37 +411,15 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev, } fbdev_cma->fb_funcs = funcs;

• helper = &fbdev_cma->fb_helper;
• drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
• ret = drm_fb_helper_init(dev, helper, max_conn_count);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to initialize drm fb helper.\n");
  

• goto err_free;
  

• }
• ret = drm_fb_helper_single_add_all_connectors(helper);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to add connectors.\n");
  

• goto err_drm_fb_helper_fini;
  

• }
• ret = drm_fb_helper_initial_config(helper, preferred_bpp);
• if (ret < 0) {

• dev_err(dev->dev, "Failed to set initial hw configuration.\n");
  

• goto err_drm_fb_helper_fini;
  

• ret = drm_fb_helper_simple_init(dev, &fbdev_cma->fb_helper,

• 			preferred_bpp, max_conn_count,
  

• 			&drm_fb_cma_helper_funcs);
  

Yeah the conversion to simple* should be split out I think. But then I thought more of this will be unified with the overall fbdev helpers?

I agree, if we find a way to support unplug within the existing helpers.

...

...

• if (ret) {

• kfree(fbdev_cma);
  

• return ERR_PTR(ret);
  

  } return fbdev_cma;

-err_drm_fb_helper_fini:

• drm_fb_helper_fini(helper);

-err_free:

• kfree(fbdev_cma);
• return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);

@@ -468,17 +448,15 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init); /**

• drm_fbdev_cma_fini() - Free drm_fbdev_cma struct
• @fbdev_cma: The drm_fbdev_cma struct

• • This function calls drm_fb_helper_simple_fini() and frees @fbdev_cma.
• • Don't use this function together with drm_fbdev_cma_dev_unplug().
  */ void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma) {

• drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);
• if (fbdev_cma->fb_helper.fbdev)

• drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
  

• if (fbdev_cma->fb_helper.fb)

• drm_framebuffer_remove(fbdev_cma->fb_helper.fb);
  

• drm_fb_helper_fini(&fbdev_cma->fb_helper);

• if (fbdev_cma)

• drm_fb_helper_simple_fini(&fbdev_cma->fb_helper);
  

  kfree(fbdev_cma); } EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);

@@ -542,3 +520,20 @@ void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma, state); } EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);

+/**

• • drm_fbdev_cma_dev_unplug - wrapper around drm_fb_helper_dev_unplug
• • @fbdev_cma: The drm_fbdev_cma struct, may be NULL
• • This unplugs the fbdev emulation for a hotpluggable DRM device. See
• • drm_fb_helper_dev_unplug() for details.
• • Drivers must call drm_mode_config_cleanup() and free @fbdev_cma in their
• • &drm_driver->release callback.
• */

+void drm_fbdev_cma_dev_unplug(struct drm_fbdev_cma *fbdev_cma) +{

• if (fbdev_cma)

• drm_fb_helper_dev_unplug(&fbdev_cma->fb_helper);
  

+}

I thought we're trying to phase out the cma helpers, why do we need a wrapper for a wrapper?

Because struct drm_fbdev_cma is private to this file. When tinydrm switches to shmem, we can remove the dirtyfb stuff from this helper and we will end up with this:

struct drm_fbdev_cma {     struct drm_fb_helper    fb_helper; };

Now it is possible to refactor and switch to using drm_fb_helper directly and drop the fbdev wrappers.

We can ofc avoid adding a wrapper by making struct drm_fbdev_cma public.

Hm, no I understand. I'd vote to make struct drm_fbdev_cma public. We generally don't try to hide stuff in structs only in .c files, makes it easier to up/downcast and have static inline helpers.

Moving the struct should be a separate patch I think. -Daniel

Noralf.

...

...

+EXPORT_SYMBOL(drm_fbdev_cma_dev_unplug); diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h index a323781..a6aafae 100644 --- a/include/drm/drm_fb_cma_helper.h +++ b/include/drm/drm_fb_cma_helper.h @@ -27,6 +27,7 @@ void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma); void drm_fbdev_cma_set_suspend(struct drm_fbdev_cma *fbdev_cma, bool state); void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma, bool state); +void drm_fbdev_cma_dev_unplug(struct drm_fbdev_cma *fbdev_cma); void drm_fb_cma_destroy(struct drm_framebuffer *fb); int drm_fb_cma_create_handle(struct drm_framebuffer *fb, -- 2.7.4

On Mon, Aug 28, 2017 at 07:17:46PM +0200, Noralf Trønnes wrote:

Might as well embed drm_device since tinydrm_device (embeds pipe struct and fbdev pointer) needs to stick around after driver-device unbind to handle open fd's after device removal.

Cc: David Lechner david@lechnology.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

I think you should be able to merge this right away, and it's definitely a nice cleanup.

Acked-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 44 ++++++++++++----------------- drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 2 +- drivers/gpu/drm/tinydrm/mi0283qt.c | 8 +++--- drivers/gpu/drm/tinydrm/mipi-dbi.c | 12 ++++---- drivers/gpu/drm/tinydrm/repaper.c | 10 +++---- drivers/gpu/drm/tinydrm/st7586.c | 16 +++++------ include/drm/tinydrm/tinydrm.h | 9 +++++- 7 files changed, 50 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index 551709e..f11f4cd 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c @@ -44,7 +44,7 @@ */ void tinydrm_lastclose(struct drm_device *drm) {

• struct tinydrm_device *tdev = drm->dev_private;

• struct tinydrm_device *tdev = drm_to_tinydrm(drm);

  DRM_DEBUG_KMS("\n"); drm_fbdev_cma_restore_mode(tdev->fbdev_cma);

@@ -126,7 +126,7 @@ static struct drm_framebuffer * tinydrm_fb_create(struct drm_device *drm, struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd) {

• struct tinydrm_device *tdev = drm->dev_private;

• struct tinydrm_device *tdev = drm_to_tinydrm(drm);

  return drm_fb_cma_create_with_funcs(drm, file_priv, mode_cmd, tdev->fb_funcs);

@@ -142,23 +142,16 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver) {

• struct drm_device *drm;

• struct drm_device *drm = &tdev->drm;

• int ret;

  mutex_init(&tdev->dirty_lock); tdev->fb_funcs = fb_funcs;

• /*

• * We don't embed drm_device, because that prevent us from using
  

• * devm_kzalloc() to allocate tinydrm_device in the driver since
  

• * drm_dev_unref() frees the structure. The devm_ functions provide
  

• * for easy error handling.
  

• */
  

• drm = drm_dev_alloc(driver, parent);
• if (IS_ERR(drm))

• return PTR_ERR(drm);
  

• tdev->drm = drm;
• drm->dev_private = tdev;

• ret = drm_dev_init(drm, driver, parent);
• if (ret)

• return ret;
  

• drm_mode_config_init(drm); drm->mode_config.funcs = &tinydrm_mode_config_funcs;

@@ -167,10 +160,9 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,

static void tinydrm_fini(struct tinydrm_device *tdev) {

• drm_mode_config_cleanup(tdev->drm);

• drm_mode_config_cleanup(&tdev->drm); mutex_destroy(&tdev->dirty_lock);

• tdev->drm->dev_private = NULL;
• drm_dev_unref(tdev->drm);

• drm_dev_unref(&tdev->drm);

}

static void devm_tinydrm_release(void *data) @@ -212,12 +204,12 @@ EXPORT_SYMBOL(devm_tinydrm_init);

static int tinydrm_register(struct tinydrm_device *tdev) {

• struct drm_device *drm = tdev->drm;

• struct drm_device *drm = &tdev->drm; int bpp = drm->mode_config.preferred_depth; struct drm_fbdev_cma *fbdev; int ret;

• ret = drm_dev_register(tdev->drm, 0);

• ret = drm_dev_register(drm, 0); if (ret) return ret;

@@ -236,10 +228,10 @@ static void tinydrm_unregister(struct tinydrm_device *tdev) { struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;

• drm_atomic_helper_shutdown(tdev->drm);

• drm_atomic_helper_shutdown(&tdev->drm); /* don't restore fbdev in lastclose, keep pipeline disabled */ tdev->fbdev_cma = NULL;

• drm_dev_unregister(tdev->drm);

• drm_dev_unregister(&tdev->drm); if (fbdev_cma) drm_fbdev_cma_fini(fbdev_cma);

} @@ -262,7 +254,7 @@ static void devm_tinydrm_register_release(void *data) */ int devm_tinydrm_register(struct tinydrm_device *tdev) {

• struct device *dev = tdev->drm->dev;

• struct device *dev = tdev->drm.dev; int ret;

  ret = tinydrm_register(tdev);

@@ -287,7 +279,7 @@ EXPORT_SYMBOL(devm_tinydrm_register); */ void tinydrm_shutdown(struct tinydrm_device *tdev) {

• drm_atomic_helper_shutdown(tdev->drm);

• drm_atomic_helper_shutdown(&tdev->drm);

} EXPORT_SYMBOL(tinydrm_shutdown);

@@ -312,7 +304,7 @@ int tinydrm_suspend(struct tinydrm_device *tdev) }

drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);

• state = drm_atomic_helper_suspend(tdev->drm);

• state = drm_atomic_helper_suspend(&tdev->drm); if (IS_ERR(state)) { drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0); return PTR_ERR(state);

@@ -346,7 +338,7 @@ int tinydrm_resume(struct tinydrm_device *tdev)

tdev->suspend_state = NULL;

• ret = drm_atomic_helper_resume(tdev->drm, state);

• ret = drm_atomic_helper_resume(&tdev->drm, state); if (ret) { DRM_ERROR("Error resuming state: %d\n", ret); return ret;

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index 177e9d8..1bcb43a 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -198,7 +198,7 @@ tinydrm_display_pipe_init(struct tinydrm_device *tdev, const struct drm_display_mode *mode, unsigned int rotation) {

• struct drm_device *drm = tdev->drm;

• struct drm_device *drm = &tdev->drm; struct drm_display_mode *mode_copy; struct drm_connector *connector; int ret;

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..77d40c9 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -23,7 +23,7 @@ static int mi0283qt_init(struct mipi_dbi *mipi) { struct tinydrm_device *tdev = &mipi->tinydrm;

• struct device *dev = tdev->drm->dev;

• struct device *dev = tdev->drm.dev; u8 addr_mode; int ret;

@@ -169,7 +169,7 @@ static int mi0283qt_probe(struct spi_device *spi) u32 rotation = 0; int ret;

• mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);

• mipi = kzalloc(sizeof(*mipi), GFP_KERNEL); if (!mipi) return -ENOMEM;

@@ -224,9 +224,9 @@ static int mi0283qt_probe(struct spi_device *spi) spi_set_drvdata(spi, mipi);

DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",

• 	 tdev->drm->driver->name, dev_name(dev),
  

• 	 tdev->drm.driver->name, dev_name(dev),
   spi->max_speed_hz / 1000000,
  

• 	 tdev->drm->primary->index);
  

• 	 tdev->drm.primary->index);
  
  

  return 0;

} diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 2caeabc..c22e352 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -199,7 +199,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, unsigned int num_clips) { struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);

• struct tinydrm_device *tdev = fb->dev->dev_private;

• struct tinydrm_device *tdev = drm_to_tinydrm(fb->dev); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); bool swap = mipi->swap_bytes; struct drm_clip_rect clip;

@@ -285,7 +285,7 @@ EXPORT_SYMBOL(mipi_dbi_pipe_enable);

static void mipi_dbi_blank(struct mipi_dbi *mipi) {

• struct drm_device *drm = mipi->tinydrm.drm;

• struct drm_device *drm = &mipi->tinydrm.drm; u16 height = drm->mode_config.min_height; u16 width = drm->mode_config.min_width; size_t len = width * height * 2;

@@ -380,13 +380,13 @@ int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi, if (ret) return ret;

• tdev->drm->mode_config.preferred_depth = 16;

• tdev->drm.mode_config.preferred_depth = 16; mipi->rotation = rotation;

• drm_mode_config_reset(tdev->drm);

• drm_mode_config_reset(&tdev->drm);

  DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",

•       tdev->drm->mode_config.preferred_depth, rotation);
  

•       tdev->drm.mode_config.preferred_depth, rotation);
  
  

  return 0;

} @@ -975,7 +975,7 @@ static const struct drm_info_list mipi_dbi_debugfs_list[] = { */ int mipi_dbi_debugfs_init(struct drm_minor *minor) {

• struct tinydrm_device *tdev = minor->dev->dev_private;

• struct tinydrm_device *tdev = drm_to_tinydrm(minor->dev); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); umode_t mode = S_IFREG | S_IWUSR;

diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c index 30dc97b..b8fc8eb 100644 --- a/drivers/gpu/drm/tinydrm/repaper.c +++ b/drivers/gpu/drm/tinydrm/repaper.c @@ -528,7 +528,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, { struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0); struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;

• struct tinydrm_device *tdev = fb->dev->dev_private;

• struct tinydrm_device *tdev = drm_to_tinydrm(fb->dev); struct repaper_epd *epd = epd_from_tinydrm(tdev); struct drm_clip_rect clip; u8 *buf = NULL;

@@ -949,7 +949,7 @@ static int repaper_probe(struct spi_device *spi) } }

• epd = devm_kzalloc(dev, sizeof(*epd), GFP_KERNEL);

• epd = kzalloc(sizeof(*epd), GFP_KERNEL); if (!epd) return -ENOMEM;

@@ -1077,7 +1077,7 @@ static int repaper_probe(struct spi_device *spi) if (ret) return ret;

• drm_mode_config_reset(tdev->drm);

• drm_mode_config_reset(&tdev->drm);

  ret = devm_tinydrm_register(tdev); if (ret)

@@ -1086,9 +1086,9 @@ static int repaper_probe(struct spi_device *spi) spi_set_drvdata(spi, tdev);

DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",

• 	 tdev->drm->driver->name, dev_name(dev),
  

• 	 tdev->drm.driver->name, dev_name(dev),
   spi->max_speed_hz / 1000000,
  

• 	 tdev->drm->primary->index);
  

• 	 tdev->drm.primary->index);
  
  

  return 0;

} diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index b439956..bc2b905 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c @@ -112,7 +112,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb, unsigned int color, struct drm_clip_rect *clips, unsigned int num_clips) {

• struct tinydrm_device *tdev = fb->dev->dev_private;

• struct tinydrm_device *tdev = drm_to_tinydrm(fb->dev); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct drm_clip_rect clip; int start, end;

@@ -178,7 +178,7 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, struct tinydrm_device *tdev = pipe_to_tinydrm(pipe); struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev); struct drm_framebuffer *fb = pipe->plane.fb;

• struct device *dev = tdev->drm->dev;

• struct device *dev = tdev->drm.dev; int ret; u8 addr_mode;

@@ -290,13 +290,13 @@ static int st7586_init(struct device *dev, struct mipi_dbi *mipi, if (ret) return ret;

• tdev->drm->mode_config.preferred_depth = 32;

• tdev->drm.mode_config.preferred_depth = 32; mipi->rotation = rotation;

• drm_mode_config_reset(tdev->drm);

• drm_mode_config_reset(&tdev->drm);

  DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",

•       tdev->drm->mode_config.preferred_depth, rotation);
  

•       tdev->drm.mode_config.preferred_depth, rotation);
  
  

  return 0;

} @@ -349,7 +349,7 @@ static int st7586_probe(struct spi_device *spi) u32 rotation = 0; int ret;

• mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);

• mipi = kzalloc(sizeof(*mipi), GFP_KERNEL); if (!mipi) return -ENOMEM;

@@ -397,9 +397,9 @@ static int st7586_probe(struct spi_device *spi) spi_set_drvdata(spi, mipi);

DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",

• 	 tdev->drm->driver->name, dev_name(dev),
  

• 	 tdev->drm.driver->name, dev_name(dev),
   spi->max_speed_hz / 1000000,
  

• 	 tdev->drm->primary->index);
  

• 	 tdev->drm.primary->index);
  
  

  return 0;

} diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h index 4774fe3..ded9817 100644 --- a/include/drm/tinydrm/tinydrm.h +++ b/include/drm/tinydrm/tinydrm.h @@ -10,6 +10,7 @@ #ifndef __LINUX_TINYDRM_H #define __LINUX_TINYDRM_H

+#include <drm/drmP.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -24,7 +25,7 @@

• @fb_funcs: Framebuffer functions used when creating framebuffers

*/ struct tinydrm_device {

• struct drm_device *drm;

• struct drm_device drm; struct drm_simple_display_pipe pipe; struct mutex dirty_lock; struct drm_fbdev_cma *fbdev_cma;

@@ -33,6 +34,12 @@ struct tinydrm_device { };

static inline struct tinydrm_device * +drm_to_tinydrm(struct drm_device *drm) +{

• return container_of(drm, struct tinydrm_device, drm);

+}

+static inline struct tinydrm_device * pipe_to_tinydrm(struct drm_simple_display_pipe *pipe) { return container_of(pipe, struct tinydrm_device, pipe); -- 2.7.4

Hi Noralf,

Thank you for the patch.

On Monday, 28 August 2017 20:17:46 EEST Noralf Trønnes wrote:

Might as well embed drm_device since tinydrm_device (embeds pipe struct and fbdev pointer) needs to stick around after driver-device unbind to handle open fd's after device removal.

Cc: David Lechner david@lechnology.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 44 ++++++++++++------------- drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 2 +- drivers/gpu/drm/tinydrm/mi0283qt.c | 8 +++--- drivers/gpu/drm/tinydrm/mipi-dbi.c | 12 ++++---- drivers/gpu/drm/tinydrm/repaper.c | 10 +++---- drivers/gpu/drm/tinydrm/st7586.c | 16 +++++------ include/drm/tinydrm/tinydrm.h | 9 +++++- 7 files changed, 50 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index 551709e..f11f4cd 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c

[snip]

@@ -142,23 +142,16 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver) {

• struct drm_device *drm;

• struct drm_device *drm = &tdev->drm;

• int ret;

  mutex_init(&tdev->dirty_lock); tdev->fb_funcs = fb_funcs;

• /*

• * We don't embed drm_device, because that prevent us from using
  

• * devm_kzalloc() to allocate tinydrm_device in the driver since
  

• * drm_dev_unref() frees the structure. The devm_ functions provide
  

• * for easy error handling.
  

Don't you then need a custom drm_driver.release handler to free the parent structure ?

• */
  

• drm = drm_dev_alloc(driver, parent);
• if (IS_ERR(drm))

• return PTR_ERR(drm);
  

• tdev->drm = drm;
• drm->dev_private = tdev;

• ret = drm_dev_init(drm, driver, parent);
• if (ret)

• return ret;
  

• drm_mode_config_init(drm); drm->mode_config.funcs = &tinydrm_mode_config_funcs;

[snip]

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..77d40c9 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c

[snip]

@@ -169,7 +169,7 @@ static int mi0283qt_probe(struct spi_device *spi) u32 rotation = 0; int ret;

• mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);

• mipi = kzalloc(sizeof(*mipi), GFP_KERNEL);

Where's the related kfree() ?

if (!mipi) return -ENOMEM;

[snip]

diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c index 30dc97b..b8fc8eb 100644 --- a/drivers/gpu/drm/tinydrm/repaper.c +++ b/drivers/gpu/drm/tinydrm/repaper.c

[snip]

@@ -949,7 +949,7 @@ static int repaper_probe(struct spi_device *spi) } }

• epd = devm_kzalloc(dev, sizeof(*epd), GFP_KERNEL);

• epd = kzalloc(sizeof(*epd), GFP_KERNEL);

Ditto.

if (!epd) return -ENOMEM;

[snip]

diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index b439956..bc2b905 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c

[snip]

@@ -349,7 +349,7 @@ static int st7586_probe(struct spi_device *spi) u32 rotation = 0; int ret;

• mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);

• mipi = kzalloc(sizeof(*mipi), GFP_KERNEL);

Ang here again.

if (!mipi) return -ENOMEM;

[snip]

Hi Noralf,

On Thursday, 31 August 2017 20:16:42 EEST Noralf Trønnes wrote:

Den 31.08.2017 12.18, skrev Laurent Pinchart:

...

On Monday, 28 August 2017 20:17:46 EEST Noralf Trønnes wrote:

...

Might as well embed drm_device since tinydrm_device (embeds pipe struct and fbdev pointer) needs to stick around after driver-device unbind to handle open fd's after device removal.

Cc: David Lechner david@lechnology.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 44 +++++++++++----------- drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 2 +- drivers/gpu/drm/tinydrm/mi0283qt.c | 8 +++--- drivers/gpu/drm/tinydrm/mipi-dbi.c | 12 ++++---- drivers/gpu/drm/tinydrm/repaper.c | 10 +++---- drivers/gpu/drm/tinydrm/st7586.c | 16 +++++------ include/drm/tinydrm/tinydrm.h | 9 +++++- 7 files changed, 50 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index 551709e..f11f4cd 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c

[snip]

...

@@ -142,23 +142,16 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, const struct drm_framebuffer_funcs *fb_funcs,

	struct drm_driver *driver)

{

• struct drm_device *drm;

• struct drm_device *drm = &tdev->drm;

• int ret;

  mutex_init(&tdev->dirty_lock); tdev->fb_funcs = fb_funcs;

• /*

• * We don't embed drm_device, because that prevent us from using
  

• * devm_kzalloc() to allocate tinydrm_device in the driver since
  

• * drm_dev_unref() frees the structure. The devm_ functions provide
  

• * for easy error handling.
  

Don't you then need a custom drm_driver.release handler to free the parent structure ?

I rely on the fact that drm_device is the first member in the driver structure and thus it will be freed in drm_dev_release(). A later patch adds a drm_driver.release function though.

That's a bit hackish. As a later patch changes this I'd be OK with this one, but you should mention that you rely on the structure layout in the commit message.

...

...

• */
  

• drm = drm_dev_alloc(driver, parent);
• if (IS_ERR(drm))

• return PTR_ERR(drm);
  

• tdev->drm = drm;
• drm->dev_private = tdev;

• ret = drm_dev_init(drm, driver, parent);

• if (ret)

• return ret;
  

• drm_mode_config_init(drm); drm->mode_config.funcs = &tinydrm_mode_config_funcs;

[snip]

...

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..77d40c9 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c

[snip]

...

@@ -169,7 +169,7 @@ static int mi0283qt_probe(struct spi_device *spi)

u32 rotation = 0; int ret;

• mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);

• mipi = kzalloc(sizeof(*mipi), GFP_KERNEL);

Where's the related kfree() ?

...

if (!mipi)

return -ENOMEM;

[snip]

...

diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c index 30dc97b..b8fc8eb 100644 --- a/drivers/gpu/drm/tinydrm/repaper.c +++ b/drivers/gpu/drm/tinydrm/repaper.c

[snip]

...

@@ -949,7 +949,7 @@ static int repaper_probe(struct spi_device *spi)

}

}

• epd = devm_kzalloc(dev, sizeof(*epd), GFP_KERNEL);

• epd = kzalloc(sizeof(*epd), GFP_KERNEL);

Ditto.

...

if (!epd)

return -ENOMEM;

[snip]

...

diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index b439956..bc2b905 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c

[snip]

...

@@ -349,7 +349,7 @@ static int st7586_probe(struct spi_device *spi)

u32 rotation = 0; int ret;

• mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);

• mipi = kzalloc(sizeof(*mipi), GFP_KERNEL);

Ang here again.

...

if (!mipi)

return -ENOMEM;

[snip]

On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:

Support device unplugging to make tinydrm suitable for USB devices.

Cc: David Lechner david@lechnology.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 69 ++++++++++++++++++++++++----- drivers/gpu/drm/tinydrm/mi0283qt.c | 4 ++ drivers/gpu/drm/tinydrm/mipi-dbi.c | 5 ++- drivers/gpu/drm/tinydrm/repaper.c | 9 +++- drivers/gpu/drm/tinydrm/st7586.c | 9 +++- include/drm/tinydrm/tinydrm.h | 5 +++ 6 files changed, 87 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index f11f4cd..3ccbcc5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c @@ -32,6 +32,29 @@

• The driver allocates &tinydrm_device, initializes it using
• devm_tinydrm_init(), sets up the pipeline using tinydrm_display_pipe_init()
• and registers the DRM device using devm_tinydrm_register().

• • Device unplug

• • ---

• • tinydrm supports device unplugging when there's still open DRM or fbdev file
• • handles.
• • There are 2 ways for driver-device unbinding to happen:
• • • The driver module is unloaded causing the driver to be unregistered.
• • This can't happen as long as there's open file handles because a reference
• • is taken on the module.

Aside: you can do that, but then it works like a hw unplug, through the unbind property in sysfs.

• • • The device is removed (USB, Device Tree overlay).
• • This can happen at any time.
• • The driver needs to protect device resources from access after the device is
• • gone. This is done checking drm_dev_is_unplugged(), typically in
• • &drm_framebuffer_funcs.dirty, &drm_simple_display_pipe_funcs.enable and
• • .disable. Resources that doesn't face userspace and is only used with the
• • device can be setup using devm_ functions, but &tinydrm_device must be
• • allocated using plain kzalloc() since it's lifetime can exceed that of the
• • device. tinydrm_release() will free the structure.

So here's a bit a dragon: There's no prevention of is_unplugged racing against a drm_dev_unplug(). There's been various attempts to fixing this, but they're all somewhat ugly.

Either way, that's a bug in the drm core :-)

*/

/** @@ -138,6 +161,29 @@ static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit, };

+/**

• • tinydrm_release - DRM driver release helper
• • @drm: DRM device
• • This function cleans up and finalizes &drm_device and frees &tinydrm_device.
• • Drivers must use this as their &drm_driver->release callback.
• */

+void tinydrm_release(struct drm_device *drm) +{

• struct tinydrm_device *tdev = drm_to_tinydrm(drm);
• DRM_DEBUG_DRIVER("\n");
• drm_mode_config_cleanup(drm);
• drm_dev_fini(drm);
• mutex_destroy(&tdev->dirty_lock);
• kfree(tdev->fbdev_cma);
• kfree(tdev);

+} +EXPORT_SYMBOL(tinydrm_release);

static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver) @@ -160,8 +206,6 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,

static void tinydrm_fini(struct tinydrm_device *tdev) {

• drm_mode_config_cleanup(&tdev->drm);
• mutex_destroy(&tdev->dirty_lock); drm_dev_unref(&tdev->drm);

}

@@ -178,8 +222,8 @@ static void devm_tinydrm_release(void *data)

• @driver: DRM driver
• This function initializes @tdev, the underlying DRM device and it's

• • mode_config. Resources will be automatically freed on driver detach (devres)
• • using drm_mode_config_cleanup() and drm_dev_unref().

• • mode_config. drm_dev_unref() is called on driver detach (devres) and when
• • all refs are dropped, tinydrm_release() is called.
  • Returns:
  • Zero on success, negative error code on failure.

@@ -226,14 +270,17 @@ static int tinydrm_register(struct tinydrm_device *tdev)

static void tinydrm_unregister(struct tinydrm_device *tdev) {

• struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
• drm_atomic_helper_shutdown(&tdev->drm);
• /* don't restore fbdev in lastclose, keep pipeline disabled */
• tdev->fbdev_cma = NULL;
• drm_dev_unregister(&tdev->drm);
• if (fbdev_cma)

• drm_fbdev_cma_fini(fbdev_cma);
  

• /* Get a ref that will be put in tinydrm_fini() */
• drm_dev_ref(&tdev->drm);

Why do we need that private ref? Grabbing references in unregister code looks like a recipe for leaks ...

• drm_fbdev_cma_dev_unplug(tdev->fbdev_cma);
• drm_dev_unplug(&tdev->drm);
• /* Make sure framebuffer flushing is done */
• mutex_lock(&tdev->dirty_lock);
• mutex_unlock(&tdev->dirty_lock);

Is this really needed? Or, doesn't it just paper over a driver bug you have already anyway, since native kms userspace can directly call fb->funcs->dirty too, and you already protect against that.

This definitely looks like the fbdev helper is leaking implementation details to callers where it shouldn't do that.

}

static void devm_tinydrm_register_release(void *data) diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 2465489..84ab8d1 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -31,6 +31,9 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,

DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))

• return;
  

• ret = regulator_enable(mipi->regulator); if (ret) { dev_err(dev, "Failed to enable regulator (%d)\n", ret);

@@ -133,6 +136,7 @@ static struct drm_driver mi0283qt_driver = { DRIVER_ATOMIC, .fops = &mi0283qt_fops, TINYDRM_GEM_DRIVER_OPS,

• .release = tinydrm_release, .lastclose = tinydrm_lastclose, .debugfs_init = mipi_dbi_debugfs_init, .name = "mi0283qt",

diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index f5b9b772..49d03ab 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -209,7 +209,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb,

mutex_lock(&tdev->dirty_lock);

• if (!mipi->enabled)

• if (!mipi->enabled || drm_dev_is_unplugged(fb->dev)) goto out_unlock;

  /* fbdev can flush even when we're not interested */

@@ -314,6 +314,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)

DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))

• return;
  

• mipi->enabled = false;

  if (mipi->backlight)

diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c index b8fc8eb..3869d362 100644 --- a/drivers/gpu/drm/tinydrm/repaper.c +++ b/drivers/gpu/drm/tinydrm/repaper.c @@ -542,7 +542,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,

mutex_lock(&tdev->dirty_lock);

• if (!epd->enabled)

• if (!epd->enabled || drm_dev_is_unplugged(fb->dev)) goto out_unlock;

  /* fbdev can flush even when we're not interested */

@@ -670,6 +670,9 @@ static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe,

DRM_DEBUG_DRIVER("\n");

• if (drm_dev_is_unplugged(&tdev->drm))

• return;
  

• /* Power up sequence */ gpiod_set_value_cansleep(epd->reset, 0); gpiod_set_value_cansleep(epd->panel_on, 0);

@@ -803,6 +806,9 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe)

DRM_DEBUG_DRIVER("\n");

• if (drm_dev_is_unplugged(&tdev->drm))

• return;
  

• mutex_lock(&tdev->dirty_lock); epd->enabled = false; mutex_unlock(&tdev->dirty_lock);

@@ -894,6 +900,7 @@ static struct drm_driver repaper_driver = { DRIVER_ATOMIC, .fops = &repaper_fops, TINYDRM_GEM_DRIVER_OPS,

• .release = tinydrm_release, .name = "repaper", .desc = "Pervasive Displays RePaper e-ink panels", .date = "20170405",

diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index bc2b905..b1bfc4e 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c @@ -120,7 +120,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb,

mutex_lock(&tdev->dirty_lock);

• if (!mipi->enabled)

• if (!mipi->enabled || drm_dev_is_unplugged(fb->dev)) goto out_unlock;

  /* fbdev can flush even when we're not interested */

@@ -184,6 +184,9 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,

DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))

• return;
  

• mipi_dbi_hw_reset(mipi); ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f); if (ret) {

@@ -252,6 +255,9 @@ static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)

DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))

• return;
  

• if (!mipi->enabled) return;

@@ -319,6 +325,7 @@ static struct drm_driver st7586_driver = { DRIVER_ATOMIC, .fops = &st7586_fops, TINYDRM_GEM_DRIVER_OPS,

• .release = tinydrm_release, .lastclose = tinydrm_lastclose, .debugfs_init = mipi_dbi_debugfs_init, .name = "st7586",

diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h index ded9817..4265f51 100644 --- a/include/drm/tinydrm/tinydrm.h +++ b/include/drm/tinydrm/tinydrm.h @@ -23,6 +23,10 @@

• @fbdev_cma: CMA fbdev structure
• @suspend_state: Atomic state when suspended
• @fb_funcs: Framebuffer functions used when creating framebuffers

• • Drivers that embed &tinydrm_device must set it as the first member because
• • it is freed in tinydrm_release(). This means that the structure must be
• • allocated with kzalloc() and not the devm_ version.
  */

struct tinydrm_device { struct drm_device drm; @@ -94,6 +98,7 @@ struct drm_gem_object * tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm, struct dma_buf_attachment *attach, struct sg_table *sgt); +void tinydrm_release(struct drm_device *drm); int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev, const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver); -- 2.7.4

On Wed, Aug 30, 2017 at 6:31 PM, Noralf Trønnes noralf@tronnes.org wrote:

Den 28.08.2017 23.56, skrev Daniel Vetter:

...

On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:

...

Support device unplugging to make tinydrm suitable for USB devices.

Cc: David Lechner david@lechnology.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 69 ++++++++++++++++++++++++----- drivers/gpu/drm/tinydrm/mi0283qt.c | 4 ++ drivers/gpu/drm/tinydrm/mipi-dbi.c | 5 ++- drivers/gpu/drm/tinydrm/repaper.c | 9 +++- drivers/gpu/drm/tinydrm/st7586.c | 9 +++- include/drm/tinydrm/tinydrm.h | 5 +++ 6 files changed, 87 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index f11f4cd..3ccbcc5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c @@ -32,6 +32,29 @@

• The driver allocates &tinydrm_device, initializes it using
• devm_tinydrm_init(), sets up the pipeline using

tinydrm_display_pipe_init()

• and registers the DRM device using devm_tinydrm_register().

• • Device unplug

• • ---

• • tinydrm supports device unplugging when there's still open DRM or

fbdev file

• • handles.
• • There are 2 ways for driver-device unbinding to happen:
• • • The driver module is unloaded causing the driver to be

unregistered.

• • This can't happen as long as there's open file handles because a

reference

• • is taken on the module.

Aside: you can do that, but then it works like a hw unplug, through the unbind property in sysfs.

The module is still pinned isn't it?

Yup, but only the code is pinned, not any of the datastructures like drm_device.

I can add sysfs unbind as a third way of triggering unbind:

• • The sysfs driver _unbind_ file can be used to unbind the driver form

the

• device. This can happen any time.

...

...

• • • The device is removed (USB, Device Tree overlay).
• • This can happen at any time.
• • The driver needs to protect device resources from access after the

device is

• • gone. This is done checking drm_dev_is_unplugged(), typically in
• • &drm_framebuffer_funcs.dirty, &drm_simple_display_pipe_funcs.enable

and

• • .disable. Resources that doesn't face userspace and is only used

with the

• • device can be setup using devm_ functions, but &tinydrm_device must

be

• • allocated using plain kzalloc() since it's lifetime can exceed that

of the

• • device. tinydrm_release() will free the structure.

So here's a bit a dragon: There's no prevention of is_unplugged racing against a drm_dev_unplug(). There's been various attempts to fixing this, but they're all somewhat ugly.

Either way, that's a bug in the drm core :-)

...

*/ /** @@ -138,6 +161,29 @@ static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit, }; +/**

• • tinydrm_release - DRM driver release helper
• • @drm: DRM device
• • This function cleans up and finalizes &drm_device and frees

&tinydrm_device.

• • Drivers must use this as their &drm_driver->release callback.
• */

+void tinydrm_release(struct drm_device *drm) +{

•   struct tinydrm_device *tdev = drm_to_tinydrm(drm);
  

•   DRM_DEBUG_DRIVER("\n");
  

•   drm_mode_config_cleanup(drm);
  

•   drm_dev_fini(drm);
  

•   mutex_destroy(&tdev->dirty_lock);
  

•   kfree(tdev->fbdev_cma);
  

•   kfree(tdev);
  

+} +EXPORT_SYMBOL(tinydrm_release);

• static int tinydrm_init(struct device *parent, struct tinydrm_device

*tdev, const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver) @@ -160,8 +206,6 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, static void tinydrm_fini(struct tinydrm_device *tdev) {

•   drm_mode_config_cleanup(&tdev->drm);
  

•   mutex_destroy(&tdev->dirty_lock);
    drm_dev_unref(&tdev->drm);
  

  } @@ -178,8 +222,8 @@ static void devm_tinydrm_release(void *data)
  • @driver: DRM driver
  • This function initializes @tdev, the underlying DRM device and it's
• • mode_config. Resources will be automatically freed on driver detach

(devres)

• • using drm_mode_config_cleanup() and drm_dev_unref().

• • mode_config. drm_dev_unref() is called on driver detach (devres) and

when

• • all refs are dropped, tinydrm_release() is called.
  • Returns:
  • Zero on success, negative error code on failure.

@@ -226,14 +270,17 @@ static int tinydrm_register(struct tinydrm_device *tdev) static void tinydrm_unregister(struct tinydrm_device *tdev) {

•   struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
  

•   drm_atomic_helper_shutdown(&tdev->drm);
  

•   /* don't restore fbdev in lastclose, keep pipeline disabled */
  

•   tdev->fbdev_cma = NULL;
  

•   drm_dev_unregister(&tdev->drm);
  

•   if (fbdev_cma)
  

•           drm_fbdev_cma_fini(fbdev_cma);
  

•   /* Get a ref that will be put in tinydrm_fini() */
  

•   drm_dev_ref(&tdev->drm);
  

Why do we need that private ref? Grabbing references in unregister code looks like a recipe for leaks ...

Yeah, it's better to take the second ref in devm_tinydrm_register().

The reason I need 2 refs is because tinydrm is set up with 2 devm_ functions. This way I can leave all error path cleanup in the hands of devres.

static int driver_probe(...) { ret = devm_tinydrm_init(...); if (ret) return ret;

ret = tinydrm_display_pipe_init(...);
if (ret)
    return ret;

drm_mode_config_reset(...);

return devm_tinydrm_register(...);

}

I don't think this works. What's worse, devm has a high chance of freeing stuff at the wrong time since it's tied to the drm_device->dev, and not to drm_device.

I'm not sure what a better solution is, but when discussing all this with Laurent we agreed that in general, devm_ needs to be considered harmful. Looks simple, very easy to create broken code that doesn't clean up properly on unplug. As much as leaks are bad, freeing before all users are gone is worse. devm_ "fixes" the former and heavily encourages the latter.

In your case I think devm_tinydrm_register is correct (but a bit strange). devm_tinydrm_init otoh looks like all the code it has should be moved into the drm_driver->release callback. You can't destroy the dirty_lock while a thread might be holding it right at that moment.

...

...

•   drm_fbdev_cma_dev_unplug(tdev->fbdev_cma);
  

•   drm_dev_unplug(&tdev->drm);
  

•   /* Make sure framebuffer flushing is done */
  

•   mutex_lock(&tdev->dirty_lock);
  

•   mutex_unlock(&tdev->dirty_lock);
  

Is this really needed? Or, doesn't it just paper over a driver bug you have already anyway, since native kms userspace can directly call fb->funcs->dirty too, and you already protect against that.

This definitely looks like the fbdev helper is leaking implementation details to callers where it shouldn't do that.

Flushing can happen while drm_dev_unplug() is called, and when we leave this function the device facing resources controlled by devres will be removed. Thus I have to make sure any such flushing is done before leaving so the next flush is stopped by the drm_dev_is_unplugged() check. I don't see any other way of ensuring that.

I see now that I should move the call to drm_atomic_helper_shutdown() after drm_dev_unplug() to properly protect the pipe .enable/.disable callbacks.

Hm, calling _shutdown when the hw is gone already won't end well. Fundamentally this race exists for all use-cases, and I'm somewhat leaning towards plugging it in the core.

The general solution probably involves something that smells a lot like srcu, i.e. at every possible entry point into a drm driver (ioctl, fbdev, dma-buf sharing, everything really) we take that super-cheap read-side look, and drop it when we leave.

Then on unplug we call synchroize_srcu, which essentially does what your lock grab&drop trick does. Just much less overhead on the read side (we can sprinkle this over everything without worrying about wasting cpu time), and a bit clearer in the intention on the _unplug side.

But yeah, another thing that'll be a pile of work to fix properly. I'd leave it at a FIXME comment in drm_dev_unplug() until we get to it. And for now not worry about all the possible races. One thing at a time and all that. -Daniel

Noralf.

...

...

} static void devm_tinydrm_register_release(void *data) diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 2465489..84ab8d1 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -31,6 +31,9 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe, DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))
  

•           return;
  

•   ret = regulator_enable(mipi->regulator);
    if (ret) {
            dev_err(dev, "Failed to enable regulator (%d)\n", ret);
  

@@ -133,6 +136,7 @@ static struct drm_driver mi0283qt_driver = { DRIVER_ATOMIC, .fops = &mi0283qt_fops, TINYDRM_GEM_DRIVER_OPS,

•   .release                = tinydrm_release,
    .lastclose              = tinydrm_lastclose,
    .debugfs_init           = mipi_dbi_debugfs_init,
    .name                   = "mi0283qt",
  

diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index f5b9b772..49d03ab 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -209,7 +209,7 @@ static int mipi_dbi_fb_dirty(struct drm_framebuffer *fb, mutex_lock(&tdev->dirty_lock);

• if (!mipi->enabled)
  

•   if (!mipi->enabled || drm_dev_is_unplugged(fb->dev))
            goto out_unlock;
    /* fbdev can flush even when we're not interested */
  

@@ -314,6 +314,9 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))
  

•           return;
  

•   mipi->enabled = false;
    if (mipi->backlight)
  

diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c index b8fc8eb..3869d362 100644 --- a/drivers/gpu/drm/tinydrm/repaper.c +++ b/drivers/gpu/drm/tinydrm/repaper.c @@ -542,7 +542,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb, mutex_lock(&tdev->dirty_lock);

• if (!epd->enabled)
  

•   if (!epd->enabled || drm_dev_is_unplugged(fb->dev))
            goto out_unlock;
    /* fbdev can flush even when we're not interested */
  

@@ -670,6 +670,9 @@ static void repaper_pipe_enable(struct drm_simple_display_pipe *pipe, DRM_DEBUG_DRIVER("\n");

• if (drm_dev_is_unplugged(&tdev->drm))
  

•           return;
  

•   /* Power up sequence */
    gpiod_set_value_cansleep(epd->reset, 0);
    gpiod_set_value_cansleep(epd->panel_on, 0);
  

@@ -803,6 +806,9 @@ static void repaper_pipe_disable(struct drm_simple_display_pipe *pipe) DRM_DEBUG_DRIVER("\n");

• if (drm_dev_is_unplugged(&tdev->drm))
  

•           return;
  

•   mutex_lock(&tdev->dirty_lock);
    epd->enabled = false;
    mutex_unlock(&tdev->dirty_lock);
  

@@ -894,6 +900,7 @@ static struct drm_driver repaper_driver = { DRIVER_ATOMIC, .fops = &repaper_fops, TINYDRM_GEM_DRIVER_OPS,

•   .release                = tinydrm_release,
    .name                   = "repaper",
    .desc                   = "Pervasive Displays RePaper e-ink
  

panels", .date = "20170405", diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c index bc2b905..b1bfc4e 100644 --- a/drivers/gpu/drm/tinydrm/st7586.c +++ b/drivers/gpu/drm/tinydrm/st7586.c @@ -120,7 +120,7 @@ static int st7586_fb_dirty(struct drm_framebuffer *fb, mutex_lock(&tdev->dirty_lock);

• if (!mipi->enabled)
  

•   if (!mipi->enabled || drm_dev_is_unplugged(fb->dev))
            goto out_unlock;
    /* fbdev can flush even when we're not interested */
  

@@ -184,6 +184,9 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe, DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))
  

•           return;
  

•   mipi_dbi_hw_reset(mipi);
    ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
    if (ret) {
  

@@ -252,6 +255,9 @@ static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe) DRM_DEBUG_KMS("\n");

• if (drm_dev_is_unplugged(&tdev->drm))
  

•           return;
  

•   if (!mipi->enabled)
            return;
  

  @@ -319,6 +325,7 @@ static struct drm_driver st7586_driver = { DRIVER_ATOMIC, .fops = &st7586_fops, TINYDRM_GEM_DRIVER_OPS,

•   .release                = tinydrm_release,
    .lastclose              = tinydrm_lastclose,
    .debugfs_init           = mipi_dbi_debugfs_init,
    .name                   = "st7586",
  

diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h index ded9817..4265f51 100644 --- a/include/drm/tinydrm/tinydrm.h +++ b/include/drm/tinydrm/tinydrm.h @@ -23,6 +23,10 @@

• @fbdev_cma: CMA fbdev structure
• @suspend_state: Atomic state when suspended
• @fb_funcs: Framebuffer functions used when creating framebuffers

• • Drivers that embed &tinydrm_device must set it as the first member

because

• • it is freed in tinydrm_release(). This means that the structure must

be

• • allocated with kzalloc() and not the devm_ version.
  */ struct tinydrm_device { struct drm_device drm;

@@ -94,6 +98,7 @@ struct drm_gem_object * tinydrm_gem_cma_prime_import_sg_table(struct drm_device *drm, struct dma_buf_attachment *attach, struct sg_table *sgt); +void tinydrm_release(struct drm_device *drm); int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev, const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver); -- 2.7.4

Den 31.08.2017 14.59, skrev Laurent Pinchart:

Hi Daniel and Noralf,

On Wednesday, 30 August 2017 20:18:49 EEST Daniel Vetter wrote:

...

On Wed, Aug 30, 2017 at 6:31 PM, Noralf Trønnes noralf@tronnes.org wrote:

...

Den 28.08.2017 23.56, skrev Daniel Vetter:

...

On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:

...

Support device unplugging to make tinydrm suitable for USB devices.

Cc: David Lechner david@lechnology.com Signed-off-by: Noralf Trønnes noralf@tronnes.org

---

drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 69 ++++++++++++++++++--- drivers/gpu/drm/tinydrm/mi0283qt.c | 4 ++ drivers/gpu/drm/tinydrm/mipi-dbi.c | 5 ++- drivers/gpu/drm/tinydrm/repaper.c | 9 +++- drivers/gpu/drm/tinydrm/st7586.c | 9 +++- include/drm/tinydrm/tinydrm.h | 5 +++ 6 files changed, 87 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index f11f4cd..3ccbcc5 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c @@ -32,6 +32,29 @@

• The driver allocates &tinydrm_device, initializes it using
• devm_tinydrm_init(), sets up the pipeline using

tinydrm_display_pipe_init()

• and registers the DRM device using devm_tinydrm_register().

• • Device unplug

• • ---

• • tinydrm supports device unplugging when there's still open DRM or

fbdev file

• • handles.
• • There are 2 ways for driver-device unbinding to happen:
• • • The driver module is unloaded causing the driver to be

unregistered.

• • This can't happen as long as there's open file handles because a

reference

• • is taken on the module.

Aside: you can do that, but then it works like a hw unplug, through the unbind property in sysfs.

The module is still pinned isn't it?

Yup, but only the code is pinned, not any of the datastructures like drm_device.

...

I can add sysfs unbind as a third way of triggering unbind:

• • The sysfs driver _unbind_ file can be used to unbind the driver form

the

• device. This can happen any time.

Sounds good to me.

...

...

...

...

• • • The device is removed (USB, Device Tree overlay).
• • This can happen at any time.
• • The driver needs to protect device resources from access after the

device is

• • gone. This is done checking drm_dev_is_unplugged(), typically in
• • &drm_framebuffer_funcs.dirty, &drm_simple_display_pipe_funcs.enable

and

• • .disable. Resources that doesn't face userspace and is only used

s/doesn't/don't/ s/and is/and are/

...

...

...

...

with the

• • device can be setup using devm_ functions, but &tinydrm_device must

be

• • allocated using plain kzalloc() since it's lifetime can exceed that

of the

s/it's/its/

...

...

...

...

• • device. tinydrm_release() will free the structure.

So here's a bit a dragon: There's no prevention of is_unplugged racing against a drm_dev_unplug(). There's been various attempts to fixing this, but they're all somewhat ugly.

Either way, that's a bug in the drm core :-)

...

*/

 /**

@@ -138,6 +161,29 @@ static const struct drm_mode_config_funcs tinydrm_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit, }; +/**

• • tinydrm_release - DRM driver release helper
• • @drm: DRM device
• • This function cleans up and finalizes &drm_device and frees

&tinydrm_device.

• • Drivers must use this as their &drm_driver->release callback.
• */

+void tinydrm_release(struct drm_device *drm) +{

•   struct tinydrm_device *tdev = drm_to_tinydrm(drm);
  

•   DRM_DEBUG_DRIVER("\n");
  

•   drm_mode_config_cleanup(drm);
  

•   drm_dev_fini(drm);
  

•   mutex_destroy(&tdev->dirty_lock);
  

•   kfree(tdev->fbdev_cma);
  

•   kfree(tdev);
  

+} +EXPORT_SYMBOL(tinydrm_release);

• static int tinydrm_init(struct device *parent, struct tinydrm_device

*tdev, const struct drm_framebuffer_funcs *fb_funcs, struct drm_driver *driver) @@ -160,8 +206,6 @@ static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, static void tinydrm_fini(struct tinydrm_device *tdev) {

•   drm_mode_config_cleanup(&tdev->drm);
  

•   mutex_destroy(&tdev->dirty_lock);
     drm_dev_unref(&tdev->drm);
  

  }

@@ -178,8 +222,8 @@ static void devm_tinydrm_release(void *data)

• @driver: DRM driver
• This function initializes @tdev, the underlying DRM device and it's

While at it, s/it's/its/

...

...

...

...

• • mode_config. Resources will be automatically freed on driver detach

(devres)

• • using drm_mode_config_cleanup() and drm_dev_unref().

• • mode_config. drm_dev_unref() is called on driver detach (devres) and

when

• • all refs are dropped, tinydrm_release() is called.
  • Returns:
  • Zero on success, negative error code on failure.

@@ -226,14 +270,17 @@ static int tinydrm_register(struct tinydrm_device *tdev)

static void tinydrm_unregister(struct tinydrm_device *tdev) {

•   struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma;
  

•    drm_atomic_helper_shutdown(&tdev->drm);
  
  

•   /* don't restore fbdev in lastclose, keep pipeline disabled */
  

•   tdev->fbdev_cma = NULL;
  

•   drm_dev_unregister(&tdev->drm);
  

•   if (fbdev_cma)
  

•           drm_fbdev_cma_fini(fbdev_cma);
  

•   /* Get a ref that will be put in tinydrm_fini() */
  

•   drm_dev_ref(&tdev->drm);
  

Why do we need that private ref? Grabbing references in unregister code looks like a recipe for leaks ...

Yeah, it's better to take the second ref in devm_tinydrm_register().

The reason I need 2 refs is because tinydrm is set up with 2 devm_ functions. This way I can leave all error path cleanup in the hands of devres.

static int driver_probe(...) { ret = devm_tinydrm_init(...); if (ret) return ret;

 ret = tinydrm_display_pipe_init(...);
 if (ret)
     return ret;

 drm_mode_config_reset(...);

 return devm_tinydrm_register(...);

}

I don't think this works. What's worse, devm has a high chance of freeing stuff at the wrong time since it's tied to the drm_device->dev, and not to drm_device.

I'm not sure what a better solution is, but when discussing all this with Laurent we agreed that in general, devm_ needs to be considered harmful. Looks simple, very easy to create broken code that doesn't clean up properly on unplug. As much as leaks are bad, freeing before all users are gone is worse. devm_ "fixes" the former and heavily encourages the latter.

I totally agree. A crash at unbind time is worse than a memory leak given that the bind/unbind cycles are unfrequent. Of course we need to aim for fixing both problems :-)

I'm not sure I see how the use of devm_ causes any other problems than the fact that the parent device has been deleted. In the case of usb, after usb_driver.disconnect it's not allowed to access the usb_device. This means that the driver has to protect against this. The same applies to the devm_ resources, after the device is gone, they're not available anymore. It ofc is easier to protect one resource than several, but in principal it's the same, isn't it?

For me the beauty of devres is that it removes the need for error paths that are almost never exercised and the probe code is simpler and easy to read.

...

In your case I think devm_tinydrm_register is correct (but a bit strange).

Yes, I don't think an explicit tinydrm_unregister() call in the driver's .remove() handler would be so difficult.

...

devm_tinydrm_init otoh looks like all the code it has should be moved into the drm_driver->release callback. You can't destroy the dirty_lock while a thread might be holding it right at that moment.

Agreed too.

Furthermore the devm_kzalloc() in tinydrm_display_pipe_init() seems suspicious to me. It should be easy to replace it with kzalloc(), the mode can be freed in tinydrm_connector_destroy().

It's safe since it's protected by connector detect(), but I agree with you.

(And on a side note the direct mode copy without calling drm_mode_duplicate() in that function is also suspicious, although it might not be an issue in practice)

...

...

...

...

•   drm_fbdev_cma_dev_unplug(tdev->fbdev_cma);
  

•   drm_dev_unplug(&tdev->drm);
  

•   /* Make sure framebuffer flushing is done */
  

•   mutex_lock(&tdev->dirty_lock);
  

•   mutex_unlock(&tdev->dirty_lock);
  

Is this really needed? Or, doesn't it just paper over a driver bug you have already anyway, since native kms userspace can directly call fb->funcs->dirty too, and you already protect against that.

This definitely looks like the fbdev helper is leaking implementation details to callers where it shouldn't do that.

Flushing can happen while drm_dev_unplug() is called, and when we leave this function the device facing resources controlled by devres will be removed. Thus I have to make sure any such flushing is done before leaving so the next flush is stopped by the drm_dev_is_unplugged() check. I don't see any other way of ensuring that.

I see now that I should move the call to drm_atomic_helper_shutdown() after drm_dev_unplug() to properly protect the pipe .enable/.disable callbacks.

Hm, calling _shutdown when the hw is gone already won't end well. Fundamentally this race exists for all use-cases, and I'm somewhat leaning towards plugging it in the core.

The general solution probably involves something that smells a lot like srcu, i.e. at every possible entry point into a drm driver (ioctl, fbdev, dma-buf sharing, everything really) we take that super-cheap read-side look, and drop it when we leave.

That's similar to what we plan to do in V4L2. The idea is to set a device removed flag at the beginning of the .remove() handler and wait for all pending operations to complete. The core will reject any new operation when the flag is set. To wait for completion, every entry point would increase a use count, and decrease it on exit. When the use count is decreased to 0 waiters will be woken up. This should solve the unplug/user race.

Ah, such a simple solution, easy to understand and difficult to get wrong! And it's even nestable, no danger of deadlocking.

Maybe I can use it with tinydrm:

* @dev_use: Tracks use of functions acessing the parent device. *           If it is zero, the device is gone. See ... struct tinydrm_device {     atomic_t dev_use; };

/**  * tinydrm_dev_enter - Enter device accessing function  * @tdev: tinydrm device  *  * This function protects against using a device and it's resources after  * it's removed. Should be called at the beginning of the function.  *  * Returns:  * False if the device is still present, true if it is gone.  */ static inline bool tinydrm_dev_enter(struct tinydrm_device *tdev) {     return !atomic_inc_not_zero(&tdev->dev_use); }

static inline void tinydrm_dev_exit(struct tinydrm_device *tdev) {     atomic_dec(&tdev->dev_use); }

static inline bool tinydrm_is_unplugged(struct tinydrm_device *tdev) {     bool ret = !atomic_read(&tdev->dev_use);     smp_rmb();     return ret; }

static int tinydrm_init(...) {     /* initialize */

/* Set device is present */     atomic_set(&tdev->dev_use, 1); }

static void tinydrm_unregister(...) {     /* Set device gone */     atomic_dec(&tdev->dev_use);

/* Wait for all device facing functions to finish */     while (!tinydrm_is_unplugged(tdev)) {         cond_resched();     }

/* proceed with unregistering */ }

static int mipi_dbi_fb_dirty(...) {     if (tinydrm_dev_enter(tdev))         return -ENODEV;

/* flush framebuffer */

tinydrm_dev_exit(tdev); }

...

Then on unplug we call synchroize_srcu, which essentially does what your lock grab&drop trick does. Just much less overhead on the read side (we can sprinkle this over everything without worrying about wasting cpu time), and a bit clearer in the intention on the _unplug side.

Looks like srcu already implements what we need. I'm not sure we need an RCU- based solution though, it might be too complex.

...

But yeah, another thing that'll be a pile of work to fix properly. I'd leave it at a FIXME comment in drm_dev_unplug() until we get to it. And for now not worry about all the possible races. One thing at a time and all that.

...

...

...

}

static void devm_tinydrm_register_release(void *data) diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index 2465489..84ab8d1 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -31,6 +31,9 @@ static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,

     DRM_DEBUG_KMS("\n");

•   if (drm_dev_is_unplugged(&tdev->drm))
  

•           return;
  

•    ret = regulator_enable(mipi->regulator);
     if (ret) {
             dev_err(dev, "Failed to enable regulator (%d)\n", ret);
  

@@ -133,6 +136,7 @@ static struct drm_driver mi0283qt_driver = { DRIVER_ATOMIC, .fops = &mi0283qt_fops, TINYDRM_GEM_DRIVER_OPS,

•   .release                = tinydrm_release,
  

Should this be included in the TINYDRM_GEM_DRIVER_OPS macro ?

I did that first, then I saw the name GEM OPS, and put it outside :-)

Noralf.

...

...

...

...

     .lastclose              = tinydrm_lastclose,
     .debugfs_init           = mipi_dbi_debugfs_init,
     .name                   = "mi0283qt",

[snip]