[PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Junwei Zhang]

From: Chunming Zhou <David1.Zhou@amd.com>

v2: get original gem handle from gobj
v3: update find bo data structure as union(in, out)
    simply some code logic

Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
 include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
 4 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 4cd20e7..46c370b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
 			  struct drm_file *filp);
 int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *filp);
+int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
+					    struct drm_file *filp);
 int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
 			  struct drm_file *filp);
 int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 71792d8..bae8417 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
 	return 0;
 }
 
+static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
+					     struct drm_gem_object *obj)
+{
+	int i;
+	struct drm_gem_object *tmp;
+
+	spin_lock(&filp->table_lock);
+	idr_for_each_entry(&filp->object_idr, tmp, i) {
+		if (obj == tmp) {
+			drm_gem_object_reference(obj);
+			spin_unlock(&filp->table_lock);
+			return i;
+		}
+	}
+	spin_unlock(&filp->table_lock);
+
+	return 0;
+}
+
+int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
+					    struct drm_file *filp)
+{
+	union drm_amdgpu_gem_find_bo *args = data;
+	struct drm_gem_object *gobj;
+	struct amdgpu_bo *bo;
+	struct ttm_buffer_object *tbo;
+	struct vm_area_struct *vma;
+	uint32_t handle;
+	int r;
+
+	if (offset_in_page(args->in.addr | args->in.size))
+		return -EINVAL;
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(current->mm, args->in.addr);
+	if (!vma || vma->vm_file != filp->filp ||
+	    (args->in.size > (vma->vm_end - args->in.addr))) {
+		args->out.handle = 0;
+		up_read(&current->mm->mmap_sem);
+		return -EINVAL;
+	}
+	args->out.offset = args->in.addr - vma->vm_start;
+
+	tbo = vma->vm_private_data;
+	bo = container_of(tbo, struct amdgpu_bo, tbo);
+	amdgpu_bo_ref(bo);
+	gobj = &bo->gem_base;
+
+	handle = amdgpu_gem_get_handle_from_object(filp, gobj);
+	if (!handle) {
+		r = drm_gem_handle_create(filp, gobj, &handle);
+		if (r) {
+			DRM_ERROR("create gem handle failed\n");
+			up_read(&current->mm->mmap_sem);
+			return r;
+		}
+	}
+	args->out.handle = handle;
+	up_read(&current->mm->mmap_sem);
+
+	return 0;
+}
+
 int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
 			     struct drm_file *filp)
 {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 7956848..1bd2cc1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
 	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
-	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
+	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
+	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO, amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
 };
 const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
 
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 94444ee..000c415 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -54,6 +54,7 @@
 #define DRM_AMDGPU_VM			0x13
 #define DRM_AMDGPU_FENCE_TO_HANDLE	0x14
 #define DRM_AMDGPU_SCHED		0x15
+#define DRM_AMDGPU_GEM_FIND_BO		0x16
 /* not upstream */
 #define DRM_AMDGPU_FREESYNC	        0x5d
 
@@ -74,6 +75,7 @@
 #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
 #define DRM_IOCTL_AMDGPU_SCHED		DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
 #define DRM_IOCTL_AMDGPU_FREESYNC	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
