amdgpu: add amdgpu_bo_handle_type_kms_noimport
PATCH 1/1 amdgpu: add amdgpu_bo_handle_type_kms_noimport
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)
amdgpu/amdgpu.h +6 -1
@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {
amdgpu/amdgpu_bo.c +4 -0
@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;case amdgpu_bo_handle_type_dma_buf_fd:amdgpu_add_handle_to_table (bo);return drmPrimeHandleToFD (bo->dev->fd, bo->handle,DRM_CLOEXEC |DRM_RDWR , (int *)shared_handle); }return -EINVAL ;
@@ -299,20 +301,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev, bo =util_hash_table_get (dev->bo_flink_names, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo =util_hash_table_get (dev->bo_handles, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:/* Importing a KMS handle in not allowed. */ pthread_mutex_unlock (&dev->bo_table_mutex);return -EPERM ;default :pthread_mutex_unlock (&dev->bo_table_mutex);return -EINVAL ; }if (bo) {
@@ -368,20 +371,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev,util_hash_table_set (dev->bo_flink_names, (void *)(uintptr_t )bo->flink_name, bo);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo->handle = shared_handle; bo->alloc_size = dma_buf_size;break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:assert (0 );/* unreachable */ }/* Initialize it. */ atomic_set (&bo->refcount,1 ); bo->dev = dev;pthread_mutex_init (&bo->cpu_access_mutex,NULL );util_hash_table_set (dev->bo_handles, (void *)(uintptr_t )bo->handle, bo);pthread_mutex_unlock (&dev->bo_table_mutex);
Signature · 6 lines
2.17.1 _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 20180712004750.2024-1-maraeo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Quoted 44 lines · Marek Olšák
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +6
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +0
-0
amdgpu/amdgpu_bo.c int amdgpu_bo_export(amdgpu_bo_handle bo, · line 243
Quoted 10 lines
amdgpu/amdgpu_bo.c +0
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo);
We may reserve below code for type_kms, which is already used by others.
*shared_handle = bo->handle; return 0;
otherwise, it may break somethings.
BTW, it's good to introduce a new type for compatibility. That reminders me to update gbm accordingly.
Jerry
Quoted 47 lines
amdgpu/amdgpu_bo.c +4
-0+/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;case amdgpu_bo_handle_type_dma_buf_fd:amdgpu_add_handle_to_table (bo);return drmPrimeHandleToFD (bo->dev->fd, bo->handle,DRM_CLOEXEC |DRM_RDWR , (int *)shared_handle); }return -EINVAL ;@@ -299,20 +301,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev, bo =util_hash_table_get (dev->bo_flink_names, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo =util_hash_table_get (dev->bo_handles, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:/* Importing a KMS handle in not allowed. */ pthread_mutex_unlock (&dev->bo_table_mutex);return -EPERM ;default :pthread_mutex_unlock (&dev->bo_table_mutex);return -EINVAL ; }if (bo) {@@ -368,20 +371,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev,util_hash_table_set (dev->bo_flink_names, (void *)(uintptr_t )bo->flink_name, bo);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo->handle = shared_handle; bo->alloc_size = dma_buf_size;break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:
amdgpu/amdgpu_bo.c int amdgpu_bo_import(amdgpu_device_handle dev, · line 391
Quoted 10 lines
amdgpu/amdgpu_bo.c +0
-0assert (0 );/* unreachable */ }/* Initialize it. */ atomic_set (&bo->refcount,1 ); bo->dev = dev;pthread_mutex_init (&bo->cpu_access_mutex,NULL );util_hash_table_set (dev->bo_handles, (void *)(uintptr_t )bo->handle, bo);pthread_mutex_unlock (&dev->bo_table_mutex);
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 59 lines · Jerry (Junwei)
Quoted 45 lines · Marek Olšák
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +6
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +0
-0amdgpu/amdgpu_bo.c int amdgpu_bo_export(amdgpu_bo_handle bo, · line 243
Quoted 10 lines
amdgpu/amdgpu_bo.c +0
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo);We may reserve below code for
type_kms, which is already used by others.
Quoted 4 lines
*shared_handle = bo->handle; return 0;otherwise, it may break somethings.
What do you mean?
Quoted 2 lines
BTW, it's good to introduce a new type for compatibility.
Thanks.
Marek
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Zhang, Jerry (Junwei)
Reviewed-by Junwei ZhangQuoted 53 lines · Jerry (Junwei)
Quoted 52 lines · Marek Olšák
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +6
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +0
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;
Quoted 8 lines
amdgpu/amdgpu_bo.c int amdgpu_bo_export(amdgpu_bo_handle bo, · line 243
Quoted 2 lines
amdgpu/amdgpu_bo.c +0
-0case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo);We may reserve below code for
type_kms, which is already used by others.*shared_handle = bo->handle; return 0;otherwise, it may break somethings.
Please ignore above comment, that will be passed through for type_kms as well.
Jerry
Quoted 55 lines
BTW, it's good to introduce a new type for compatibility. That reminders me to update gbm accordingly.
Jerry
Quoted 49 lines
amdgpu/amdgpu_bo.c +4
-0+/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;case amdgpu_bo_handle_type_dma_buf_fd:amdgpu_add_handle_to_table (bo);return drmPrimeHandleToFD (bo->dev->fd, bo->handle,DRM_CLOEXEC |DRM_RDWR , (int *)shared_handle); }return -EINVAL ;@@ -299,20 +301,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev, bo =util_hash_table_get (dev->bo_flink_names, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo =util_hash_table_get (dev->bo_handles, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:/* Importing a KMS handle in not allowed. */ pthread_mutex_unlock (&dev->bo_table_mutex);return -EPERM ;default :pthread_mutex_unlock (&dev->bo_table_mutex);return -EINVAL ; }if (bo) {@@ -368,20 +371,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev,util_hash_table_set (dev->bo_flink_names, (void *)(uintptr_t )bo->flink_name, bo);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo->handle = shared_handle; bo->alloc_size = dma_buf_size;break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:assert (0 );/* unreachable */ }
Quoted 8 lines
amdgpu/amdgpu_bo.c int amdgpu_bo_import(amdgpu_device_handle dev, · line 391
Quoted 8 lines
amdgpu/amdgpu_bo.c +0
-0/* Initialize it. */ atomic_set (&bo->refcount,1 ); bo->dev = dev;pthread_mutex_init (&bo->cpu_access_mutex,NULL );util_hash_table_set (dev->bo_handles, (void *)(uintptr_t )bo->handle, bo);pthread_mutex_unlock (&dev->bo_table_mutex);
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Quoted 1 line · Marek Olšák
From: Marek Olšák <[email protected]>
less patch comment to describe why amdgpu_bo_handle_type_kms doesn't meet requriement and what patch does. less Signed-off-by.
Quoted 18 lines
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +0
-0@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */
amdgpu/amdgpu.h struct drm_amdgpu_info_hw_ip; · line 92
Quoted 10 lines
amdgpu/amdgpu.h +6
-1amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport = 3,
I'm always curious that these enum members are lowercase, could we change them to uppercase by this time?
Thanks,
David Zhou
Quoted 71 lines
amdgpu/amdgpu.h +0
-0};/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +4
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;case amdgpu_bo_handle_type_dma_buf_fd:amdgpu_add_handle_to_table (bo);return drmPrimeHandleToFD (bo->dev->fd, bo->handle,DRM_CLOEXEC |DRM_RDWR , (int *)shared_handle); }return -EINVAL ;@@ -299,20 +301,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev, bo =util_hash_table_get (dev->bo_flink_names, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo =util_hash_table_get (dev->bo_handles, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:/* Importing a KMS handle in not allowed. */ pthread_mutex_unlock (&dev->bo_table_mutex);return -EPERM ;default :pthread_mutex_unlock (&dev->bo_table_mutex);return -EINVAL ; }if (bo) {@@ -368,20 +371,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev,util_hash_table_set (dev->bo_flink_names, (void *)(uintptr_t )bo->flink_name, bo);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo->handle = shared_handle; bo->alloc_size = dma_buf_size;break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:
amdgpu/amdgpu_bo.c int amdgpu_bo_import(amdgpu_device_handle dev, · line 391
Quoted 10 lines
amdgpu/amdgpu_bo.c +0
-0assert (0 );/* unreachable */ }/* Initialize it. */ atomic_set (&bo->refcount,1 ); bo->dev = dev;pthread_mutex_init (&bo->cpu_access_mutex,NULL );util_hash_table_set (dev->bo_handles, (void *)(uintptr_t )bo->handle, bo);pthread_mutex_unlock (&dev->bo_table_mutex);
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): ccb1d49f-ab4f-f561-f328-809ea34edcb0-5C7GfCeVMHo@public.gmane.org
Quoted 8 lines · On Wed, Jul 11, 2018 at 10:09 PM, zhoucm1 <[email protected]> wrote:
Quoted 2 lines · Marek Olšák
From: Marek Olšák <[email protected]>
less patch comment to describe why
amdgpu_bo_handle_type_kmsdoesn't meet requriement and what patch does.
The comment in amdgpu.h describes it well. (at least I hope so)
Quoted 1 line
less Signed-off-by.
Not needed for libdrm as far as I know, but some people add it anyway.
Quoted 22 lines
Quoted 22 lines
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +1
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 ,
Quoted 8 lines
amdgpu/amdgpu.h struct drm_amdgpu_info_hw_ip; · line 92
Quoted 5 lines
amdgpu/amdgpu.h +5
-0+ +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport = 3,I'm always curious that these enum members are lowercase, could we change them to uppercase by this time?
Existing open source projects #including amdgpu.h shouldn't fail compilation with any newer libdrm, which limits us as to what we can change in amdgpu.h.
Marek
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Quoted 48 lines · Marek Olšák
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +6
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +0
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r;
amdgpu/amdgpu_bo.c int amdgpu_bo_export(amdgpu_bo_handle bo, · line 247
Quoted 10 lines
amdgpu/amdgpu_bo.c +2
-0*shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;
What is the rationale for this? I.e. why do you want to not store some handles in the hash table? And how can code using amdgpu_bo_handle_type_kms_noimport be sure that the BO will never be re-imported via dma-buf?
The experience with the previous patch has shown that it's hard to keep track of all possible ways in which BOs are imported, and that if we miss one, this breaks pretty spectacularly.
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 9a8d8f7f-a468-fd5b-dec5-472ce9c88483-otUistvHUpPR7s880joybQ@public.gmane.org
Quoted 53 lines · Michel Dänzer
Quoted 52 lines · Marek Olšák
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +6
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +0
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;
Quoted 9 lines
amdgpu/amdgpu_bo.c int amdgpu_bo_export(amdgpu_bo_handle bo, · line 247
Quoted 6 lines
amdgpu/amdgpu_bo.c +2
-0case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;What is the rationale for this? I.e. why do you want to not store some handles in the hash table?
Because I have the option.
And how can code using
Quoted 3 lines
amdgpu_bo_handle_type_kms_noimportbe sure that the BO will never be re-imported via dma-buf?
That's for the user to decide and prove when it's safe.
Quoted 4 lines
The experience with the previous patch has shown that it's hard to keep track of all possible ways in which BOs are imported, and that if we miss one, this breaks pretty spectacularly.
You are assuming that it will be used incorrectly based on your previous bad experience. All I need to do is not to hand the handle to components that would misuse it.
Marek
Quoted 5 lines
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): CAAxE2A6Vz08sWXiaB6B-F4_P7Fy6uDuJ7aF2gHLXZ4N3zj9sXw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 28 lines · Marek Olšák
Quoted 16 lines · Michel Dänzer
Quoted 15 lines · Marek Olšák
From: Marek Olšák <[email protected]>
amdgpu/amdgpu_bo.c +0
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;Quoted 9 lines
amdgpu/amdgpu_bo.c int amdgpu_bo_export(amdgpu_bo_handle bo, · line 247
Quoted 6 lines
amdgpu/amdgpu_bo.c +2
-0case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;What is the rationale for this? I.e. why do you want to not store some handles in the hash table?
Quoted 1 line
Because I have the option.
Seems like you're expecting this patch to be accepted without providing any real justification for it (here or in the corresponding Mesa patch). NAK from me if so.
Quoted 4 lines
Quoted 2 lines
And how can code using
amdgpu_bo_handle_type_kms_noimportbe sure that the BO will never be re-imported via dma-buf?That's for the user to decide and prove when it's safe.
We shouldn't even have to think about this, let's use the mental capacity for more useful things. :)
I'd rather add the handle to the hash table in amdgpu_bo_alloc, amdgpu_create_bo_from_user_mem and amdgpu_bo_import instead of in amdgpu_bo_export, making amdgpu_bo_export(bo, amdgpu_bo_handle_type_kms, ...) essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): a291b3b5-25bc-385a-5242-9bd75ec423e9-otUistvHUpPR7s880joybQ@public.gmane.org
Quoted 29 lines · Michel Dänzer
Quoted 28 lines · Marek Olšák
Quoted 25 lines · Michel Dänzer
Quoted 11 lines · Marek Olšák
From: Marek Olšák <[email protected]>
amdgpu/amdgpu_bo.c +0
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r;amdgpu/amdgpu_bo.c int amdgpu_bo_export(amdgpu_bo_handle bo, · line 247
Quoted 10 lines
amdgpu/amdgpu_bo.c +2
-0*shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;What is the rationale for this? I.e. why do you want to not store some handles in the hash table?
Quoted 5 lines
Quoted 1 line
Because I have the option.
Seems like you're expecting this patch to be accepted without providing any real justification for it (here or in the corresponding Mesa patch). NAK from me if so.
The real justification is implied by the patch. See: amdgpu_add_handle_to_table Like I said: There is no risk of regression and it simplifies one simple case trivially. We shouldn't have to even talk about it.
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
And how can code using
amdgpu_bo_handle_type_kms_noimportbe sure that the BO will never be re-imported via dma-buf?That's for the user to decide and prove when it's safe.
We shouldn't even have to think about this, let's use the mental capacity for more useful things. :)
Mental capacity spent to write the patch: 15 seconds Mental capacity spent for bike-shedding: Minutes? Tens of minutes?
Quoted 7 lines
I'd rather add the handle to the hash table in
amdgpu_bo_alloc,amdgpu_create_bo_from_user_memandamdgpu_bo_importinstead of inamdgpu_bo_export, makingamdgpu_bo_export(bo, amdgpu_bo_handle_type_kms, ...)essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.
The hash table isn't very good for high BO counts. The time complexity of a lookup is O(n).
Marek
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): CAAxE2A7ffmO=Tv_DSWhPciF+y5q_wbaBYgcqHvxQVkeVfhBhdg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 10 lines · Marek Olšák
Quoted 9 lines · Michel Dänzer
Quoted 7 lines · Marek Olšák
Quoted 3 lines · Michel Dänzer
What is the rationale for this? I.e. why do you want to not store some handles in the hash table?
Because I have the option.
Quoted 7 lines
Quoted 3 lines
Seems like you're expecting this patch to be accepted without providing any real justification for it (here or in the corresponding Mesa patch). NAK from me if so.
The real justification is implied by the patch. See:
amdgpu_add_handle_to_tableLike I said: There is no risk of regression and it simplifies one simple case trivially. We shouldn't have to even talk about it.
IMO you haven't provided enough justification for adding API which is prone to breakage if used incorrectly.
Other opinions?
Quoted 9 lines
Quoted 6 lines
I'd rather add the handle to the hash table in
amdgpu_bo_alloc,amdgpu_create_bo_from_user_memandamdgpu_bo_importinstead of inamdgpu_bo_export, makingamdgpu_bo_export(bo, amdgpu_bo_handle_type_kms, ...)essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.The hash table isn't very good for high BO counts. The time complexity of a lookup is O(n).
A lookup is only needed in amdgpu_bo_import. amdgpu_bo_alloc and amdgpu_create_bo_from_user_mem can just add the handle to the hash bucket directly.
Do you know of, or can you imagine, any workload where amdgpu_bo_import is called often enough for this to be a concern?
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 31980a46-1d16-8ead-cc2b-5a5b9eb4d530-otUistvHUpPR7s880joybQ@public.gmane.org
Quoted 35 lines · Michel Dänzer
Quoted 10 lines · Marek Olšák
Quoted 9 lines · Michel Dänzer
Quoted 7 lines · Marek Olšák
Quoted 3 lines · Michel Dänzer
What is the rationale for this? I.e. why do you want to not store some handles in the hash table?
Because I have the option.
Quoted 7 lines
Quoted 3 lines
Seems like you're expecting this patch to be accepted without providing any real justification for it (here or in the corresponding Mesa patch). NAK from me if so.
The real justification is implied by the patch. See:
amdgpu_add_handle_to_tableLike I said: There is no risk of regression and it simplifies one simple case trivially. We shouldn't have to even talk about it.IMO you haven't provided enough justification for adding API which is prone to breakage if used incorrectly.
Other opinions?
Quoted 9 lines
Quoted 6 lines
I'd rather add the handle to the hash table in
amdgpu_bo_alloc,amdgpu_create_bo_from_user_memandamdgpu_bo_importinstead of inamdgpu_bo_export, makingamdgpu_bo_export(bo, amdgpu_bo_handle_type_kms, ...)essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.The hash table isn't very good for high BO counts. The time complexity of a lookup is O(n).
Quoted 6 lines
A lookup is only needed in
amdgpu_bo_import.amdgpu_bo_allocandamdgpu_create_bo_from_user_memcan just add the handle to the hash bucket directly.Do you know of, or can you imagine, any workload where
amdgpu_bo_importis called often enough for this to be a concern?
Fullscreen DRI2 or DRI3 re-imports buffers every frame. It might show up in a profiler.
Marek
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): CAAxE2A5SUypT+huhKKdSAWXZ8x+wgPSy2FGqLRTj_io+L1qLqA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 14 lines · Marek Olšák
Quoted 13 lines · Michel Dänzer
Quoted 11 lines · Marek Olšák
Quoted 6 lines · Michel Dänzer
I'd rather add the handle to the hash table in
amdgpu_bo_alloc,amdgpu_create_bo_from_user_memandamdgpu_bo_importinstead of inamdgpu_bo_export, makingamdgpu_bo_export(bo, amdgpu_bo_handle_type_kms, ...)essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.The hash table isn't very good for high BO counts. The time complexity of a lookup is O(n).
Quoted 8 lines
Quoted 6 lines
A lookup is only needed in
amdgpu_bo_import.amdgpu_bo_allocandamdgpu_create_bo_from_user_memcan just add the handle to the hash bucket directly.Do you know of, or can you imagine, any workload where
amdgpu_bo_importis called often enough for this to be a concern?Fullscreen DRI2 or DRI3 re-imports buffers every frame.
DRI3 doesn't. The X server only imports each DRI3 buffer once, after that it's referred to via the pixmap XID.
With DRI2 page flipping (ignoring that basically nobody's using that anymore with radeonsi :), it's always the same set of buffers, so the lookup can be made fast as discussed in the sub-thread with Christian. (Also, DRI2 can only use page flipping with sync-to-vblank enabled, so this happens on the order of hundreds of times per second max)
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 97d0f539-1a98-f9fc-1ed5-5de7c5a8c3d0-otUistvHUpPR7s880joybQ@public.gmane.org
Michel, I think you are wasting your time. This change can be misused as easily as any other API. It's not more dangerous that any other amdgpu libdrm function. You won't achieve anything by optimizing the hash table (= losing time), and you also won't achieve anything by NAKing this (= losing performance on the lookup). Both are lose-lose solutions, because you'll lose and others will lose too.
Marek
Quoted 35 lines · Michel Dänzer
Quoted 14 lines · Marek Olšák
Quoted 13 lines · Michel Dänzer
Quoted 11 lines · Marek Olšák
Quoted 6 lines · Michel Dänzer
I'd rather add the handle to the hash table in
amdgpu_bo_alloc,amdgpu_create_bo_from_user_memandamdgpu_bo_importinstead of inamdgpu_bo_export, makingamdgpu_bo_export(bo, amdgpu_bo_handle_type_kms, ...)essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.The hash table isn't very good for high BO counts. The time complexity of a lookup is O(n).
Quoted 8 lines
Quoted 6 lines
A lookup is only needed in
amdgpu_bo_import.amdgpu_bo_allocandamdgpu_create_bo_from_user_memcan just add the handle to the hash bucket directly.Do you know of, or can you imagine, any workload where
amdgpu_bo_importis called often enough for this to be a concern?Fullscreen DRI2 or DRI3 re-imports buffers every frame.
DRI3 doesn't. The X server only imports each DRI3 buffer once, after that it's referred to via the pixmap XID.
With DRI2 page flipping (ignoring that basically nobody's using that anymore with radeonsi :), it's always the same set of buffers, so the lookup can be made fast as discussed in the sub-thread with Christian. (Also, DRI2 can only use page flipping with sync-to-vblank enabled, so this happens on the order of hundreds of times per second max)
Quoted 3 lines
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): CAAxE2A5ij49A-62_wqD6923nn1w6gT6iY-jeC2HUP3ksYacZQQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 3 lines · Marek Olšák
Michel, I think you are wasting your time. This change can be misused as easily as any other API. It's not more dangerous that any other amdgpu libdrm function.
That's trivially false.
Quoted 2 lines
You won't achieve anything by optimizing the hash table (= losing time), [...]
I think you're focusing too much on your immediate desire instead of the big(ger) picture.
E.g. I see amdgpu_bo_export getting called from surprising places (in Xorg), performing a hash table lookup each time. Fixing that would achieve something, though probably not much.
Anyway, adding dangerous API (keep in mind that we don't control all libdrm_amdgpu users, or even know how they're using it) for something that can also be achieved without is just a bad idea. Avoiding that is achievement enough.
This is my opinion. I don't have veto power over libdrm_amdgpu. At least Christian seems in favour of this change.
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): b2335e6f-0553-a3a8-0157-f4dc99ac9cea-otUistvHUpPR7s880joybQ@public.gmane.org
Quoted 10 lines · Michel Dänzer
Quoted 3 lines · Marek Olšák
Michel, I think you are wasting your time. This change can be misused as easily as any other API. It's not more dangerous that any other amdgpu libdrm function.
That's trivially false.
Quoted 2 lines
You won't achieve anything by optimizing the hash table (= losing time), [...]
Quoted 6 lines
I think you're focusing too much on your immediate desire instead of the big(ger) picture.
E.g. I see
amdgpu_bo_exportgetting called from surprising places (in Xorg), performing a hash table lookup each time. Fixing that would achieve something, though probably not much.
I know about the use in Xorg and this patch actually indirectly mentions it (it mentions Glamor in the code). The flag contains _noimport to self-document itself to mitigate incorrect usage.
Quoted 5 lines
Anyway, adding dangerous API (keep in mind that we don't control all
libdrm_amdgpuusers, or even know how they're using it) for something that can also be achieved without is just a bad idea. Avoiding that is achievement enough.
We don't need to control other libdrm users. They can control themselves. :) I'm totally fine with incorrect usage leading to bad things, like any other bug. Much worse things can be done with the CS ioctl.
Marek
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Am 16.07.2018 um 18:05 schrieb Michel Dänzer:
Quoted 11 lines · Marek Olšák
Quoted 10 lines · Michel Dänzer
Quoted 9 lines · Marek Olšák
Quoted 5 lines · Michel Dänzer
Quoted 2 lines
What is the rationale for this? I.e. why do you want to not store some handles in the hash table?
Because I have the option.
Seems like you're expecting this patch to be accepted without providing any real justification for it (here or in the corresponding Mesa patch). NAK from me if so.
Quoted 7 lines
Quoted 3 lines
The real justification is implied by the patch. See:
amdgpu_add_handle_to_tableLike I said: There is no risk of regression and it simplifies one simple case trivially. We shouldn't have to even talk about it.IMO you haven't provided enough justification for adding API which is prone to breakage if used incorrectly.
Other opinions?
I understand the reason why Marek wants to do this, but I agree that this is a little bit dangerous if used incorrectly.
On the other hand I don't see any other way to sanely handle it either.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
I'd rather add the handle to the hash table in
amdgpu_bo_alloc,amdgpu_create_bo_from_user_memandamdgpu_bo_importinstead of inamdgpu_bo_export, makingamdgpu_bo_export(bo, amdgpu_bo_handle_type_kms, ...)essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.
Quoted 8 lines
Quoted 2 lines
The hash table isn't very good for high BO counts. The time complexity of a lookup is O(n).
A lookup is only needed in
amdgpu_bo_import.amdgpu_bo_allocandamdgpu_create_bo_from_user_memcan just add the handle to the hash bucket directly.Do you know of, or can you imagine, any workload where
amdgpu_bo_importis called often enough for this to be a concern?
MM interop with GFX usually imports BOs on each frame, so that can hurt here.
Christian.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 5374dffa-45c9-1bd6-79b9-cc4450a96aeb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Quoted 23 lines · Christian König
Am 16.07.2018 um 18:05 schrieb Michel Dänzer:
Quoted 22 lines · Marek Olšák
Quoted 10 lines
Quoted 8 lines · Michel Dänzer
Quoted 7 lines · Marek Olšák
Quoted 3 lines · Michel Dänzer
What is the rationale for this? I.e. why do you want to not store some
handles in the hash table?Because I have the option.
Quoted 8 lines
Quoted 4 lines
Seems like you're expecting this patch to be accepted without providing any real justification for it (here or in the corresponding Mesa patch).
NAK from me if so.The real justification is implied by the patch. See:
amdgpu_add_handle_to_table
Like I said: There is no risk of regression and it simplifies one simple case trivially. We shouldn't have to even talk about it.IMO you haven't provided enough justification for adding API which is prone to breakage if used incorrectly.
Quoted 6 lines
Quoted 1 line
Other opinions?
I understand the reason why Marek wants to do this, but I agree that this is a little bit dangerous if used incorrectly.
On the other hand I don't see any other way to sanely handle it either.
Sanely handle what exactly? :) I still haven't seen any description of an actual problem, other than "the handle is stored in the hash table".
Quoted 13 lines
Quoted 13 lines
Quoted 9 lines
Quoted 7 lines
I'd rather add the handle to the hash table in
amdgpu_bo_alloc,amdgpu_create_bo_from_user_memandamdgpu_bo_importinstead of inamdgpu_bo_export, making amdgpu_bo_export(bo,amdgpu_bo_handle_type_kms,
...) essentially free. In the unlikely (since allocating a BO from the kernel is expensive) case that the hash table shows up on profiles, we can optimize it.The hash table isn't very good for high BO counts. The time complexity of a lookup is O(n).
A lookup is only needed in
amdgpu_bo_import.amdgpu_bo_allocandamdgpu_create_bo_from_user_memcan just add the handle to the hash bucket directly.
Quoted 5 lines
Quoted 2 lines
Do you know of, or can you imagine, any workload where
amdgpu_bo_importis called often enough for this to be a concern?MM interop with GFX usually imports BOs on each frame, so that can hurt here.
That's always the same set of BOs in the steady state, right? So it's easy to make the repeated lookups fast, by moving them to the start of their hash buckets.
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 126a68a9-43f4-c747-89d1-114dc29ea4e9-otUistvHUpPR7s880joybQ@public.gmane.org
Am 17.07.2018 um 09:26 schrieb Michel Dänzer:
Quoted 11 lines · Christian König
Quoted 8 lines
Am 16.07.2018 um 18:05 schrieb Michel Dänzer:
Quoted 3 lines · Marek Olšák
[SNIP]
Other opinions?I understand the reason why Marek wants to do this, but I agree that this is a little bit dangerous if used incorrectly.
On the other hand I don't see any other way to sanely handle it either.
Sanely handle what exactly? :) I still haven't seen any description of an actual problem, other than "the handle is stored in the hash table".
Well the problem is that it's not "the handle" but rather "all handles" which are now stored in the hash table.
To begin with that is quite a bunch of wasted memory, not talking about the extra CPU cycles.
Quoted 5 lines
Quoted 2 lines
MM interop with GFX usually imports BOs on each frame, so that can hurt here.
That's always the same set of BOs in the steady state, right? So it's easy to make the repeated lookups fast, by moving them to the start of their hash buckets.
Yeah, that can help with the CPU cycles but it is still not ideal.
I think that Mareks change is justified, but we should add a comment explaining the restrictions.
Christian.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 3cc456c1-929d-4aee-2e8b-7d4edb023382-5C7GfCeVMHo@public.gmane.org
Quoted 10 lines · Christian König
Am 17.07.2018 um 09:26 schrieb Michel Dänzer:
Quoted 9 lines · Christian König
Quoted 8 lines
Am 16.07.2018 um 18:05 schrieb Michel Dänzer:
Quoted 3 lines · Marek Olšák
[SNIP]
Other opinions?I understand the reason why Marek wants to do this, but I agree that this is a little bit dangerous if used incorrectly.
On the other hand I don't see any other way to sanely handle it either.
Quoted 8 lines
Quoted 2 lines
Sanely handle what exactly? :) I still haven't seen any description of an actual problem, other than "the handle is stored in the hash table".
Well the problem is that it's not "the handle" but rather "all handles" which are now stored in the hash table.
To begin with that is quite a bunch of wasted memory, not talking about the extra CPU cycles.
All that should be needed is one struct list_head per BO, 16 bytes on 64-bit.
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 0f28de35-1928-2af3-8b8d-13c61b25e285-otUistvHUpPR7s880joybQ@public.gmane.org
Am 17.07.2018 um 09:46 schrieb Michel Dänzer:
Quoted 13 lines · Christian König
Quoted 12 lines
Am 17.07.2018 um 09:26 schrieb Michel Dänzer:
Quoted 11 lines · Christian König
Quoted 8 lines
Am 16.07.2018 um 18:05 schrieb Michel Dänzer:
Quoted 3 lines · Marek Olšák
[SNIP]
Other opinions?I understand the reason why Marek wants to do this, but I agree that this is a little bit dangerous if used incorrectly.
On the other hand I don't see any other way to sanely handle it either.
Sanely handle what exactly? :) I still haven't seen any description of an actual problem, other than "the handle is stored in the hash table".
Quoted 7 lines
Quoted 5 lines
Well the problem is that it's not "the handle" but rather "all handles" which are now stored in the hash table.
To begin with that is quite a bunch of wasted memory, not talking about the extra CPU cycles.
All that should be needed is one struct
list_headper BO, 16 bytes on 64-bit.
+malloc overhead and that for *every* BO the application/driver allocated. The last time I looked we could easily have a few thousands of that (but not in the same CS).
So I would guess that the wasted memory can easily be in the lower kb range, compared to adding just a flag that we never going to import the handle again.
Christian.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 45bab640-d574-b822-e5c8-69fe67626a9a-5C7GfCeVMHo@public.gmane.org
Quoted 18 lines · Christian König
Am 17.07.2018 um 09:46 schrieb Michel Dänzer:
Quoted 17 lines · Christian König
Quoted 16 lines
Am 17.07.2018 um 09:26 schrieb Michel Dänzer:
Quoted 5 lines · Christian König
Quoted 4 lines
Am 16.07.2018 um 18:05 schrieb Michel Dänzer:
Quoted 3 lines · Marek Olšák
[SNIP]
Other opinions?Quoted 7 lines
Quoted 5 lines
I understand the reason why Marek wants to do this, but I agree that this is a little bit dangerous if used incorrectly.
On the other hand I don't see any other way to sanely handle it either.
Sanely handle what exactly? :) I still haven't seen any description of an actual problem, other than "the handle is stored in the hash table".
Well the problem is that it's not "the handle" but rather "all handles" which are now stored in the hash table.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
To begin with that is quite a bunch of wasted memory, not talking about the extra CPU cycles.
All that should be needed is one struct
list_headper BO, 16 bytes on 64-bit.+malloc overhead and that for *every* BO the application/driver allocated.
The struct list_head can be stored in struct amdgpu_bo, no additional malloc necessary.
Quoted 6 lines
The last time I looked we could easily have a few thousands of that (but not in the same CS).
So I would guess that the wasted memory can easily be in the lower kb range, compared to adding just a flag that we never going to import the handle again.
I wouldn't call the memory "wasted", as it serves a clear purpose.
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 988daf71-51ef-65e7-48ed-9dc2f3acb3a1-otUistvHUpPR7s880joybQ@public.gmane.org
Am 17.07.2018 um 10:03 schrieb Michel Dänzer:
Quoted 11 lines · Christian König
Quoted 8 lines
Am 17.07.2018 um 09:46 schrieb Michel Dänzer:
Quoted 5 lines · Christian König
Quoted 2 lines
Am 17.07.2018 um 09:26 schrieb Michel Dänzer:
[SNIP]All that should be needed is one struct
list_headper BO, 16 bytes on 64-bit.+malloc overhead and that for *every* BO the application/driver allocated.
The struct
list_headcan be stored in structamdgpu_bo, no additional malloc necessary.
Well that sounds we are not talking about the same code, do we?
IIRC the hashtable implementation in libdrm is using an ever growing array for the BOs and *NOT* a linked list.
So we have at least two mallocs involved here, the one for the key/value pair and the one for the node array.
Regards,
Christian.
Quoted 11 lines
Quoted 8 lines
The last time I looked we could easily have a few thousands of that (but not in the same CS).
So I would guess that the wasted memory can easily be in the lower kb range, compared to adding just a flag that we never going to import the handle again.
I wouldn't call the memory "wasted", as it serves a clear purpose.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): a5be38fa-4da6-642f-b297-21261820a908-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Quoted 10 lines · Christian König
Am 17.07.2018 um 10:03 schrieb Michel Dänzer:
Quoted 9 lines · Christian König
Quoted 8 lines
Am 17.07.2018 um 09:46 schrieb Michel Dänzer:
Quoted 5 lines · Christian König
Quoted 2 lines
Am 17.07.2018 um 09:26 schrieb Michel Dänzer:
[SNIP]All that should be needed is one struct
list_headper BO, 16 bytes on 64-bit.+malloc overhead and that for *every* BO the application/driver allocated.
Quoted 7 lines
Quoted 2 lines
The struct
list_headcan be stored in structamdgpu_bo, no additional malloc necessary.Well that sounds we are not talking about the same code, do we?
IIRC the hashtable implementation in libdrm is using an ever growing array for the BOs and *NOT* a linked list.
So let's use something more suitable, e.g.:
An array of 2^n struct list_head in struct amdgpu_device for the hash buckets. The BO's handle is hashed to the bucket number
handle & (2^n - 1)
and linked in there via struct list_head in struct amdgpu_bo. amdgpu_bo_alloc and amdgpu_create_bo_from_user_mem add the handle at the end of the list, amdgpu_bo_import adds it at or moves it to the beginning.
Signature · 6 lines
Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): 9a3c0fb3-c6a8-d59f-89cf-ec32ff6ed630-otUistvHUpPR7s880joybQ@public.gmane.org
Am 17.07.2018 um 10:30 schrieb Michel Dänzer:
Quoted 19 lines · Christian König
Quoted 8 lines
Am 17.07.2018 um 10:03 schrieb Michel Dänzer:
Quoted 7 lines · Christian König
Quoted 6 lines
Am 17.07.2018 um 09:46 schrieb Michel Dänzer:
Quoted 5 lines · Christian König
Quoted 2 lines
Am 17.07.2018 um 09:26 schrieb Michel Dänzer:
[SNIP]All that should be needed is one struct
list_headper BO, 16 bytes on 64-bit.Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
+malloc overhead and that for *every* BO the application/driver allocated.
The struct
list_headcan be stored in structamdgpu_bo, no additional malloc necessary.Well that sounds we are not talking about the same code, do we?
IIRC the hashtable implementation in libdrm is using an ever growing array for the BOs and *NOT* a linked list.
So let's use something more suitable, e.g.:
Quoted 8 lines
An array of 2^n struct
list_headin structamdgpu_devicefor the hash buckets. The BO's handle is hashed to the bucket numberhandle & (2^n - 1)
and linked in there via struct
list_headin structamdgpu_bo.amdgpu_bo_allocandamdgpu_create_bo_from_user_memadd the handle at the end of the list,amdgpu_bo_importadds it at or moves it to the beginning.
Yeah, that would certainly reduce the problem quite a bit and would allow us to get rid of the util_hash* implementation which to me always seemed to be a bit overkill.
I actually don't see a reason why amdgpu_create_bo_from_user_mem() should add the handle at all, those BOs are not exportable.
Christian.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Christian,
Would you please give me an Rb if the patch is OK with you? I have spoken with Michel and he would be OK with me pushing it as long as it gets an Rb from either you or Alex.
Thanks,
Marek
Quoted 111 lines · Marek Olšák
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +6
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +4
-1@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;case amdgpu_bo_handle_type_dma_buf_fd:amdgpu_add_handle_to_table (bo);return drmPrimeHandleToFD (bo->dev->fd, bo->handle,DRM_CLOEXEC |DRM_RDWR , (int *)shared_handle); }return -EINVAL ;@@ -299,20 +301,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev, bo =util_hash_table_get (dev->bo_flink_names, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo =util_hash_table_get (dev->bo_handles, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:/* Importing a KMS handle in not allowed. */ pthread_mutex_unlock (&dev->bo_table_mutex);return -EPERM ;default :pthread_mutex_unlock (&dev->bo_table_mutex);return -EINVAL ; }if (bo) {@@ -368,20 +371,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev,util_hash_table_set (dev->bo_flink_names, (void *)(uintptr_t )bo->flink_name, bo);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo->handle = shared_handle; bo->alloc_size = dma_buf_size;break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:assert (0 );/* unreachable */ }/* Initialize it. */ atomic_set (&bo->refcount,1 ); bo->dev = dev;pthread_mutex_init (&bo->cpu_access_mutex,NULL );util_hash_table_set (dev->bo_handles, (void *)(uintptr_t )bo->handle, bo);pthread_mutex_unlock (&dev->bo_table_mutex); --
Quoted 2 lines
2.17.1
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
A message that is not in the archive (sent off-list or not yet received): CAAxE2A54HMsqwyGhTn_0kt483qKd5G5UFuZD1-X+JWDZYn-5fg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Patch is Reviewed-by: Christian König <[email protected]>
Regards,
Christian.
Am 24.07.2018 um 20:11 schrieb Marek Olšák:
Quoted 118 lines
Christian,
Would you please give me an Rb if the patch is OK with you? I have spoken with Michel and he would be OK with me pushing it as long as it gets an Rb from either you or Alex.
Thanks,
MarekQuoted 108 lines · Marek Olšák
From: Marek Olšák <[email protected]>
--- amdgpu/amdgpu.h | 7 ++++++- amdgpu/amdgpu_bo.c | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-)amdgpu/amdgpu.h +6
-1@@ -77,21 +77,26 @@ struct drm_amdgpu_info_hw_ip;* */ enum amdgpu_bo_handle_type {/** GEM flink name (needs DRM authentication, used by DRI2) */ amdgpu_bo_handle_type_gem_flink_name =0 ,/** KMS handle which is used by all driver ioctls */ amdgpu_bo_handle_type_kms =1 ,/** DMA-buf fd handle */ - amdgpu_bo_handle_type_dma_buf_fd =2 + amdgpu_bo_handle_type_dma_buf_fd =2 , + +/** KMS handle, but re-importing as a DMABUF handle through +* drmPrimeHandleToFD is forbidden. (Glamor does that) +*/ + amdgpu_bo_handle_type_kms_noimport =3 , };/** Define known types of GPU VM VA ranges */ enum amdgpu_gpu_va_range {/** Allocate from "normal"/general range */ amdgpu_gpu_va_range_general =0 };enum amdgpu_sw_info {amdgpu/amdgpu_bo.c +4
-0@@ -234,20 +234,22 @@ int amdgpu_bo_export(amdgpu_bo_handle bo,case amdgpu_bo_handle_type_gem_flink_name: r =amdgpu_bo_export_flink (bo);if (r)return r; *shared_handle = bo->flink_name;return 0 ;case amdgpu_bo_handle_type_kms:amdgpu_add_handle_to_table (bo); +/* fall through */ +case amdgpu_bo_handle_type_kms_noimport: *shared_handle = bo->handle;return 0 ;case amdgpu_bo_handle_type_dma_buf_fd:amdgpu_add_handle_to_table (bo);return drmPrimeHandleToFD (bo->dev->fd, bo->handle,DRM_CLOEXEC |DRM_RDWR , (int *)shared_handle); }return -EINVAL ;@@ -299,20 +301,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev, bo =util_hash_table_get (dev->bo_flink_names, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo =util_hash_table_get (dev->bo_handles, (void *)(uintptr_t )shared_handle);break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:/* Importing a KMS handle in not allowed. */ pthread_mutex_unlock (&dev->bo_table_mutex);return -EPERM ;default :pthread_mutex_unlock (&dev->bo_table_mutex);return -EINVAL ; }if (bo) {@@ -368,20 +371,21 @@ int amdgpu_bo_import(amdgpu_device_handle dev,util_hash_table_set (dev->bo_flink_names, (void *)(uintptr_t )bo->flink_name, bo);break ;case amdgpu_bo_handle_type_dma_buf_fd: bo->handle = shared_handle; bo->alloc_size = dma_buf_size;break ;case amdgpu_bo_handle_type_kms: +case amdgpu_bo_handle_type_kms_noimport:assert (0 );/* unreachable */ }/* Initialize it. */ atomic_set (&bo->refcount,1 ); bo->dev = dev;pthread_mutex_init (&bo->cpu_access_mutex,NULL );
Quoted 9 lines
Quoted 5 lines
amdgpu/amdgpu_bo.c +0
-1util_hash_table_set (dev->bo_handles, (void *)(uintptr_t )bo->handle, bo);pthread_mutex_unlock (&dev->bo_table_mutex); --2.17.1
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx