linux-kernel - Re: [PATCH 10/29] drm/atomic: Add atomic_state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250915-active-placid-bustard-6e1faa@penduick>
Date: Mon, 15 Sep 2025 10:42:22 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Thomas Zimmermann <tzimmermann@...e.de>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>, 
	Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	Jyri Sarha <jyri.sarha@....fi>, Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, 
	Devarsh Thakkar <devarsht@...com>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/29] drm/atomic: Add atomic_state_readout infrastructure

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

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)