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

[Rob Clark]

On Mon, Sep 8, 2014 at 9:50 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Mon, Sep 08, 2014 at 09:39:40AM -0400, Rob Clark wrote:
>> 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.. ;-)
>
> It's the locking for the drm core layer, it better be done in drm core.
> Drivers are free to use it, but if it doesn't fit they can always do their
> own. And if you look at i915 we actually have a metric pile of mutexes
> taken from modeset code and other places exactly because the drm core
> modeset locking can't work for everyone.

note that I'm not actually advocating removing/changing the locking in
core.. I was just pointing out that the way it is currently structured
can get in the way.

>> 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.)
>
> Now _this_ smells like a proper midlayer - it enforces behaviour
> sequencing by providing a bunch of callbacks for drivers to hook into.

well, I was thinking more in terms of an optional callback so that
drivers which (at least currently) need to work like omapdrm can
actually do what they need to do.

that said, I think when atomic eventually lands, it would be a good
idea to revisit how omapdrm works.. I think atomic should give a
better way for it to do what it needs to do..

In the mean time, we need *something*.  Although I think dropping and
re-grabbing locks is an ok temporary solution.  It wouldn't exactly be
the only driver that does this.

BR,
-R


> Also, locking isn't to enforce ordering constraints at all, that's the job
> of flush_workqueue, completion and friends. And if such a sync point would
> result in deadlocks if your worker also uses the modeset locks then your
> driver needs to grow internal mutexes to coordinate the data accessed by
> workers. In i915 we've added a pile of such cases in the past (and will
> add more), e.g. the pps mutex, backlight spinlock, edp mutex, ...
>
> ADF does the same mistake of "hey let's do the synchronization in the core
> because driver writers get it wrong all the time". Which doesn't make it
> better ;-)
>
> Now if your idea was to provide this as a helper, which either enforces
> the ordering correctly to not need locks, or has it's own locking of
> in-flux requests or similar, then I'm in favour. But adding such a
> callback at the core is nacked with extreme prejudice from me ...
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch