Re: [PATCH] drm/amdgpu: revert "XGMI pstate switch initial support"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/19/2019 8:49 AM, Christian König wrote:
> Yeah, all that is perfectly fine.
>
> The problem is Shaoyun didn't put this into the mapping code, but 
> rather into the VM state machine. So this won't work at all (the 
> counter and increment/decrement unbalanced and multiple times).

We tried to consider all the possible ways that this could go wrong. 
Basically, every time a mapping is updated, we update the is_xgmi state 
and update the counter if it changed. Have you seen the counter become 
unbalanced?


>
> The correct place to add this is amdgpu_vm_bo_add/amdgpu_vm_bo_rmv.

I think we considered that. The problem is that a BO can be migrated 
between bo_add and bo_rmv. I found that even bo->preferred_domain can 
change with AMDGPU_GEM_OP_SET_PLACEMENT. So you can't reliably know 
whether to increment your counter, and your counter can become 
unbalanced if a migration or AMDGPU_GEM_OP_SET_PLACEMENT happens between 
bo_add and bo_rmv.

Therefore we're trying to check for XGMI mappings every time the mapping 
changes and keep track of the state in amdgpu_bo_va_mapping.


>
> Additional to that the approach with the counter doesn't work because 
> you don't have a lock protecting the hw update itself. E.g. while 
> powering down you can add a mapping which needs to power it up again 
> and so powering down and powering up race with each other.

That's a good point.

Regards,
   Felix


