Re: [PATCH v4] drm/amdgpu: Transfer fences to dmabuf importer

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Am 07.08.2018 um 20:14 schrieb Chris Wilson:
> Quoting Christian König (2018-08-07 18:57:16)
>> Am 07.08.2018 um 18:08 schrieb Chris Wilson:
>>> amdgpu only uses shared-fences internally, but dmabuf importers rely on
>>> implicit write hazard tracking via the reservation_object.fence_excl.
>>> For example, the importer use the write hazard for timing a page flip to
>>> only occur after the exporter has finished flushing its write into the
>>> surface. As such, on exporting a dmabuf, we must either flush all
>>> outstanding fences (for we do not know which are writes and should have
>>> been exclusive) or alternatively create a new exclusive fence that is
>>> the composite of all the existing shared fences, and so will only be
>>> signaled when all earlier fences are signaled (ensuring that we can not
>>> be signaled before the completion of any earlier write).
>>>
>>> v2: reservation_object is already locked by amdgpu_bo_reserve()
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
>>> Testcase: igt/amd_prime/amd-to-i915
>>> References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: "Christian König" <christian.koenig@amd.com>
>>> ---
>>>
>>> This time, hopefully proofread and references complete.
>>> -Chris
>>>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
>>>    1 file changed, 60 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> index 1c5d97f4b4dd..dff2b01a3d89 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> @@ -37,6 +37,7 @@
>>>    #include "amdgpu_display.h"
>>>    #include <drm/amdgpu_drm.h>
>>>    #include <linux/dma-buf.h>
>>> +#include <linux/dma-fence-array.h>
>>>    
>>>    static const struct dma_buf_ops amdgpu_dmabuf_ops;
>>>    
>>> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>>>        return ERR_PTR(ret);
>>>    }
>>>    
>>> +static int
>>> +__reservation_object_make_exclusive(struct reservation_object *obj)
>>> +{
>>> +     struct reservation_object_list *fobj;
>>> +     struct dma_fence_array *array;
>>> +     struct dma_fence **fences;
>>> +     unsigned int count, i;
>>> +
>>> +     fobj = reservation_object_get_list(obj);
>>> +     if (!fobj)
>>> +             return 0;
>>> +
>>> +     count = !!rcu_access_pointer(obj->fence_excl);
>>> +     count += fobj->shared_count;
>>> +
>>> +     fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
>>> +     if (!fences)
>>> +             return -ENOMEM;
>>> +
>>> +     for (i = 0; i < fobj->shared_count; i++) {
>>> +             struct dma_fence *f =
>>> +                     rcu_dereference_protected(fobj->shared[i],
>>> +                                               reservation_object_held(obj));
>>> +
>>> +             fences[i] = dma_fence_get(f);
>>> +     }
>>> +
>>> +     if (rcu_access_pointer(obj->fence_excl)) {
>>> +             struct dma_fence *f =
>>> +                     rcu_dereference_protected(obj->fence_excl,
>>> +                                               reservation_object_held(obj));
>>> +
>>> +             fences[i] = dma_fence_get(f);
>>> +     }
>>> +
>>> +     array = dma_fence_array_create(count, fences,
>>> +                                    dma_fence_context_alloc(1), 0,
>>> +                                    false);
>>> +     if (!array)
>>> +             goto err_fences_put;
>>> +
>>> +     reservation_object_add_excl_fence(obj, &array->base);
>>> +     return 0;
>>> +
>>> +err_fences_put:
>>> +     for (i = 0; i < count; i++)
>>> +             dma_fence_put(fences[i]);
>>> +     kfree(fences);
>>> +     return -ENOMEM;
>>> +}
>>> +
>> This can be simplified a lot. See amdgpu_pasid_free_delayed() for an
>> example:
> {
> 	if (!reservation_object_get_list(obj))
> 		return 0;
>
> 	r = reservation_object_get_fences_rcu(obj, NULL, &count, &fences);
> 	if (r)
> 		return r;
>
> 	array = dma_fence_array_create(count, fences,
> 				       dma_fence_context_alloc(1), 0,
> 				       false);
> 	if (!array)
> 		goto err_fences_put;
>
> 	reservation_object_add_excl_fence(obj, &array->base);
> 	return 0;
>
> err:
> 	...
> }
>
> My starting point was going to be use get_fences_rcu, but get_fences_rcu
> can hardly be called simple for where the lock is required to be held
> start to finish ;)

What are you talking about? get_fences_rcu doesn't require any locking 
at all.

You only need to the locking to make sure that between creating the 
fence array and calling reservation_object_add_excl_fence() no other 
fence is added.

Christian.

> -Chris
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx