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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 23.09.25 um 11:37 schrieb Maxime Ripard:
> On Mon, Sep 15, 2025 at 11:11:39AM +0200, Thomas Zimmermann wrote:
>> 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.
> That sounds like a good idea, but I wonder how we would deal with reset
> then? Should we remove it entirely? Still call it? What do you think?

I think in that design, a reset would be atomic_clean_state + 
atomic_commit. The clean_state step would likely always happen, were we 
currently do the reset. In the long term, the commit step could be moved 
into drm_dev_register() or even the DRM client setup. The later place 
should make it easy for in-kernel DRM clients to adopt the current 
display config.  If there's no client enabled, a plain atomic_commit 
would set the HW to whatever is in the state.

The state could, of course, also come from atomic_readout_state in which 
case in which the display would just keep running as-is on the 
atomic_commit.

Best regards
Thomas

>
>>>> 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.
> Yeah, that sounds fair.
>
> 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)