Re: [PATCH 3/3] drm/i915: Use vblank evade mechanism in mmio_flip

Chris Wilson
2014-11-04T11:10:29Z

On Tue, Nov 04, 2014 at 12:04:03PM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:10:14PM +0200, Ander Conselvan de Oliveira wrote:
> > @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > int ret;
> >
> > - if (WARN_ON(intel_crtc->mmio_flip.seqno))
> > + if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE))
> > return -EBUSY;
>
> Ok I've merged the patch, but I think this check is wrong. do_mmio_flip
> only sets this to idle _after_ the hw register writing is done. Which
> means the flip could complete and userspace could receive the event all
> before we set status to IDLE. Which means you can WARN and return -EBUSY
> here when the flip definitely completed.
>
> So either clear IDLE at the head of do_mmio_flip before doing anything, or
> drop this state again. After all the previous seqno check only looked for
> waiting or not. tbh I'm not even sure whether this buys us anything at all
> - we already check that the flip completed, which should ensure ordering.
> I think we could just rip out mmio_flip.status completely.
>
> Can you pls do a fixup patch?

A better strategy would be to use the generic seqno mechanism for the
flip from a worker which then wouldn't need this extra vblank state
machinery on top of the vblank evade...
-Chris

--
Chris Wilson, Intel Open Source Technology Centre