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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 15.09.25 um 10:42 schrieb Maxime Ripard:
> 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?

Probably not. The reset helper is supposed to initialize the object's 
software and hardware state. But in most drivers, we're currently mostly 
setting the minimal software state here and simply assume that hardware 
is off. Returning the state would water down semantics even further.

Having said that, I could imaging building an atomic_clean_state 
callback that replaces the reset callback. It would work alongside the 
new atomic_readout_state callback.  Current reset could be build upon 
that callback. The atomic_clean_state would intentionally only take care 
of the software state and leave hardware state undefined. This reflects 
the current realities of most DRM drivers.   From that clean state, DRM 
could do an atomic commit that also initializes the hardware.

>
>> 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.

That's what I meant, I think.

>
> 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.

If there's an element in the pipeline that's missing the readout helper, 
it might be safer to fallback to that modeset instead of ending up with 
inconsistent state.

Best regards
Thomas

>
> 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

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)