On Thu, Jan 16, 2025 at 03:36:12AM +0200, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 10:05:08PM +0100, Maxime Ripard wrote:
> > Even though the commit_tail () drm_atomic_state parameter is called
> > old_state, it's actually the state being committed which is confusing.
> >
> > It's even more confusing since the atomic_commit_tail hook being called
> > by commit_tail() parameter is called state.
>
> Do you have any kind of history and/or explanation, why it's called
> old_state all over the place?
>
> I think that the renaming is correct, but I'd like to understand the
> reason behind it.
So originally drm_atomic_state only had a single set of state pointers, so
was truly just a state collection and not a state transition/commit like
it is now.
During atomic check it contained the new states, and the old ones you
could get by looking at obj->state pointers. After
drm_atomic_helper_swap_state it contained the old states, and the new ones
could be found by looking at obj->state.
This caused endless amounts of confusions, and eventually we settled on a
new design:
- Ville added both old and new state pointers to drm_atomic_state, so now
it's not just a state collection, but really a state transition/commit.
We did discuss whether we should also rename it, but for lack of time
and good name this hasn't happened yet.
- Instead of trying to pass the individual states to callbacks we've moved
over to just passing drm_atomic_state, and let the callbacks grab
whatever they need. That's also not yet done everywhere yet, but I think
we're pretty close.
But one of the interim attempts at reducing the confusion was to rename
the drm_atomic_state argument to old_state anywhere after we've called
swap_states(). Didn't really help, and not it's just adding to the
confusion.
If we haven't yet I guess we should document the above two design
principles in the drm_atomic_state kerneldoc.
Cheers, Sima
>
> > Let's rename the variable from old_state to state to make it less
> > confusing.
> >
> > Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 40e4e1b6c9110677c1c4981eeb15dc93966f4cf6..913d94d664d885323ad7e41a6424633c28c787e1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1818,13 +1818,13 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> >
> > drm_atomic_helper_cleanup_planes(dev, old_state);
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm);
> >
> > -static void commit_tail(struct drm_atomic_state *old_state)
> > +static void commit_tail(struct drm_atomic_state *state)
> > {
> > - struct drm_device *dev = old_state->dev;
> > + struct drm_device *dev = state->dev;
> > const struct drm_mode_config_helper_funcs *funcs;
> > struct drm_crtc_state *new_crtc_state;
> > struct drm_crtc *crtc;
> > ktime_t start;
> > s64 commit_time_ms;
> > @@ -1842,37 +1842,37 @@ static void commit_tail(struct drm_atomic_state *old_state)
> > * These times will be averaged out in the self refresh helpers to avoid
> > * overreacting over one outlier frame
> > */
> > start = ktime_get();
> >
> > - drm_atomic_helper_wait_for_fences(dev, old_state, false);
> > + drm_atomic_helper_wait_for_fences(dev, state, false);
> >
> > - drm_atomic_helper_wait_for_dependencies(old_state);
> > + drm_atomic_helper_wait_for_dependencies(state);
> >
> > /*
> > * We cannot safely access new_crtc_state after
> > * drm_atomic_helper_commit_hw_done() so figure out which crtc's have
> > * self-refresh active beforehand:
> > */
> > - for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i)
> > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > if (new_crtc_state->self_refresh_active)
> > new_self_refresh_mask |= BIT(i);
> >
> > if (funcs && funcs->atomic_commit_tail)
> > - funcs->atomic_commit_tail(old_state);
> > + funcs->atomic_commit_tail(state);
> > else
> > - drm_atomic_helper_commit_tail(old_state);
> > + drm_atomic_helper_commit_tail(state);
> >
> > commit_time_ms = ktime_ms_delta(ktime_get(), start);
> > if (commit_time_ms > 0)
> > - drm_self_refresh_helper_update_avg_times(old_state,
> > + drm_self_refresh_helper_update_avg_times(state,
> > (unsigned long)commit_time_ms,
> > new_self_refresh_mask);
> >
> > - drm_atomic_helper_commit_cleanup_done(old_state);
> > + drm_atomic_helper_commit_cleanup_done(state);
> >
> > - drm_atomic_state_put(old_state);
> > + drm_atomic_state_put(state);
> > }
> >
> > static void commit_work(struct work_struct *work)
> > {
> > struct drm_atomic_state *state = container_of(work,
> >
> > --
> > 2.47.1
> >
>
> --
> With best wishes
> Dmitry
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
