[PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

On Wed, Dec 21, 2016 at 12:08:45PM +0100, Maarten Lankhorst wrote:
> Op 21-12-16 om 11:36 schreef Chris Wilson:
> > On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> >> 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
> >> evevntually 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.
> > Or make the event a part of the atomic state?
> > -Chris
> >
> afaict crtc commit is already taken to wait for completion, so this patch makes sense.
> 
> There's just a minor typo in the subject. :)
> Not sure that release_commit should be a function pointer, regardless..
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

It didn't help the bug reporters against oopses (but the reporters are
supremely confusing, I have no idea what's really being tested, the
bugzilla is a mess), but I still think the patch is useful for more
robuestness, I dropped the cc: stable and applied it to drm-misc.

Thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch