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