Re: [PATCH v3 06/11] drm/atomic: Create function to insert connector state into a commit

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 08.09.26 um 16:35 schrieb Maxime Ripard:
> drm_atomic_get_connector_state() allocates a new connector 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 connector state into a
> commit without going through the full allocation and duplication path
> in drm_atomic_get_connector_state().
>
> Extract the state insertion logic, including the dynamic array
> reallocation, into a new static
> drm_atomic_commit_set_connector_state() helper, and convert
> drm_atomic_get_connector_state() to use it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/drm_atomic.c | 67 +++++++++++++++++++++++++++-----------------
>   1 file changed, 42 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1be4af940ed1..19678e8a2bd4 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1412,10 +1412,46 @@ drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_commit *state,
>   
>   	return conn_state->crtc;
>   }
>   EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_encoder);
>   
> +static int drm_atomic_commit_set_connector_state(struct drm_atomic_commit *commit,
> +						 struct drm_connector *connector,
> +						 struct drm_connector_state *connector_state)
> +{
> +	struct drm_mode_config *config = &connector->dev->mode_config;
> +	int index;
> +
> +	drm_modeset_lock_assert_held(&config->connection_mutex);
> +
> +	index = drm_connector_index(connector);
> +	if (index >= commit->num_connector) {
> +		struct __drm_connnectors_state *c;
> +		int alloc = max(index + 1, config->num_connector);
> +
> +		c = krealloc_array(commit->connectors, alloc,
> +				   sizeof(*commit->connectors), GFP_KERNEL);

This entire branch looks like it mostly open-codes the functionality of 
krealloc_array(). If you can use __GFP_ZERO here, it appears that this 
whole branching could be collapsed to a single unconditional 
reallocation plus an update of num_connector.

Best regards
Thomas

> +		if (!c)
> +			return -ENOMEM;
> +
> +		commit->connectors = c;
> +		memset(&commit->connectors[commit->num_connector], 0,
> +		       sizeof(*commit->connectors) * (alloc - commit->num_connector));
> +
> +		commit->num_connector = alloc;
> +	}
> +
> +	drm_connector_get(connector);
> +	commit->connectors[index].state_to_destroy = connector_state;
> +	commit->connectors[index].old_state = connector->state;
> +	commit->connectors[index].new_state = connector_state;
> +	commit->connectors[index].ptr = connector;
> +	connector_state->state = commit;
> +
> +	return 0;
> +}
> +
>   /**
>    * drm_atomic_get_connector_state - get connector state
>    * @state: global atomic state object
>    * @connector: connector to get state object for
>    *
> @@ -1430,53 +1466,34 @@ EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_encoder);
>    */
>   struct drm_connector_state *
>   drm_atomic_get_connector_state(struct drm_atomic_commit *state,
>   			  struct drm_connector *connector)
>   {
> -	int ret, index;
> +	int ret;
>   	struct drm_mode_config *config = &connector->dev->mode_config;
>   	struct drm_connector_state *connector_state;
>   
>   	WARN_ON(!state->acquire_ctx);
>   	drm_WARN_ON(state->dev, state->checked);
>   
>   	ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> -	index = drm_connector_index(connector);
> -
> -	if (index >= state->num_connector) {
> -		struct __drm_connnectors_state *c;
> -		int alloc = max(index + 1, config->num_connector);
> -
> -		c = krealloc_array(state->connectors, alloc,
> -				   sizeof(*state->connectors), GFP_KERNEL);
> -		if (!c)
> -			return ERR_PTR(-ENOMEM);
> -
> -		state->connectors = c;
> -		memset(&state->connectors[state->num_connector], 0,
> -		       sizeof(*state->connectors) * (alloc - state->num_connector));
> -
> -		state->num_connector = alloc;
> -	}
> -
>   	connector_state = drm_atomic_get_new_connector_state(state, connector);
>   	if (connector_state)
>   		return connector_state;
>   
>   	connector_state = connector->funcs->atomic_duplicate_state(connector);
>   	if (!connector_state)
>   		return ERR_PTR(-ENOMEM);
>   
> -	drm_connector_get(connector);
> -	state->connectors[index].state_to_destroy = connector_state;
> -	state->connectors[index].old_state = connector->state;
> -	state->connectors[index].new_state = connector_state;
> -	state->connectors[index].ptr = connector;
> -	connector_state->state = state;
> +	ret = drm_atomic_commit_set_connector_state(state, connector, connector_state);
> +	if (ret) {
> +		connector->funcs->atomic_destroy_state(connector, connector_state);
> +		return ERR_PTR(ret);
> +	}
>   
>   	drm_dbg_atomic(connector->dev, "Added [CONNECTOR:%d:%s] %p state to %p\n",
>   			 connector->base.id, connector->name,
>   			 connector_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)