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

[Rob Clark]

On Wed, Sep 3, 2014 at 10:25 AM, Daniel Vetter <daniel at ffwll.ch> 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.

what about just dropping and re-acquiring crtc->mutex, since that is
all the worker actually needs..

it would be a bigger task to get rid of the mutex on the worker, since
we'd have to double buffer state.  It seemed like rather than
re-invent atomic, it would be better just to wait for atomic to land
and then restructure the driver

BR,
-R

> 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 ;-)
>
> Cheers, Daniel
>
>> +     omap_crtc_flush(crtc);
>> +     drm_modeset_lock_all(dev);
>>  }
>>
>>  static int omap_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch