Hello Maxime, Dmitry,
On Wed Oct 15, 2025 at 2:17 AM CEST, Dmitry Baryshkov wrote:
> 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.
AFAICT Dmitry's point looks reasonable to me. Should you go along this path,
why not adding a
WARN_ON((!state && !obj->funcs->atomic_create_state) ||
(state && obj->funcs->atomic_create_state));
to prevent bad usage?
Just my 2c,
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