+#define DRM_IOCTL_AMDGPU_GEM_FIND_BO	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
 
 /**
  * DOC: memory domains
@@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
 	struct drm_amdgpu_gem_wait_idle_out out;
 };
 
+struct drm_amdgpu_gem_find_bo_in {
+	/* CPU address */
+	__u64			addr;
+	/* memory size from CPU address */
+	__u64			size;
+};
+
+struct drm_amdgpu_gem_find_bo_out {
+	/* offset in bo */
+	__u64			offset;
+	/* resulting GEM handle */
+	__u32			handle;
+};
+
+union drm_amdgpu_gem_find_bo {
+	struct drm_amdgpu_gem_find_bo_in in;
+	struct drm_amdgpu_gem_find_bo_out out;
+};
+
 struct drm_amdgpu_wait_cs_in {
 	/* Command submission handle
          * handle equals 0 means none to wait for
-- 
1.9.1

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

[PATCH 2/2] drm/amdgpu: bump version for API to find bo by cpu mapping

[Junwei Zhang]

bump version for new API involved.

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 8843a06..6b52551 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -70,9 +70,10 @@
  * - 3.25.0 - Add support for sensor query info (stable pstate sclk/mclk).
  * - 3.26.0 - GFX9: Process AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE.
  * - 3.27.0 - Add new chunk to to AMDGPU_CS to enable BO_LIST creation.
+ * - 3.28.0 - Add API to find bo by cpu mapping for UMD bo validation.
  */
 #define KMS_DRIVER_MAJOR	3
-#define KMS_DRIVER_MINOR	27
+#define KMS_DRIVER_MINOR	28
 #define KMS_DRIVER_PATCHLEVEL	0
 
 int amdgpu_vram_limit = 0;
-- 
1.9.1

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
> From: Chunming Zhou <David1.Zhou@amd.com>
>
> v2: get original gem handle from gobj
> v3: update find bo data structure as union(in, out)
>      simply some code logic

Do we now have an open source user for this, so that we can upstream it? 
One more point below.

>
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>   4 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 4cd20e7..46c370b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *filp);
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			struct drm_file *filp);
> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
> +					    struct drm_file *filp);
>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *filp);
>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 71792d8..bae8417 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   	return 0;
>   }
>   
> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
> +					     struct drm_gem_object *obj)
> +{
> +	int i;
> +	struct drm_gem_object *tmp;
> +
> +	spin_lock(&filp->table_lock);
> +	idr_for_each_entry(&filp->object_idr, tmp, i) {
> +		if (obj == tmp) {
> +			drm_gem_object_reference(obj);
> +			spin_unlock(&filp->table_lock);
> +			return i;
> +		}
> +	}

Please double check if that is still up to date.

I think we could as well try to use the DMA-buf handle tree for that.

Christian.

> +	spin_unlock(&filp->table_lock);
> +
> +	return 0;
> +}
> +
> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
> +					    struct drm_file *filp)
> +{
> +	union drm_amdgpu_gem_find_bo *args = data;
> +	struct drm_gem_object *gobj;
> +	struct amdgpu_bo *bo;
> +	struct ttm_buffer_object *tbo;
> +	struct vm_area_struct *vma;
> +	uint32_t handle;
> +	int r;
> +
> +	if (offset_in_page(args->in.addr | args->in.size))
> +		return -EINVAL;
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(current->mm, args->in.addr);
> +	if (!vma || vma->vm_file != filp->filp ||
> +	    (args->in.size > (vma->vm_end - args->in.addr))) {
> +		args->out.handle = 0;
> +		up_read(&current->mm->mmap_sem);
> +		return -EINVAL;
> +	}
> +	args->out.offset = args->in.addr - vma->vm_start;
> +
> +	tbo = vma->vm_private_data;
> +	bo = container_of(tbo, struct amdgpu_bo, tbo);
> +	amdgpu_bo_ref(bo);
> +	gobj = &bo->gem_base;
> +
> +	handle = amdgpu_gem_get_handle_from_object(filp, gobj);
> +	if (!handle) {
> +		r = drm_gem_handle_create(filp, gobj, &handle);
> +		if (r) {
> +			DRM_ERROR("create gem handle failed\n");
> +			up_read(&current->mm->mmap_sem);
> +			return r;
> +		}
> +	}
> +	args->out.handle = handle;
> +	up_read(&current->mm->mmap_sem);
> +
> +	return 0;
> +}
> +
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			     struct drm_file *filp)
>   {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 7956848..1bd2cc1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
> +	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
> +	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO, amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>   };
>   const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>   
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 94444ee..000c415 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -54,6 +54,7 @@
>   #define DRM_AMDGPU_VM			0x13
>   #define DRM_AMDGPU_FENCE_TO_HANDLE	0x14
>   #define DRM_AMDGPU_SCHED		0x15
> +#define DRM_AMDGPU_GEM_FIND_BO		0x16
>   /* not upstream */
>   #define DRM_AMDGPU_FREESYNC	        0x5d
>   
> @@ -74,6 +75,7 @@
>   #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>   #define DRM_IOCTL_AMDGPU_SCHED		DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>   #define DRM_IOCTL_AMDGPU_FREESYNC	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>   
>   /**
>    * DOC: memory domains
> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>   	struct drm_amdgpu_gem_wait_idle_out out;
>   };
>   
> +struct drm_amdgpu_gem_find_bo_in {
> +	/* CPU address */
> +	__u64			addr;
> +	/* memory size from CPU address */
> +	__u64			size;
> +};
> +
> +struct drm_amdgpu_gem_find_bo_out {
> +	/* offset in bo */
> +	__u64			offset;
> +	/* resulting GEM handle */
> +	__u32			handle;
> +};
> +
> +union drm_amdgpu_gem_find_bo {
> +	struct drm_amdgpu_gem_find_bo_in in;
> +	struct drm_amdgpu_gem_find_bo_out out;
> +};
> +
>   struct drm_amdgpu_wait_cs_in {
>   	/* Command submission handle
>            * handle equals 0 means none to wait for

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

RE: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhou, David(ChunMing)]

Thanks for Jerry  still  remembers  this series.

Hi Christian,

For upstream of this feature, seems we already had agreement long time ago. Two reasons for upstreaming:
1. this bug was found by an opengl game, so this bug also is in mesa driver in theory.
2. after upstream these patches, we can reduce pro specific patches, and close to open source.

Btw, an unit test is excepted when upstreaming, I remember Alex mentioned.

Thanks,
David Zhou
-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com> 
Sent: Monday, July 30, 2018 6:48 PM
To: Zhang, Jerry <Jerry.Zhang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
> From: Chunming Zhou <David1.Zhou@amd.com>
>
> v2: get original gem handle from gobj
> v3: update find bo data structure as union(in, out)
>      simply some code logic

Do we now have an open source user for this, so that we can upstream it? 
One more point below.

>
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>   4 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 4cd20e7..46c370b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *filp);
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			struct drm_file *filp);
> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
> +					    struct drm_file *filp);
>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *filp);
>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data, 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 71792d8..bae8417 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   	return 0;
>   }
>   
> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
> +					     struct drm_gem_object *obj) {
> +	int i;
> +	struct drm_gem_object *tmp;
> +
> +	spin_lock(&filp->table_lock);
> +	idr_for_each_entry(&filp->object_idr, tmp, i) {
> +		if (obj == tmp) {
> +			drm_gem_object_reference(obj);
> +			spin_unlock(&filp->table_lock);
> +			return i;
> +		}
> +	}

Please double check if that is still up to date.

I think we could as well try to use the DMA-buf handle tree for that.

Christian.

> +	spin_unlock(&filp->table_lock);
> +
> +	return 0;
> +}
> +
> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
> +					    struct drm_file *filp)
> +{
> +	union drm_amdgpu_gem_find_bo *args = data;
> +	struct drm_gem_object *gobj;
> +	struct amdgpu_bo *bo;
> +	struct ttm_buffer_object *tbo;
> +	struct vm_area_struct *vma;
> +	uint32_t handle;
> +	int r;
> +
> +	if (offset_in_page(args->in.addr | args->in.size))
> +		return -EINVAL;
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(current->mm, args->in.addr);
> +	if (!vma || vma->vm_file != filp->filp ||
> +	    (args->in.size > (vma->vm_end - args->in.addr))) {
> +		args->out.handle = 0;
> +		up_read(&current->mm->mmap_sem);
> +		return -EINVAL;
> +	}
> +	args->out.offset = args->in.addr - vma->vm_start;
> +
> +	tbo = vma->vm_private_data;
> +	bo = container_of(tbo, struct amdgpu_bo, tbo);
> +	amdgpu_bo_ref(bo);
> +	gobj = &bo->gem_base;
> +
> +	handle = amdgpu_gem_get_handle_from_object(filp, gobj);
> +	if (!handle) {
> +		r = drm_gem_handle_create(filp, gobj, &handle);
> +		if (r) {
> +			DRM_ERROR("create gem handle failed\n");
> +			up_read(&current->mm->mmap_sem);
> +			return r;
> +		}
> +	}
> +	args->out.handle = handle;
> +	up_read(&current->mm->mmap_sem);
> +
> +	return 0;
> +}
> +
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			     struct drm_file *filp)
>   {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 7956848..1bd2cc1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
> +	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
> +	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO, 
> +amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>   };
>   const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>   
> diff --git a/include/uapi/drm/amdgpu_drm.h 
> b/include/uapi/drm/amdgpu_drm.h index 94444ee..000c415 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -54,6 +54,7 @@
>   #define DRM_AMDGPU_VM			0x13
>   #define DRM_AMDGPU_FENCE_TO_HANDLE	0x14
>   #define DRM_AMDGPU_SCHED		0x15
> +#define DRM_AMDGPU_GEM_FIND_BO		0x16
>   /* not upstream */
>   #define DRM_AMDGPU_FREESYNC	        0x5d
>   
> @@ -74,6 +75,7 @@
>   #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>   #define DRM_IOCTL_AMDGPU_SCHED		DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>   #define DRM_IOCTL_AMDGPU_FREESYNC	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>   
>   /**
>    * DOC: memory domains
> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>   	struct drm_amdgpu_gem_wait_idle_out out;
>   };
>   
> +struct drm_amdgpu_gem_find_bo_in {
> +	/* CPU address */
> +	__u64			addr;
> +	/* memory size from CPU address */
> +	__u64			size;
> +};
> +
> +struct drm_amdgpu_gem_find_bo_out {
> +	/* offset in bo */
> +	__u64			offset;
> +	/* resulting GEM handle */
> +	__u32			handle;
> +};
> +
> +union drm_amdgpu_gem_find_bo {
> +	struct drm_amdgpu_gem_find_bo_in in;
> +	struct drm_amdgpu_gem_find_bo_out out; };
> +
>   struct drm_amdgpu_wait_cs_in {
>   	/* Command submission handle
>            * handle equals 0 means none to wait for

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

RE: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhou, David(ChunMing)]

Typo, excepted -> expected

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhou, David(ChunMing)
Sent: Tuesday, July 31, 2018 9:41 AM
To: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Jerry <Jerry.Zhang@amd.com>; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

Thanks for Jerry  still  remembers  this series.

Hi Christian,

For upstream of this feature, seems we already had agreement long time ago. Two reasons for upstreaming:
1. this bug was found by an opengl game, so this bug also is in mesa driver in theory.
2. after upstream these patches, we can reduce pro specific patches, and close to open source.

Btw, an unit test is excepted when upstreaming, I remember Alex mentioned.

Thanks,
David Zhou
-----Original Message-----
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Monday, July 30, 2018 6:48 PM
To: Zhang, Jerry <Jerry.Zhang@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
> From: Chunming Zhou <David1.Zhou@amd.com>
>
> v2: get original gem handle from gobj
> v3: update find bo data structure as union(in, out)
>      simply some code logic

Do we now have an open source user for this, so that we can upstream it? 
One more point below.

>
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>   4 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 4cd20e7..46c370b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *filp);
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			struct drm_file *filp);
> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
> +					    struct drm_file *filp);
>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *filp);
>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data, 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 71792d8..bae8417 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>   	return 0;
>   }
>   
> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
> +					     struct drm_gem_object *obj) {
> +	int i;
> +	struct drm_gem_object *tmp;
> +
> +	spin_lock(&filp->table_lock);
> +	idr_for_each_entry(&filp->object_idr, tmp, i) {
> +		if (obj == tmp) {
> +			drm_gem_object_reference(obj);
> +			spin_unlock(&filp->table_lock);
> +			return i;
> +		}
> +	}

Please double check if that is still up to date.

I think we could as well try to use the DMA-buf handle tree for that.

Christian.

> +	spin_unlock(&filp->table_lock);
> +
> +	return 0;
> +}
> +
> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
> +					    struct drm_file *filp)
> +{
> +	union drm_amdgpu_gem_find_bo *args = data;
> +	struct drm_gem_object *gobj;
> +	struct amdgpu_bo *bo;
> +	struct ttm_buffer_object *tbo;
> +	struct vm_area_struct *vma;
> +	uint32_t handle;
> +	int r;
> +
> +	if (offset_in_page(args->in.addr | args->in.size))
> +		return -EINVAL;
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(current->mm, args->in.addr);
> +	if (!vma || vma->vm_file != filp->filp ||
> +	    (args->in.size > (vma->vm_end - args->in.addr))) {
> +		args->out.handle = 0;
> +		up_read(&current->mm->mmap_sem);
> +		return -EINVAL;
> +	}
> +	args->out.offset = args->in.addr - vma->vm_start;
> +
> +	tbo = vma->vm_private_data;
> +	bo = container_of(tbo, struct amdgpu_bo, tbo);
> +	amdgpu_bo_ref(bo);
> +	gobj = &bo->gem_base;
> +
> +	handle = amdgpu_gem_get_handle_from_object(filp, gobj);
> +	if (!handle) {
> +		r = drm_gem_handle_create(filp, gobj, &handle);
> +		if (r) {
> +			DRM_ERROR("create gem handle failed\n");
> +			up_read(&current->mm->mmap_sem);
> +			return r;
> +		}
> +	}
> +	args->out.handle = handle;
> +	up_read(&current->mm->mmap_sem);
> +
> +	return 0;
> +}
> +
>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>   			     struct drm_file *filp)
>   {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 7956848..1bd2cc1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
> +	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
> +	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO,
> +amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>   };
>   const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>   
> diff --git a/include/uapi/drm/amdgpu_drm.h 
> b/include/uapi/drm/amdgpu_drm.h index 94444ee..000c415 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -54,6 +54,7 @@
>   #define DRM_AMDGPU_VM			0x13
>   #define DRM_AMDGPU_FENCE_TO_HANDLE	0x14
>   #define DRM_AMDGPU_SCHED		0x15
> +#define DRM_AMDGPU_GEM_FIND_BO		0x16
>   /* not upstream */
>   #define DRM_AMDGPU_FREESYNC	        0x5d
>   
> @@ -74,6 +75,7 @@
>   #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>   #define DRM_IOCTL_AMDGPU_SCHED		DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>   #define DRM_IOCTL_AMDGPU_FREESYNC	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>   
>   /**
>    * DOC: memory domains
> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>   	struct drm_amdgpu_gem_wait_idle_out out;
>   };
>   
> +struct drm_amdgpu_gem_find_bo_in {
> +	/* CPU address */
> +	__u64			addr;
> +	/* memory size from CPU address */
> +	__u64			size;
> +};
> +
> +struct drm_amdgpu_gem_find_bo_out {
> +	/* offset in bo */
> +	__u64			offset;
> +	/* resulting GEM handle */
> +	__u32			handle;
> +};
> +
> +union drm_amdgpu_gem_find_bo {
> +	struct drm_amdgpu_gem_find_bo_in in;
> +	struct drm_amdgpu_gem_find_bo_out out; };
> +
>   struct drm_amdgpu_wait_cs_in {
>   	/* Command submission handle
>            * handle equals 0 means none to wait for

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhang, Jerry (Junwei)]

On 07/31/2018 09:49 AM, Zhou, David(ChunMing) wrote:
> Typo, excepted -> expected
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhou, David(ChunMing)
> Sent: Tuesday, July 31, 2018 9:41 AM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, Jerry <Jerry.Zhang@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)
>
> Thanks for Jerry  still  remembers  this series.
>
> Hi Christian,
>
> For upstream of this feature, seems we already had agreement long time ago. Two reasons for upstreaming:
> 1. this bug was found by an opengl game, so this bug also is in mesa driver in theory.
> 2. after upstream these patches, we can reduce pro specific patches, and close to open source.

Thanks for explanation.

>
> Btw, an unit test is excepted when upstreaming, I remember Alex mentioned.

Yes, I will.

>
> Thanks,
> David Zhou
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Monday, July 30, 2018 6:48 PM
> To: Zhang, Jerry <Jerry.Zhang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)
>
> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>> From: Chunming Zhou <David1.Zhou@amd.com>
>>
>> v2: get original gem handle from gobj
>> v3: update find bo data structure as union(in, out)
>>       simply some code logic
>
> Do we now have an open source user for this, so that we can upstream it?
> One more point below.
>
>>
>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>    include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>    4 files changed, 88 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 4cd20e7..46c370b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>>    			  struct drm_file *filp);
>>    int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>    			struct drm_file *filp);
>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>> +					    struct drm_file *filp);
>>    int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>    			  struct drm_file *filp);
>>    int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 71792d8..bae8417 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>    	return 0;
>>    }
>>
>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>> +					     struct drm_gem_object *obj) {
>> +	int i;
>> +	struct drm_gem_object *tmp;
>> +
>> +	spin_lock(&filp->table_lock);
>> +	idr_for_each_entry(&filp->object_idr, tmp, i) {
>> +		if (obj == tmp) {
>> +			drm_gem_object_reference(obj);
>> +			spin_unlock(&filp->table_lock);
>> +			return i;
>> +		}
>> +	}
>
> Please double check if that is still up to date.

We may have to replace drm_gem_object_reference() with drm_gem_object_get().

On 2nd thought, do we really need to do reference every time?
if UMD find the same gem object for 3 times, it also need to explicitly free(put) that object for 3 times?


>
> I think we could as well try to use the DMA-buf handle tree for that.

If the bo is not import/export, DMA-buf will be NULL always.

Regards,
Jerry

>
> Christian.
>
>> +	spin_unlock(&filp->table_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>> +					    struct drm_file *filp)
>> +{
>> +	union drm_amdgpu_gem_find_bo *args = data;
>> +	struct drm_gem_object *gobj;
>> +	struct amdgpu_bo *bo;
>> +	struct ttm_buffer_object *tbo;
>> +	struct vm_area_struct *vma;
>> +	uint32_t handle;
>> +	int r;
>> +
>> +	if (offset_in_page(args->in.addr | args->in.size))
>> +		return -EINVAL;
>> +
>> +	down_read(&current->mm->mmap_sem);
>> +	vma = find_vma(current->mm, args->in.addr);
>> +	if (!vma || vma->vm_file != filp->filp ||
>> +	    (args->in.size > (vma->vm_end - args->in.addr))) {
>> +		args->out.handle = 0;
>> +		up_read(&current->mm->mmap_sem);
>> +		return -EINVAL;
>> +	}
>> +	args->out.offset = args->in.addr - vma->vm_start;
>> +
>> +	tbo = vma->vm_private_data;
>> +	bo = container_of(tbo, struct amdgpu_bo, tbo);
>> +	amdgpu_bo_ref(bo);
>> +	gobj = &bo->gem_base;
>> +
>> +	handle = amdgpu_gem_get_handle_from_object(filp, gobj);
>> +	if (!handle) {
>> +		r = drm_gem_handle_create(filp, gobj, &handle);
>> +		if (r) {
>> +			DRM_ERROR("create gem handle failed\n");
>> +			up_read(&current->mm->mmap_sem);
>> +			return r;
>> +		}
>> +	}
>> +	args->out.handle = handle;
>> +	up_read(&current->mm->mmap_sem);
>> +
>> +	return 0;
>> +}
>> +
>>    int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>    			     struct drm_file *filp)
>>    {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 7956848..1bd2cc1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>>    	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>    	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>    	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>> -	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
>> +	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
>> +	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO,
>> +amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>>    };
>>    const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 94444ee..000c415 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -54,6 +54,7 @@
>>    #define DRM_AMDGPU_VM			0x13
>>    #define DRM_AMDGPU_FENCE_TO_HANDLE	0x14
>>    #define DRM_AMDGPU_SCHED		0x15
>> +#define DRM_AMDGPU_GEM_FIND_BO		0x16
>>    /* not upstream */
>>    #define DRM_AMDGPU_FREESYNC	        0x5d
>>
>> @@ -74,6 +75,7 @@
>>    #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>>    #define DRM_IOCTL_AMDGPU_SCHED		DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>    #define DRM_IOCTL_AMDGPU_FREESYNC	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
>> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>>
>>    /**
>>     * DOC: memory domains
>> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>>    	struct drm_amdgpu_gem_wait_idle_out out;
>>    };
>>
>> +struct drm_amdgpu_gem_find_bo_in {
>> +	/* CPU address */
>> +	__u64			addr;
>> +	/* memory size from CPU address */
>> +	__u64			size;
>> +};
>> +
>> +struct drm_amdgpu_gem_find_bo_out {
>> +	/* offset in bo */
>> +	__u64			offset;
>> +	/* resulting GEM handle */
>> +	__u32			handle;
>> +};
>> +
>> +union drm_amdgpu_gem_find_bo {
>> +	struct drm_amdgpu_gem_find_bo_in in;
>> +	struct drm_amdgpu_gem_find_bo_out out; };
>> +
>>    struct drm_amdgpu_wait_cs_in {
>>    	/* Command submission handle
>>             * handle equals 0 means none to wait for
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 31.07.2018 um 03:40 schrieb Zhou, David(ChunMing):
> Thanks for Jerry  still  remembers  this series.
>
> Hi Christian,
>
> For upstream of this feature, seems we already had agreement long time ago. Two reasons for upstreaming:
> 1. this bug was found by an opengl game, so this bug also is in mesa driver in theory.
> 2. after upstream these patches, we can reduce pro specific patches, and close to open source.

I mean the functionality is actually rather simple, so I don't see much 
of an issue with that.

> Btw, an unit test is excepted when upstreaming, I remember Alex mentioned.

Yeah, agree an unit test is really a must have for that.

Christian.

>
> Thanks,
> David Zhou
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Monday, July 30, 2018 6:48 PM
> To: Zhang, Jerry <Jerry.Zhang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)
>
> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>> From: Chunming Zhou <David1.Zhou@amd.com>
>>
>> v2: get original gem handle from gobj
>> v3: update find bo data structure as union(in, out)
>>       simply some code logic
> Do we now have an open source user for this, so that we can upstream it?
> One more point below.
>
>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>    include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>    4 files changed, 88 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 4cd20e7..46c370b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>>    			  struct drm_file *filp);
>>    int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>    			struct drm_file *filp);
>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>> +					    struct drm_file *filp);
>>    int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>    			  struct drm_file *filp);
>>    int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 71792d8..bae8417 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>    	return 0;
>>    }
>>    
>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>> +					     struct drm_gem_object *obj) {
>> +	int i;
>> +	struct drm_gem_object *tmp;
>> +
>> +	spin_lock(&filp->table_lock);
>> +	idr_for_each_entry(&filp->object_idr, tmp, i) {
>> +		if (obj == tmp) {
>> +			drm_gem_object_reference(obj);
>> +			spin_unlock(&filp->table_lock);
>> +			return i;
>> +		}
>> +	}
> Please double check if that is still up to date.
>
> I think we could as well try to use the DMA-buf handle tree for that.
>
> Christian.
>
>> +	spin_unlock(&filp->table_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>> +					    struct drm_file *filp)
>> +{
>> +	union drm_amdgpu_gem_find_bo *args = data;
>> +	struct drm_gem_object *gobj;
>> +	struct amdgpu_bo *bo;
>> +	struct ttm_buffer_object *tbo;
>> +	struct vm_area_struct *vma;
>> +	uint32_t handle;
>> +	int r;
>> +
>> +	if (offset_in_page(args->in.addr | args->in.size))
>> +		return -EINVAL;
>> +
>> +	down_read(&current->mm->mmap_sem);
>> +	vma = find_vma(current->mm, args->in.addr);
>> +	if (!vma || vma->vm_file != filp->filp ||
>> +	    (args->in.size > (vma->vm_end - args->in.addr))) {
>> +		args->out.handle = 0;
>> +		up_read(&current->mm->mmap_sem);
>> +		return -EINVAL;
>> +	}
>> +	args->out.offset = args->in.addr - vma->vm_start;
>> +
>> +	tbo = vma->vm_private_data;
>> +	bo = container_of(tbo, struct amdgpu_bo, tbo);
>> +	amdgpu_bo_ref(bo);
>> +	gobj = &bo->gem_base;
>> +
>> +	handle = amdgpu_gem_get_handle_from_object(filp, gobj);
>> +	if (!handle) {
>> +		r = drm_gem_handle_create(filp, gobj, &handle);
>> +		if (r) {
>> +			DRM_ERROR("create gem handle failed\n");
>> +			up_read(&current->mm->mmap_sem);
>> +			return r;
>> +		}
>> +	}
>> +	args->out.handle = handle;
>> +	up_read(&current->mm->mmap_sem);
>> +
>> +	return 0;
>> +}
>> +
>>    int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>    			     struct drm_file *filp)
>>    {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 7956848..1bd2cc1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>>    	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>    	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>    	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>> -	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
>> +	DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
>> +	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO,
>> +amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>>    };
>>    const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>>    
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h index 94444ee..000c415 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -54,6 +54,7 @@
>>    #define DRM_AMDGPU_VM			0x13
>>    #define DRM_AMDGPU_FENCE_TO_HANDLE	0x14
>>    #define DRM_AMDGPU_SCHED		0x15
>> +#define DRM_AMDGPU_GEM_FIND_BO		0x16
>>    /* not upstream */
>>    #define DRM_AMDGPU_FREESYNC	        0x5d
>>    
>> @@ -74,6 +75,7 @@
>>    #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>>    #define DRM_IOCTL_AMDGPU_SCHED		DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>    #define DRM_IOCTL_AMDGPU_FREESYNC	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
>> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>>    
>>    /**
>>     * DOC: memory domains
>> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>>    	struct drm_amdgpu_gem_wait_idle_out out;
>>    };
>>    
>> +struct drm_amdgpu_gem_find_bo_in {
>> +	/* CPU address */
>> +	__u64			addr;
>> +	/* memory size from CPU address */
>> +	__u64			size;
>> +};
>> +
>> +struct drm_amdgpu_gem_find_bo_out {
>> +	/* offset in bo */
>> +	__u64			offset;
>> +	/* resulting GEM handle */
>> +	__u32			handle;
>> +};
>> +
>> +union drm_amdgpu_gem_find_bo {
>> +	struct drm_amdgpu_gem_find_bo_in in;
>> +	struct drm_amdgpu_gem_find_bo_out out; };
>> +
>>    struct drm_amdgpu_wait_cs_in {
>>    	/* Command submission handle
>>             * handle equals 0 means none to wait for
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhang, Jerry (Junwei)]

On 07/30/2018 06:47 PM, Christian König wrote:
> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>> From: Chunming Zhou <David1.Zhou@amd.com>
>>
>> v2: get original gem handle from gobj
>> v3: update find bo data structure as union(in, out)
>>      simply some code logic
>
> Do we now have an open source user for this, so that we can upstream it? One more point below.
>
>>
>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 4cd20e7..46c370b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>>                 struct drm_file *filp);
>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>               struct drm_file *filp);
>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>> +                        struct drm_file *filp);
>>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>                 struct drm_file *filp);
>>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 71792d8..bae8417 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>       return 0;
>>   }
>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>> +                         struct drm_gem_object *obj)
>> +{
>> +    int i;
>> +    struct drm_gem_object *tmp;
>> +
>> +    spin_lock(&filp->table_lock);
>> +    idr_for_each_entry(&filp->object_idr, tmp, i) {
>> +        if (obj == tmp) {
>> +            drm_gem_object_reference(obj);
>> +            spin_unlock(&filp->table_lock);
>> +            return i;
>> +        }
>> +    }
>
> Please double check if that is still up to date.

We may have to replace drm_gem_object_reference() with drm_gem_object_get().

On 2nd thought, do we really need to do reference every time?
if UMD find the same gem object for 3 times, it also need to explicitly free(put) that object for 3 times?


>
> I think we could as well try to use the DMA-buf handle tree for that.

If the bo is not import/export, DMA-buf will be NULL always.

Regards,
Jerry
>
> Christian.
>
>> +    spin_unlock(&filp->table_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>> +                        struct drm_file *filp)
>> +{
>> +    union drm_amdgpu_gem_find_bo *args = data;
>> +    struct drm_gem_object *gobj;
>> +    struct amdgpu_bo *bo;
>> +    struct ttm_buffer_object *tbo;
>> +    struct vm_area_struct *vma;
>> +    uint32_t handle;
>> +    int r;
>> +
>> +    if (offset_in_page(args->in.addr | args->in.size))
>> +        return -EINVAL;
>> +
>> +    down_read(&current->mm->mmap_sem);
>> +    vma = find_vma(current->mm, args->in.addr);
>> +    if (!vma || vma->vm_file != filp->filp ||
>> +        (args->in.size > (vma->vm_end - args->in.addr))) {
>> +        args->out.handle = 0;
>> +        up_read(&current->mm->mmap_sem);
>> +        return -EINVAL;
>> +    }
>> +    args->out.offset = args->in.addr - vma->vm_start;
>> +
>> +    tbo = vma->vm_private_data;
>> +    bo = container_of(tbo, struct amdgpu_bo, tbo);
>> +    amdgpu_bo_ref(bo);
>> +    gobj = &bo->gem_base;
>> +
>> +    handle = amdgpu_gem_get_handle_from_object(filp, gobj);
>> +    if (!handle) {
>> +        r = drm_gem_handle_create(filp, gobj, &handle);
>> +        if (r) {
>> +            DRM_ERROR("create gem handle failed\n");
>> +            up_read(&current->mm->mmap_sem);
>> +            return r;
>> +        }
>> +    }
>> +    args->out.handle = handle;
>> +    up_read(&current->mm->mmap_sem);
>> +
>> +    return 0;
>> +}
>> +
>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>                    struct drm_file *filp)
>>   {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 7956848..1bd2cc1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>> -    DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
>> +    DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
>> +    DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO, amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>>   };
>>   const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>> index 94444ee..000c415 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -54,6 +54,7 @@
>>   #define DRM_AMDGPU_VM            0x13
>>   #define DRM_AMDGPU_FENCE_TO_HANDLE    0x14
>>   #define DRM_AMDGPU_SCHED        0x15
>> +#define DRM_AMDGPU_GEM_FIND_BO        0x16
>>   /* not upstream */
>>   #define DRM_AMDGPU_FREESYNC            0x5d
>> @@ -74,6 +75,7 @@
>>   #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>>   #define DRM_IOCTL_AMDGPU_SCHED        DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>   #define DRM_IOCTL_AMDGPU_FREESYNC    DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
>> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO    DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>>   /**
>>    * DOC: memory domains
>> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>>       struct drm_amdgpu_gem_wait_idle_out out;
>>   };
>> +struct drm_amdgpu_gem_find_bo_in {
>> +    /* CPU address */
>> +    __u64            addr;
>> +    /* memory size from CPU address */
>> +    __u64            size;
>> +};
>> +
>> +struct drm_amdgpu_gem_find_bo_out {
>> +    /* offset in bo */
>> +    __u64            offset;
>> +    /* resulting GEM handle */
>> +    __u32            handle;
>> +};
>> +
>> +union drm_amdgpu_gem_find_bo {
>> +    struct drm_amdgpu_gem_find_bo_in in;
>> +    struct drm_amdgpu_gem_find_bo_out out;
>> +};
>> +
>>   struct drm_amdgpu_wait_cs_in {
>>       /* Command submission handle
>>            * handle equals 0 means none to wait for
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
> On 07/30/2018 06:47 PM, Christian König wrote:
>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>> From: Chunming Zhou <David1.Zhou@amd.com>
>>>
>>> v2: get original gem handle from gobj
>>> v3: update find bo data structure as union(in, out)
>>>      simply some code logic
>>
>> Do we now have an open source user for this, so that we can upstream 
>> it? One more point below.
>>
>>>
>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 
>>> +++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 4cd20e7..46c370b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device 
>>> *dev, void *data,
>>>                 struct drm_file *filp);
>>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>>               struct drm_file *filp);
>>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, 
>>> void *data,
>>> +                        struct drm_file *filp);
>>>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>>                 struct drm_file *filp);
>>>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 71792d8..bae8417 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device 
>>> *dev, void *data,
>>>       return 0;
>>>   }
>>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>>> +                         struct drm_gem_object *obj)
>>> +{
>>> +    int i;
>>> +    struct drm_gem_object *tmp;
>>> +
>>> +    spin_lock(&filp->table_lock);
>>> +    idr_for_each_entry(&filp->object_idr, tmp, i) {
>>> +        if (obj == tmp) {
>>> +            drm_gem_object_reference(obj);
>>> +            spin_unlock(&filp->table_lock);
>>> +            return i;
>>> +        }
>>> +    }
>>
>> Please double check if that is still up to date.
>
> We may have to replace drm_gem_object_reference() with 
> drm_gem_object_get().
>
> On 2nd thought, do we really need to do reference every time?

Yes, that's a must have. Otherwise the handle could be freed and reused 
already when we return.

> if UMD find the same gem object for 3 times, it also need to 
> explicitly free(put) that object for 3 times?

Correct yes. Thinking more about this the real problem is to translate 
the handle into a structure in libdrm.

Here we are back to the problem Marek and Michel has been working on for 
a while that we always need to be able to translate a handle into a bo 
structure.....

So that needs to be solved before we can upstream the changes.

Regards,
Christian.

>
>
>>
>> I think we could as well try to use the DMA-buf handle tree for that.
>
> If the bo is not import/export, DMA-buf will be NULL always.
>
> Regards,
> Jerry
>>
>> Christian.
>>
>>> + spin_unlock(&filp->table_lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, 
>>> void *data,
>>> +                        struct drm_file *filp)
>>> +{
>>> +    union drm_amdgpu_gem_find_bo *args = data;
>>> +    struct drm_gem_object *gobj;
>>> +    struct amdgpu_bo *bo;
>>> +    struct ttm_buffer_object *tbo;
>>> +    struct vm_area_struct *vma;
>>> +    uint32_t handle;
>>> +    int r;
>>> +
>>> +    if (offset_in_page(args->in.addr | args->in.size))
>>> +        return -EINVAL;
>>> +
>>> +    down_read(&current->mm->mmap_sem);
>>> +    vma = find_vma(current->mm, args->in.addr);
>>> +    if (!vma || vma->vm_file != filp->filp ||
>>> +        (args->in.size > (vma->vm_end - args->in.addr))) {
>>> +        args->out.handle = 0;
>>> +        up_read(&current->mm->mmap_sem);
>>> +        return -EINVAL;
>>> +    }
>>> +    args->out.offset = args->in.addr - vma->vm_start;
>>> +
>>> +    tbo = vma->vm_private_data;
>>> +    bo = container_of(tbo, struct amdgpu_bo, tbo);
>>> +    amdgpu_bo_ref(bo);
>>> +    gobj = &bo->gem_base;
>>> +
>>> +    handle = amdgpu_gem_get_handle_from_object(filp, gobj);
>>> +    if (!handle) {
>>> +        r = drm_gem_handle_create(filp, gobj, &handle);
>>> +        if (r) {
>>> +            DRM_ERROR("create gem handle failed\n");
>>> +            up_read(&current->mm->mmap_sem);
>>> +            return r;
>>> +        }
>>> +    }
>>> +    args->out.handle = handle;
>>> +    up_read(&current->mm->mmap_sem);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>>                    struct drm_file *filp)
>>>   {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 7956848..1bd2cc1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct 
>>> drm_device *dev, unsigned int pipe)
>>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, 
>>> DRM_AUTH|DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, 
>>> DRM_AUTH|DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, 
>>> amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>> -    DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, 
>>> amdgpu_display_freesync_ioctl, DRM_MASTER)
>>> +    DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, 
>>> amdgpu_display_freesync_ioctl, DRM_MASTER),
>>> +    DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO, 
>>> amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>>>   };
>>>   const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>>> diff --git a/include/uapi/drm/amdgpu_drm.h 
>>> b/include/uapi/drm/amdgpu_drm.h
>>> index 94444ee..000c415 100644
>>> --- a/include/uapi/drm/amdgpu_drm.h
>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>> @@ -54,6 +54,7 @@
>>>   #define DRM_AMDGPU_VM            0x13
>>>   #define DRM_AMDGPU_FENCE_TO_HANDLE    0x14
>>>   #define DRM_AMDGPU_SCHED        0x15
>>> +#define DRM_AMDGPU_GEM_FIND_BO        0x16
>>>   /* not upstream */
>>>   #define DRM_AMDGPU_FREESYNC            0x5d
>>> @@ -74,6 +75,7 @@
>>>   #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE 
>>> + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>>>   #define DRM_IOCTL_AMDGPU_SCHED DRM_IOW(DRM_COMMAND_BASE + 
>>> DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>>   #define DRM_IOCTL_AMDGPU_FREESYNC DRM_IOWR(DRM_COMMAND_BASE + 
>>> DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
>>> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO DRM_IOWR(DRM_COMMAND_BASE + 
>>> DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>>>   /**
>>>    * DOC: memory domains
>>> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>>>       struct drm_amdgpu_gem_wait_idle_out out;
>>>   };
>>> +struct drm_amdgpu_gem_find_bo_in {
>>> +    /* CPU address */
>>> +    __u64            addr;
>>> +    /* memory size from CPU address */
>>> +    __u64            size;
>>> +};
>>> +
>>> +struct drm_amdgpu_gem_find_bo_out {
>>> +    /* offset in bo */
>>> +    __u64            offset;
>>> +    /* resulting GEM handle */
>>> +    __u32            handle;
>>> +};
>>> +
>>> +union drm_amdgpu_gem_find_bo {
>>> +    struct drm_amdgpu_gem_find_bo_in in;
>>> +    struct drm_amdgpu_gem_find_bo_out out;
>>> +};
>>> +
>>>   struct drm_amdgpu_wait_cs_in {
>>>       /* Command submission handle
>>>            * handle equals 0 means none to wait for
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhang, Jerry (Junwei)]

On 07/31/2018 03:03 PM, Christian König wrote:
> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>> On 07/30/2018 06:47 PM, Christian König wrote:
>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>> From: Chunming Zhou <David1.Zhou@amd.com>
>>>>
>>>> v2: get original gem handle from gobj
>>>> v3: update find bo data structure as union(in, out)
>>>>      simply some code logic
>>>
>>> Do we now have an open source user for this, so that we can upstream it? One more point below.
>>>
>>>>
>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>>>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> index 4cd20e7..46c370b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>>>>                 struct drm_file *filp);
>>>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>>>               struct drm_file *filp);
>>>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>>>> +                        struct drm_file *filp);
>>>>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>>>                 struct drm_file *filp);
>>>>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index 71792d8..bae8417 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>       return 0;
>>>>   }
>>>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>>>> +                         struct drm_gem_object *obj)
>>>> +{
>>>> +    int i;
>>>> +    struct drm_gem_object *tmp;
>>>> +
>>>> +    spin_lock(&filp->table_lock);
>>>> +    idr_for_each_entry(&filp->object_idr, tmp, i) {
>>>> +        if (obj == tmp) {
>>>> +            drm_gem_object_reference(obj);
>>>> +            spin_unlock(&filp->table_lock);
>>>> +            return i;
>>>> +        }
>>>> +    }
>>>
>>> Please double check if that is still up to date.
>>
>> We may have to replace drm_gem_object_reference() with drm_gem_object_get().
>>
>> On 2nd thought, do we really need to do reference every time?
>
> Yes, that's a must have. Otherwise the handle could be freed and reused already when we return.
>
>> if UMD find the same gem object for 3 times, it also need to explicitly free(put) that object for 3 times?
>
> Correct yes. Thinking more about this the real problem is to translate the handle into a structure in libdrm.
>
> Here we are back to the problem Marek and Michel has been working on for a while that we always need to be able to translate a handle into a bo structure.....
>
> So that needs to be solved before we can upstream the changes.

Thanks for your info.
It's better to fix that before upstream.

Regards,
Jerry

>
> Regards,
> Christian.
>
>>
>>
>>>
>>> I think we could as well try to use the DMA-buf handle tree for that.
>>
>> If the bo is not import/export, DMA-buf will be NULL always.
>>
>> Regards,
>> Jerry
>>>
>>> Christian.
>>>
>>>> + spin_unlock(&filp->table_lock);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>>>> +                        struct drm_file *filp)
>>>> +{
>>>> +    union drm_amdgpu_gem_find_bo *args = data;
>>>> +    struct drm_gem_object *gobj;
>>>> +    struct amdgpu_bo *bo;
>>>> +    struct ttm_buffer_object *tbo;
>>>> +    struct vm_area_struct *vma;
>>>> +    uint32_t handle;
>>>> +    int r;
>>>> +
>>>> +    if (offset_in_page(args->in.addr | args->in.size))
>>>> +        return -EINVAL;
>>>> +
>>>> +    down_read(&current->mm->mmap_sem);
>>>> +    vma = find_vma(current->mm, args->in.addr);
>>>> +    if (!vma || vma->vm_file != filp->filp ||
>>>> +        (args->in.size > (vma->vm_end - args->in.addr))) {
>>>> +        args->out.handle = 0;
>>>> +        up_read(&current->mm->mmap_sem);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +    args->out.offset = args->in.addr - vma->vm_start;
>>>> +
>>>> +    tbo = vma->vm_private_data;
>>>> +    bo = container_of(tbo, struct amdgpu_bo, tbo);
>>>> +    amdgpu_bo_ref(bo);
>>>> +    gobj = &bo->gem_base;
>>>> +
>>>> +    handle = amdgpu_gem_get_handle_from_object(filp, gobj);
>>>> +    if (!handle) {
>>>> +        r = drm_gem_handle_create(filp, gobj, &handle);
>>>> +        if (r) {
>>>> +            DRM_ERROR("create gem handle failed\n");
>>>> +            up_read(&current->mm->mmap_sem);
>>>> +            return r;
>>>> +        }
>>>> +    }
>>>> +    args->out.handle = handle;
>>>> +    up_read(&current->mm->mmap_sem);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>>>                    struct drm_file *filp)
>>>>   {
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> index 7956848..1bd2cc1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> @@ -1100,7 +1100,8 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>>>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>>>       DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>>>> -    DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER)
>>>> +    DRM_IOCTL_DEF_DRV(AMDGPU_FREESYNC, amdgpu_display_freesync_ioctl, DRM_MASTER),
>>>> +    DRM_IOCTL_DEF_DRV(AMDGPU_GEM_FIND_BO, amdgpu_gem_find_bo_by_cpu_mapping_ioctl, DRM_AUTH|DRM_RENDER_ALLOW)
>>>>   };
>>>>   const int amdgpu_max_kms_ioctl = ARRAY_SIZE(amdgpu_ioctls_kms);
>>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>>> index 94444ee..000c415 100644
>>>> --- a/include/uapi/drm/amdgpu_drm.h
>>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>>> @@ -54,6 +54,7 @@
>>>>   #define DRM_AMDGPU_VM            0x13
>>>>   #define DRM_AMDGPU_FENCE_TO_HANDLE    0x14
>>>>   #define DRM_AMDGPU_SCHED        0x15
>>>> +#define DRM_AMDGPU_GEM_FIND_BO        0x16
>>>>   /* not upstream */
>>>>   #define DRM_AMDGPU_FREESYNC            0x5d
>>>> @@ -74,6 +75,7 @@
>>>>   #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>>>>   #define DRM_IOCTL_AMDGPU_SCHED DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>>>   #define DRM_IOCTL_AMDGPU_FREESYNC DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FREESYNC, struct drm_amdgpu_freesync)
>>>> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, union drm_amdgpu_gem_find_bo)
>>>>   /**
>>>>    * DOC: memory domains
>>>> @@ -392,6 +394,25 @@ struct drm_amdgpu_gem_wait_idle_out {
>>>>       struct drm_amdgpu_gem_wait_idle_out out;
>>>>   };
>>>> +struct drm_amdgpu_gem_find_bo_in {
>>>> +    /* CPU address */
>>>> +    __u64            addr;
>>>> +    /* memory size from CPU address */
>>>> +    __u64            size;
>>>> +};
>>>> +
>>>> +struct drm_amdgpu_gem_find_bo_out {
>>>> +    /* offset in bo */
>>>> +    __u64            offset;
>>>> +    /* resulting GEM handle */
>>>> +    __u32            handle;
>>>> +};
>>>> +
>>>> +union drm_amdgpu_gem_find_bo {
>>>> +    struct drm_amdgpu_gem_find_bo_in in;
>>>> +    struct drm_amdgpu_gem_find_bo_out out;
>>>> +};
>>>> +
>>>>   struct drm_amdgpu_wait_cs_in {
>>>>       /* Command submission handle
>>>>            * handle equals 0 means none to wait for
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 31.07.2018 um 10:05 schrieb Zhang, Jerry (Junwei):
> On 07/31/2018 03:03 PM, Christian König wrote:
>> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>>> On 07/30/2018 06:47 PM, Christian König wrote:
>>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>>> From: Chunming Zhou <David1.Zhou@amd.com>
>>>>>
>>>>> v2: get original gem handle from gobj
>>>>> v3: update find bo data structure as union(in, out)
>>>>>      simply some code logic
>>>>
>>>> Do we now have an open source user for this, so that we can 
>>>> upstream it? One more point below.
>>>>
>>>>>
>>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 
>>>>> +++++++++++++++++++++++++++++++++
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>>>>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>>>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> index 4cd20e7..46c370b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device 
>>>>> *dev, void *data,
>>>>>                 struct drm_file *filp);
>>>>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>>>>               struct drm_file *filp);
>>>>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device 
>>>>> *dev, void *data,
>>>>> +                        struct drm_file *filp);
>>>>>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>>>>                 struct drm_file *filp);
>>>>>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 71792d8..bae8417 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device 
>>>>> *dev, void *data,
>>>>>       return 0;
>>>>>   }
>>>>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>>>>> +                         struct drm_gem_object *obj)
>>>>> +{
>>>>> +    int i;
>>>>> +    struct drm_gem_object *tmp;
>>>>> +
>>>>> +    spin_lock(&filp->table_lock);
>>>>> +    idr_for_each_entry(&filp->object_idr, tmp, i) {
>>>>> +        if (obj == tmp) {
>>>>> +            drm_gem_object_reference(obj);
>>>>> +            spin_unlock(&filp->table_lock);
>>>>> +            return i;
>>>>> +        }
>>>>> +    }
>>>>
>>>> Please double check if that is still up to date.
>>>
>>> We may have to replace drm_gem_object_reference() with 
>>> drm_gem_object_get().
>>>
>>> On 2nd thought, do we really need to do reference every time?
>>
>> Yes, that's a must have. Otherwise the handle could be freed and 
>> reused already when we return.
>>
>>> if UMD find the same gem object for 3 times, it also need to 
>>> explicitly free(put) that object for 3 times?
>>
>> Correct yes. Thinking more about this the real problem is to 
>> translate the handle into a structure in libdrm.
>>
>> Here we are back to the problem Marek and Michel has been working on 
>> for a while that we always need to be able to translate a handle into 
>> a bo structure.....
>>
>> So that needs to be solved before we can upstream the changes.
>
> Thanks for your info.
> It's better to fix that before upstream.

Thinking more about this the hash currently used in libdrm is not 
adequate any more.

E.g. we now need to be able to find all BOs based on their handle. Since 
the handles are dense either an r/b tree or a radix tree now sounds like 
the best approach to me.

Christian.

>
> Regards,
> Jerry

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhang, Jerry (Junwei)]

On 07/31/2018 04:13 PM, Christian König wrote:
> Am 31.07.2018 um 10:05 schrieb Zhang, Jerry (Junwei):
>> On 07/31/2018 03:03 PM, Christian König wrote:
>>> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>>>> On 07/30/2018 06:47 PM, Christian König wrote:
>>>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>>>> From: Chunming Zhou <David1.Zhou@amd.com>
>>>>>>
>>>>>> v2: get original gem handle from gobj
>>>>>> v3: update find bo data structure as union(in, out)
>>>>>>      simply some code logic
>>>>>
>>>>> Do we now have an open source user for this, so that we can upstream it? One more point below.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
>>>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com> (v3)
>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  2 ++
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 63 +++++++++++++++++++++++++++++++++
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +-
>>>>>>   include/uapi/drm/amdgpu_drm.h           | 21 +++++++++++
>>>>>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> index 4cd20e7..46c370b 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>> @@ -1213,6 +1213,8 @@ int amdgpu_gem_info_ioctl(struct drm_device *dev, void *data,
>>>>>>                 struct drm_file *filp);
>>>>>>   int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void *data,
>>>>>>               struct drm_file *filp);
>>>>>> +int amdgpu_gem_find_bo_by_cpu_mapping_ioctl(struct drm_device *dev, void *data,
>>>>>> +                        struct drm_file *filp);
>>>>>>   int amdgpu_gem_mmap_ioctl(struct drm_device *dev, void *data,
>>>>>>                 struct drm_file *filp);
>>>>>>   int amdgpu_gem_wait_idle_ioctl(struct drm_device *dev, void *data,
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> index 71792d8..bae8417 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> @@ -288,6 +288,69 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
>>>>>>       return 0;
>>>>>>   }
>>>>>> +static int amdgpu_gem_get_handle_from_object(struct drm_file *filp,
>>>>>> +                         struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    int i;
>>>>>> +    struct drm_gem_object *tmp;
>>>>>> +
>>>>>> +    spin_lock(&filp->table_lock);
>>>>>> +    idr_for_each_entry(&filp->object_idr, tmp, i) {
>>>>>> +        if (obj == tmp) {
>>>>>> +            drm_gem_object_reference(obj);
>>>>>> +            spin_unlock(&filp->table_lock);
>>>>>> +            return i;
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> Please double check if that is still up to date.
>>>>
>>>> We may have to replace drm_gem_object_reference() with drm_gem_object_get().
>>>>
>>>> On 2nd thought, do we really need to do reference every time?
>>>
>>> Yes, that's a must have. Otherwise the handle could be freed and reused already when we return.
>>>
>>>> if UMD find the same gem object for 3 times, it also need to explicitly free(put) that object for 3 times?
>>>
>>> Correct yes. Thinking more about this the real problem is to translate the handle into a structure in libdrm.
>>>
>>> Here we are back to the problem Marek and Michel has been working on for a while that we always need to be able to translate a handle into a bo structure.....
>>>
>>> So that needs to be solved before we can upstream the changes.
>>
>> Thanks for your info.
>> It's better to fix that before upstream.
>
> Thinking more about this the hash currently used in libdrm is not adequate any more.
>
> E.g. we now need to be able to find all BOs based on their handle. Since the handles are dense either an r/b tree or a radix tree now sounds like the best approach to me.

Not sure the exact reason that we added hash table in libdrm.
But it really costs much less time than calling IOCTL to find BO by their handles.

In this case, UMD seems not to be able to get BO handle and try to verify it by cpu address then.
In another word, UMD would like to find if the memory is created as BO or system memory, I suppose.

Regards,
Jerry


>
> Christian.
>
>>
>> Regards,
>> Jerry
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 31.07.2018 um 10:58 schrieb Zhang, Jerry (Junwei):
> On 07/31/2018 04:13 PM, Christian König wrote:
>> Am 31.07.2018 um 10:05 schrieb Zhang, Jerry (Junwei):
>>> On 07/31/2018 03:03 PM, Christian König wrote:
>>>> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>>>>> On 07/30/2018 06:47 PM, Christian König wrote:
>>>>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>>>> [SNIP]
>>>>>> Please double check if that is still up to date.
>>>>>
>>>>> We may have to replace drm_gem_object_reference() with 
>>>>> drm_gem_object_get().
>>>>>
>>>>> On 2nd thought, do we really need to do reference every time?
>>>>
>>>> Yes, that's a must have. Otherwise the handle could be freed and 
>>>> reused already when we return.
>>>>
>>>>> if UMD find the same gem object for 3 times, it also need to 
>>>>> explicitly free(put) that object for 3 times?
>>>>
>>>> Correct yes. Thinking more about this the real problem is to 
>>>> translate the handle into a structure in libdrm.
>>>>
>>>> Here we are back to the problem Marek and Michel has been working 
>>>> on for a while that we always need to be able to translate a handle 
>>>> into a bo structure.....
>>>>
>>>> So that needs to be solved before we can upstream the changes.
>>>
>>> Thanks for your info.
>>> It's better to fix that before upstream.
>>
>> Thinking more about this the hash currently used in libdrm is not 
>> adequate any more.
>>
>> E.g. we now need to be able to find all BOs based on their handle. 
>> Since the handles are dense either an r/b tree or a radix tree now 
>> sounds like the best approach to me.
>
> Not sure the exact reason that we added hash table in libdrm.

The reason for that was that when a kernel function returns a handle we 
need to make sure that we always use the same struct amdgpu_bo for it.

Otherwise you run into quite some problems with syncing etc...

> But it really costs much less time than calling IOCTL to find BO by 
> their handles.

Well we could just completely drop the kernel implementation and use an 
userspace implementation.

And yes I agree when we need a tree anyway it would probably be faster 
than calling the IOCTL to find the BO.

Christian.

>
> In this case, UMD seems not to be able to get BO handle and try to 
> verify it by cpu address then.
> In another word, UMD would like to find if the memory is created as BO 
> or system memory, I suppose.
>
> Regards,
> Jerry
>
>
>>
>> Christian.
>>
>>>
>>> Regards,
>>> Jerry
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhang, Jerry (Junwei)]

On 07/31/2018 05:04 PM, Christian König wrote:
> Am 31.07.2018 um 10:58 schrieb Zhang, Jerry (Junwei):
>> On 07/31/2018 04:13 PM, Christian König wrote:
>>> Am 31.07.2018 um 10:05 schrieb Zhang, Jerry (Junwei):
>>>> On 07/31/2018 03:03 PM, Christian König wrote:
>>>>> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>>>>>> On 07/30/2018 06:47 PM, Christian König wrote:
>>>>>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>>>>> [SNIP]
>>>>>>> Please double check if that is still up to date.
>>>>>>
>>>>>> We may have to replace drm_gem_object_reference() with drm_gem_object_get().
>>>>>>
>>>>>> On 2nd thought, do we really need to do reference every time?
>>>>>
>>>>> Yes, that's a must have. Otherwise the handle could be freed and reused already when we return.
>>>>>
>>>>>> if UMD find the same gem object for 3 times, it also need to explicitly free(put) that object for 3 times?
>>>>>
>>>>> Correct yes. Thinking more about this the real problem is to translate the handle into a structure in libdrm.
>>>>>
>>>>> Here we are back to the problem Marek and Michel has been working on for a while that we always need to be able to translate a handle into a bo structure.....
>>>>>
>>>>> So that needs to be solved before we can upstream the changes.
>>>>
>>>> Thanks for your info.
>>>> It's better to fix that before upstream.
>>>
>>> Thinking more about this the hash currently used in libdrm is not adequate any more.
>>>
>>> E.g. we now need to be able to find all BOs based on their handle. Since the handles are dense either an r/b tree or a radix tree now sounds like the best approach to me.
>>
>> Not sure the exact reason that we added hash table in libdrm.
>
> The reason for that was that when a kernel function returns a handle we need to make sure that we always use the same struct amdgpu_bo for it.
>
> Otherwise you run into quite some problems with syncing etc...

Thanks for your explanation.

>
>> But it really costs much less time than calling IOCTL to find BO by their handles.
>
> Well we could just completely drop the kernel implementation and use an userspace implementation.

Do you mean to implement finding bo by cpu address in libdrm completely?
e.g. to create a tree to manage bo handle in libdrm?

Jerry

>
> And yes I agree when we need a tree anyway it would probably be faster than calling the IOCTL to find the BO.
>
> Christian.
>
>>
>> In this case, UMD seems not to be able to get BO handle and try to verify it by cpu address then.
>> In another word, UMD would like to find if the memory is created as BO or system memory, I suppose.
>>
>> Regards,
>> Jerry
>>
>>
>>>
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> Jerry
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 31.07.2018 um 11:54 schrieb Zhang, Jerry (Junwei):
> On 07/31/2018 05:04 PM, Christian König wrote:
>> Am 31.07.2018 um 10:58 schrieb Zhang, Jerry (Junwei):
>>> On 07/31/2018 04:13 PM, Christian König wrote:
>>>> Am 31.07.2018 um 10:05 schrieb Zhang, Jerry (Junwei):
>>>>> On 07/31/2018 03:03 PM, Christian König wrote:
>>>>>> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>>>>>>> On 07/30/2018 06:47 PM, Christian König wrote:
>>>>>>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>>>>>> [SNIP]
>>>>>>>> Please double check if that is still up to date.
>>>>>>>
>>>>>>> We may have to replace drm_gem_object_reference() with 
>>>>>>> drm_gem_object_get().
>>>>>>>
>>>>>>> On 2nd thought, do we really need to do reference every time?
>>>>>>
>>>>>> Yes, that's a must have. Otherwise the handle could be freed and 
>>>>>> reused already when we return.
>>>>>>
>>>>>>> if UMD find the same gem object for 3 times, it also need to 
>>>>>>> explicitly free(put) that object for 3 times?
>>>>>>
>>>>>> Correct yes. Thinking more about this the real problem is to 
>>>>>> translate the handle into a structure in libdrm.
>>>>>>
>>>>>> Here we are back to the problem Marek and Michel has been working 
>>>>>> on for a while that we always need to be able to translate a 
>>>>>> handle into a bo structure.....
>>>>>>
>>>>>> So that needs to be solved before we can upstream the changes.
>>>>>
>>>>> Thanks for your info.
>>>>> It's better to fix that before upstream.
>>>>
>>>> Thinking more about this the hash currently used in libdrm is not 
>>>> adequate any more.
>>>>
>>>> E.g. we now need to be able to find all BOs based on their handle. 
>>>> Since the handles are dense either an r/b tree or a radix tree now 
>>>> sounds like the best approach to me.
>>>
>>> Not sure the exact reason that we added hash table in libdrm.
>>
>> The reason for that was that when a kernel function returns a handle 
>> we need to make sure that we always use the same struct amdgpu_bo for 
>> it.
>>
>> Otherwise you run into quite some problems with syncing etc...
>
> Thanks for your explanation.
>
>>
>>> But it really costs much less time than calling IOCTL to find BO by 
>>> their handles.
>>
>> Well we could just completely drop the kernel implementation and use 
>> an userspace implementation.
>
> Do you mean to implement finding bo by cpu address in libdrm completely?

Yes, exactly.

> e.g. to create a tree to manage bo handle in libdrm?

I mean when we need to create a tree to map the handle to a BO you could 
also create a tree to map the CPU pointer to the BO directly and avoid 
the IOCTL overhead completely.

Christian.

>
> Jerry
>
>>
>> And yes I agree when we need a tree anyway it would probably be 
>> faster than calling the IOCTL to find the BO.
>>
>> Christian.
>>
>>>
>>> In this case, UMD seems not to be able to get BO handle and try to 
>>> verify it by cpu address then.
>>> In another word, UMD would like to find if the memory is created as 
>>> BO or system memory, I suppose.
>>>
>>> Regards,
>>> Jerry
>>>
>>>
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> Regards,
>>>>> Jerry
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Marek Olšák]

Can this be implemented as a wrapper on top of libdrm? So that the
tree (or hash table) isn't created for UMDs that don't need it.

Marek

On Tue, Jul 31, 2018 at 6:13 AM, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
> Am 31.07.2018 um 11:54 schrieb Zhang, Jerry (Junwei):
>>
>> On 07/31/2018 05:04 PM, Christian König wrote:
>>>
>>> Am 31.07.2018 um 10:58 schrieb Zhang, Jerry (Junwei):
>>>>
>>>> On 07/31/2018 04:13 PM, Christian König wrote:
>>>>>
>>>>> Am 31.07.2018 um 10:05 schrieb Zhang, Jerry (Junwei):
>>>>>>
>>>>>> On 07/31/2018 03:03 PM, Christian König wrote:
>>>>>>>
>>>>>>> Am 31.07.2018 um 08:58 schrieb Zhang, Jerry (Junwei):
>>>>>>>>
>>>>>>>> On 07/30/2018 06:47 PM, Christian König wrote:
>>>>>>>>>
>>>>>>>>> Am 30.07.2018 um 12:02 schrieb Junwei Zhang:
>>>>>>>>> [SNIP]
>>>>>>>>> Please double check if that is still up to date.
>>>>>>>>
>>>>>>>>
>>>>>>>> We may have to replace drm_gem_object_reference() with
>>>>>>>> drm_gem_object_get().
>>>>>>>>
>>>>>>>> On 2nd thought, do we really need to do reference every time?
>>>>>>>
>>>>>>>
>>>>>>> Yes, that's a must have. Otherwise the handle could be freed and
>>>>>>> reused already when we return.
>>>>>>>
>>>>>>>> if UMD find the same gem object for 3 times, it also need to
>>>>>>>> explicitly free(put) that object for 3 times?
>>>>>>>
>>>>>>>
>>>>>>> Correct yes. Thinking more about this the real problem is to
>>>>>>> translate the handle into a structure in libdrm.
>>>>>>>
>>>>>>> Here we are back to the problem Marek and Michel has been working on
>>>>>>> for a while that we always need to be able to translate a handle into a bo
>>>>>>> structure.....
>>>>>>>
>>>>>>> So that needs to be solved before we can upstream the changes.
>>>>>>
>>>>>>
>>>>>> Thanks for your info.
>>>>>> It's better to fix that before upstream.
>>>>>
>>>>>
>>>>> Thinking more about this the hash currently used in libdrm is not
>>>>> adequate any more.
>>>>>
>>>>> E.g. we now need to be able to find all BOs based on their handle.
>>>>> Since the handles are dense either an r/b tree or a radix tree now sounds
>>>>> like the best approach to me.
>>>>
>>>>
>>>> Not sure the exact reason that we added hash table in libdrm.
>>>
>>>
>>> The reason for that was that when a kernel function returns a handle we
>>> need to make sure that we always use the same struct amdgpu_bo for it.
>>>
>>> Otherwise you run into quite some problems with syncing etc...
>>
>>
>> Thanks for your explanation.
>>
>>>
>>>> But it really costs much less time than calling IOCTL to find BO by
>>>> their handles.
>>>
>>>
>>> Well we could just completely drop the kernel implementation and use an
>>> userspace implementation.
>>
>>
>> Do you mean to implement finding bo by cpu address in libdrm completely?
>
>
> Yes, exactly.
>
>> e.g. to create a tree to manage bo handle in libdrm?
>
>
> I mean when we need to create a tree to map the handle to a BO you could
> also create a tree to map the CPU pointer to the BO directly and avoid the
> IOCTL overhead completely.
>
> Christian.
>
>
>>
>> Jerry
>>
>>>
>>> And yes I agree when we need a tree anyway it would probably be faster
>>> than calling the IOCTL to find the BO.
>>>
>>> Christian.
>>>
>>>>
>>>> In this case, UMD seems not to be able to get BO handle and try to
>>>> verify it by cpu address then.
>>>> In another word, UMD would like to find if the memory is created as BO
>>>> or system memory, I suppose.
>>>>
>>>> Regards,
>>>> Jerry
>>>>
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Jerry
>>>>>
>>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 01.08.2018 um 00:07 schrieb Marek Olšák:
> Can this be implemented as a wrapper on top of libdrm? So that the
> tree (or hash table) isn't created for UMDs that don't need it.

No, the problem is that an application gets a CPU pointer from one API 
and tries to import that pointer into another one.

In other words we need to implement this independent of the UMD who 
mapped the BO.

Christian.

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Marek Olšák]

On Wed, Aug 1, 2018 at 2:32 AM, Christian König
<christian.koenig@amd.com> wrote:
> Am 01.08.2018 um 00:07 schrieb Marek Olšák:
>>
>> Can this be implemented as a wrapper on top of libdrm? So that the
>> tree (or hash table) isn't created for UMDs that don't need it.
>
>
> No, the problem is that an application gets a CPU pointer from one API and
> tries to import that pointer into another one.
>
> In other words we need to implement this independent of the UMD who mapped
> the BO.

Yeah, it could be an optional feature of libdrm, and other components
should be able to disable it to remove the overhead.

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 01.08.2018 um 19:39 schrieb Marek Olšák:
> On Wed, Aug 1, 2018 at 2:32 AM, Christian König
> <christian.koenig@amd.com> wrote:
>> Am 01.08.2018 um 00:07 schrieb Marek Olšák:
>>> Can this be implemented as a wrapper on top of libdrm? So that the
>>> tree (or hash table) isn't created for UMDs that don't need it.
>>
>> No, the problem is that an application gets a CPU pointer from one API and
>> tries to import that pointer into another one.
>>
>> In other words we need to implement this independent of the UMD who mapped
>> the BO.
> Yeah, it could be an optional feature of libdrm, and other components
> should be able to disable it to remove the overhead.

The overhead is negligible, the real problem is the memory footprint.

A brief look at the hash implementation in libdrm showed that this is 
actually really inefficient.

I think we have the choice of implementing a r/b tree to map the CPU 
pointer addresses or implement a quadratic tree to map the handles.

The later is easy to do and would also allow to get rid of the hash 
table as well.

Christian.

>
> Marek

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Marek Olšák]

On Wed, Aug 1, 2018 at 1:52 PM, Christian König
<christian.koenig@amd.com> wrote:
> Am 01.08.2018 um 19:39 schrieb Marek Olšák:
>>
>> On Wed, Aug 1, 2018 at 2:32 AM, Christian König
>> <christian.koenig@amd.com> wrote:
>>>
>>> Am 01.08.2018 um 00:07 schrieb Marek Olšák:
>>>>
>>>> Can this be implemented as a wrapper on top of libdrm? So that the
>>>> tree (or hash table) isn't created for UMDs that don't need it.
>>>
>>>
>>> No, the problem is that an application gets a CPU pointer from one API
>>> and
>>> tries to import that pointer into another one.
>>>
>>> In other words we need to implement this independent of the UMD who
>>> mapped
>>> the BO.
>>
>> Yeah, it could be an optional feature of libdrm, and other components
>> should be able to disable it to remove the overhead.
>
>
> The overhead is negligible, the real problem is the memory footprint.
>
> A brief look at the hash implementation in libdrm showed that this is
> actually really inefficient.
>
> I think we have the choice of implementing a r/b tree to map the CPU pointer
> addresses or implement a quadratic tree to map the handles.
>
> The later is easy to do and would also allow to get rid of the hash table as
> well.

We can also use the hash table from mesa/src/util.

I don't think the overhead would be negligible. It would be a log(n)
insertion in bo_map and a log(n) deletion in bo_unmap. If you did
bo_map+bo_unmap 10000 times, would it be negligible?

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 01.08.2018 um 19:59 schrieb Marek Olšák:
> On Wed, Aug 1, 2018 at 1:52 PM, Christian König
> <christian.koenig@amd.com> wrote:
>> Am 01.08.2018 um 19:39 schrieb Marek Olšák:
>>> On Wed, Aug 1, 2018 at 2:32 AM, Christian König
>>> <christian.koenig@amd.com> wrote:
>>>> Am 01.08.2018 um 00:07 schrieb Marek Olšák:
>>>>> Can this be implemented as a wrapper on top of libdrm? So that the
>>>>> tree (or hash table) isn't created for UMDs that don't need it.
>>>>
>>>> No, the problem is that an application gets a CPU pointer from one API
>>>> and
>>>> tries to import that pointer into another one.
>>>>
>>>> In other words we need to implement this independent of the UMD who
>>>> mapped
>>>> the BO.
>>> Yeah, it could be an optional feature of libdrm, and other components
>>> should be able to disable it to remove the overhead.
>>
>> The overhead is negligible, the real problem is the memory footprint.
>>
>> A brief look at the hash implementation in libdrm showed that this is
>> actually really inefficient.
>>
>> I think we have the choice of implementing a r/b tree to map the CPU pointer
>> addresses or implement a quadratic tree to map the handles.
>>
>> The later is easy to do and would also allow to get rid of the hash table as
>> well.
> We can also use the hash table from mesa/src/util.
>
> I don't think the overhead would be negligible. It would be a log(n)
> insertion in bo_map and a log(n) deletion in bo_unmap. If you did
> bo_map+bo_unmap 10000 times, would it be negligible?

Compared to what the kernel needs to do for updating the page tables it 
is less than 1% of the total work.

The real question is if it wouldn't be simpler to use a tree for the 
handles. Since the handles are dense you can just use an unbalanced tree 
which is really easy.

For a tree of the CPU mappings we would need an r/b interval tree, which 
is hard to implement and quite some overkill.

Do you have any numbers how many BOs really get a CPU mapping in a real 
world application?

Christian.

>
> Marek

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Marek Olšák]

On Wed, Aug 1, 2018 at 2:29 PM, Christian König
<christian.koenig@amd.com> wrote:
> Am 01.08.2018 um 19:59 schrieb Marek Olšák:
>>
>> On Wed, Aug 1, 2018 at 1:52 PM, Christian König
>> <christian.koenig@amd.com> wrote:
>>>
>>> Am 01.08.2018 um 19:39 schrieb Marek Olšák:
>>>>
>>>> On Wed, Aug 1, 2018 at 2:32 AM, Christian König
>>>> <christian.koenig@amd.com> wrote:
>>>>>
>>>>> Am 01.08.2018 um 00:07 schrieb Marek Olšák:
>>>>>>
>>>>>> Can this be implemented as a wrapper on top of libdrm? So that the
>>>>>> tree (or hash table) isn't created for UMDs that don't need it.
>>>>>
>>>>>
>>>>> No, the problem is that an application gets a CPU pointer from one API
>>>>> and
>>>>> tries to import that pointer into another one.
>>>>>
>>>>> In other words we need to implement this independent of the UMD who
>>>>> mapped
>>>>> the BO.
>>>>
>>>> Yeah, it could be an optional feature of libdrm, and other components
>>>> should be able to disable it to remove the overhead.
>>>
>>>
>>> The overhead is negligible, the real problem is the memory footprint.
>>>
>>> A brief look at the hash implementation in libdrm showed that this is
>>> actually really inefficient.
>>>
>>> I think we have the choice of implementing a r/b tree to map the CPU
>>> pointer
>>> addresses or implement a quadratic tree to map the handles.
>>>
>>> The later is easy to do and would also allow to get rid of the hash table
>>> as
>>> well.
>>
>> We can also use the hash table from mesa/src/util.
>>
>> I don't think the overhead would be negligible. It would be a log(n)
>> insertion in bo_map and a log(n) deletion in bo_unmap. If you did
>> bo_map+bo_unmap 10000 times, would it be negligible?
>
>
> Compared to what the kernel needs to do for updating the page tables it is
> less than 1% of the total work.
>
> The real question is if it wouldn't be simpler to use a tree for the
> handles. Since the handles are dense you can just use an unbalanced tree
> which is really easy.
>
> For a tree of the CPU mappings we would need an r/b interval tree, which is
> hard to implement and quite some overkill.
>
> Do you have any numbers how many BOs really get a CPU mapping in a real
> world application?

Without our suballocator, we sometimes exceeded the max. mmap limit
(~64K). It should be much less with the suballocator with 128KB slabs,
probably a few thousands.

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Zhang, Jerry (Junwei)]

On 08/02/2018 08:00 AM, Marek Olšák wrote:
> On Wed, Aug 1, 2018 at 2:29 PM, Christian König
> <christian.koenig@amd.com> wrote:
>> Am 01.08.2018 um 19:59 schrieb Marek Olšák:
>>>
>>> On Wed, Aug 1, 2018 at 1:52 PM, Christian König
>>> <christian.koenig@amd.com> wrote:
>>>>
>>>> Am 01.08.2018 um 19:39 schrieb Marek Olšák:
>>>>>
>>>>> On Wed, Aug 1, 2018 at 2:32 AM, Christian König
>>>>> <christian.koenig@amd.com> wrote:
>>>>>>
>>>>>> Am 01.08.2018 um 00:07 schrieb Marek Olšák:
>>>>>>>
>>>>>>> Can this be implemented as a wrapper on top of libdrm? So that the
>>>>>>> tree (or hash table) isn't created for UMDs that don't need it.
>>>>>>
>>>>>>
>>>>>> No, the problem is that an application gets a CPU pointer from one API
>>>>>> and
>>>>>> tries to import that pointer into another one.
>>>>>>
>>>>>> In other words we need to implement this independent of the UMD who
>>>>>> mapped
>>>>>> the BO.
>>>>>
>>>>> Yeah, it could be an optional feature of libdrm, and other components
>>>>> should be able to disable it to remove the overhead.
>>>>
>>>>
>>>> The overhead is negligible, the real problem is the memory footprint.
>>>>
>>>> A brief look at the hash implementation in libdrm showed that this is
>>>> actually really inefficient.
>>>>
>>>> I think we have the choice of implementing a r/b tree to map the CPU
>>>> pointer
>>>> addresses or implement a quadratic tree to map the handles.
>>>>
>>>> The later is easy to do and would also allow to get rid of the hash table
>>>> as
>>>> well.
>>>
>>> We can also use the hash table from mesa/src/util.
>>>
>>> I don't think the overhead would be negligible. It would be a log(n)
>>> insertion in bo_map and a log(n) deletion in bo_unmap. If you did
>>> bo_map+bo_unmap 10000 times, would it be negligible?
>>
>>
>> Compared to what the kernel needs to do for updating the page tables it is
>> less than 1% of the total work.
>>
>> The real question is if it wouldn't be simpler to use a tree for the
>> handles. Since the handles are dense you can just use an unbalanced tree
>> which is really easy.
>>
>> For a tree of the CPU mappings we would need an r/b interval tree, which is
>> hard to implement and quite some overkill.
>>
>> Do you have any numbers how many BOs really get a CPU mapping in a real
>> world application?
>
> Without our suballocator, we sometimes exceeded the max. mmap limit
> (~64K). It should be much less with the suballocator with 128KB slabs,
> probably a few thousands.

Is there a way to verify that it has performance issue if moving the cpu mapping in libdrm hash table
from kernel side?

AFAIW, only one game will use that with close OGL.

Regards,
Jerry

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

Re: [PATCH 1/2] drm/amdgpu: return bo itself if userptr is cpu addr of bo (v3)

[Christian König]

Am 02.08.2018 um 08:20 schrieb Zhang, Jerry (Junwei):
> On 08/02/2018 08:00 AM, Marek Olšák wrote:
>> On Wed, Aug 1, 2018 at 2:29 PM, Christian König
>> <christian.koenig@amd.com> wrote:
>>> Am 01.08.2018 um 19:59 schrieb Marek Olšák:
>>>> [SNIP]
>>>
>>> Compared to what the kernel needs to do for updating the page tables 
>>> it is
>>> less than 1% of the total work.
>>>
>>> The real question is if it wouldn't be simpler to use a tree for the
>>> handles. Since the handles are dense you can just use an unbalanced 
>>> tree
>>> which is really easy.
>>>
>>> For a tree of the CPU mappings we would need an r/b interval tree, 
>>> which is
>>> hard to implement and quite some overkill.
>>>
>>> Do you have any numbers how many BOs really get a CPU mapping in a real
>>> world application?
>>
>> Without our suballocator, we sometimes exceeded the max. mmap limit
>> (~64K). It should be much less with the suballocator with 128KB slabs,
>> probably a few thousands.
>
> Is there a way to verify that it has performance issue if moving the 
> cpu mapping in libdrm hash table
> from kernel side?

Going to take a look at this, but I have the feeling that the CPU 
overhead we need to add in userspace isn't that much.

>
> AFAIW, only one game will use that with close OGL.

Well, I would also say that this is a bug in the game and should be 
fixed there.

Christian.

>
> Regards,
> Jerry
>
>>
>> Marek
>>

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