[PATCH 4/9] drm/omap: make modesetting synchronous

[Rob Clark]

On Mon, Sep 8, 2014 at 9:24 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Mon, Sep 08, 2014 at 03:53:18PM +0300, Tomi Valkeinen wrote:
>> On 03/09/14 17:25, Daniel Vetter wrote:
>> > On Wed, Sep 03, 2014 at 02:55:05PM +0300, Tomi Valkeinen wrote:
>> >> Currently modesetting is not done synchronously, but it queues work that
>> >> is done later. In theory this is fine, but the driver doesn't handle it
>> >> at properly. This means that if an application first does a full
>> >> modeset, then immediately afterwards starts page flipping, the page
>> >> flipping will not work properly as there's modeset work still in the
>> >> queue.
>> >>
>> >> The result with my test application was that a backbuffer was shown on
>> >> the screen.
>> >>
>> >> Fixing this properly would be rather big undertaking. Thus this patch
>> >> fixes the issue by making the modesetting synchronous, by waiting for
>> >> the queued work to be done at the end of omap_crtc->commit().
>> >>
>> >> The ugly part here is that the background work takes crtc->mutex, and
>> >> the modesetting also holds that lock, which means that the background
>> >> work never gets done. To get around this, we unclock, wait, and lock
>> >> again.
>> >>
>> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
>> >> ---
>> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> index 193979f97bdb..3261fbf94957 100644
>> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> >> @@ -277,8 +277,13 @@ static void omap_crtc_prepare(struct drm_crtc *crtc)
>> >>  static void omap_crtc_commit(struct drm_crtc *crtc)
>> >>  {
>> >>    struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> >> +  struct drm_device *dev = crtc->dev;
>> >>    DBG("%s", omap_crtc->name);
>> >>    omap_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
>> >> +
>> >> +  drm_modeset_unlock_all(dev);
>> >
>> > This will run afoul of the upcoming locking rework in the atomic work. And
>> > I'm fairly sure that the crtc helpers will fall over badly if someone
>> > submits a concurrent setCrtc while you've dropped the locks here.
>> >
>> > Can't you instead just drop the locking from the worker? As long as you
>> > synchronize like here at the right places it can't race. I expect that you
>> > might want to synchronize in the crtc_prepare hook, too. But beyond that
>> > it should work.
>> >
>> > As-is nacked because future headaches for me ;-)
>>
>> Yes, it's ugly. But doing it with either queuing or caching would be a
>> major change. I was just trying to do smallish fixes to the driver for now.
>>
>> How about only unlocking/locking the crtc->mutex as Rob suggested? I
>> think it should work, but I didn't try it yet. Would that be as bad for
>> the locking rework?
>
> Same problem really, you shouldn't drop ww mutexes and reacquire them in
> the atomic world. ww mutexes have some debug infrastructure for that
> (ww_acquire_done) to catch abusers of this.
>
> So if you want to go forward with this it needs at least a big FIXME
> comment explaining that this is wrong. If you use locking to enforce
> ordering constraints that usually doesn't work well, and dropping locks to
> wait for async workers is plainly a locking design bug.

well, the locking in core makes it in some ways a bit of a midlayer.. ;-)

Some crtc->funcs->wait_until_flushed() sort of thing that
drm_mode_setcrtc() could call after dropping locks would go a long
ways to fix this case.  (Although post-atomic, may no longer be
required.)

BR,
-R


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch