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@xxxxxxxxx>
Sent: Monday, July 30, 2018 6:48 PM
To: Zhang, Jerry <Jerry.Zhang at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Zhou, David(ChunMing) <David1.Zhou at amd.com>; Koenig, Christian <Christian.Koenig at 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 at 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 at amd.com>
> Signed-off-by: Junwei Zhang <Jerry.Zhang at amd.com> (v3)
> Reviewed-by: Christian König <christian.koenig at amd.com>
> Reviewed-by: Jammy Zhou <Jammy.Zhou at 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(¤t->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(¤t->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(¤t->mm->mmap_sem);
> + return r;
> + }
> + }
> + args->out.handle = handle;
> + up_read(¤t->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
