Re: [PATCH] drm/i915: Flush pending operations to the CRTC prior to modeset

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 20, 2012 at 11:17:51AM +0200, Daniel Vetter wrote:
> On Thu, Sep 20, 2012 at 10:56 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > We need to wait for pending operations on the CRTC to retire before we
> > can modify the CRTC. For example, if userspace has queued a batch that
> > uses a WAIT_FOR_EVENT associated with the current FB, we can not modify
> > the pipe with that outstanding, as we may then prevent that
> > WAIT_FOR_EVENT from ever completing and so hanging the GPU. (Imagine a
> > scanline wait waiting for line 1024 and the pipe being adjusted to
> > 600-line mode.) There is also the sequencing issue of the immediate
> > update versus a pending pageflip.
> >
> > In both cases the function to serialise the modeset with the pending
> > operations existed but was simply not being called, or called after the
> > damage was already done.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I've looked at this situation again and we do have a
> wait_for_pending_flips already in per-platform crtc_disable functions,
> which are called for for switching off crtcs and also only just
> disabling them for a modeset.
> 
> So I think this finish_fb call in set_base is totally unnecessary and
> can be just removed. Moving it to the crtc_set_config function doesn't
> help (and this patch misses the case where we disable other crtcs than
> set->crtc).

This whole wait for pending flips mechanism looks iffy to me.

First, when we schedule a flip, we record the fact that there is a
pending flip into the old bo's pending_flip atomic mask thingy. When we
later want to wait for the CRTC operations to finish, we do
'intel_finish_fb(crtc->fb)'. But notice that 'crtc->fb' is now the fb we
flipped _to_, so it can't actually tell us whether there's still a flip
pending.

-- 
Ville Syrjälä
Intel OTC