>
> Regards,
> Christian.
>
> Am 19.03.19 um 13:42 schrieb Kuehling, Felix:
>> We discussed a few different approaches before settling on this one.
>>
>> Maybe it needs some more background. XGMI links are quite power hungry.
>> Being able to power them down improves performance for power-limited
>> workloads that don't need XGMI. In machine learning, pretty much all
>> workloads are power limited on our GPUs, so this is not just a
>> theoretical thing. The problem is, how do you know whether you need
>> XGMI? You need to know whether there are P2P memory mappings involving
>> XGMI. So the natural place to put that is in the memory mapping code.
>>
>> If you do spot a race condition in the code, let's talk about how to 
>> fix it.
>>
>> Regards,
>>     Felix
>>
>> On 3/19/2019 8:07 AM, Christian König wrote:
>>> This reverts commit c9115f8904eef0f880d3b4f8306f553b1bb1c532.
>>>
>>> Adding this to the mapping is complete nonsense and the whole
>>> implementation looks racy. This patch wasn't thoughtfully reviewed
>>> and should be reverted for now.
>>>
>>> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  1 -
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 29 
>>> +---------------------
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c   | 15 -----------
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h   |  2 --
>>>    6 files changed, 1 insertion(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index b5720c1610e1..1db192150532 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -931,9 +931,6 @@ struct amdgpu_device {
>>>        int asic_reset_res;
>>>        struct work_struct        xgmi_reset_work;
>>>    -    /* counter of mapped memory through xgmi */
>>> -    atomic_t            xgmi_map_counter;
>>> -
>>>        bool                            in_baco_reset;
>>>    };
>>>    diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 964a4d3f1f43..206583707124 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2018,9 +2018,6 @@ static void 
>>> amdgpu_device_ip_late_init_func_handler(struct work_struct *work)
>>>        r = amdgpu_device_enable_mgpu_fan_boost();
>>>        if (r)
>>>            DRM_ERROR("enable mgpu fan boost failed (%d).\n", r);
>>> -
>>> -    /*set to low pstate by default */
>>> -    amdgpu_xgmi_set_pstate(adev, 0);
>>>    }
>>>       static void amdgpu_device_delay_enable_gfx_off(struct 
>>> work_struct *work)
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> index 6f176bbe4cf2..220a6a7b1bc1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> @@ -54,7 +54,6 @@ struct amdgpu_bo_va_mapping {
>>>        uint64_t            __subtree_last;
>>>        uint64_t            offset;
>>>        uint64_t            flags;
>>> -    bool                is_xgmi;
>>>    };
>>>       /* User space allocated BO in a VM */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index c5230a9fb7f6..c8f0e4ca05fb 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -34,7 +34,6 @@
>>>    #include "amdgpu_trace.h"
>>>    #include "amdgpu_amdkfd.h"
>>>    #include "amdgpu_gmc.h"
>>> -#include "amdgpu_xgmi.h"
>>>       /**
>>>     * DOC: GPUVM
>>> @@ -2022,9 +2021,8 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
>>> *adev,
>>>        struct ttm_mem_reg *mem;
>>>        struct drm_mm_node *nodes;
>>>        struct dma_fence *exclusive, **last_update;
>>> -    struct amdgpu_device *bo_adev = adev;
>>> -    bool is_xgmi = false;
>>>        uint64_t flags;
>>> +    struct amdgpu_device *bo_adev = adev;
>>>        int r;
>>>           if (clear || !bo) {
>>> @@ -2046,10 +2044,6 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
>>> *adev,
>>>        if (bo) {
>>>            flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, mem);
>>>            bo_adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>> -        if (adev != bo_adev &&
>>> -            adev->gmc.xgmi.hive_id &&
>>> -            adev->gmc.xgmi.hive_id == bo_adev->gmc.xgmi.hive_id)
>>> -            is_xgmi = true;
>>>        } else {
>>>            flags = 0x0;
>>>        }
>>> @@ -2068,19 +2062,6 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
>>> *adev,
>>>        }
>>>           list_for_each_entry(mapping, &bo_va->invalids, list) {
>>> -        if (mapping->is_xgmi != is_xgmi) {
>>> -            if (is_xgmi) {
>>> -                /* Adding an XGMI mapping to the PT */
>>> -                if (atomic_inc_return(&adev->xgmi_map_counter) == 1)
>>> -                    amdgpu_xgmi_set_pstate(adev, 1);
>>> -            } else {
>>> -                /* Removing an XGMI mapping from the PT */
>>> -                if (atomic_dec_return(&adev->xgmi_map_counter) == 0)
>>> -                    amdgpu_xgmi_set_pstate(adev, 0);
>>> -            }
>>> -            mapping->is_xgmi = is_xgmi;
>>> -        }
>>> -
>>>            r = amdgpu_vm_bo_split_mapping(adev, exclusive, 
>>> pages_addr, vm,
>>>                               mapping, flags, bo_adev, nodes,
>>>                               last_update);
>>> @@ -2298,13 +2279,6 @@ int amdgpu_vm_clear_freed(struct 
>>> amdgpu_device *adev,
>>>            r = amdgpu_vm_bo_update_mapping(adev, NULL, NULL, vm,
>>>                            mapping->start, mapping->last,
>>>                            init_pte_value, 0, &f);
>>> -
>>> -        if (mapping->is_xgmi) {
>>> -            /* Removing an XGMI mapping from the PT */
>>> -            if (atomic_dec_return(&adev->xgmi_map_counter) == 0)
>>> -                amdgpu_xgmi_set_pstate(adev, 0);
>>> -        }
>>> -
>>>            amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>>            if (r) {
>>>                dma_fence_put(f);
>>> @@ -2501,7 +2475,6 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
>>>        mapping->last = eaddr;
>>>        mapping->offset = offset;
>>>        mapping->flags = flags;
>>> -    mapping->is_xgmi = false;
>>>           amdgpu_vm_bo_insert_map(adev, bo_va, mapping);
>>>    diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>> index 807440d3edff..fcc4b05c745c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>>> @@ -200,7 +200,6 @@ struct amdgpu_hive_info 
>>> *amdgpu_get_xgmi_hive(struct amdgpu_device *adev, int lo
>>>           if (lock)
>>>            mutex_lock(&tmp->hive_lock);
>>> -    tmp->pstate = -1;
>>>           mutex_unlock(&xgmi_mutex);
>>>    @@ -322,17 +321,3 @@ void amdgpu_xgmi_remove_device(struct 
>>> amdgpu_device *adev)
>>>            mutex_unlock(&hive->hive_lock);
>>>        }
>>>    }
>>> -
>>> -int amdgpu_xgmi_set_pstate(struct amdgpu_device *adev, int pstate)
>>> -{
>>> -    int ret = 0;
>>> -    struct amdgpu_hive_info *hive = amdgpu_get_xgmi_hive(adev, 0);
>>> -
>>> -    if (!hive)
>>> -        return 0;
>>> -
>>> -    if (hive->pstate == pstate)
>>> -        return 0;
>>> -    /* Todo : sent the message to SMU for pstate change */
>>> -    return ret;
>>> -}
>>> \ No newline at end of file
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>>> index 7e1710fcbef2..24a3b0362f98 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>>> @@ -33,13 +33,11 @@ struct amdgpu_hive_info {
>>>        struct kobject *kobj;
>>>        struct device_attribute dev_attr;
>>>        struct amdgpu_device *adev;
>>> -    int pstate; /*0 -- low , 1 -- high , -1 unknown*/
>>>    };
>>>       struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct 
>>> amdgpu_device *adev, int lock);
>>>    int amdgpu_xgmi_update_topology(struct amdgpu_hive_info *hive, 
>>> struct amdgpu_device *adev);
>>>    int amdgpu_xgmi_add_device(struct amdgpu_device *adev);
>>>    void amdgpu_xgmi_remove_device(struct amdgpu_device *adev);
>>> -int amdgpu_xgmi_set_pstate(struct amdgpu_device *adev, int pstate);
>>>       #endif
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux