[PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue

[Rob Clark]

On Tue, Sep 9, 2014 at 3:07 AM, Tomi Valkeinen <tomi.valkeinen at ti.com> wrote:
>
>>>> lockdep won't complain though since you essentially open-code a
>>>> workqueue_flush, and lockdep also doesn't complain about all possible
>>>> deadlocks (due to some design issues with lockdep).
>>>
>>> What do you mean "open-code a workqueue_flush"?. I use flush_workqueue
>>> there. We have two things to wait for: work on the workqueue and work
>>> which is triggered by the vsync irq. So we loop and test for both of
>>> those, until there's no more work.
>>
>> Oops, missed that. Ordering looks wrong though since if the irq can latch
>> the workqueue you need to wait for irqs to happen first before flushing.
>> And obviously queue the work before signalling the completion of the
>> interrupt. But since this seems to lack locking anyway and is only used
>> for unload it doesn't really matter.

from a quick look at omap_crtc_flush(), you probably also race w/
apply worker list manipulation (like moving from queued_applies to
pending_applies)

> Yeah, well, the workqueue can create work for the irq also. I don't know
> if it does, currently, but I think it's safer to presume that both
> workqueue and the irq can create work to the other.
>
> But that's why I have a loop there. So we flush, then check if there is
> work for the irq. If yes, sleep a bit and go back to start. So if the
> irq work created new work for the wq, we flush that. And if that work
> created new work for the irq, we check that again. Etc.

I think adding an internal per-crtc apply_lock would solve a lot of
problems.  I was originally shying away from recommending that, mostly
because I didn't want to think about it and I though there was an
easier solution.

But with an apply_lock we could at least add some locking to _flush()
and make it not racy as well..

Although, I wonder if some waitqueue scheme would be a bit more sane..
ie. end of apply_worker, if there is nothing more queued up, signal
the event that _flush() is waiting on.

BR,
-R