Re: [PATCH v5 12/19] drm/crtc: Add new atomic_create_state callback

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 19.05.26 um 11:01 schrieb Maxime Ripard:
> Commit 47b5ac7daa46 ("drm/atomic: Add new atomic_create_state callback
> to drm_private_obj") introduced a new pattern for allocating drm object
> states.
>
> Instead of relying on the reset() callback, it created a new
> atomic_create_state hook. This is helpful because reset is a bit
> overloaded: it's used to create the initial software state, reset it,
> but also reset the hardware.
>
> It can also be used either at probe time, to create the initial state
> and possibly reset the hardware to an expected default, but also during
> suspend/resume.
>
> Both these cases come with different expectations too: during the
> initialization, we want to initialize all states, but during
> suspend/resume, drm_private_states for example are expected to be kept
> around.
>
> reset() also isn't fallible, which makes it harder to handle
> initialization errors properly. This is only really relevant for some
> drivers though, since all the helpers for reset only create a new
> state, and don't touch the hardware at all.
>
> It was thus decided to create a new hook that would allocate and
> initialize a pristine state without any side effect:
> atomic_create_state to untangle a bit some of it, and to separate the
> initialization with the actual reset one might need during a
> suspend/resume.
>
> Continue the transition to the new pattern with CRTCs.
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

[...]
> +
> +	crtc_state = crtc->funcs->atomic_create_state(crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	if (drm_dev_has_vblank(crtc->dev))
> +		drm_crtc_vblank_reset(crtc);

I've recently looked at the vblank init and reset code. At some point, 
we'll have to make the vblank reset controllable by drivers. Not an 
issue for now though.

Best regards
Thomas

> +
> +	crtc->state = crtc_state;
> +
> +	return 0;
> +}
> +
> +static int drm_mode_config_crtc_reset_with_create_state(struct drm_crtc *crtc)
> +{
> +	if (crtc->state) {
> +		crtc->funcs->atomic_destroy_state(crtc, crtc->state);
> +		crtc->state = NULL;
> +	}
> +
> +	return drm_mode_config_crtc_create_state(crtc);
> +}
> +
>   /**
>    * drm_mode_config_reset - call ->reset callbacks
>    * @dev: drm device
>    *
>    * This functions calls all the crtc's, encoder's and connector's ->reset
> @@ -237,13 +266,16 @@ void drm_mode_config_reset(struct drm_device *dev)
>   			plane->funcs->reset(plane);
>   		else if (plane->funcs->atomic_create_state)
>   			drm_mode_config_plane_reset_with_create_state(plane);
>   	}
>   
> -	drm_for_each_crtc(crtc, dev)
> +	drm_for_each_crtc(crtc, dev) {
>   		if (crtc->funcs->reset)
>   			crtc->funcs->reset(crtc);
> +		else if (crtc->funcs->atomic_create_state)
> +			drm_mode_config_crtc_reset_with_create_state(crtc);
> +	}
>   
>   	drm_for_each_encoder(encoder, dev)
>   		if (encoder->funcs && encoder->funcs->reset)
>   			encoder->funcs->reset(encoder);
>   
> diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h
> index 0bb72453464a..213f7e298008 100644
> --- a/include/drm/drm_atomic_state_helper.h
> +++ b/include/drm/drm_atomic_state_helper.h
> @@ -43,10 +43,12 @@ struct drm_device;
>   void __drm_atomic_helper_crtc_state_init(struct drm_crtc_state *state,
>   					  struct drm_crtc *crtc);
>   void __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
>   				    struct drm_crtc_state *state);
>   void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
> +struct drm_crtc_state *
> +drm_atomic_helper_crtc_create_state(struct drm_crtc *crtc);
>   void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>   					      struct drm_crtc_state *state);
>   struct drm_crtc_state *
>   drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc);
>   void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c6dbe8b7db9e..152349f973e3 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -636,10 +636,26 @@ struct drm_crtc_funcs {
>   	 * 0 on success or a negative error code on failure.
>   	 */
>   	int (*set_property)(struct drm_crtc *crtc,
>   			    struct drm_property *property, uint64_t val);
>   
> +	/**
> +	 * @atomic_create_state:
> +	 *
> +	 * Allocate a pristine, initialized, state for the CRTC object
> +	 * and return it. This callback must have no side effects: in
> +	 * particular, the returned state must not be assigned to the
> +	 * object's state pointer and it must not affect the hardware
> +	 * state.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * A new, pristine, CRTC state instance or an error pointer
> +	 * on failure.
> +	 */
> +	struct drm_crtc_state *(*atomic_create_state)(struct drm_crtc *crtc);
> +
>   	/**
>   	 * @atomic_duplicate_state:
>   	 *
>   	 * Duplicate the current atomic state for this CRTC and return it.
>   	 * The core and helpers guarantee that any atomic state duplicated with
>

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