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

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

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