Re: [PATCH 1/6] dma-buf: remove shared fence staging in reservation object

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 09.08.2018 um 14:08 schrieb Chris Wilson:
> Quoting Christian König (2018-08-09 12:37:08)
> >   void reservation_object_add_shared_fence(struct reservation_object *obj,
> >                                           struct dma_fence *fence)
> >   {
> > -       struct reservation_object_list *old, *fobj = obj->staged;
> > +       struct reservation_object_list *fobj;
> > +       unsigned int i;
> >   
> > -       old = reservation_object_get_list(obj);
> > -       obj->staged = NULL;
> > +       dma_fence_get(fence);
> > +
> > +       fobj = reservation_object_get_list(obj);
> >   
> > -       if (!fobj) {
> > -               BUG_ON(old->shared_count >= old->shared_max);
> > -               reservation_object_add_shared_inplace(obj, old, fence);
> > -       } else
> > -               reservation_object_add_shared_replace(obj, old, fobj, fence);
> > +       preempt_disable();
> > +       write_seqcount_begin(&obj->seq);
> > +
> > +       for (i = 0; i < fobj->shared_count; ++i) {
> > +               struct dma_fence *old_fence;
> > +
> > +               old_fence = rcu_dereference_protected(fobj->shared[i],
> > +                                                     reservation_object_held(obj));
> > +               if (old_fence->context == fence->context ||
> > +                   dma_fence_is_signaled(old_fence)) {
> Are you happy with the possibility that the shared[] may contain two
> fences belonging to the same context? That was a sticking point earlier.

Yeah, that is fixed by now. I've removed the dependency on this in our 
VM handling code quite a while ago.

> > +                       /* memory barrier is added by write_seqcount_begin */
> > +                       RCU_INIT_POINTER(fobj->shared[i], fence);
> > +                       write_seqcount_end(&obj->seq);
> > +                       preempt_enable();
> > +                       dma_fence_put(old_fence);
> You can rearrange this to have a single exit.

Good point, going to rearrange the code.

Christian.

>         for (i = 0; i < fobj->shared_count; ++i) {
>                 struct dma_fence *old_fence;
>
>                 old_fence = rcu_dereference_protected(fobj->shared[i],
>                                                       reservation_object_held(obj));
>                 if (old_fence->context == fence->context ||
>                     dma_fence_is_signaled(old_fence)) {
> 		   	dma_fence_put(old_fence);
> 			goto replace;
> 		}
> 	}
> 	fobj->shared_count++;
> replace:
>         /*
>          * memory barrier is added by write_seqcount_begin,
>          * fobj->shared_count is protected by this lock too
>          */
> 	RCU_INIT_POINTER(fobj->shared[i], fence);
> 	write_seqcount_end(&obj->seq);
> 	preempt_enable();
> }
> -Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx