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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
