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

[Rob Clark]

On Wed, Sep 3, 2014 at 11:21 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Wed, Sep 03, 2014 at 11:05:08AM -0400, Rob Clark wrote:
>> 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
>
> Well atomic kinda has the same idea to not take locks in the worker and
> instead relying on ordering.

so, I am not quite sure that would work..  (and maybe this is an extra
thing to think about for atomic).

The problem is that hw has no hw cursor.  So cursor is implemented
using plane.  But thanks to retarded userspace that rapidly
enables/disables cursor in some cases, we really need plane updates to
be async.  The way it currently works is userspace thread updates
plane state as rapidly as it wants.  When worker eventually gets to
run, it takes the most current state, possibly skipping 1000's of
enable/disable that happened in the mean time.

But for that to work, at least the way the driver is structured
currently, the worker needs to synchronize against userspace somehow.

BR,
-R

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