Am 27.06.19 um 17:34 schrieb Daniel Vetter:
> On Thu, Jun 27, 2019 at 3:19 PM Christian König
> <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>> Am 27.06.19 um 12:43 schrieb Daniel Vetter:
>>> On Thu, Jun 27, 2019 at 12:18 PM Christian König
>>> <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>>>> While freeing up memory it is easier to remove a fence from a reservation
>>>> object instead of signaling it and installing a new one in all other objects.
>>>>
>>>> Clean this up by adding the removal function to the core reservation object
>>>> code instead of messing with the internal in amdgpu.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
>>>> ---
>>>> drivers/dma-buf/reservation.c | 62 +++++++++++++++++++
>>>> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 45 +-------------
>>>> include/linux/reservation.h | 3 +-
>>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
>>>> index ef710effedfa..e43a316a005d 100644
>>>> --- a/drivers/dma-buf/reservation.c
>>>> +++ b/drivers/dma-buf/reservation.c
>>>> @@ -231,6 +231,68 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
>>>> }
>>>> EXPORT_SYMBOL(reservation_object_add_shared_fence);
>>>>
>>>> +/**
>>>> + * reservation_object_remove_shared_fence - remove shared fences
>>>> + * @obj: the reservation object
>>>> + * @context: the context of the fences to remove
>>>> + *
>>>> + * Remove shared fences based on their fence context.
>>>> + */
>>> This needs a serious explanation of "why?". Removing fences without
>>> guaranteeing they're all signaled is a good way to create havoc. Your
>>> commit message has a few words about what you're doing here, but it
>>> still doesn't explain why this is safe and when exactly it should be
>>> used.
>> Yeah, I'm not very keen about this either.
>>
>> The key point is the memory is not accessible by the hardware any more
>> because it is freed and removed from the page tables.
>>
>> So further access is prevented and in this special case it is actually
>> valid to do this even if the operation represented by the fence is still
>> ongoing.
> Hm, why do you have to remove these fences then? Can't you just let
> them signal and get collected as usual? As soon as you share buffers
> these fences can get anywhere, so you need to correctly unwind them no
> matter what.
>
> You're kinda still just describing what you're doing, not why.
It is simply more efficient to remove the fence from one reservation
object than to add a new fence to all other reservation objects in the
same process.
It's just an optimization,
Christian.
>
>> How should we word this? Something like:
>>
>> * Remove shared fences based on their fence context. Removing a fence
>> before it is signaled is only valid if hardware access is prevented by
>> some other means like IOMMU or similar virtual memory protection.
> The part that freaks me out is whether we break the fence chaing
> anywhere by removing fences. But I guess if you're only removing the
> shared fences that shoudl be fine ...
>
>>> Also I'm not sure (depending upon what you use this for) this is
>>> actually correct outside of amdgpu and it's ignorance of the exclusive
>>> fence.
>> No, that is completely unrelated. But I thought that I clean this up
>> before I start to tackle the exclusive fence issue.
> ... except amdgpu has internally a very strange idea of shared fences
> with auto-exclusive promotion. And I think removing exclusive fences
> will break the fence dependencies of other (earlier) dma drivers by
> other operations. Or is that why you filter on the context,
> essentially amd's way of saying "remove all shared fences for this
> thing, keep only the exclusive ones".
>
> I guess I need to read what this actually does in the code, since I
> still have no idea why you need to do this.
> -Daniel
>
>> Christian.
>>
>>> -Daniel
>>>
>>>> +int reservation_object_remove_shared_fence(struct reservation_object *obj,
>>>> + uint64_t context)
>>>> +{
>>>> + struct reservation_object_list *old, *new;
>>>> + unsigned int i, j, k;
>>>> +
>>>> + reservation_object_assert_held(obj);
>>>> +
>>>> + old = reservation_object_get_list(obj);
>>>> + if (!old)
>>>> + return 0;
>>>> +
>>>> + new = kmalloc(offsetof(typeof(*new), shared[old->shared_max]),
>>>> + GFP_KERNEL);
>>>> + if (!new)
>>>> + return -ENOMEM;
>>>> +
>>>> + /* Go through all the shared fences in the resevation object and sort
>>>> + * the interesting ones to the end of the new list.
>>>> + */
>>>> + for (i = 0, j = old->shared_count, k = 0; i < old->shared_count; ++i) {
>>>> + struct dma_fence *f;
>>>> +
>>>> + f = rcu_dereference_protected(old->shared[i],
>>>> + reservation_object_held(obj));
>>>> +
>>>> + if (f->context == context)
>>>> + RCU_INIT_POINTER(new->shared[--j], f);
>>>> + else
>>>> + RCU_INIT_POINTER(new->shared[k++], f);
>>>> + }
>>>> + new->shared_max = old->shared_max;
>>>> + new->shared_count = k;
>>>> +
>>>> + /* Install the new fence list, seqcount provides the barriers */
>>>> + preempt_disable();
>>>> + write_seqcount_begin(&obj->seq);
>>>> + RCU_INIT_POINTER(obj->fence, new);
>>>> + write_seqcount_end(&obj->seq);
>>>> + preempt_enable();
>>>> +
>>>> + /* Drop the references to the removed fences */
>>>> + for (i = j, k = 0; i < old->shared_count; ++i) {
>>>> + struct dma_fence *f;
>>>> +
>>>> + f = rcu_dereference_protected(new->shared[i],
>>>> + reservation_object_held(obj));
>>>> + dma_fence_put(f);
>>>> + }
>>>> + kfree_rcu(old, rcu);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(reservation_object_remove_shared_fence);
>>>> +
>>>> /**
>>>> * reservation_object_add_excl_fence - Add an exclusive fence.
>>>> * @obj: the reservation object
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> index 10abae398e51..9b25ad3d003e 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> @@ -224,50 +224,7 @@ static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo,
>>>> if (!ef)
>>>> return -EINVAL;
>>>>
>>>> - old = reservation_object_get_list(resv);
>>>> - if (!old)
>>>> - return 0;
>>>> -
>>>> - new = kmalloc(offsetof(typeof(*new), shared[old->shared_max]),
>>>> - GFP_KERNEL);
>>>> - if (!new)
>>>> - return -ENOMEM;
>>>> -
>>>> - /* Go through all the shared fences in the resevation object and sort
>>>> - * the interesting ones to the end of the list.
>>>> - */
>>>> - for (i = 0, j = old->shared_count, k = 0; i < old->shared_count; ++i) {
>>>> - struct dma_fence *f;
>>>> -
>>>> - f = rcu_dereference_protected(old->shared[i],
>>>> - reservation_object_held(resv));
>>>> -
>>>> - if (f->context == ef->base.context)
>>>> - RCU_INIT_POINTER(new->shared[--j], f);
>>>> - else
>>>> - RCU_INIT_POINTER(new->shared[k++], f);
>>>> - }
>>>> - new->shared_max = old->shared_max;
>>>> - new->shared_count = k;
>>>> -
>>>> - /* Install the new fence list, seqcount provides the barriers */
>>>> - preempt_disable();
>>>> - write_seqcount_begin(&resv->seq);
>>>> - RCU_INIT_POINTER(resv->fence, new);
>>>> - write_seqcount_end(&resv->seq);
>>>> - preempt_enable();
>>>> -
>>>> - /* Drop the references to the removed fences or move them to ef_list */
>>>> - for (i = j, k = 0; i < old->shared_count; ++i) {
>>>> - struct dma_fence *f;
>>>> -
>>>> - f = rcu_dereference_protected(new->shared[i],
>>>> - reservation_object_held(resv));
>>>> - dma_fence_put(f);
>>>> - }
>>>> - kfree_rcu(old, rcu);
>>>> -
>>>> - return 0;
>>>> + return reservation_object_remove_shared_fence(resv, ef->base.context);
>>>> }
>>>>
>>>> static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
>>>> diff --git a/include/linux/reservation.h b/include/linux/reservation.h
>>>> index f47e8196d039..1c833a56b678 100644
>>>> --- a/include/linux/reservation.h
>>>> +++ b/include/linux/reservation.h
>>>> @@ -229,7 +229,8 @@ int reservation_object_reserve_shared(struct reservation_object *obj,
>>>> unsigned int num_fences);
>>>> void reservation_object_add_shared_fence(struct reservation_object *obj,
>>>> struct dma_fence *fence);
>>>> -
>>>> +int reservation_object_remove_shared_fence(struct reservation_object *obj,
>>>> + uint64_t context);
>>>> void reservation_object_add_excl_fence(struct reservation_object *obj,
>>>> struct dma_fence *fence);
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
