On Fri, Jun 11, 2021 at 04:53:11PM +0200, Christian König wrote:
>
>
> Am 11.06.21 um 16:47 schrieb Daniel Vetter:
> > On Fri, Jun 11, 2021 at 02:02:57PM +0200, Christian König wrote:
> > > As the name implies if testing all fences is requested we
> > > should indeed test all fences and not skip the exclusive
> > > one because we see shared ones.
> > >
> > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > Hm I thought we've had the rule that when both fences exist, then
> > collectively the shared ones must signale no earlier than the exclusive
> > one.
> >
> > That's at least the contract we've implemented in dma_resv.h. But I've
> > also found a bunch of drivers who are a lot more yolo on this.
> >
> > I think there's a solid case here to just always take all the fences if we
> > ask for all the shared ones, but if we go that way then I'd say
> > - clear kerneldoc patch to really hammer this in (currently we're not good
> > at all in this regard)
> > - going through drivers a bit to check for this (I have some of that done
> > already in my earlier series, need to respin it and send it out)
> >
> > But I'm kinda not seeing why this needs to be in this patch series here.
>
> You mentioned that this is a problem in the last patch and if you ask me
> that's just a bug or at least very inconsistent.
>
> See dma_resv_wait_timeout() always waits for all fences, including the
> exclusive one even if shared ones are present. But dma_resv_test_signaled()
> ignores the exclusive one if shared ones are present.
Hm the only one I thought I've mentioned is that dma_buf_poll doesn't use
dma_fence_get_rcu_safe where I think it should. Different problem. I think
this is one you spotted.
> The only other driver I could find trying to make use of this is nouveau and
> I already provided a fix for this as well.
i915 also does this, and I think I've found a few more.
> I just think that this is the more defensive approach to fix this and have
> at least the core functions consistent on the handling.
Oh fully agree, it's just current dma_resv docs aren't the greatest, and
hacking on semantics without updating the docs isn't great. Especially
when it's ad-hoc.
-Daniel
>
> Christian.
>
> > -Daniel
> >
> > > ---
> > > drivers/dma-buf/dma-resv.c | 33 ++++++++++++---------------------
> > > 1 file changed, 12 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> > > index f26c71747d43..c66bfdde9454 100644
> > > --- a/drivers/dma-buf/dma-resv.c
> > > +++ b/drivers/dma-buf/dma-resv.c
> > > @@ -615,25 +615,21 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence)
> > > */
> > > bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all)
> > > {
> > > - unsigned int seq, shared_count;
> > > + struct dma_fence *fence;
> > > + unsigned int seq;
> > > int ret;
> > > rcu_read_lock();
> > > retry:
> > > ret = true;
> > > - shared_count = 0;
> > > seq = read_seqcount_begin(&obj->seq);
> > > if (test_all) {
> > > struct dma_resv_list *fobj = dma_resv_shared_list(obj);
> > > - unsigned int i;
> > > -
> > > - if (fobj)
> > > - shared_count = fobj->shared_count;
> > > + unsigned int i, shared_count;
> > > + shared_count = fobj ? fobj->shared_count : 0;
> > > for (i = 0; i < shared_count; ++i) {
> > > - struct dma_fence *fence;
> > > -
> > > fence = rcu_dereference(fobj->shared[i]);
> > > ret = dma_resv_test_signaled_single(fence);
> > > if (ret < 0)
> > > @@ -641,24 +637,19 @@ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all)
> > > else if (!ret)
> > > break;
> > > }
> > > -
> > > - if (read_seqcount_retry(&obj->seq, seq))
> > > - goto retry;
> > > }
> > > - if (!shared_count) {
> > > - struct dma_fence *fence_excl = dma_resv_excl_fence(obj);
> > > -
> > > - if (fence_excl) {
> > > - ret = dma_resv_test_signaled_single(fence_excl);
> > > - if (ret < 0)
> > > - goto retry;
> > > + fence = dma_resv_excl_fence(obj);
> > > + if (fence) {
> > > + ret = dma_resv_test_signaled_single(fence);
> > > + if (ret < 0)
> > > + goto retry;
> > > - if (read_seqcount_retry(&obj->seq, seq))
> > > - goto retry;
> > > - }
> > > }
> > > + if (read_seqcount_retry(&obj->seq, seq))
> > > + goto retry;
> > > +
> > > rcu_read_unlock();
> > > return ret;
> > > }
> > > --
> > > 2.25.1
> > >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
