Message-ID: <Z4jtKHY4qN3RNZNG@phenom.ffwll.local>
Date: Thu, 16 Jan 2025 12:27:36 +0100
From: Simona Vetter <simona.vetter@...ll.ch>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Maxime Ripard <mripard@...nel.org>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>,
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
	Jonas Karlman <jonas@...boo.se>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Douglas Anderson <dianders@...omium.org>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/29] drm/atomic-helper: Fix commit_tail state variable
 name

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@...nel.org>
> > ---
> >  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

Powered by blists - more mailing lists