From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Koenig, Christian" Subject: Re: [PATCH] drm/amdgpu: Add preferred_domain check when determine XGMI state Date: Wed, 27 Mar 2019 15:39:09 +0000 Message-ID: <68f7ccfa-646e-492f-bdd7-b24f2e2029da@email.android.com> References: <1553632516-23346-1-git-send-email-shaoyun.liu@amd.com> <5d6e562d-012b-f22b-7ec9-2fdf33a9319d@amd.com> <3e28a7df-4165-4bde-8a2e-926453ceb1d1@email.android.com>, Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1284381917==" Return-path: In-Reply-To: Content-Language: de-DE List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Liu, Shaoyun" Cc: "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============1284381917== Content-Language: de-DE Content-Type: multipart/alternative; boundary="_000_68f7ccfa646e492fbdd7b24f2e2029daemailandroidcom_" --_000_68f7ccfa646e492fbdd7b24f2e2029daemailandroidcom_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable 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 o= ut a different approach. Christian. Am 27.03.2019 16:34 schrieb "Liu, Shaoyun" : you mean struct amdgpu_vm_bo_base ? It only has few pointer and a list_he= ad , 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" : 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=F6nig 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 >> --- >> 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 =3D dev->dev_private; >> struct drm_amdgpu_gem_op *args =3D 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 =3D robj->vm_bo; base; base =3D base->next) { >> + bo_va =3D 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 =3D -EINVAL; >> + amdgpu_bo_unreserve(robj); >> + goto out; >> + } >> + } >> + >> robj->preferred_domains =3D 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 =3D true; >> mutex_lock(&adev->vm_manager.lock_pstate); >> /* Power up XGMI if it can be potentially used */ > --_000_68f7ccfa646e492fbdd7b24f2e2029daemailandroidcom_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
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 w= e need to figure out a different approach.

Christian.

Am 27.03.2019 16:34 schrieb "Liu, Shaoyun&q= uot; <Shaoyun.Liu-5C7GfCeVMHo@public.gmane.org>:

you mean struct amdgpu_vm_bo_base ? It only  has few pointer and&nb= sp; a list_head , should be  pretty small .

struct amdgpu_vm_bo_base {
    /* constant after initialization */
    struct amdgpu_vm        *v= m;
    struct amdgpu_bo        *b= o;

    /* 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          &n= bsp;     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-5C7GfCeVMHo@public.gmane.org>:
The  next in struct amdgpu_vm_bo_base  i= s 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=F6nig 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-5C7GfCeVMHo@public.gmane.org>
>> ---
>>   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 =3D dev-= >dev_private;
>>       struct drm_amdgpu_gem_op *args =3D = 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 =3D robj= ->vm_bo; base; base =3D base->next) {
>> +          &= nbsp; bo_va =3D container_of(base, struct amdgpu_bo_va, base);
>
> Please use list_for_each_entry here instead of open coding it.
>
> Christian.
>
>> +          &= nbsp; if (bo_va && bo_va->is_xgmi) {
>> +          &= nbsp;     r =3D -EINVAL;
>> +          &= nbsp;     amdgpu_bo_unreserve(robj);
>> +          &= nbsp;     goto out;
>> +          &= nbsp; }
>> +        }
>> +
>>           robj->pr= eferred_domains =3D args->value &
>> (AMDGPU_GEM_DOMAIN_VRAM |
>>            =             &nb= sp;      AMDGPU_GEM_DOMAIN_GTT |
>>            =             &nb= sp;      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(st= ruct
>> amdgpu_device *adev,
>>       INIT_LIST_HEAD(&bo_va->valid= s);
>>       INIT_LIST_HEAD(&bo_va->inval= ids);
>>   -    if (bo && amdgpu_xgmi_same_hive= (adev,
>> amdgpu_ttm_adev(bo->tbo.bdev))) {
>> +    if (bo && amdgpu_xgmi_same_hive(ad= ev,
>> amdgpu_ttm_adev(bo->tbo.bdev)) &&
>> +        (bo->preferred_= domains & AMDGPU_GEM_DOMAIN_VRAM)) {
>>           bo_va->i= s_xgmi =3D true;
>>           mutex_lock(= &adev->vm_manager.lock_pstate);
>>           /* Power up= XGMI if it can be potentially used */
>
--_000_68f7ccfa646e492fbdd7b24f2e2029daemailandroidcom_-- --===============1284381917== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4 --===============1284381917==--