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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 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.

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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux