Am 30.01.2018 um 16:28 schrieb Kasiviswanathan, Harish:
>>> [+Harish, forgot to acknowledge him in the commit description, will fix
>>> that in v2]
>>>
>>> Harish, please see Christian's question below in amd_kfd_fence_signal.
>>> Did I understand this correctly?
> [HK]: Yes the lifetime of eviction fences is tied to the lifetime of the process associated with it. When the process terminates the fence is signaled and released. For all the BOs that belong to this process the eviction should be detached from it when the BO is released. However, this eviction fence could be still attached to shared BOs. So signaling it frees those BOs.
>
>
> On 2018-01-29 08:43 AM, Christian König wrote:
>> Hi Felix & Harish,
>>
>> maybe explain why I found that odd: dma_fence_add_callback() sets the
>> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT flag before adding the callback.
>>
>> So the flag should always be set when there are callbacks.
>> Did I miss anything?
> I don't think we add any callbacks to our eviction fences.
>
> [HK] Setting DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT is not required. It was my oversight. Since, dma_fence_signal() function called cb_list functions only if DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT is set, I thought it was safe to set it. However, the cb_list would be empty if no callbacks are added. So setting DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT is redundant.
Ok in this case let's just remove that and also use the
dma_fence_signal() function (not the _locked variant) for signaling the
DMA fence.
Thanks,
Christian.
>
> Best Regards,
> Harish
>
>
>
> Regards,
> Â Felix
>
>> Regards,
>> Christian.
>>
>> Am 29.01.2018 um 00:55 schrieb Felix Kuehling:
>>> [+Harish, forgot to acknowledge him in the commit description, will fix
>>> that in v2]
>>>
>>> Harish, please see Christian's question below in amd_kfd_fence_signal.
>>> Did I understand this correctly?
>>>
>>> Regards,
>>> Â Â Felix
>>>
>>> On 2018-01-28 06:42 PM, Felix Kuehling wrote:
>>>> On 2018-01-27 04:16 AM, Christian König wrote:
>>>>> Am 27.01.2018 um 02:09 schrieb Felix Kuehling:
>>>> [snip]
>>>>>> +struct amdgpu_amdkfd_fence *amdgpu_amdkfd_fence_create(u64 context,
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â void *mm)
>>>>>> +{
>>>>>> +Â Â Â struct amdgpu_amdkfd_fence *fence = NULL;
>>>>>> +
>>>>>> +Â Â Â fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>>>>>> +Â Â Â if (fence == NULL)
>>>>>> +Â Â Â Â Â Â Â return NULL;
>>>>>> +
>>>>>> +Â Â Â /* mm_struct mm is used as void pointer to identify the parent
>>>>>> +Â Â Â Â * KFD process. Don't dereference it. Fence and any threads
>>>>>> using
>>>>>> +Â Â Â Â * mm is guranteed to be released before process termination.
>>>>>> +Â Â Â Â */
>>>>>> +Â Â Â fence->mm = mm;
>>>>> That won't work. Fences can live much longer than any process who
>>>>> created them.
>>>>>
>>>>> I've already found a fence in a BO still living hours after the
>>>>> process was killed and the pid long recycled.
>>>>>
>>>>> I suggest to make fence->mm a real mm_struct pointer with reference
>>>>> counting and then set it to NULL and drop the reference in
>>>>> enable_signaling.
>>>> I agree. But enable_signaling may be too early to drop the reference.
>>>> amd_kfd_fence_check_mm could still be called later from
>>>> amdgpu_ttm_bo_eviction_valuable, as long as the fence hasn't
>>>> signaled yet.
>>>>
>>>> The safe place is problably in amd_kfd_fence_release.
>>>>
>>>>>> +Â Â Â get_task_comm(fence->timeline_name, current);
>>>>>> +Â Â Â spin_lock_init(&fence->lock);
>>>>>> +
>>>>>> +Â Â Â dma_fence_init(&fence->base, &amd_kfd_fence_ops, &fence->lock,
>>>>>> +Â Â Â Â Â Â Â Â Â Â context, atomic_inc_return(&fence_seq));
>>>>>> +
>>>>>> +Â Â Â return fence;
>>>>>> +}
>>>>>> +
>>>>>> +struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct
>>>>>> dma_fence *f)
>>>>>> +{
>>>>>> +Â Â Â struct amdgpu_amdkfd_fence *fence;
>>>>>> +
>>>>>> +Â Â Â if (!f)
>>>>>> +Â Â Â Â Â Â Â return NULL;
>>>>>> +
>>>>>> +Â Â Â fence = container_of(f, struct amdgpu_amdkfd_fence, base);
>>>>>> +Â Â Â if (fence && f->ops == &amd_kfd_fence_ops)
>>>>>> +Â Â Â Â Â Â Â return fence;
>>>>>> +
>>>>>> +Â Â Â return NULL;
>>>>>> +}
>>>>>> +
>>>>>> +static const char *amd_kfd_fence_get_driver_name(struct dma_fence
>>>>>> *f)
>>>>>> +{
>>>>>> +Â Â Â return "amdgpu_amdkfd_fence";
>>>>>> +}
>>>>>> +
>>>>>> +static const char *amd_kfd_fence_get_timeline_name(struct
>>>>>> dma_fence *f)
>>>>>> +{
>>>>>> +Â Â Â struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
>>>>>> +
>>>>>> +Â Â Â return fence->timeline_name;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * amd_kfd_fence_enable_signaling - This gets called when TTM wants
>>>>>> to evict
>>>>>> + *Â a KFD BO and schedules a job to move the BO.
>>>>>> + *Â If fence is already signaled return true.
>>>>>> + *Â If fence is not signaled schedule a evict KFD process work item.
>>>>>> + */
>>>>>> +static bool amd_kfd_fence_enable_signaling(struct dma_fence *f)
>>>>>> +{
>>>>>> +Â Â Â struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
>>>>>> +
>>>>>> +Â Â Â if (!fence)
>>>>>> +Â Â Â Â Â Â Â return false;
>>>>>> +
>>>>>> +Â Â Â if (dma_fence_is_signaled(f))
>>>>>> +Â Â Â Â Â Â Â return true;
>>>>>> +
>>>>>> +Â Â Â if (!kgd2kfd->schedule_evict_and_restore_process(
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (struct mm_struct *)fence->mm, f))
>>>>>> +Â Â Â Â Â Â Â return true;
>>>>>> +
>>>>>> +Â Â Â return false;
>>>>>> +}
>>>>>> +
>>>>>> +static int amd_kfd_fence_signal(struct dma_fence *f)
>>>>>> +{
>>>>>> +Â Â Â unsigned long flags;
>>>>>> +Â Â Â int ret;
>>>>>> +
>>>>>> +Â Â Â spin_lock_irqsave(f->lock, flags);
>>>>>> +Â Â Â /* Set enabled bit so cb will called */
>>>>>> +Â Â Â set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &f->flags);
>>>>> Mhm, why is that necessary?
>>>> This only gets called from fence_release below. I think this is to
>>>> avoid
>>>> needlessly scheduling an eviction/restore cycle when an eviction fence
>>>> gets destroyed that hasn't been triggered before, probably during
>>>> process termination.
>>>>
>>>> Harish, do you remember any other reason for this?
>>>>
>>>>>> +Â Â Â ret = dma_fence_signal_locked(f);
>>>>>> +Â Â Â spin_unlock_irqrestore(f->lock, flags);
>>>>>> +
>>>>>> +Â Â Â return ret;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * amd_kfd_fence_release - callback that fence can be freed
>>>>>> + *
>>>>>> + * @fence: fence
>>>>>> + *
>>>>>> + * This function is called when the reference count becomes zero.
>>>>>> + * It just RCU schedules freeing up the fence.
>>>>>> + */
>>>>>> +static void amd_kfd_fence_release(struct dma_fence *f)
>>>>>> +{
>>>>>> +Â Â Â struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
>>>>>> +Â Â Â /* Unconditionally signal the fence. The process is getting
>>>>>> +Â Â Â Â * terminated.
>>>>>> +Â Â Â Â */
>>>>>> +Â Â Â if (WARN_ON(!fence))
>>>>>> +Â Â Â Â Â Â Â return; /* Not an amdgpu_amdkfd_fence */
>>>>>> +
>>>>>> +Â Â Â amd_kfd_fence_signal(f);
>>>>>> +Â Â Â kfree_rcu(f, rcu);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * amd_kfd_fence_check_mm - Check if @mm is same as that of the
>>>>>> fence @f
>>>>>> + *Â if same return TRUE else return FALSE.
>>>>>> + *
>>>>>> + * @f: [IN] fence
>>>>>> + * @mm: [IN] mm that needs to be verified
>>>>>> + */
>>>>>> +bool amd_kfd_fence_check_mm(struct dma_fence *f, void *mm)
>>>>>> +{
>>>>>> +Â Â Â struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
>>>>>> +
>>>>>> +Â Â Â if (!fence)
>>>>>> +Â Â Â Â Â Â Â return false;
>>>>>> +Â Â Â else if (fence->mm == mm)
>>>>>> +Â Â Â Â Â Â Â return true;
>>>>>> +
>>>>>> +Â Â Â return false;
>>>>>> +}
>>>>>> +
>>>>>> +const struct dma_fence_ops amd_kfd_fence_ops = {
>>>>>> +Â Â Â .get_driver_name = amd_kfd_fence_get_driver_name,
>>>>>> +Â Â Â .get_timeline_name = amd_kfd_fence_get_timeline_name,
>>>>>> +Â Â Â .enable_signaling = amd_kfd_fence_enable_signaling,
>>>>>> +Â Â Â .signaled = NULL,
>>>>>> +Â Â Â .wait = dma_fence_default_wait,
>>>>>> +Â Â Â .release = amd_kfd_fence_release,
>>>>>> +};
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>> index 65d5a4e..ca00dd2 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>> @@ -36,8 +36,9 @@
>>>>>> Â Â #define AMDGPU_MAX_UVD_ENC_RINGSÂ Â Â 2
>>>>>> Â Â Â /* some special values for the owner field */
>>>>>> -#define AMDGPU_FENCE_OWNER_UNDEFINEDÂ Â Â ((void*)0ul)
>>>>>> -#define AMDGPU_FENCE_OWNER_VMÂ Â Â Â Â Â Â ((void*)1ul)
>>>>>> +#define AMDGPU_FENCE_OWNER_UNDEFINEDÂ Â Â ((void *)0ul)
>>>>>> +#define AMDGPU_FENCE_OWNER_VMÂ Â Â Â Â Â Â ((void *)1ul)
>>>>>> +#define AMDGPU_FENCE_OWNER_KFDÂ Â Â Â Â Â Â ((void *)2ul)
>>>>>> Â Â Â #define AMDGPU_FENCE_FLAG_64BITÂ Â Â Â Â Â Â Â (1 << 0)
>>>>>> Â Â #define AMDGPU_FENCE_FLAG_INTÂ Â Â Â Â Â Â Â Â Â (1 << 1)
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>>>> index df65c66..0cb31d9 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>>>> @@ -31,6 +31,7 @@
>>>>>> Â Â #include <drm/drmP.h>
>>>>>> Â Â #include "amdgpu.h"
>>>>>> Â Â #include "amdgpu_trace.h"
>>>>>> +#include "amdgpu_amdkfd.h"
>>>>>> Â Â Â struct amdgpu_sync_entry {
>>>>>>       struct hlist_node   node;
>>>>>> @@ -86,10 +87,18 @@ static bool amdgpu_sync_same_dev(struct
>>>>>> amdgpu_device *adev,
>>>>>> Â Â static void *amdgpu_sync_get_owner(struct dma_fence *f)
>>>>>> Â Â {
>>>>>> Â Â Â Â Â Â struct drm_sched_fence *s_fence = to_drm_sched_fence(f);
>>>>>> +Â Â Â struct amdgpu_amdkfd_fence *kfd_fence;
>>>>>> +
>>>>>> +Â Â Â if (!f)
>>>>>> +Â Â Â Â Â Â Â return AMDGPU_FENCE_OWNER_UNDEFINED;
>>>>> When you add the extra NULL check here then please move the
>>>>> to_drm_sched_fence() after it as well.
>>>> Yeah, makes sense.
>>>>
>>>> Regards,
>>>> Â Â Felix
>>>>
>>>>> Christian.
>>>>>
>>>>>> Â Â Â Â Â Â Â if (s_fence)
>>>>>> Â Â Â Â Â Â Â Â Â Â return s_fence->owner;
>>>>>> Â Â +Â Â Â kfd_fence = to_amdgpu_amdkfd_fence(f);
>>>>>> +Â Â Â if (kfd_fence)
>>>>>> +Â Â Â Â Â Â Â return AMDGPU_FENCE_OWNER_KFD;
>>>>>> +
>>>>>> Â Â Â Â Â Â return AMDGPU_FENCE_OWNER_UNDEFINED;
>>>>>> Â Â }
>>>>>> Â Â @@ -204,11 +213,18 @@ int amdgpu_sync_resv(struct amdgpu_device
>>>>>> *adev,
>>>>>> Â Â Â Â Â Â for (i = 0; i < flist->shared_count; ++i) {
>>>>>> Â Â Â Â Â Â Â Â Â Â f = rcu_dereference_protected(flist->shared[i],
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â reservation_object_held(resv));
>>>>>> +Â Â Â Â Â Â Â /* We only want to trigger KFD eviction fences on
>>>>>> +Â Â Â Â Â Â Â Â * evict or move jobs. Skip KFD fences otherwise.
>>>>>> +Â Â Â Â Â Â Â Â */
>>>>>> +Â Â Â Â Â Â Â fence_owner = amdgpu_sync_get_owner(f);
>>>>>> +Â Â Â Â Â Â Â if (fence_owner == AMDGPU_FENCE_OWNER_KFD &&
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â owner != AMDGPU_FENCE_OWNER_UNDEFINED)
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â continue;
>>>>>> +
>>>>>> Â Â Â Â Â Â Â Â Â Â if (amdgpu_sync_same_dev(adev, f)) {
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* VM updates are only interesting
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â * for other VM updates and moves.
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â */
>>>>>> -Â Â Â Â Â Â Â Â Â Â Â fence_owner = amdgpu_sync_get_owner(f);
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â if ((owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
>>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ((owner == AMDGPU_FENCE_OWNER_VM) !=
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> index e4bb435..c3f33d3 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>>> @@ -46,6 +46,7 @@
>>>>>> Â Â #include "amdgpu.h"
>>>>>> Â Â #include "amdgpu_object.h"
>>>>>> Â Â #include "amdgpu_trace.h"
>>>>>> +#include "amdgpu_amdkfd.h"
>>>>>> Â Â #include "bif/bif_4_1_d.h"
>>>>>> Â Â Â #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>>>>>> @@ -1170,6 +1171,23 @@ static bool
>>>>>> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>>>>>> Â Â {
>>>>>> Â Â Â Â Â Â unsigned long num_pages = bo->mem.num_pages;
>>>>>> Â Â Â Â Â Â struct drm_mm_node *node = bo->mem.mm_node;
>>>>>> +Â Â Â struct reservation_object_list *flist;
>>>>>> +Â Â Â struct dma_fence *f;
>>>>>> +Â Â Â int i;
>>>>>> +
>>>>>> +Â Â Â /* If bo is a KFD BO, check if the bo belongs to the current
>>>>>> process.
>>>>>> +Â Â Â Â * If true, then return false as any KFD process needs all its
>>>>>> BOs to
>>>>>> +Â Â Â Â * be resident to run successfully
>>>>>> +Â Â Â Â */
>>>>>> +Â Â Â flist = reservation_object_get_list(bo->resv);
>>>>>> +Â Â Â if (flist) {
>>>>>> +Â Â Â Â Â Â Â for (i = 0; i < flist->shared_count; ++i) {
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â f = rcu_dereference_protected(flist->shared[i],
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â reservation_object_held(bo->resv));
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â if (amd_kfd_fence_check_mm(f, current->mm))
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return false;
>>>>>> +Â Â Â Â Â Â Â }
>>>>>> +Â Â Â }
>>>>>> Â Â Â Â Â Â Â switch (bo->mem.mem_type) {
>>>>>> Â Â Â Â Â Â case TTM_PL_TT:
>>>>>> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>>>>> b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>>>>> index 94eab54..9e35249 100644
>>>>>> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>>>>> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>>>>>> @@ -30,6 +30,7 @@
>>>>>> Â Â Â #include <linux/types.h>
>>>>>> Â Â #include <linux/bitmap.h>
>>>>>> +#include <linux/dma-fence.h>
>>>>>> Â Â Â struct pci_dev;
>>>>>> Â Â @@ -286,6 +287,9 @@ struct kfd2kgd_calls {
>>>>>> Â Â Â *
>>>>>> Â Â Â * @resume: Notifies amdkfd about a resume action done to a kgd
>>>>>> device
>>>>>> Â Â Â *
>>>>>> + * @schedule_evict_and_restore_process: Schedules work queue that
>>>>>> will prepare
>>>>>> + * for safe eviction of KFD BOs that belong to the specified
>>>>>> process.
>>>>>> + *
>>>>>> Â Â Â * This structure contains function callback pointers so the kgd
>>>>>> driver
>>>>>> Â Â Â * will notify to the amdkfd about certain status changes.
>>>>>> Â Â Â *
>>>>>> @@ -300,6 +304,8 @@ struct kgd2kfd_calls {
>>>>>> Â Â Â Â Â Â void (*interrupt)(struct kfd_dev *kfd, const void
>>>>>> *ih_ring_entry);
>>>>>> Â Â Â Â Â Â void (*suspend)(struct kfd_dev *kfd);
>>>>>> Â Â Â Â Â Â int (*resume)(struct kfd_dev *kfd);
>>>>>> +Â Â Â int (*schedule_evict_and_restore_process)(struct mm_struct *mm,
>>>>>> +Â Â Â Â Â Â Â Â Â Â Â struct dma_fence *fence);
>>>>>> Â Â };
>>>>>> Â Â Â int kgd2kfd_init(unsigned interface_version,
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
