Re: [PATCH 2/2] amdgpu: add amdgpu_find_bo_by_cpu_mapping interface

From: "Christian König" <christian.koenig@amd.com>
To: Chunming Zhou <David1.Zhou@amd.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/2] amdgpu: add amdgpu_find_bo_by_cpu_mapping interface
Date: Fri, 4 Dec 2015 09:32:46 +0100	[thread overview]
Message-ID: <56614FAE.1020200@amd.com> (raw)
In-Reply-To: <1449210353-24082-3-git-send-email-David1.Zhou@amd.com>

On 04.12.2015 07:25, Chunming Zhou wrote:
> userspace needs to know if the user memory is from BO or malloc.
>
> Change-Id: Ie2dbc13f1c02bc0a996f64f9db83a21da63c1d70
> Signed-off-by: Chunming Zhou <David1.Zhou@amd.com>
> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
Looks like I was a bit too quick with my rb. After reading the code once 
more I've found we missed something important, see below.

> +	args.addr = (uintptr_t)cpu;
> +	args.size = size;

You need to grab the bo_table_mutex here, otherwise we could race with 
closing the handle and accessing bo_handles.

That could certainly cause a crash.

Regards,
Christian.

> +	r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_GEM_FIND_BO,
> +				&args, sizeof(args));
> +	if (r)
> +		return r;
> +	if (args.handle == 0)
> +		return -EINVAL;
> +	bo = util_hash_table_get(dev->bo_handles,
> +				 (void*)(uintptr_t)args.handle);
> +	if (!bo) {
> +		bo = calloc(1, sizeof(struct amdgpu_bo));
> +		if (!bo)
> +			return -ENOMEM;
> +		atomic_set(&bo->refcount, 1);
> +		bo->dev = dev;
> +		bo->alloc_size = size;
> +		bo->handle = args.handle;
> +	} else
> +		atomic_inc(&bo->refcount);
> +
> +	*buf_handle = bo;
> +	*offset_in_bo = args.offset;
> +	return r;
> +}
> +
> +
>   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,
>   				    void *cpu,
>   				    uint64_t size,
> diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
> index 050e7fe..e07904c 100644
> --- a/include/drm/amdgpu_drm.h
> +++ b/include/drm/amdgpu_drm.h
> @@ -47,6 +47,7 @@
>   #define DRM_AMDGPU_GEM_OP		0x10
>   #define DRM_AMDGPU_GEM_USERPTR		0x11
>   #define DRM_AMDGPU_WAIT_FENCES		0x12
> +#define DRM_AMDGPU_GEM_FIND_BO          0x13
>   
>   #define DRM_IOCTL_AMDGPU_GEM_CREATE	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_CREATE, union drm_amdgpu_gem_create)
>   #define DRM_IOCTL_AMDGPU_GEM_MMAP	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_MMAP, union drm_amdgpu_gem_mmap)
> @@ -61,6 +62,7 @@
>   #define DRM_IOCTL_AMDGPU_GEM_OP		DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_OP, struct drm_amdgpu_gem_op)
>   #define DRM_IOCTL_AMDGPU_GEM_USERPTR	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_USERPTR, struct drm_amdgpu_gem_userptr)
>   #define DRM_IOCTL_AMDGPU_WAIT_FENCES	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_WAIT_FENCES, union drm_amdgpu_wait_fences)
> +#define DRM_IOCTL_AMDGPU_GEM_FIND_BO      DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_FIND_BO, struct drm_amdgpu_gem_find_bo)
>   
>   #define AMDGPU_GEM_DOMAIN_CPU		0x1
>   #define AMDGPU_GEM_DOMAIN_GTT		0x2
> @@ -201,6 +203,16 @@ struct drm_amdgpu_gem_userptr {
>   	uint32_t		handle;
>   };
>   
> +struct drm_amdgpu_gem_find_bo {
> +       uint64_t                addr;
> +       uint64_t                size;
> +       uint32_t                flags;
> +       /* Resulting GEM handle */
> +       uint32_t                handle;
> +       /* offset in bo */
> +       uint64_t                offset;
> +};
> +
>   /* same meaning as the GB_TILE_MODE and GL_MACRO_TILE_MODE fields */
>   #define AMDGPU_TILING_ARRAY_MODE_SHIFT			0
>   #define AMDGPU_TILING_ARRAY_MODE_MASK			0xf

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel