Re: [PATCH 10/29] drm/atomic: Add atomic_state_readout infrastructure

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4317 bytes --]

Hi Tohmas,

On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > +/**
> > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > + * @dev: DRM device to build the state for
> > + *
> > + * This function allocates a &struct drm_atomic_state, calls the
> > + * atomic_readout_state callbacks, and fills the global state old states
> > + * by what the callbacks returned.
> > + *
> > + * Returns:
> > + *
> > + * A partially initialized &struct drm_atomic_state on success, an error
> > + * pointer otherwise.
> > + */
> > +static struct drm_atomic_state *
> > +drm_atomic_build_readout_state(struct drm_device *dev)
> > +{
> > +	struct drm_connector_list_iter conn_iter;
> > +	struct drm_atomic_state *state;
> > +	struct drm_mode_config *config =
> > +		&dev->mode_config;
> > +	struct drm_connector *connector;
> > +	struct drm_printer p =
> > +		drm_info_printer(dev->dev);
> > +	struct drm_encoder *encoder;
> > +	struct drm_plane *plane;
> > +	struct drm_crtc *crtc;
> > +	int ret;
> > +
> > +	drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > +
> > +	state = drm_atomic_state_alloc(dev);
> > +	if (WARN_ON(!state))
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > +	if (WARN_ON(!state->connectors)) {
> > +		ret = -ENOMEM;
> > +		goto err_state_put;
> > +	}
> > +
> > +	state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > +	if (WARN_ON(!state->private_objs)) {
> > +		ret = -ENOMEM;
> > +		goto err_state_put;
> > +	}
> > +
> > +	drm_for_each_crtc(crtc, dev) {
> > +		const struct drm_crtc_funcs *crtc_funcs =
> > +			crtc->funcs;
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > +
> > +		if (crtc_funcs->atomic_readout_state) {
> > +			crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > +		} else if (crtc_funcs->reset) {
> > +			crtc_funcs->reset(crtc);
> > +
> > +			/*
> > +			 * We don't want to set crtc->state field yet. Let's save and clear it up.
> > +			 */
> > +			crtc_state = crtc->state;
> > +			crtc->state = NULL;
> 
> Chancing the crtc->state pointer behind the back of the reset callback seems
> fragile. We never how if some other piece of the driver refers to it
> (although illegally).

I agree that it's clunky. I'm not sure who would use it at this point
though: we're in the middle of the drm_mode_config_reset(), so the
drivers' involvement is pretty minimal.

I did wonder if changing reset to return the object instead of setting
$OBJECT->state would be a better interface?

> For now, wouldn't it be better to require a read-out helper for all elements
> of the driver's mode-setting pipeline?  The trivial implementation would
> copy the existing reset function and keep crtc->state to NULL.

I also considered that, but I'm not sure we can expect bridges to have
readout hooks filled for every configuration in the wild.

But maybe we can look during drm_mode_config_reset() at whether all the
objects have their hook filled, and if not fall back on reset for
everything.

It would make the implementation easier, but missing bridges
implementations would trigger a mode change when it might actually work
just fine since bridge state is pretty minimal.

Idk.

> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -490,10 +490,31 @@ struct drm_bridge_funcs {
> >   	 * The @atomic_post_disable callback is optional.
> >   	 */
> >   	void (*atomic_post_disable)(struct drm_bridge *bridge,
> >   				    struct drm_atomic_state *state);
> > +	/**
> > +	 * @atomic_readout_state:
> > +	 *
> > +	 * Initializes,this bridge atomic state.
> > +	 *
> > +	 * It's meant to be used by drivers that wants to implement fast
> 
> 'want'
> 
> > +	 * / flicker-free boot and allows to initialize the atomic state
> 
> I think we should only call it flicker-free boot. Fast boot is misleading.

I agree, but it's also how it's been called by the only implementation
of it we have so far (i915), and the name of the module parameter that
controls it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]