On 07/11/2018 11:05 AM, Christian König wrote:
> Am 11.07.2018 um 16:51 schrieb Andrey Grodzovsky:
>> This change is to support MESA performace optimization.
>> Modify CS IOCTL to allow its input as command buffer and an array of
>> buffer handles to create a temporay bo list and then destroy it
>> when IOCTL completes.
>> This saves on calling for BO_LIST create and destry IOCTLs in MESA
>> and by this improves performance.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 12 ++++++
>> Â drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 62
>> ++++++++++++++++++-----------
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c     | 52
>> +++++++++++++++++++++---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 3 +-
>>  include/uapi/drm/amdgpu_drm.h              | 1 +
>> Â 5 files changed, 101 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 8eaba0f..e082eba 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -732,6 +732,16 @@ void amdgpu_bo_list_get_list(struct
>> amdgpu_bo_list *list,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct list_head *validated);
>> Â void amdgpu_bo_list_put(struct amdgpu_bo_list *list);
>> Â void amdgpu_bo_list_free(struct amdgpu_bo_list *list);
>> +int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_amdgpu_bo_list_entry **info_param);
>> +
>> +int amdgpu_bo_list_create(struct amdgpu_device *adev,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_file *filp,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_amdgpu_bo_list_entry *info,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned num_entries,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int *id);
>> +
>> +void amdgpu_bo_list_destroy(struct amdgpu_fpriv *fpriv, int id);
>> Â Â /*
>> Â Â * GFX stuff
>> @@ -1048,6 +1058,8 @@ struct amdgpu_cs_parser {
>> Â Â Â Â Â Â unsigned num_post_dep_syncobjs;
>> Â Â Â Â Â struct drm_syncobj **post_dep_syncobjs;
>> +
>> +Â Â Â bool destroy_bo_list;
>
> I think you don't need this. IIRC the bo_list structure was reference
> counted.
>
> So all you need to do is to make sure that the temporary bo_list you
> create has a reference count of 1 and so is destroyed when the CS
> IOCTL calls amdgpu_bo_list_put() on it.
>
> That would simplify the patch quite a bit.
amdgpu_bo_list_destroy is essential because it removes the list from idr
struct bo_list_handles, amdgpu_bo_list_put doesn't do it.
Regarding the refcount, it's 2 because it's 1 on list create in
amdgpu_cs_bo_handles_chunk->amdgpu_bo_list_create and then it's incremented
to 2 in amdgpu_cs_parser_bos->amdgpu_bo_list_get. So by calling
amdgpu_bo_list_put and then amdgpu_bo_list_destroy the count properly
drops down to 0.
Andrey
>
> Apart from that looks really good to me, especially that you only need
> a new chunk ID for the UAPI is quite nice.
>
> Thanks,
> Christian.
>
>> Â };
>> Â Â #define AMDGPU_PREAMBLE_IB_PRESENTÂ Â Â Â Â Â Â Â Â (1 << 0) /* bit set
>> means command submit involves a preamble IB */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> index 92be7f6..9acdacc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> @@ -55,7 +55,7 @@ static void amdgpu_bo_list_release_rcu(struct kref
>> *ref)
>> Â Â Â Â Â kfree_rcu(list, rhead);
>> Â }
>> Â -static int amdgpu_bo_list_create(struct amdgpu_device *adev,
>> +int amdgpu_bo_list_create(struct amdgpu_device *adev,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_file *filp,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_amdgpu_bo_list_entry *info,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned num_entries,
>> @@ -91,7 +91,7 @@ static int amdgpu_bo_list_create(struct
>> amdgpu_device *adev,
>> Â Â Â Â Â return 0;
>> Â }
>> Â -static void amdgpu_bo_list_destroy(struct amdgpu_fpriv *fpriv, int
>> id)
>> +void amdgpu_bo_list_destroy(struct amdgpu_fpriv *fpriv, int id)
>> Â {
>> Â Â Â Â Â struct amdgpu_bo_list *list;
>> Â @@ -263,49 +263,64 @@ void amdgpu_bo_list_free(struct
>> amdgpu_bo_list *list)
>> Â Â Â Â Â kfree(list);
>> Â }
>> Â -int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_file *filp)
>> +int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_amdgpu_bo_list_entry **info_param)
>> Â {
>> -Â Â Â const uint32_t info_size = sizeof(struct drm_amdgpu_bo_list_entry);
>> -
>> -Â Â Â struct amdgpu_device *adev = dev->dev_private;
>> -Â Â Â struct amdgpu_fpriv *fpriv = filp->driver_priv;
>> -Â Â Â union drm_amdgpu_bo_list *args = data;
>> -Â Â Â uint32_t handle = args->in.list_handle;
>> -Â Â Â const void __user *uptr = u64_to_user_ptr(args->in.bo_info_ptr);
>> -
>> Â Â Â Â Â struct drm_amdgpu_bo_list_entry *info;
>> -Â Â Â struct amdgpu_bo_list *list;
>> -
>> Â Â Â Â Â int r;
>> +Â Â Â const void __user *uptr = u64_to_user_ptr(in->bo_info_ptr);
>> +Â Â Â const uint32_t info_size = sizeof(struct drm_amdgpu_bo_list_entry);
>> Â -Â Â Â info = kvmalloc_array(args->in.bo_number,
>> +Â Â Â info = kvmalloc_array(in->bo_number,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â sizeof(struct drm_amdgpu_bo_list_entry), GFP_KERNEL);
>> Â Â Â Â Â if (!info)
>> Â Â Â Â Â Â Â Â Â return -ENOMEM;
>> Â Â Â Â Â Â /* copy the handle array from userspace to a kernel buffer */
>> Â Â Â Â Â r = -EFAULT;
>> -Â Â Â if (likely(info_size == args->in.bo_info_size)) {
>> -Â Â Â Â Â Â Â unsigned long bytes = args->in.bo_number *
>> -Â Â Â Â Â Â Â Â Â Â Â args->in.bo_info_size;
>> +Â Â Â if (likely(info_size == in->bo_info_size)) {
>> +Â Â Â Â Â Â Â unsigned long bytes = in->bo_number *
>> +Â Â Â Â Â Â Â Â Â Â Â in->bo_info_size;
>> Â Â Â Â Â Â Â Â Â Â if (copy_from_user(info, uptr, bytes))
>> Â Â Â Â Â Â Â Â Â Â Â Â Â goto error_free;
>> Â Â Â Â Â Â } else {
>> -Â Â Â Â Â Â Â unsigned long bytes = min(args->in.bo_info_size, info_size);
>> +Â Â Â Â Â Â Â unsigned long bytes = min(in->bo_info_size, info_size);
>> Â Â Â Â Â Â Â Â Â unsigned i;
>> Â -Â Â Â Â Â Â Â memset(info, 0, args->in.bo_number * info_size);
>> -Â Â Â Â Â Â Â for (i = 0; i < args->in.bo_number; ++i) {
>> +Â Â Â Â Â Â Â memset(info, 0, in->bo_number * info_size);
>> +Â Â Â Â Â Â Â for (i = 0; i < in->bo_number; ++i) {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â if (copy_from_user(&info[i], uptr, bytes))
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto error_free;
>> Â -Â Â Â Â Â Â Â Â Â Â Â uptr += args->in.bo_info_size;
>> +Â Â Â Â Â Â Â Â Â Â Â uptr += in->bo_info_size;
>> Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â }
>> Â +Â Â Â *info_param = info;
>> +Â Â Â return 0;
>> +
>> +error_free:
>> +Â Â Â kvfree(info);
>> +Â Â Â return r;
>> +}
>> +
>> +int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_file *filp)
>> +{
>> +Â Â Â struct amdgpu_device *adev = dev->dev_private;
>> +Â Â Â struct amdgpu_fpriv *fpriv = filp->driver_priv;
>> +Â Â Â union drm_amdgpu_bo_list *args = data;
>> +Â Â Â uint32_t handle = args->in.list_handle;
>> +Â Â Â struct drm_amdgpu_bo_list_entry *info = NULL;
>> +Â Â Â struct amdgpu_bo_list *list;
>> +Â Â Â int r;
>> +
>> +Â Â Â r = amdgpu_bo_create_list_entry_array(&args->in, &info);
>> +Â Â Â if (r)
>> +Â Â Â Â Â Â Â goto error_free;
>> +
>> Â Â Â Â Â switch (args->in.operation) {
>> Â Â Â Â Â case AMDGPU_BO_LIST_OP_CREATE:
>> Â Â Â Â Â Â Â Â Â r = amdgpu_bo_list_create(adev, filp, info,
>> args->in.bo_number,
>> @@ -345,6 +360,7 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev,
>> void *data,
>> Â Â Â Â Â return 0;
>> Â Â error_free:
>> -Â Â Â kvfree(info);
>> +Â Â Â if (info)
>> +Â Â Â Â Â Â Â kvfree(info);
>> Â Â Â Â Â return r;
>> Â }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 9881a1e..1b17f6b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -66,11 +66,35 @@ static int amdgpu_cs_user_fence_chunk(struct
>> amdgpu_cs_parser *p,
>> Â Â Â Â Â return 0;
>> Â }
>> Â -static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void
>> *data)
>> +static int amdgpu_cs_bo_handles_chunk(struct amdgpu_cs_parser *p,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_amdgpu_bo_list_in *data,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int *id)
>> +{
>> +Â Â Â int r;
>> +Â Â Â struct drm_amdgpu_bo_list_entry *info = NULL;
>> +
>> +Â Â Â r = amdgpu_bo_create_list_entry_array(data, &info);
>> +Â Â Â if (r)
>> +Â Â Â Â Â Â Â return r;
>> +
>> +Â Â Â r = amdgpu_bo_list_create(p->adev, p->filp, info,
>> data->bo_number, id);
>> +Â Â Â if (r)
>> +Â Â Â Â Â Â Â goto error_free;
>> +
>> +Â Â Â kvfree(info);
>> +Â Â Â return 0;
>> +
>> +error_free:
>> +Â Â Â if (info)
>> +Â Â Â Â Â Â Â kvfree(info);
>> +
>> +Â Â Â return r;
>> +}
>> +
>> +static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union
>> drm_amdgpu_cs *cs)
>> Â {
>> Â Â Â Â Â struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
>> Â Â Â Â Â struct amdgpu_vm *vm = &fpriv->vm;
>> -Â Â Â union drm_amdgpu_cs *cs = data;
>> Â Â Â Â Â uint64_t *chunk_array_user;
>> Â Â Â Â Â uint64_t *chunk_array;
>> Â Â Â Â Â unsigned size, num_ibs = 0;
>> @@ -164,6 +188,20 @@ static int amdgpu_cs_parser_init(struct
>> amdgpu_cs_parser *p, void *data)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>> Â +Â Â Â Â Â Â Â case AMDGPU_CHUNK_ID_BO_HANDLES:
>> +Â Â Â Â Â Â Â Â Â Â Â size = sizeof(struct drm_amdgpu_bo_list_in);
>> +Â Â Â Â Â Â Â Â Â Â Â if (p->chunks[i].length_dw * sizeof(uint32_t) < size) {
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret = -EINVAL;
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto free_partial_kdata;
>> +Â Â Â Â Â Â Â Â Â Â Â }
>> +
>> +Â Â Â Â Â Â Â Â Â Â Â ret = amdgpu_cs_bo_handles_chunk(p, p->chunks[i].kdata,
>> &cs->in.bo_list_handle);
>> +Â Â Â Â Â Â Â Â Â Â Â p->destroy_bo_list = true;
>> +Â Â Â Â Â Â Â Â Â Â Â if (ret)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â goto free_partial_kdata;
>> +
>> +Â Â Â Â Â Â Â Â Â Â Â break;
>> +
>> Â Â Â Â Â Â Â Â Â case AMDGPU_CHUNK_ID_DEPENDENCIES:
>> Â Â Â Â Â Â Â Â Â case AMDGPU_CHUNK_ID_SYNCOBJ_IN:
>> Â Â Â Â Â Â Â Â Â case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
>> @@ -747,7 +785,7 @@ static int amdgpu_cs_sync_rings(struct
>> amdgpu_cs_parser *p)
>> Â Â * used by parsing context.
>> Â Â **/
>> Â static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser,
>> int error,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bool backoff)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â bool backoff, int id)
>> Â {
>> Â Â Â Â Â unsigned i;
>> Â @@ -765,9 +803,13 @@ static void amdgpu_cs_parser_fini(struct
>> amdgpu_cs_parser *parser, int error,
>> Â Â Â Â Â Â Â Â Â mutex_unlock(&parser->ctx->lock);
>> Â Â Â Â Â Â Â Â Â amdgpu_ctx_put(parser->ctx);
>> Â Â Â Â Â }
>> -Â Â Â if (parser->bo_list)
>> +Â Â Â if (parser->bo_list) {
>> Â Â Â Â Â Â Â Â Â amdgpu_bo_list_put(parser->bo_list);
>> Â +Â Â Â Â Â Â Â if (parser->destroy_bo_list)
>> + amdgpu_bo_list_destroy(parser->filp->driver_priv, id);
>> +Â Â Â }
>> +
>> Â Â Â Â Â for (i = 0; i < parser->nchunks; i++)
>> Â Â Â Â Â Â Â Â Â kvfree(parser->chunks[i].kdata);
>> Â Â Â Â Â kfree(parser->chunks);
>> @@ -1277,7 +1319,7 @@ int amdgpu_cs_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *filp)
>> Â Â Â Â Â r = amdgpu_cs_submit(&parser, cs);
>> Â Â out:
>> -Â Â Â amdgpu_cs_parser_fini(&parser, r, reserved_buffers);
>> +Â Â Â amdgpu_cs_parser_fini(&parser, r, reserved_buffers,
>> cs->in.bo_list_handle);
>> Â Â Â Â Â return r;
>> Â }
>> Â diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 06aede1..529500c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -69,9 +69,10 @@
>> Â Â * - 3.24.0 - Add high priority compute support for gfx9
>> Â Â * - 3.25.0 - Add support for sensor query info (stable pstate
>> sclk/mclk).
>> Â Â * - 3.26.0 - GFX9: Process AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE.
>> + * - 3.27.0 - Add new chunk to to AMDGPU_CS to enable BO_LIST creation.
>> Â Â */
>> Â #define KMS_DRIVER_MAJORÂ Â Â 3
>> -#define KMS_DRIVER_MINORÂ Â Â 26
>> +#define KMS_DRIVER_MINORÂ Â Â 27
>> Â #define KMS_DRIVER_PATCHLEVELÂ Â Â 0
>> Â Â int amdgpu_vram_limit = 0;
>> diff --git a/include/uapi/drm/amdgpu_drm.h
>> b/include/uapi/drm/amdgpu_drm.h
>> index 09741ba..94444ee 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -519,6 +519,7 @@ struct drm_amdgpu_gem_va {
>> Â #define AMDGPU_CHUNK_ID_DEPENDENCIESÂ Â Â 0x03
>> Â #define AMDGPU_CHUNK_ID_SYNCOBJ_INÂ Â Â Â Â 0x04
>> Â #define AMDGPU_CHUNK_ID_SYNCOBJ_OUTÂ Â Â Â 0x05
>> +#define AMDGPU_CHUNK_ID_BO_HANDLESÂ Â Â Â Â 0x06
>> Â Â struct drm_amdgpu_cs_chunk {
>> Â Â Â Â Â __u32Â Â Â Â Â Â Â chunk_id;
>
