Re: [PATCH] drm/amdgpu: Add preferred_domain check when determine XGMI state

["Koenig, Christian" <Christian.Koenig@xxxxxxx>]

Yeah, feel free to add my RB.

Christian.

Am 27.03.2019 16:43 schrieb "Liu, Shaoyun" <Shaoyun.Liu@xxxxxxx>:

bool  is_xgmi is added in bo_va structure , this change won't increase the size of   struct amdgpu_vm_bo_base. Anyway , can I said it's reviewed by you now?

shaoyun.liu

On 2019-03-27 11:39 a.m., Koenig, Christian wrote:

Yeah, that one is intentionally very small because it is used so often.

When it grows over the next power of two by this change we need to figure out a different approach.

Christian.

Am 27.03.2019 16:34 schrieb "Liu, Shaoyun" <Shaoyun.Liu@xxxxxxx>:

you mean struct amdgpu_vm_bo_base ? It only  has few pointer and  a list_head , should be  pretty small .

struct amdgpu_vm_bo_base {
    /* constant after initialization */
    struct amdgpu_vm        *vm;
    struct amdgpu_bo        *bo;

    /* protected by bo being reserved */
    struct amdgpu_vm_bo_base    *next;

    /* protected by spinlock */
    struct list_head        vm_status;

    /* protected by the BO being reserved */
    bool                moved;
};

shaoyun.liu

On 2019-03-27 11:30 a.m., Koenig, Christian wrote:

Ah, of course. In this case feel free to go ahead.

 BTW: What's the size of the structure now?

Christian.

Am 27.03.2019 16:22 schrieb "Liu, Shaoyun" <Shaoyun.Liu@xxxxxxx>:

The  next in struct amdgpu_vm_bo_base  is not a  list_head,  I'm not
sure whether we can use list_for_each_entry here.

Regards

shaoyun.liu

On 2019-03-27 3:41 a.m., Christian König wrote:
> Am 26.03.19 um 21:35 schrieb Liu, Shaoyun:
>> Avoid unnecessary XGMI hight pstate trigger when mapping none-vram
>> memory for peer device
>>
>> Change-Id: I1881deff3da19f1f4b58d5765db03a590092a5b2
>> Signed-off-by: shaoyunl <shaoyun.liu@xxxxxxx>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  |  3 ++-
>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index a82c3b1..a0f56e4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -666,6 +666,8 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>> void *data,
>>       struct amdgpu_device *adev = dev->dev_private;
>>       struct drm_amdgpu_gem_op *args = data;
>>       struct drm_gem_object *gobj;
>> +    struct amdgpu_vm_bo_base *base;
>> +    struct amdgpu_bo_va *bo_va;
>>       struct amdgpu_bo *robj;
>>       int r;
>>   @@ -704,6 +706,15 @@ int amdgpu_gem_op_ioctl(struct drm_device
>> *dev, void *data,
>>               amdgpu_bo_unreserve(robj);
>>               break;
>>           }
>> +        for (base = robj->vm_bo; base; base = base->next) {
>> +            bo_va = container_of(base, struct amdgpu_bo_va, base);
>
> Please use list_for_each_entry here instead of open coding it.
>
> Christian.
>
>> +            if (bo_va && bo_va->is_xgmi) {
>> +                r = -EINVAL;
>> +                amdgpu_bo_unreserve(robj);
>> +                goto out;
>> +            }
>> +        }
>> +
>>           robj->preferred_domains = args->value &
>> (AMDGPU_GEM_DOMAIN_VRAM |
>>                               AMDGPU_GEM_DOMAIN_GTT |
>>                               AMDGPU_GEM_DOMAIN_CPU);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 76eee7e..8ed23d2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2048,7 +2048,8 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct
>> amdgpu_device *adev,
>>       INIT_LIST_HEAD(&bo_va->valids);
>>       INIT_LIST_HEAD(&bo_va->invalids);
>>   -    if (bo && amdgpu_xgmi_same_hive(adev,
>> amdgpu_ttm_adev(bo->tbo.bdev))) {
>> +    if (bo && amdgpu_xgmi_same_hive(adev,
>> amdgpu_ttm_adev(bo->tbo.bdev)) &&
>> +        (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM)) {
>>           bo_va->is_xgmi = true;
>>           mutex_lock(&adev->vm_manager.lock_pstate);
>>           /* Power up XGMI if it can be potentially used */
>

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

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