Re: [PATCH v3 08/11] drm/atomic: Allow filling a commit with pristine object states

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 08.09.26 um 16:35 schrieb Maxime Ripard:
> The upcoming DRM_MODE_ATOMIC_RESET flag will need to create an atomic
> commit that brings the entire device back to a pristine state, as if
> no configuration had ever been applied.
>
> Create drm_atomic_commit_fill_with_defaults() which iterates over all
> CRTCs, planes, connectors, and color operations in the device and
> inserts a fresh default state for each one into the commit. This uses
> the atomic_create_state() hooks rather than atomic_duplicate_state(),
> since atomic_create_state() provides exactly this pristine state on a
> per-object basis.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>   drivers/gpu/drm/drm_atomic.c | 123 +++++++++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/drm_ioctl.c  |   1 +
>   include/drm/drm_atomic.h     |   1 +
>   3 files changed, 125 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 508a027398af..4c15757308d6 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1661,10 +1661,133 @@ bool drm_atomic_can_create_state(struct drm_device *dev)
>   
>   	return true;
>   }
>   EXPORT_SYMBOL(drm_atomic_can_create_state);
>   
> +/**
> + * drm_atomic_commit_fill_with_defaults - populate a commit with pristine states
> + * @commit: atomic commit to fill
> + *
> + * Iterate over all CRTCs, planes, connectors, and color operations in
> + * the device and insert a freshly created default state for each one
> + * into @commit. The states are created through the atomic_create_state()
> + * hooks, producing the same initial state the driver starts with rather
> + * than a copy of the current hardware state.
> + *
> + * This is meant to be used with the %DRM_MODE_ATOMIC_RESET flag, which
> + * needs to bring the device back to a known baseline before applying
> + * userspace property changes on top.
> + *
> + * Returns:
> + * 0 on success, or a negative error code on failure.
> + */
> +int drm_atomic_commit_fill_with_defaults(struct drm_atomic_commit *commit)
> +{
> +	struct drm_device *dev = commit->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
> +	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_colorop *colorop;
> +	int ret;
> +
> +	WARN_ON(!commit->acquire_ctx);

drm_WARN_ON

> +
> +	if (!drm_atomic_can_create_state(dev))
> +		return -EOPNOTSUPP;

I'd push this test into the caller and maybe do a 
drm_WARN_ON(!_can_create_state) here.   I think that this helper 
shouldn't even be called if there's no reset support.

Best regards
Thomas


> +
> +	/*
> +	 * Private objects are ignored because none have userspace
> +	 * properties we might want to reset. atomic_check
> +	 * implementations will derive or infer their private obj state
> +	 * from the state that will end up being committed anyway.
> +	 */
> +	drm_for_each_colorop(colorop, dev) {
> +		struct drm_colorop_state *colorop_state;
> +
> +		ret = drm_modeset_lock(&colorop->plane->mutex, commit->acquire_ctx);
> +		if (ret)
> +			return ret;
> +
> +		colorop_state = drm_atomic_helper_colorop_create_state(colorop);
> +		if (IS_ERR(colorop_state))
> +			return PTR_ERR(colorop_state);
> +
> +		ret = drm_atomic_commit_set_colorop_state(commit, colorop, colorop_state);
> +		if (ret) {
> +			drm_colorop_atomic_destroy_state(colorop, colorop_state);
> +			return ret;
> +		}
> +	}
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx);
> +		if (ret)
> +			return ret;
> +
> +		plane_state = plane->funcs->atomic_create_state(plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);
> +
> +		ret = drm_atomic_commit_set_plane_state(commit, plane, plane_state);
> +		if (ret) {
> +			plane->funcs->atomic_destroy_state(plane, plane_state);
> +			return ret;
> +		}
> +	}
> +
> +	drm_for_each_crtc(crtc, dev) {
> +		struct drm_crtc_state *crtc_state;
> +
> +		ret = drm_modeset_lock(&crtc->mutex, commit->acquire_ctx);
> +		if (ret)
> +			return ret;
> +
> +		crtc_state = crtc->funcs->atomic_create_state(crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		ret = drm_atomic_commit_set_crtc_state(commit, crtc, crtc_state);
> +		if (ret) {
> +			crtc->funcs->atomic_destroy_state(crtc, crtc_state);
> +			return ret;
> +		}
> +	}
> +
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *connector_state;
> +
> +		ret = drm_modeset_lock(&config->connection_mutex, commit->acquire_ctx);
> +		if (ret) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return ret;
> +		}
> +
> +		connector_state = connector->funcs->atomic_create_state(connector);
> +		if (IS_ERR(connector_state)) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			ret = PTR_ERR(connector_state);
> +			return ret;
> +		}
> +
> +		ret = drm_atomic_commit_set_connector_state(commit, connector, connector_state);
> +		if (ret) {
> +			connector->funcs->atomic_destroy_state(connector, connector_state);
> +			drm_connector_list_iter_end(&conn_iter);
> +			return ret;
> +		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_commit_fill_with_defaults);
> +
>   /**
>    * drm_atomic_add_encoder_bridges - add bridges attached to an encoder
>    * @state: atomic state
>    * @encoder: DRM encoder
>    *
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 9039a39c4324..0dbf04d4aa9e 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -31,10 +31,11 @@
>   #include <linux/export.h>
>   #include <linux/nospec.h>
>   #include <linux/pci.h>
>   #include <linux/uaccess.h>
>   
> +#include <drm/drm_atomic.h>
>   #include <drm/drm_auth.h>
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_ioctl.h>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 7dc26e3da65c..b60f67619bfa 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -701,10 +701,11 @@ int drm_crtc_commit_wait(struct drm_crtc_commit *commit);
>   struct drm_atomic_commit * __must_check
>   drm_atomic_commit_alloc(struct drm_device *dev);
>   void drm_atomic_commit_clear(struct drm_atomic_commit *state);
>   
>   bool drm_atomic_can_create_state(struct drm_device *dev);
> +int drm_atomic_commit_fill_with_defaults(struct drm_atomic_commit *commit);
>   
>   /**
>    * drm_atomic_commit_get - acquire a reference to the atomic state
>    * @state: The atomic 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)