On Mon, Sep 8, 2014 at 9:24 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxx>
>> >> ---
>> >> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
