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

ckoenig.leichtzumerken@xxxxxxxxx (Christian König) · Tue, 31 Jul 2018 09:03:43 +0200

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 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.
>
> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx