[libdrm,2/2] amdgpu: don't track handles for non-memory allocations

Submitted by Marek Olšák on Oct. 23, 2018, 7:07 p.m.

Details

Message ID 20181023190733.15251-2-maraeo@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Marek Olšák Oct. 23, 2018, 7:07 p.m.
From: Marek Olšák <marek.olsak@amd.com>

---
 amdgpu/amdgpu_bo.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index 81f8a5f7..00b9b54a 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -91,26 +91,29 @@  drm_public int amdgpu_bo_alloc(amdgpu_device_handle dev,
 	if (r)
 		goto out;
 
 	r = amdgpu_bo_create(dev, alloc_buffer->alloc_size, args.out.handle,
 			     buf_handle);
 	if (r) {
 		amdgpu_close_kms_handle(dev, args.out.handle);
 		goto out;
 	}
 
-	pthread_mutex_lock(&dev->bo_table_mutex);
-	r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
-				*buf_handle);
-	pthread_mutex_unlock(&dev->bo_table_mutex);
-	if (r)
-		amdgpu_bo_free(*buf_handle);
+	if (alloc_buffer->preferred_heap &
+	    (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT)) {
+		pthread_mutex_lock(&dev->bo_table_mutex);
+		r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
+					*buf_handle);
+		pthread_mutex_unlock(&dev->bo_table_mutex);
+		if (r)
+			amdgpu_bo_free(*buf_handle);
+	}
 out:
 	return r;
 }
 
 drm_public int amdgpu_bo_set_metadata(amdgpu_bo_handle bo,
 				      struct amdgpu_bo_metadata *info)
 {
 	struct drm_amdgpu_gem_metadata args = {};
 
 	args.handle = bo->handle;

Comments

On 10/24/18 3:07 AM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>

commit log and sign-off here as well.
And any reason for that?

Regards,
Jerry

>
> ---
>   amdgpu/amdgpu_bo.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index 81f8a5f7..00b9b54a 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -91,26 +91,29 @@ drm_public int amdgpu_bo_alloc(amdgpu_device_handle dev,
>   	if (r)
>   		goto out;
>   
>   	r = amdgpu_bo_create(dev, alloc_buffer->alloc_size, args.out.handle,
>   			     buf_handle);
>   	if (r) {
>   		amdgpu_close_kms_handle(dev, args.out.handle);
>   		goto out;
>   	}
>   
> -	pthread_mutex_lock(&dev->bo_table_mutex);
> -	r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
> -				*buf_handle);
> -	pthread_mutex_unlock(&dev->bo_table_mutex);
> -	if (r)
> -		amdgpu_bo_free(*buf_handle);
> +	if (alloc_buffer->preferred_heap &
> +	    (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT)) {
> +		pthread_mutex_lock(&dev->bo_table_mutex);
> +		r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
> +					*buf_handle);
> +		pthread_mutex_unlock(&dev->bo_table_mutex);
> +		if (r)
> +			amdgpu_bo_free(*buf_handle);
> +	}
>   out:
>   	return r;
>   }
>   
>   drm_public int amdgpu_bo_set_metadata(amdgpu_bo_handle bo,
>   				      struct amdgpu_bo_metadata *info)
>   {
>   	struct drm_amdgpu_gem_metadata args = {};
>   
>   	args.handle = bo->handle;
On 2018-10-23 9:07 p.m., Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> ---
>  amdgpu/amdgpu_bo.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index 81f8a5f7..00b9b54a 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -91,26 +91,29 @@ drm_public int amdgpu_bo_alloc(amdgpu_device_handle dev,
>  	if (r)
>  		goto out;
>  
>  	r = amdgpu_bo_create(dev, alloc_buffer->alloc_size, args.out.handle,
>  			     buf_handle);
>  	if (r) {
>  		amdgpu_close_kms_handle(dev, args.out.handle);
>  		goto out;
>  	}
>  
> -	pthread_mutex_lock(&dev->bo_table_mutex);
> -	r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
> -				*buf_handle);
> -	pthread_mutex_unlock(&dev->bo_table_mutex);
> -	if (r)
> -		amdgpu_bo_free(*buf_handle);
> +	if (alloc_buffer->preferred_heap &
> +	    (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT)) {

What about AMDGPU_GEM_DOMAIN_CPU? I mean, that's unlikely to actually be
used here, but if it were, exporting and importing the resulting BO
should work fine?

Instead of white-listing the domains which can be shared, it might be
better to black-list those which can't, i.e. GDS/GWS/OA.
Am 24.10.18 um 10:04 schrieb Michel Dänzer:
> On 2018-10-23 9:07 p.m., Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> ---
>>   amdgpu/amdgpu_bo.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>> index 81f8a5f7..00b9b54a 100644
>> --- a/amdgpu/amdgpu_bo.c
>> +++ b/amdgpu/amdgpu_bo.c
>> @@ -91,26 +91,29 @@ drm_public int amdgpu_bo_alloc(amdgpu_device_handle dev,
>>   	if (r)
>>   		goto out;
>>   
>>   	r = amdgpu_bo_create(dev, alloc_buffer->alloc_size, args.out.handle,
>>   			     buf_handle);
>>   	if (r) {
>>   		amdgpu_close_kms_handle(dev, args.out.handle);
>>   		goto out;
>>   	}
>>   
>> -	pthread_mutex_lock(&dev->bo_table_mutex);
>> -	r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
>> -				*buf_handle);
>> -	pthread_mutex_unlock(&dev->bo_table_mutex);
>> -	if (r)
>> -		amdgpu_bo_free(*buf_handle);
>> +	if (alloc_buffer->preferred_heap &
>> +	    (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT)) {
> What about AMDGPU_GEM_DOMAIN_CPU? I mean, that's unlikely to actually be
> used here, but if it were, exporting and importing the resulting BO
> should work fine?
>
> Instead of white-listing the domains which can be shared, it might be
> better to black-list those which can't, i.e. GDS/GWS/OA.

Well first of all GDS can be shared between applications.

Then adding a BO to the tracking doesn't add much overhead (only 8 bytes 
and only if it was the last allocated).

So I don't really see a reason why we should do this?

Christian.
OK. I'll drop this patch.

Marek

On Wed, Oct 24, 2018 at 4:14 AM Christian König <
ckoenig.leichtzumerken@gmail.com> wrote:

> Am 24.10.18 um 10:04 schrieb Michel Dänzer:
> > On 2018-10-23 9:07 p.m., Marek Olšák wrote:
> >> From: Marek Olšák <marek.olsak@amd.com>
> >>
> >> ---
> >>   amdgpu/amdgpu_bo.c | 15 +++++++++------
> >>   1 file changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> >> index 81f8a5f7..00b9b54a 100644
> >> --- a/amdgpu/amdgpu_bo.c
> >> +++ b/amdgpu/amdgpu_bo.c
> >> @@ -91,26 +91,29 @@ drm_public int amdgpu_bo_alloc(amdgpu_device_handle
> dev,
> >>      if (r)
> >>              goto out;
> >>
> >>      r = amdgpu_bo_create(dev, alloc_buffer->alloc_size,
> args.out.handle,
> >>                           buf_handle);
> >>      if (r) {
> >>              amdgpu_close_kms_handle(dev, args.out.handle);
> >>              goto out;
> >>      }
> >>
> >> -    pthread_mutex_lock(&dev->bo_table_mutex);
> >> -    r = handle_table_insert(&dev->bo_handles, (*buf_handle)->handle,
> >> -                            *buf_handle);
> >> -    pthread_mutex_unlock(&dev->bo_table_mutex);
> >> -    if (r)
> >> -            amdgpu_bo_free(*buf_handle);
> >> +    if (alloc_buffer->preferred_heap &
> >> +        (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT)) {
> > What about AMDGPU_GEM_DOMAIN_CPU? I mean, that's unlikely to actually be
> > used here, but if it were, exporting and importing the resulting BO
> > should work fine?
> >
> > Instead of white-listing the domains which can be shared, it might be
> > better to black-list those which can't, i.e. GDS/GWS/OA.
>
> Well first of all GDS can be shared between applications.
>
> Then adding a BO to the tracking doesn't add much overhead (only 8 bytes
> and only if it was the last allocated).
>
> So I don't really see a reason why we should do this?
>
> Christian.
>