Re: [PATCH i915 v7 1/2] i915: wait for fence in mmio_flip_work_func

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 24, 2015 at 06:26:00PM -0800, Alex Goins wrote:
> Thanks, Daniel. There sure are a lot of Daniels.
> 
> > > +       else if (obj->base.dma_buf && obj->base.dma_buf->resv->fence_excl)
> > > +               return true;
> > 
> > I'm not sure if this is really doing exactly what you want.
> > When a reservation object's exclusive fence has signaled, I think the
> > old pointer in fence_excl is not actually cleared; it keeps pointing
> > to the old exclusive fence until that replaced by another.
> > 
> > So, just nake null-check here is like saying "did this reservation
> > object ever have an exclusive fence at some point in the past", which
> > is not necessarily the same as "is there an exclusive fence associated
> > with the buffer that I am about to flip"?
> > 
> > The reservation object is a complicated rcu & ww_mutex protected
> > beast, so I would be shy to access any of its fields directly.
> 
> Hm, well, the goal is to ensure that mmio_flip is used if we might need to wait
> on a fence, so if there are false positives it shouldn't be lethal.
> 
> Nonetheless, you're probably right. Maybe it would be better to use
> !reservation_object_test_signaled_rcu(test_all=FALSE), which should be TRUE iff
> there is an unsignaled exclusive fence attached, meaning we might have to wait.

Yeah I think that's a good idea for better encapsulation of
reservation/fence internals.
-Daniel

> 
> > >         if (mmio_flip->req)
> > >                 WARN_ON(__i915_wait_request(mmio_flip->req,
> > > @@ -11196,6 +11203,12 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
> > >                                             false, NULL,
> > >                                             &mmio_flip->i915->rps.mmioflips));
> > >
> > > +       /* For framebuffer backed by dmabuf, wait for fence */
> > > +       if (obj->base.dma_buf)
> > > +               WARN_ON(reservation_object_wait_timeout_rcu(obj->base.dma_buf->resv,
> > > +                                                           false, false,
> > > +                                                           MAX_SCHEDULE_TIMEOUT) < 0);
> > > +
> > 
> > Hmm, don't you want an interrupt-able wait here?
> > And if so, you probably don't want to WARN_ON() -ERESTARTSYS.
> 
> I had this as an interruptable wait previously, but I'm not sure how we would
> actually handle an interrupt in that case. Notice also how
> __i915_wait_request(interruptable=FALSE) just above is also an uninterruptable
> wait.
> 
> Thanks,
> Alex
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel