Re: [PATCH v3 05/11] drm/atomic: Create function to insert private obj state into a commit

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 08.09.26 um 16:35 schrieb Maxime Ripard:
> drm_atomic_get_private_obj_state() allocates a new private object
> state by duplicating the current one and inserts it into the atomic
> commit as a single operation.
>
> However, a later change will need to insert a private object state
> into a commit without going through the full allocation and
> duplication path in drm_atomic_get_private_obj_state().
>
> Extract the state insertion logic, including the array reallocation,
> into a new static drm_atomic_commit_set_private_obj_state() helper,
> and convert drm_atomic_get_private_obj_state() to use it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/drm_atomic.c | 57 ++++++++++++++++++++++++++++----------------
>   1 file changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f6a31c9d477d..1be4af940ed1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1101,10 +1101,41 @@ drm_atomic_private_obj_fini(struct drm_private_obj *obj)
>   	obj->funcs->atomic_destroy_state(obj, obj->state);
>   	drm_modeset_lock_fini(&obj->lock);
>   }
>   EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>   
> +static int drm_atomic_commit_set_private_obj_state(struct drm_atomic_commit *commit,
> +						   struct drm_private_obj *obj,
> +						   struct drm_private_state *obj_state)
> +{
> +	struct __drm_private_objs_state *arr;
> +	int index, num_objs;
> +	size_t size;
> +
> +	drm_modeset_lock_assert_held(&obj->lock);
> +
> +	num_objs = commit->num_private_objs + 1;
> +	size = sizeof(*commit->private_objs) * num_objs;
> +	arr = krealloc(commit->private_objs, size, GFP_KERNEL);
> +	if (!arr)
> +		return -ENOMEM;

Just nitpicks: Would this be a candidate for krealloc_array() ? Could 
you avoid the additional memset by allocating with __GFP_ZERO?

Best regards
Thomas

> +
> +	commit->private_objs = arr;
> +	index = commit->num_private_objs;
> +	memset(&commit->private_objs[index], 0, sizeof(*commit->private_objs));
> +
> +	commit->private_objs[index].state_to_destroy = obj_state;
> +	commit->private_objs[index].old_state = obj->state;
> +	commit->private_objs[index].new_state = obj_state;
> +	commit->private_objs[index].ptr = obj;
> +	obj_state->state = commit;
> +
> +	commit->num_private_objs = num_objs;
> +
> +	return 0;
> +}
> +
>   /**
>    * drm_atomic_get_private_obj_state - get private object state
>    * @state: global atomic state
>    * @obj: private object to get the state for
>    *
> @@ -1117,13 +1148,11 @@ EXPORT_SYMBOL(drm_atomic_private_obj_fini);
>    */
>   struct drm_private_state *
>   drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
>   				 struct drm_private_obj *obj)
>   {
> -	int index, num_objs, ret;
> -	size_t size;
> -	struct __drm_private_objs_state *arr;
> +	int ret;
>   	struct drm_private_state *obj_state;
>   
>   	WARN_ON(!state->acquire_ctx);
>   	drm_WARN_ON(state->dev, state->checked);
>   
> @@ -1133,31 +1162,19 @@ drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
>   
>   	ret = drm_modeset_lock(&obj->lock, state->acquire_ctx);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> -	num_objs = state->num_private_objs + 1;
> -	size = sizeof(*state->private_objs) * num_objs;
> -	arr = krealloc(state->private_objs, size, GFP_KERNEL);
> -	if (!arr)
> -		return ERR_PTR(-ENOMEM);
> -
> -	state->private_objs = arr;
> -	index = state->num_private_objs;
> -	memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
> -
>   	obj_state = obj->funcs->atomic_duplicate_state(obj);
>   	if (!obj_state)
>   		return ERR_PTR(-ENOMEM);
>   
> -	state->private_objs[index].state_to_destroy = obj_state;
> -	state->private_objs[index].old_state = obj->state;
> -	state->private_objs[index].new_state = obj_state;
> -	state->private_objs[index].ptr = obj;
> -	obj_state->state = state;
> -
> -	state->num_private_objs = num_objs;
> +	ret = drm_atomic_commit_set_private_obj_state(state, obj, obj_state);
> +	if (ret) {
> +		obj->funcs->atomic_destroy_state(obj, obj_state);
> +		return ERR_PTR(ret);
> +	}
>   
>   	drm_dbg_atomic(state->dev,
>   		       "Added new private object %p state %p to %p\n",
>   		       obj, obj_state, state);
>   
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)