[Intel-gfx] [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank

[Chris Wilson]

On Tue, Jun 10, 2014 at 02:25:50PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 10, 2014 at 11:04:00AM +0100, Chris Wilson wrote:
> > +static inline int crtc_sbc(struct intel_crtc *crtc)
> > +{
> > +	return atomic_read(&crtc->base.dev->vblank[crtc->pipe].count);
> > +}
> 
> Still says 'sbc' which doesn't make sense to me.

I just don't like the term msc. :-p
crtc_vblank_counter()?
 
> > +
> >  static inline void intel_mark_page_flip_active(struct intel_crtc *intel_crtc)
> >  {
> >  	/* Ensure that the work item is consistent when activating it ... */
> >  	smp_wmb();
> >  	atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> > +	intel_crtc->unpin_work->sbc = crtc_sbc(intel_crtc);
> >  	/* and that it is marked active as soon as the irq could fire. */
> >  	smp_wmb();
> >  }
> > @@ -9265,6 +9279,69 @@ static int intel_default_queue_flip(struct drm_device *dev,
> >  	return -ENODEV;
> >  }
> >  
> > +static bool i915_gem_object_is_idle(struct drm_i915_gem_object *obj,
> > +				    bool readonly)
> > +{
> > +	struct intel_engine_cs *ring = obj->ring;
> > +	u32 seqno = readonly ? obj->last_write_seqno : obj->last_read_seqno;
> > +
> > +	if (ring == NULL || seqno == 0)
> > +		return true;
> > +
> > +	return i915_seqno_passed(ring->get_seqno(ring, true), seqno);
> > +}
> > +
> > +static bool __intel_pageflip_stall_check(struct drm_device *dev,
> > +					 struct drm_crtc *crtc)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_unpin_work *work = intel_crtc->unpin_work;
> > +	u32 addr;
> > +
> > +	if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
> > +		return true;
> > +
> > +	if (!work->enable_stall_check)
> > +		return false;
> > +
> > +	if ((crtc_sbc(intel_crtc) - intel_crtc->unpin_work->sbc) < 3)
> > +		return false;
> > +
> > +	if (!i915_gem_object_is_idle(work->pending_flip_obj, true))
> > +		return false;
> 
> I take it this is done to mitigate my premature flip completion
> concerns? Should work for the common case I suppose. Although if
> someone does something like this "write,read(s),flip" it could
> still complete the flip too soon. Waiting for last_read_seqno would
> avoid that.

It actually predated your concerns. I considered the person who continues
to write to the pending fb and decided that I didn't care too much for
them. What I actually wanted to do was to capture the vblank counter for
when the object was idle and then start watching for a missed flip.
Indeed, tracking when we believe the flip was queued would be better
again.
 
> And double checking the hardware flip counter should avoid this
> problem entirely. We already have it sampled in unpin_work->flip_count
> for the mmio vs. cs flip race so it should be easy to check it here as
> well. I suppose having both the flip counter and seqno checks should
> provide the best protection for all gens.

That was half the reason for waiting for that series to land first. Just
I never adapted to the framecounter code.
 
> As far as the seqno check goes, I was wondering if we should sample
> the seqno when submitting the flip? I'm slightly worried how this will
> work if userspace submitted a flip and already managed to pile more
> rendering on top. This code would then wait for the seqno for those
> later rendering operations.

Right that is how mmio flips avoid this issue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre