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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Sep 23, 2025 at 01:43:29PM +0300, Dmitry Baryshkov wrote:
> On Tue, Sep 23, 2025 at 01:32:23PM +0300, Laurent Pinchart wrote:
> > On Tue, Sep 23, 2025 at 01:28:57PM +0300, Dmitry Baryshkov wrote:
> > > On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> > > > On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > > > > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > > > > 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.
> > > > > 
> > > > > DP bridge drivers have a pretty big state (DPCD and all the features).
> > > > 
> > > > I meant drm_bridge_state. Subclasses would have their own implementation
> > > > anyway.
> > > > 
> > > > > Other bridge drivers randomly leak state to the non-state structs.
> > > > 
> > > > I'm not sure what you mean by that though. Can you expand?
> > > 
> > > I think I've seen bridge drivers which stored subclassed drm_bridge
> > > instead of drm_bridge_state or stored the data in the long-living data
> > > structures. YEs, that's a bug, which should be fixed on its own.
> > 
> > There's one exception to the "all state in state structure" rules if I
> > understand things correctly (and I'm mentioning it here partly to be
> > corrected if my understanding is wrong). Active state data that needs to
> > be accessed from a non-sleepable context need to be copied to the
> > device-specific structure when the state is applied, as we can't take
> > the mutex protecting state access there.
> 
> I'd even relax this slightly (please correct me if I'm wrong here): the
> state can be extracted to the device-specific structure in
> atomic_pre_enable() and cleared in atomic_post_disable().

Overall I agree. I think this should be minimized when reasonably
possible to avoid data duplication though.

> Likewise it's generally fine to store non-state changing data in the
> private data (e.g. hw params or infoframe from the HDMI codec data).

Most hardware parameters are a translation of the software state. I
would store them in the state when possible (e.g. when they need to be
computed at atomic check time, and when the computation is expensive
enough to make caching useful), and store in private data only the data
that needs to also be accessed from a location where state access is not
easily possible.

> > I'd expect that to be uncommon
> > for bridges.

-- 
Regards,

Laurent Pinchart