On 12/19/25 14:42, Tvrtko Ursulin wrote:
> Removing the output parameter from a few functions should result in more
> readable code and also enables us to save some lines.
>
> Signed-off-by: Tvrtko Ursulin <[email protected]>
Looks good but probably needs to be rebased on changes in previous commits in
this series, so I'm skipping that one for now.
Regards,
Christian.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 82 ++++++++++-----------
> drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 17 ++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 54 +++++++-------
> 3 files changed, 72 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index 59def86cdc04..2c907f9e8258 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -58,9 +58,9 @@ static int amdgpu_bo_list_entry_cmp(const void *_a, const
> void *_b)
> return (int)a->priority - (int)b->priority;
> }
>
> -int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
> - struct drm_amdgpu_bo_list_entry *info,
> - size_t num_entries, struct amdgpu_bo_list **result)
> +struct amdgpu_bo_list *
> +amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
> + struct drm_amdgpu_bo_list_entry *info, size_t num_entries)
> {
> unsigned last_entry = 0, first_userptr = num_entries;
> struct amdgpu_bo_list_entry *array;
> @@ -71,7 +71,7 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev,
> struct drm_file *filp,
>
> list = kvzalloc(struct_size(list, entries, num_entries), GFP_KERNEL);
> if (!list)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> kref_init(&list->refcount);
>
> @@ -126,8 +126,7 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev,
> struct drm_file *filp,
>
> trace_amdgpu_cs_bo_status(list->num_entries, total_size);
>
> - *result = list;
> - return 0;
> + return list;
>
> error_free:
> for (i = 0; i < last_entry; ++i)
> @@ -135,12 +134,11 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev,
> struct drm_file *filp,
> for (i = first_userptr; i < num_entries; ++i)
> amdgpu_bo_unref(&array[i].bo);
> kvfree(list);
> - return r;
> + return ERR_PTR(r);
>
> }
>
> -int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id,
> - struct amdgpu_bo_list **result)
> +struct amdgpu_bo_list *amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id)
> {
> struct amdgpu_bo_list *list;
>
> @@ -148,11 +146,11 @@ int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32
> id,
> list = xa_load(&fpriv->bo_list_handles, id);
> if (list)
> kref_get(&list->refcount);
> + else
> + list = ERR_PTR(-ENOENT);
> xa_unlock(&fpriv->bo_list_handles);
>
> - *result = list;
> -
> - return list ? 0 : -ENOENT;
> + return list;
> }
>
> void amdgpu_bo_list_put(struct amdgpu_bo_list *list)
> @@ -160,8 +158,8 @@ void amdgpu_bo_list_put(struct amdgpu_bo_list *list)
> kref_put(&list->refcount, amdgpu_bo_list_free);
> }
>
> -int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in,
> - struct drm_amdgpu_bo_list_entry
> **info_param)
> +struct drm_amdgpu_bo_list_entry *
> +amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in)
> {
> const uint32_t info_size = sizeof(struct drm_amdgpu_bo_list_entry);
> const void __user *uptr = u64_to_user_ptr(in->bo_info_ptr);
> @@ -172,26 +170,23 @@ int amdgpu_bo_create_list_entry_array(struct
> drm_amdgpu_bo_list_in *in,
> /* copy the handle array from userspace to a kernel buffer */
> if (likely(info_size == bo_info_size)) {
> info = vmemdup_array_user(uptr, bo_number, info_size);
> - if (IS_ERR(info))
> - return PTR_ERR(info);
> } else {
> const uint32_t bytes = min(bo_info_size, info_size);
> unsigned i;
>
> info = kvmalloc_array(bo_number, info_size, GFP_KERNEL);
> if (!info)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> for (i = 0; i < bo_number; ++i, uptr += bo_info_size) {
> if (copy_from_user(&info[i], uptr, bytes)) {
> kvfree(info);
> - return -EFAULT;
> + return ERR_PTR(-EFAULT);
> }
> }
> }
>
> - *info_param = info;
> - return 0;
> + return info;
> }
>
> int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
> @@ -199,27 +194,24 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void
> *data,
> {
> struct amdgpu_fpriv *fpriv = filp->driver_priv;
> struct amdgpu_device *adev = drm_to_adev(dev);
> - struct drm_amdgpu_bo_list_entry *info = NULL;
> struct amdgpu_bo_list *list, *prev, *curr;
> union drm_amdgpu_bo_list *args = data;
> uint32_t handle = args->in.list_handle;
> + struct drm_amdgpu_bo_list_entry *info;
> int r;
>
> - r = amdgpu_bo_create_list_entry_array(&args->in, &info);
> - if (r)
> - return r;
> -
> switch (args->in.operation) {
> case AMDGPU_BO_LIST_OP_CREATE:
> - r = amdgpu_bo_list_create(adev, filp, info, args->in.bo_number,
> - &list);
> - if (r)
> - goto error_free;
> + case AMDGPU_BO_LIST_OP_UPDATE:
> + info = amdgpu_bo_create_list_entry_array(&args->in);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
>
> - r = xa_alloc(&fpriv->bo_list_handles, &handle, list,
> - xa_limit_32b, GFP_KERNEL);
> - if (r)
> - goto error_put_list;
> + list = amdgpu_bo_list_create(adev, filp, info,
> + args->in.bo_number);
> + kvfree(info);
> + if (IS_ERR(list))
> + return PTR_ERR(list);
>
> break;
>
> @@ -231,12 +223,20 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void
> *data,
>
> break;
>
> - case AMDGPU_BO_LIST_OP_UPDATE:
> - r = amdgpu_bo_list_create(adev, filp, info, args->in.bo_number,
> - &list);
> + default:
> + return -EINVAL;
> + };
> +
> + switch (args->in.operation) {
> + case AMDGPU_BO_LIST_OP_CREATE:
> + r = xa_alloc(&fpriv->bo_list_handles, &handle, list,
> + xa_limit_32b, GFP_KERNEL);
> if (r)
> - goto error_free;
> + goto error_put_list;
>
> + break;
> +
> + case AMDGPU_BO_LIST_OP_UPDATE:
> curr = xa_load(&fpriv->bo_list_handles, handle);
> if (!curr) {
> r = -ENOENT;
> @@ -256,21 +256,17 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void
> *data,
> amdgpu_bo_list_put(curr);
> break;
>
> + case AMDGPU_BO_LIST_OP_DESTROY:
> default:
> - r = -EINVAL;
> - goto error_free;
> + /* Handled above. */
> }
>
> memset(args, 0, sizeof(*args));
> args->out.list_handle = handle;
> - kvfree(info);
>
> return 0;
>
> error_put_list:
> amdgpu_bo_list_put(list);
> -
> -error_free:
> - kvfree(info);
> return r;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> index cf127bc66f53..bde912150824 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> @@ -53,17 +53,16 @@ struct amdgpu_bo_list {
> struct amdgpu_bo_list_entry entries[] __counted_by(num_entries);
> };
>
> -int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32 id,
> - struct amdgpu_bo_list **result);
> +struct amdgpu_bo_list *amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, u32
> id);
> void amdgpu_bo_list_put(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);
> +struct drm_amdgpu_bo_list_entry *
> +amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in);
>
> -int amdgpu_bo_list_create(struct amdgpu_device *adev,
> - struct drm_file *filp,
> - struct drm_amdgpu_bo_list_entry *info,
> - size_t num_entries,
> - struct amdgpu_bo_list **list);
> +struct amdgpu_bo_list *
> +amdgpu_bo_list_create(struct amdgpu_device *adev,
> + struct drm_file *filp,
> + struct drm_amdgpu_bo_list_entry *info,
> + size_t num_entries);
>
> #define amdgpu_bo_list_for_each_entry(e, list) \
> for (e = list->entries; \
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index bc9cee3fdf27..69d0cf67d150 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -152,24 +152,19 @@ static int amdgpu_cs_p1_bo_handles(struct
> amdgpu_cs_parser *p,
> struct drm_amdgpu_bo_list_in *data)
> {
> struct drm_amdgpu_bo_list_entry *info;
> - int r;
> + struct amdgpu_bo_list *list;
>
> - 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,
> - &p->bo_list);
> - if (r)
> - goto error_free;
> + info = amdgpu_bo_create_list_entry_array(data);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
>
> + list = amdgpu_bo_list_create(p->adev, p->filp, info, data->bo_number);
> kvfree(info);
> + if (IS_ERR(list))
> + return PTR_ERR(list);
> +
> + p->bo_list = list;
> return 0;
> -
> -error_free:
> - kvfree(info);
> -
> - return r;
> }
>
> /* Copy the data from userspace and go over it the first time */
> @@ -857,6 +852,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
> *p,
> {
> struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
> struct ttm_operation_ctx ctx = { true, false };
> + struct amdgpu_bo_list *list = NULL;
> struct amdgpu_vm *vm = &fpriv->vm;
> struct amdgpu_bo_list_entry *e;
> struct drm_gem_object *obj;
> @@ -869,23 +865,24 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
> *p,
> if (p->bo_list)
> return -EINVAL;
>
> - r = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle,
> - &p->bo_list);
> - if (r)
> - return r;
> + list = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle);
> } else if (!p->bo_list) {
> /* Create a empty bo_list when no handle is provided */
> - r = amdgpu_bo_list_create(p->adev, p->filp, NULL, 0,
> - &p->bo_list);
> - if (r)
> - return r;
> + list = amdgpu_bo_list_create(p->adev, p->filp, NULL, 0);
> }
>
> + if (IS_ERR(list))
> + return PTR_ERR(list);
> + else if (list)
> + p->bo_list = list;
> + else
> + list = p->bo_list;
> +
> /* Get userptr backing pages. If pages are updated after registered
> * in amdgpu_gem_userptr_ioctl(), amdgpu_cs_list_validate() will do
> * amdgpu_ttm_backend_bind() to flush and invalidate new pages
> */
> - amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> + amdgpu_bo_list_for_each_userptr_entry(e, list) {
> bool userpage_invalidated = false;
> struct amdgpu_bo *bo = e->bo;
>
> @@ -913,7 +910,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
> *p,
> if (unlikely(r))
> goto out_free_user_pages;
>
> - amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> + amdgpu_bo_list_for_each_entry(e, list) {
> /* One fence for TTM and one for each CS job */
> r = drm_exec_prepare_obj(&p->exec, &e->bo->tbo.base,
> 1 + p->gang_size);
> @@ -933,7 +930,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
> *p,
> }
> }
>
> - amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> + amdgpu_bo_list_for_each_userptr_entry(e, list) {
> struct mm_struct *usermm;
>
> usermm = amdgpu_ttm_tt_get_usermm(e->bo->tbo.ttm);
> @@ -986,13 +983,12 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser
> *p,
> p->bytes_moved_vis);
>
> for (i = 0; i < p->gang_size; ++i)
> - amdgpu_job_set_resources(p->jobs[i], p->bo_list->gds_obj,
> - p->bo_list->gws_obj,
> - p->bo_list->oa_obj);
> + amdgpu_job_set_resources(p->jobs[i], list->gds_obj,
> + list->gws_obj, list->oa_obj);
> return 0;
>
> out_free_user_pages:
> - amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
> + amdgpu_bo_list_for_each_userptr_entry(e, list) {
> amdgpu_hmm_range_free(e->range);
> e->range = NULL;
> }
