* [PATCH] drm/amd: Simplify the size check funciton
@ 2023-08-25 5:22 Ma Jun
2023-08-25 8:08 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Ma Jun @ 2023-08-25 5:22 UTC (permalink / raw)
To: amd-gfx, christian.koenig, Alexander.Deucher; +Cc: Ma Jun
Simplify the code logic of size check function amdgpu_bo_validate_size
Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 +++++++++-------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 807ea74ece25..4c95db954a76 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -480,7 +480,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
*cpu_addr = NULL;
}
-/* Validate bo size is bit bigger then the request domain */
+/* Validate bo size is bit bigger than the request domain */
static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
unsigned long size, u32 domain)
{
@@ -490,29 +490,23 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
* If GTT is part of requested domains the check must succeed to
* allow fall back to GTT.
*/
- if (domain & AMDGPU_GEM_DOMAIN_GTT) {
+ if (domain & AMDGPU_GEM_DOMAIN_GTT)
man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
-
- if (man && size < man->size)
- return true;
- else if (!man)
- WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
- goto fail;
- } else if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
+ else if (domain & AMDGPU_GEM_DOMAIN_VRAM)
man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
+ else
+ return true;
- if (man && size < man->size)
- return true;
- goto fail;
+ if (!man) {
+ WARN_ON_ONCE("Mem mananger of mem domain %d is uninitialized", domain);
+ return false;
}
/* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
- return true;
+ if (size < man->size)
+ return true;
-fail:
- if (man)
- DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size,
- man->size);
+ DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size, man->size);
return false;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd: Simplify the size check funciton
2023-08-25 5:22 [PATCH] drm/amd: Simplify the size check funciton Ma Jun
@ 2023-08-25 8:08 ` Christian König
2023-08-28 5:09 ` Ma, Jun
0 siblings, 1 reply; 5+ messages in thread
From: Christian König @ 2023-08-25 8:08 UTC (permalink / raw)
To: Ma Jun, amd-gfx, Alexander.Deucher
Am 25.08.23 um 07:22 schrieb Ma Jun:
> Simplify the code logic of size check function amdgpu_bo_validate_size
>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 +++++++++-------------
> 1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 807ea74ece25..4c95db954a76 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -480,7 +480,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
> *cpu_addr = NULL;
> }
>
> -/* Validate bo size is bit bigger then the request domain */
> +/* Validate bo size is bit bigger than the request domain */
> static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
> unsigned long size, u32 domain)
> {
> @@ -490,29 +490,23 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
> * If GTT is part of requested domains the check must succeed to
> * allow fall back to GTT.
> */
> - if (domain & AMDGPU_GEM_DOMAIN_GTT) {
> + if (domain & AMDGPU_GEM_DOMAIN_GTT)
> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
> -
> - if (man && size < man->size)
> - return true;
> - else if (!man)
> - WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
> - goto fail;
> - } else if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
> + else if (domain & AMDGPU_GEM_DOMAIN_VRAM)
> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
> + else
> + return true;
>
> - if (man && size < man->size)
> - return true;
> - goto fail;
> + if (!man) {
> + WARN_ON_ONCE("Mem mananger of mem domain %d is uninitialized", domain);
> + return false;
> }
That change here is not correct. It's perfectly valid for userspace to
request VRAM even if VRAM isn't initialized.
Only the GTT manager is mandatory. That's why the code previously looked
like it does.
regards,
Christian.
>
> /* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
> - return true;
> + if (size < man->size)
> + return true;
>
> -fail:
> - if (man)
> - DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size,
> - man->size);
> + DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size, man->size);
> return false;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd: Simplify the size check funciton
2023-08-25 8:08 ` Christian König
@ 2023-08-28 5:09 ` Ma, Jun
2023-08-28 6:00 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Ma, Jun @ 2023-08-28 5:09 UTC (permalink / raw)
To: Christian König, Ma Jun, amd-gfx, Alexander.Deucher
Hi Christian,
On 8/25/2023 4:08 PM, Christian König wrote:
>
>
> Am 25.08.23 um 07:22 schrieb Ma Jun:
>> Simplify the code logic of size check function amdgpu_bo_validate_size
>>
>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 +++++++++-------------
>> 1 file changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 807ea74ece25..4c95db954a76 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -480,7 +480,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>> *cpu_addr = NULL;
>> }
>>
>> -/* Validate bo size is bit bigger then the request domain */
>> +/* Validate bo size is bit bigger than the request domain */
>> static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>> unsigned long size, u32 domain)
>> {
>> @@ -490,29 +490,23 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>> * If GTT is part of requested domains the check must succeed to
>> * allow fall back to GTT.
>> */
>> - if (domain & AMDGPU_GEM_DOMAIN_GTT) {
>> + if (domain & AMDGPU_GEM_DOMAIN_GTT)
>> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>> -
>> - if (man && size < man->size)
>> - return true;
>> - else if (!man)
>> - WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
>> - goto fail;
>> - } else if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
>> + else if (domain & AMDGPU_GEM_DOMAIN_VRAM)
>> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
>> + else
>> + return true;
>>
>> - if (man && size < man->size)
>> - return true;
>> - goto fail;
>> + if (!man) {
>> + WARN_ON_ONCE("Mem mananger of mem domain %d is uninitialized", domain);
>> + return false;
>> }
>
> That change here is not correct. It's perfectly valid for userspace to
> request VRAM even if VRAM isn't initialized.
>
> Only the GTT manager is mandatory. That's why the code previously looked
> like it does.
>
Thanks for your explanation.
How about changing the code to the following?
+ if (!man) {
+ if (domain & AMDGPU_GEM_DOMAIN_GTT)
+ WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
+ return false;
}
Regards,
Ma Jun
> regards,
> Christian.
>
>>
>> /* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
>> - return true;
>> + if (size < man->size)
>> + return true;
>>
>> -fail:
>> - if (man)
>> - DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size,
>> - man->size);
>> + DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size, man->size);
>> return false;
>> }
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd: Simplify the size check funciton
2023-08-28 5:09 ` Ma, Jun
@ 2023-08-28 6:00 ` Christian König
2023-08-28 8:33 ` Ma, Jun
0 siblings, 1 reply; 5+ messages in thread
From: Christian König @ 2023-08-28 6:00 UTC (permalink / raw)
To: Ma, Jun, Ma Jun, amd-gfx, Alexander.Deucher
Am 28.08.23 um 07:09 schrieb Ma, Jun:
> Hi Christian,
>
> On 8/25/2023 4:08 PM, Christian König wrote:
>>
>> Am 25.08.23 um 07:22 schrieb Ma Jun:
>>> Simplify the code logic of size check function amdgpu_bo_validate_size
>>>
>>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 +++++++++-------------
>>> 1 file changed, 11 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 807ea74ece25..4c95db954a76 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -480,7 +480,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>>> *cpu_addr = NULL;
>>> }
>>>
>>> -/* Validate bo size is bit bigger then the request domain */
>>> +/* Validate bo size is bit bigger than the request domain */
>>> static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>>> unsigned long size, u32 domain)
>>> {
>>> @@ -490,29 +490,23 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>>> * If GTT is part of requested domains the check must succeed to
>>> * allow fall back to GTT.
>>> */
>>> - if (domain & AMDGPU_GEM_DOMAIN_GTT) {
>>> + if (domain & AMDGPU_GEM_DOMAIN_GTT)
>>> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>>> -
>>> - if (man && size < man->size)
>>> - return true;
>>> - else if (!man)
>>> - WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
>>> - goto fail;
>>> - } else if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
>>> + else if (domain & AMDGPU_GEM_DOMAIN_VRAM)
>>> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
>>> + else
>>> + return true;
>>>
>>> - if (man && size < man->size)
>>> - return true;
>>> - goto fail;
>>> + if (!man) {
>>> + WARN_ON_ONCE("Mem mananger of mem domain %d is uninitialized", domain);
>>> + return false;
>>> }
>> That change here is not correct. It's perfectly valid for userspace to
>> request VRAM even if VRAM isn't initialized.
>>
>> Only the GTT manager is mandatory. That's why the code previously looked
>> like it does.
>>
> Thanks for your explanation.
> How about changing the code to the following?
>
> + if (!man) {
> + if (domain & AMDGPU_GEM_DOMAIN_GTT)
> + WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
> + return false;
> }
Of hand that looks like it should work, but I need to see the full patch.
Regards,
Christian.
>
> Regards,
> Ma Jun
>
>> regards,
>> Christian.
>>
>>>
>>> /* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
>>> - return true;
>>> + if (size < man->size)
>>> + return true;
>>>
>>> -fail:
>>> - if (man)
>>> - DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size,
>>> - man->size);
>>> + DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size, man->size);
>>> return false;
>>> }
>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd: Simplify the size check funciton
2023-08-28 6:00 ` Christian König
@ 2023-08-28 8:33 ` Ma, Jun
0 siblings, 0 replies; 5+ messages in thread
From: Ma, Jun @ 2023-08-28 8:33 UTC (permalink / raw)
To: Christian König, Ma Jun, amd-gfx, Alexander.Deucher
On 8/28/2023 2:00 PM, Christian König wrote:
> Am 28.08.23 um 07:09 schrieb Ma, Jun:
>> Hi Christian,
>>
>> On 8/25/2023 4:08 PM, Christian König wrote:
>>>
>>> Am 25.08.23 um 07:22 schrieb Ma Jun:
>>>> Simplify the code logic of size check function amdgpu_bo_validate_size
>>>>
>>>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 +++++++++-------------
>>>> 1 file changed, 11 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index 807ea74ece25..4c95db954a76 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -480,7 +480,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>>>> *cpu_addr = NULL;
>>>> }
>>>>
>>>> -/* Validate bo size is bit bigger then the request domain */
>>>> +/* Validate bo size is bit bigger than the request domain */
>>>> static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>>>> unsigned long size, u32 domain)
>>>> {
>>>> @@ -490,29 +490,23 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>>>> * If GTT is part of requested domains the check must succeed to
>>>> * allow fall back to GTT.
>>>> */
>>>> - if (domain & AMDGPU_GEM_DOMAIN_GTT) {
>>>> + if (domain & AMDGPU_GEM_DOMAIN_GTT)
>>>> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
>>>> -
>>>> - if (man && size < man->size)
>>>> - return true;
>>>> - else if (!man)
>>>> - WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
>>>> - goto fail;
>>>> - } else if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
>>>> + else if (domain & AMDGPU_GEM_DOMAIN_VRAM)
>>>> man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
>>>> + else
>>>> + return true;
>>>>
>>>> - if (man && size < man->size)
>>>> - return true;
>>>> - goto fail;
>>>> + if (!man) {
>>>> + WARN_ON_ONCE("Mem mananger of mem domain %d is uninitialized", domain);
>>>> + return false;
>>>> }
>>> That change here is not correct. It's perfectly valid for userspace to
>>> request VRAM even if VRAM isn't initialized.
>>>
>>> Only the GTT manager is mandatory. That's why the code previously looked
>>> like it does.
>>>
>> Thanks for your explanation.
>> How about changing the code to the following?
>>
>> + if (!man) {
>> + if (domain & AMDGPU_GEM_DOMAIN_GTT)
>> + WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
>> + return false;
>> }
>
> Of hand that looks like it should work, but I need to see the full patch.
>
Thanks, I'll send v2 with this change.
Regards,
Ma Jun
> Regards,
> Christian.
>
>>
>> Regards,
>> Ma Jun
>>
>>> regards,
>>> Christian.
>>>
>>>>
>>>> /* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
>>>> - return true;
>>>> + if (size < man->size)
>>>> + return true;
>>>>
>>>> -fail:
>>>> - if (man)
>>>> - DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size,
>>>> - man->size);
>>>> + DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size, man->size);
>>>> return false;
>>>> }
>>>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-28 8:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-25 5:22 [PATCH] drm/amd: Simplify the size check funciton Ma Jun
2023-08-25 8:08 ` Christian König
2023-08-28 5:09 ` Ma, Jun
2023-08-28 6:00 ` Christian König
2023-08-28 8:33 ` Ma, Jun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.