drm: reference count event->completion

BugLink: http://bugs.launchpad.net/bugs/1677589
commit 24835e442f289813aa568d142a755672a740503c upstream.

When writing the generic nonblocking commit code I assumed that
through clever lifetime management I can assure that the completion
(stored in drm_crtc_commit) only gets freed after it is completed. And
that worked.

I also wanted to make nonblocking helpers resilient against driver
bugs, by having timeouts everywhere. And that worked too.

Unfortunately taking boths things together results in oopses :( Well,
at least sometimes: What seems to happen is that the drm event hangs
around forever stuck in limbo land. The nonblocking helpers eventually
time out, move on and release it. Now the bug I tested all this
against is drivers that just entirely fail to deliver the vblank
events like they should, and in those cases the event is simply
leaked. But what seems to happen, at least sometimes, on i915 is that
the event is set up correctly, but somohow the vblank fails to fire in
time. Which means the event isn't leaked, it's still there waiting for
eventually a vblank to fire. That tends to happen when re-enabling the
pipe, and then the trap springs and the kernel oopses.

The correct fix here is simply to refcount the crtc commit to make
sure that the event sticks around even for drivers which only
sometimes fail to deliver vblanks for some arbitrary reasons. Since
crtc commits are already refcounted that's easy to do.

References: https://bugs.freedesktop.org/show_bug.cgi?id=96781
Cc: Jim Rees <rees@umich.edu>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20161221102331.31033-1-daniel.vetter@ffwll.ch
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 55e7372ea0a05e0dbbd8e43fe345a4d9ab4b0c70..205251fae5396e6f52412e45b7dd8a36325d7d33 100644 (file)
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1389,6 +1389,15 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
        return ret < 0 ? ret : 0;
 }
 
+void release_crtc_commit(struct completion *completion)
+{
+       struct drm_crtc_commit *commit = container_of(completion,
+                                                     typeof(*commit),
+                                                     flip_done);
+
+       drm_crtc_commit_put(commit);
+}
+
 /**
  * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
  * @state: new modeset state to be committed
@@ -1481,6 +1490,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
                }
 
                crtc_state->event->base.completion = &commit->flip_done;
+               crtc_state->event->base.completion_release = release_crtc_commit;
+               drm_crtc_commit_get(commit);
        }
 
        return 0;

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 5d96de40b63fb8813ba1200117f6f908d387f41a..30c20f90520ad2b861787f827f077c222c948b4c 100644 (file)
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -689,8 +689,8 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
        assert_spin_locked(&dev->event_lock);
 
        if (e->completion) {
-               /* ->completion might disappear as soon as it signalled. */
                complete_all(e->completion);
+               e->completion_release(e->completion);
                e->completion = NULL;
        }
 

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9c4ee144b5f6b799baf92a9722cddcaee3eb2b4f..1871ca60e079f4c0aefd02a3e24348ca341c184e 100644 (file)
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -360,6 +360,7 @@ struct drm_ioctl_desc {
 /* Event queued up for userspace to read */
 struct drm_pending_event {
        struct completion *completion;
+       void (*completion_release)(struct completion *completion);
        struct drm_event *event;
        struct dma_fence *fence;
        struct list_head link;