On Wed, Oct 15, 2025 at 01:49:33AM +0300, Dmitry Baryshkov wrote:
> On Tue, Oct 14, 2025 at 11:31:47AM +0200, Maxime Ripard wrote:
> > The drm_private_obj initialization was inconsistent with the rest of the
> > KMS objects. Indeed, it required to pass a preallocated state in
> > drm_private_obj_init(), while all the others objects would have a reset
> > callback that would be called later on to create the state.
> >
> > However, reset really is meant to reset the hardware and software state.
> > That it creates an initial state is a side-effect that has been used in
> > all objects but drm_private_obj. This is made more complex since some
> > drm_private_obj, the DisplayPort ones in particular, need to be
> > persistent across and suspend/resume cycle, and such a cycle would call
> > drm_mode_config_reset().
>
> Doesn't that mean that we need to save private objects's state in
> drm_atomic_helper_duplicate_state() and restore it in
> drm_atomic_helper_commit_duplicated_state()? Private objects don't have
> .atomic_commit() callbacks, but they can be used by other objects during
> drm_atomic_commit().
>
> > Thus, we need to add a new callback to allocate a pristine state for a
> > given private object.
> >
> > This discussion has also came up during the atomic state readout
> > discussion, so it might be introduced into the other objects later on.
> >
> > Until all drivers are converted to that new allocation pattern, we will
> > only call it if the passed state is NULL. This will be removed
> > eventually.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 20 ++++++++++++++++++--
> > include/drm/drm_atomic.h | 13 +++++++++++++
> > 2 files changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index
> > a5c5617266ae1dfe6038baeee6dfa3828c626683..36b56c71cb4e1ddc57577df724efe7d89b4fb6a9
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -793,15 +793,31 @@ int drm_atomic_private_obj_init(struct drm_device
> > *dev,
> > memset(obj, 0, sizeof(*obj));
> >
> > drm_modeset_lock_init(&obj->lock);
> >
> > obj->dev = dev;
> > - obj->state = state;
> > obj->funcs = funcs;
> > list_add_tail(&obj->head, &dev->mode_config.privobj_list);
> >
> > - state->obj = obj;
> > + /*
> > + * Not all users of drm_atomic_private_obj_init have been
> > + * converted to using &drm_private_obj_funcs.reset yet. For the
> > + * time being, let's only call reset if the passed state is
> > + * NULL. Otherwise, we will fallback to the previous behaviour.
>
> This comment does no longer reflect the code.
>
> > + */
> > + if (!state) {
> > + if (obj->funcs->atomic_create_state) {
Shouldn't this callback be mandatory here? Otherwise we can easily end
up with the object without a connected state, if the driver doesn't
implement it.
> > + state = obj->funcs->atomic_create_state(obj);
> > + if (IS_ERR(state))
> > + return PTR_ERR(state);
> > +
> > + obj->state = state;
> > + }
> > + } else {
> > + obj->state = state;
> > + state->obj = obj;
> > + }
> >
> > return 0;
> > }
> > EXPORT_SYMBOL(drm_atomic_private_obj_init);
> >
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index
> > 9b3fb98b1e88c38877abdcb9df4d1c9540768833..10a71c4b6afc316f07023756be4cd3ed1d1d2974
> > 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -205,10 +205,23 @@ struct drm_private_state;
> > * added to the atomic states is expected to have an implementation of
> > these
> > * hooks and pass a pointer to its drm_private_state_funcs struct to
> > * drm_atomic_get_private_obj_state().
> > */
> > struct drm_private_state_funcs {
> > + /**
> > + * @atomic_create_state:
> > + *
> > + * Allocates a pristine, initialized, state for the private
> > + * object and returns it.
> > + *
> > + * RETURNS:
> > + *
> > + * A new, pristine, private state instance or an error pointer
> > + * on failure.
> > + */
> > + struct drm_private_state *(*atomic_create_state)(struct drm_private_obj
> > *obj);
> > +
> > /**
> > * @atomic_duplicate_state:
> > *
> > * Duplicate the current state of the private object and return it. It
> > * is an error to call this before obj->state has been initialized.
> >
> > --
> > 2.51.0
> >
>
> --
> With best wishes
> Dmitry
--
With best wishes
Dmitry
