Re: [PATCH 08/14] drm/mode-config: Create drm_mode_config_create_state()

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6879 bytes --]

Hi Laurent,

On Mon, Mar 16, 2026 at 06:32:16PM +0200, Laurent Pinchart wrote:
> On Tue, Mar 10, 2026 at 05:07:00PM +0100, Maxime Ripard wrote:
> > drm_mode_config_reset() can be used to create the initial state, but
> > also to return to the initial state, when doing a suspend/resume cycle
> > for example.
> > 
> > It also affects both the software and the hardware, and drivers can
> > either choose to reset the hardware as well. Most will just create an
> 
> "either" requires an "or".
> 
> > empty state and the synchronisation between hardware and software states
> > will effectively be done when the first commit is done.
> > 
> > That dual role can be harmful, since some objects do need to be
> > initialized but also need to be preserved across a suspend/resume cycle.
> > drm_private_obj are such objects for example.
> > 
> > Thus, let's create another helper for drivers to call to initialize
> > their state when the driver is loaded, so we can make
> > drm_mode_config_reset() only about handling suspend/resume and similar.
> 
> Does that mean that the state initialization at probe time from the
> hardware state is planned to be handled by .atomic_create_state() ?

Not atomic_create_state itself, but through an extra
atomic_readout_state callback (that's the current plan at least).

The hardware readout is only really relevant at driver probe anyway,
while we could need to create a new blank state in multiple occasions
(starting with reset if we decouple sw and hw reset).

> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/drm_atomic.c      | 12 +++++-
> >  drivers/gpu/drm/drm_mode_config.c | 83 +++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_mode_config.h     |  1 +
> >  3 files changed, 94 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 92c6afc8f22c8307a59dc266aacdb8e03351409d..d1885f895cce78725419b6291f4dbe5563e3b240 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -58,12 +58,20 @@
> >   * checking or doing the update, while object states are allocated while
> >   * the state will be, or is active in the hardware.
> >   *
> >   * Their respective lifetimes are:
> >   *
> > - * - at reset time, the object reset implementation will allocate a new,
> > - *   default, state and will store it in the object state pointer.
> > + * - at driver initialization time, the driver will allocate an initial,
> > + *   pristine, state and will store it using
> > + *   drm_mode_config_create_state(). Historically, this was one of
> > + *   drm_mode_config_reset() job, so one might still encounter it in a
> > + *   driver.
> 
> Do you have plans to port drivers to the new API, to drop this last
> sentence sooner than later ?

Yes, definitely. I didn't want to do it in the first version because I
wasn't too sure how it was going to be received, but that's definitely
something I intend to do once the dust settles a bit.

> > + *
> > + * - at reset time, for example during suspend/resume,
> > + *   drm_mode_config_reset() will reset the software and hardware state
> > + *   to a known default and will store it in the object state pointer.
> > + *   Not all objects are affected by drm_mode_config_reset() though.
> 
> I have a bit of trouble understanding the design. How can
> drm_mode_config_reset() decide which objects should have their state
> reset (i.e. all objects but private objects in the current
> implementation), shouldn't that be a driver decision ?

It's not really what I meant. drm_mode_config_reset() should reset
everything it can (ie. everything with a reset callback). That being
said, private_objs are explicitly excluded from that everything. Whether
you want to implement reset or not is a driver decision, whether we want
to reset private_objs or not isn't.

> >   *
> >   * - whenever a new update is needed:
> >   *
> >   *   + we allocate a new &struct drm_atomic_state using drm_atomic_state_alloc().
> >   *
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 95759a4b5176ff17032a9a267ec9de6980be0abc..1a84ac063d381014eb8afd7116e1e8b7a8fc92ca 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -21,10 +21,11 @@
> >   */
> >  
> >  #include <linux/export.h>
> >  #include <linux/uaccess.h>
> >  
> > +#include <drm/drm_atomic.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_encoder.h>
> >  #include <drm/drm_file.h>
> >  #include <drm/drm_framebuffer.h>
> >  #include <drm/drm_managed.h>
> > @@ -278,10 +279,92 @@ void drm_mode_config_reset(struct drm_device *dev)
> >  			drm_mode_config_connector_create_state(connector);
> >  	drm_connector_list_iter_end(&conn_iter);
> >  }
> >  EXPORT_SYMBOL(drm_mode_config_reset);
> >  
> > +/**
> > + * drm_mode_config_create_state - Allocates the initial state
> > + * @dev: drm device
> > + *
> > + * This functions creates the initial state for all the objects. Drivers
> > + * can use this in e.g. their driver load to initialize its software
> 
> I think you meant s/its/their/
> 
> > + * state.
> > + *
> > + * It has two main differences with drm_mode_config_reset(): the reset()
> > + * hooks aren't called and thus the hardware will be left untouched, but
> > + * also the @drm_private_obj structures will be initialized as opposed
> > + * to drm_mode_config_reset() that skips them.
> > + *
> > + * Returns: 0 on success, negative error value on failure.
> > + */
> > +int drm_mode_config_create_state(struct drm_device *dev)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_colorop *colorop;
> > +	struct drm_plane *plane;
> > +	struct drm_connector *connector;
> > +	struct drm_connector_list_iter conn_iter;
> > +	struct drm_private_obj *privobj;
> > +	int ret;
> > +
> > +	drm_for_each_privobj(privobj, dev) {
> > +		struct drm_private_state *privobj_state;
> > +
> > +		if (privobj->state)
> > +			continue;
> > +
> > +		if (!privobj->funcs->atomic_create_state)
> > +			continue;
> > +
> > +		privobj_state = privobj->funcs->atomic_create_state(privobj);
> > +		if (IS_ERR(privobj_state))
> > +			return PTR_ERR(privobj_state);
> > +
> > +		privobj->state = privobj_state;
> 
> Either a helper function is missing, or you forgot to call it.

Did I? I'm calling atomic_create_state right above, what else did you
have in mind?

> > +	}
> > +
> > +	drm_for_each_colorop(colorop, dev) {
> > +		if (colorop->state)
> > +			continue;
> > +
> > +		// TODO: Implement atomic_create_state for colorop.
> 
> Oops :-)

WIP:
https://lore.kernel.org/dri-devel/20260319-nocturnal-mighty-chupacabra-87cbd0@houat/ :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]