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

[Daniel Vetter]

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