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: <20250923-debonair-earwig-of-abracadabra-940fa8@penduick>
Date: Tue, 23 Sep 2025 11:38:17 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Thomas Zimmermann <tzimmermann@...e.de>, 
	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

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

Thanks!
Maxime

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