[PATCH 00/29] drm/bridge: Various quality of life improvements

[PATCH 00/29] drm/bridge: Various quality of life improvements

[Maxime Ripard]

Hi,

Here's a series of changes after to the KMS helpers and bridge API
following a bunch of reviews I did.

It's mostly centered across providing an easier time to deal with bridge
states, and a somewhat consistent with the other entities API.

It's build tested only.

Maxime

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Maxime Ripard (29):
      drm/atomic-helper: Fix commit_tail state variable name
      drm/atomic-helper: Change parameter name of drm_atomic_helper_wait_for_dependencies()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_tail()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_tail_rpm()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_modeset_disables()
      drm/atomic-helper: Change parameter name of disable_outputs()
      drm/bridge: Change parameter name of drm_atomic_bridge_chain_disable()
      drm/bridge: Change parameter name of drm_atomic_bridge_chain_post_disable()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_update_legacy_modeset_state()
      drm/atomic-helper: Change parameter name of crtc_set_mode()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_planes()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_modeset_enables()
      drm/bridge: Change parameter name of drm_atomic_bridge_chain_pre_enable()
      drm/bridge: Change parameter name of drm_atomic_bridge_chain_enable()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_writebacks()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_fake_vblank()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_hw_done()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_wait_for_vblanks()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_cleanup_planes()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_cleanup_done()
      drm/atomic-helper: Change parameter name of drm_atomic_helper_wait_for_flip_done()
      drm/bridge: Rename atomic hooks parameters to drop old prefix
      drm/bridge: Provide a helper to retrieve current bridge state
      drm/bridge: Provide a helper to get the global state from a bridge state
      drm/bridge: Provide pointers to the connector and crtc in bridge state
      drm/bridge: cdns-csi: Switch to atomic helpers
      drm/bridge: tc358775: Switch to atomic commit
      drm/bridge: tc358768: Convert to atomic helpers
      drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c |  22 ++-
 drivers/gpu/drm/bridge/tc358768.c              |  28 ++-
 drivers/gpu/drm/bridge/tc358775.c              |  32 ++--
 drivers/gpu/drm/bridge/ti-sn65dsi86.c          |  41 +++--
 drivers/gpu/drm/drm_atomic_helper.c            | 242 ++++++++++++-------------
 drivers/gpu/drm/drm_atomic_state_helper.c      |   5 +
 drivers/gpu/drm/drm_bridge.c                   |  61 ++++---
 include/drm/drm_atomic.h                       |  30 +++
 include/drm/drm_bridge.h                       |  25 ++-
 9 files changed, 279 insertions(+), 207 deletions(-)
---
base-commit: 573b73e5ac2ce0d58859eace8218f3a7e9212186
change-id: 20250115-bridge-connector-f93194e267ff

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH 01/29] drm/atomic-helper: Fix commit_tail state variable name

[Maxime Ripard]

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.

Let's rename the variable from old_state to state to make it less
confusing.

Signed-off-by: Maxime Ripard <mripard@kernel.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

Re: [PATCH 01/29] drm/atomic-helper: Fix commit_tail state variable name

[Dmitry Baryshkov]

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

Re: [PATCH 01/29] drm/atomic-helper: Fix commit_tail state variable name

[Simona Vetter]

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

[PATCH 02/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_wait_for_dependencies()

[Maxime Ripard]

drm_atomic_helper_wait_for_dependencies() waits for all the dependencies
a commit has before going forward with it. It takes the drm_atomic_state
being committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 913d94d664d885323ad7e41a6424633c28c787e1..48dc4f18e757549b67940cc5ca4d76812ef13985 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2411,48 +2411,48 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
 
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceding commits
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This function waits for all preceding commits that touch the same CRTC as
- * @old_state to both be committed to the hardware (as signalled by
+ * @state to both be committed to the hardware (as signalled by
  * drm_atomic_helper_commit_hw_done()) and executed by the hardware (as signalled
  * by calling drm_crtc_send_vblank_event() on the &drm_crtc_state.event).
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
+void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state;
 	struct drm_connector *conn;
 	struct drm_connector_state *old_conn_state;
 	int i;
 	long ret;
 
-	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		ret = drm_crtc_commit_wait(old_crtc_state->commit);
 		if (ret)
 			drm_err(crtc->dev,
 				"[CRTC:%d:%s] commit wait timed out\n",
 				crtc->base.id, crtc->name);
 	}
 
-	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
+	for_each_old_connector_in_state(state, conn, old_conn_state, i) {
 		ret = drm_crtc_commit_wait(old_conn_state->commit);
 		if (ret)
 			drm_err(conn->dev,
 				"[CONNECTOR:%d:%s] commit wait timed out\n",
 				conn->base.id, conn->name);
 	}
 
-	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
 		ret = drm_crtc_commit_wait(old_plane_state->commit);
 		if (ret)
 			drm_err(plane->dev,
 				"[PLANE:%d:%s] commit wait timed out\n",
 				plane->base.id, plane->name);

-- 
2.47.1

[PATCH 03/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_tail()

[Maxime Ripard]

drm_atomic_helper_commit_tail() is the final part of an atomic commit,
and is given a parameter with the drm_atomic_state being committed.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.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 48dc4f18e757549b67940cc5ca4d76812ef13985..c80d2ee0247a5316dd103d6178c7fa96eb366c9a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1756,38 +1756,38 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done);
 
 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This is the default implementation for the
  * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers
  * that do not support runtime_pm or do not need the CRTC to be
  * enabled to perform a commit. Otherwise, see
  * drm_atomic_helper_commit_tail_rpm().
  *
  * Note that the default ordering of how the various stages are called is to
  * match the legacy modeset helper library closest.
  */
-void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
+void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = old_state->dev;
+	struct drm_device *dev = state->dev;
 
-	drm_atomic_helper_commit_modeset_disables(dev, old_state);
+	drm_atomic_helper_commit_modeset_disables(dev, state);
 
-	drm_atomic_helper_commit_planes(dev, old_state, 0);
+	drm_atomic_helper_commit_planes(dev, state, 0);
 
-	drm_atomic_helper_commit_modeset_enables(dev, old_state);
+	drm_atomic_helper_commit_modeset_enables(dev, state);
 
-	drm_atomic_helper_fake_vblank(old_state);
+	drm_atomic_helper_fake_vblank(state);
 
-	drm_atomic_helper_commit_hw_done(old_state);
+	drm_atomic_helper_commit_hw_done(state);
 
-	drm_atomic_helper_wait_for_vblanks(dev, old_state);
+	drm_atomic_helper_wait_for_vblanks(dev, state);
 
-	drm_atomic_helper_cleanup_planes(dev, old_state);
+	drm_atomic_helper_cleanup_planes(dev, state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
 
 /**
  * drm_atomic_helper_commit_tail_rpm - commit atomic update to hardware

-- 
2.47.1

[PATCH 04/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_tail_rpm()

[Maxime Ripard]

drm_atomic_helper_commit_tail_rpm() is the final part of an atomic
commit, and is given the state being committed as a parameter.

However, that parameter is named old_state, but documented as the "new
modeset state" which is all super confusing.

Let's rename that parameter to state.

Signed-off-by: Maxime Ripard <mripard@kernel.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 c80d2ee0247a5316dd103d6178c7fa96eb366c9a..cb08fda752c65d6d0c4388bb4bf7054e74a053a8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1789,36 +1789,36 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
 
 /**
  * drm_atomic_helper_commit_tail_rpm - commit atomic update to hardware
- * @old_state: new modeset state to be committed
+ * @state: new modeset state to be committed
  *
  * This is an alternative implementation for the
  * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers
  * that support runtime_pm or need the CRTC to be enabled to perform a
  * commit. Otherwise, one should use the default implementation
  * drm_atomic_helper_commit_tail().
  */
-void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
+void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = old_state->dev;
+	struct drm_device *dev = state->dev;
 
-	drm_atomic_helper_commit_modeset_disables(dev, old_state);
+	drm_atomic_helper_commit_modeset_disables(dev, state);
 
-	drm_atomic_helper_commit_modeset_enables(dev, old_state);
+	drm_atomic_helper_commit_modeset_enables(dev, state);
 
-	drm_atomic_helper_commit_planes(dev, old_state,
+	drm_atomic_helper_commit_planes(dev, state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 
-	drm_atomic_helper_fake_vblank(old_state);
+	drm_atomic_helper_fake_vblank(state);
 
-	drm_atomic_helper_commit_hw_done(old_state);
+	drm_atomic_helper_commit_hw_done(state);
 
-	drm_atomic_helper_wait_for_vblanks(dev, old_state);
+	drm_atomic_helper_wait_for_vblanks(dev, state);
 
-	drm_atomic_helper_cleanup_planes(dev, old_state);
+	drm_atomic_helper_cleanup_planes(dev, state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm);
 
 static void commit_tail(struct drm_atomic_state *state)
 {

-- 
2.47.1

[PATCH 05/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_modeset_disables()

[Maxime Ripard]

drm_atomic_helper_modeset_disables() disables all the outputs affected
by a commit. It takes the drm_atomic_state being committed as a
parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index cb08fda752c65d6d0c4388bb4bf7054e74a053a8..d5129de2278d5f62c136f8b6c831f903d9e53927 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1427,11 +1427,11 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 }
 
 /**
  * drm_atomic_helper_commit_modeset_disables - modeset commit to disable outputs
  * @dev: DRM device
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This function shuts down all the outputs that need to be shut down and
  * prepares them (if required) with the new mode.
  *
  * For compatibility with legacy CRTC helpers this should be called before
@@ -1439,18 +1439,18 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
  * does. But drivers with different needs can group the modeset commits together
  * and do the plane commits at the end. This is useful for drivers doing runtime
  * PM since planes updates then only happen when the CRTC is actually enabled.
  */
 void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev,
-					       struct drm_atomic_state *old_state)
+					       struct drm_atomic_state *state)
 {
-	disable_outputs(dev, old_state);
+	disable_outputs(dev, state);
 
-	drm_atomic_helper_update_legacy_modeset_state(dev, old_state);
-	drm_atomic_helper_calc_timestamping_constants(old_state);
+	drm_atomic_helper_update_legacy_modeset_state(dev, state);
+	drm_atomic_helper_calc_timestamping_constants(state);
 
-	crtc_set_mode(dev, old_state);
+	crtc_set_mode(dev, state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_disables);
 
 static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 						struct drm_atomic_state *old_state)

-- 
2.47.1

[PATCH 06/29] drm/atomic-helper: Change parameter name of disable_outputs()

[Maxime Ripard]

disable_outputs() disables all connectors and CRTCs affected by a
commit. It takes the drm_atomic_state being committed as a parameter.

However, that parameter name is called as old_state, which is pretty
confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d5129de2278d5f62c136f8b6c831f903d9e53927..0d561b5c249a79a723e047f061d4103ef7742e87 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1148,19 +1148,19 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 	       (old_state->self_refresh_active && !new_state->active) ||
 	       new_state->self_refresh_active;
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *old_conn_state, *new_conn_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 
-	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 		struct drm_bridge *bridge;
 
 		/*
@@ -1168,15 +1168,15 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		 * still on. So need to check the old, saved state.
 		 */
 		if (!old_conn_state->crtc)
 			continue;
 
-		old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
+		old_crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);
 
 		if (new_conn_state->crtc)
 			new_crtc_state = drm_atomic_get_new_crtc_state(
-						old_state,
+						state,
 						new_conn_state->crtc);
 		else
 			new_crtc_state = NULL;
 
 		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
@@ -1199,28 +1199,28 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call disable hooks twice.
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_disable(bridge, old_state);
+		drm_atomic_bridge_chain_disable(bridge, state);
 
 		/* Right function depends upon target state. */
 		if (funcs) {
 			if (funcs->atomic_disable)
-				funcs->atomic_disable(encoder, old_state);
+				funcs->atomic_disable(encoder, state);
 			else if (new_conn_state->crtc && funcs->prepare)
 				funcs->prepare(encoder);
 			else if (funcs->disable)
 				funcs->disable(encoder);
 			else if (funcs->dpms)
 				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
 		}
 
-		drm_atomic_bridge_chain_post_disable(bridge, old_state);
+		drm_atomic_bridge_chain_post_disable(bridge, state);
 	}
 
-	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 		int ret;
 
 		/* Shut down everything that needs a full modeset. */
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
@@ -1237,11 +1237,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		/* Right function depends upon target state. */
 		if (new_crtc_state->enable && funcs->prepare)
 			funcs->prepare(crtc);
 		else if (funcs->atomic_disable)
-			funcs->atomic_disable(crtc, old_state);
+			funcs->atomic_disable(crtc, state);
 		else if (funcs->disable)
 			funcs->disable(crtc);
 		else if (funcs->dpms)
 			funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
 

-- 
2.47.1

[PATCH 07/29] drm/bridge: Change parameter name of drm_atomic_bridge_chain_disable()

[Maxime Ripard]

drm_atomic_bridge_chain_disable() disables all bridges affected by a new
commit. It takes the drm_atomic_state being committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_bridge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c6af46dd02bfa9e15b59e4c460debdd7fd84be44..e714c8ddd9d564473a41c465e437a6022dfd031c 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -544,21 +544,21 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 EXPORT_SYMBOL(drm_bridge_chain_mode_set);
 
 /**
  * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
- * @old_state: old atomic state
+ * @state: atomic state being committed
  *
  * Calls &drm_bridge_funcs.atomic_disable (falls back on
  * &drm_bridge_funcs.disable) op for all the bridges in the encoder chain,
  * starting from the last bridge to the first. These are called before calling
  * &drm_encoder_helper_funcs.atomic_disable
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
-				     struct drm_atomic_state *old_state)
+				     struct drm_atomic_state *state)
 {
 	struct drm_encoder *encoder;
 	struct drm_bridge *iter;
 
 	if (!bridge)
@@ -568,11 +568,11 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
 		if (iter->funcs->atomic_disable) {
 			struct drm_bridge_state *old_bridge_state;
 
 			old_bridge_state =
-				drm_atomic_get_old_bridge_state(old_state,
+				drm_atomic_get_old_bridge_state(state,
 								iter);
 			if (WARN_ON(!old_bridge_state))
 				return;
 
 			iter->funcs->atomic_disable(iter, old_bridge_state);

-- 
2.47.1

[PATCH 08/29] drm/bridge: Change parameter name of drm_atomic_bridge_chain_post_disable()

[Maxime Ripard]

drm_atomic_bridge_chain_post_disable() disables all bridges affected by
a new commit. It takes the drm_atomic_state being committed as a
parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_bridge.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index e714c8ddd9d564473a41c465e437a6022dfd031c..9755761d0d0f2659cf8d71e1734aee9772cc8c0f 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -585,17 +585,17 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
 
 static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
-						struct drm_atomic_state *old_state)
+						struct drm_atomic_state *state)
 {
-	if (old_state && bridge->funcs->atomic_post_disable) {
+	if (state && bridge->funcs->atomic_post_disable) {
 		struct drm_bridge_state *old_bridge_state;
 
 		old_bridge_state =
-			drm_atomic_get_old_bridge_state(old_state,
+			drm_atomic_get_old_bridge_state(state,
 							bridge);
 		if (WARN_ON(!old_bridge_state))
 			return;
 
 		bridge->funcs->atomic_post_disable(bridge,
@@ -607,11 +607,11 @@ static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
 
 /**
  * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
  *					  in the encoder chain
  * @bridge: bridge control structure
- * @old_state: old atomic state
+ * @state: atomic state being committed
  *
  * Calls &drm_bridge_funcs.atomic_post_disable (falls back on
  * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
  * starting from the first bridge to the last. These are called after completing
  * &drm_encoder_helper_funcs.atomic_disable
@@ -628,11 +628,11 @@ static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
  * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
-					  struct drm_atomic_state *old_state)
+					  struct drm_atomic_state *state)
 {
 	struct drm_encoder *encoder;
 	struct drm_bridge *next, *limit;
 
 	if (!bridge)
@@ -675,16 +675,16 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 								 chain_node) {
 					if (next == bridge)
 						break;
 
 					drm_atomic_bridge_call_post_disable(next,
-									    old_state);
+									    state);
 				}
 			}
 		}
 
-		drm_atomic_bridge_call_post_disable(bridge, old_state);
+		drm_atomic_bridge_call_post_disable(bridge, state);
 
 		if (limit)
 			/* Jump all bridges that we have already post_disabled */
 			bridge = limit;
 	}

-- 
2.47.1

[PATCH 09/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_update_legacy_modeset_state()

[Maxime Ripard]

drm_atomic_helper_update_legacy_modeset_state() updates all the legacy
modeset pointers a connector, encoder or CRTC might have with the ones
being setup by a given commit. It takes the drm_atomic_state being
committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0d561b5c249a79a723e047f061d4103ef7742e87..50b3fdb033eaae8e852a4fd298d2e958be1201f3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1265,11 +1265,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 }
 
 /**
  * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
  * @dev: DRM device
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This function updates all the various legacy modeset state pointers in
  * connectors, encoders and CRTCs.
  *
  * Drivers can use this for building their own atomic commit if they don't have
@@ -1281,20 +1281,20 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
  * the legacy state pointers are only really useful for transitioning an
  * existing driver to the atomic world.
  */
 void
 drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
-					      struct drm_atomic_state *old_state)
+					      struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *old_conn_state, *new_conn_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state;
 	int i;
 
 	/* clear out existing links and update dpms */
-	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
 		if (connector->encoder) {
 			WARN_ON(!connector->encoder->crtc);
 
 			connector->encoder->crtc = NULL;
 			connector->encoder = NULL;
@@ -1311,11 +1311,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 			connector->dpms = mode;
 		}
 	}
 
 	/* set new links */
-	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
 		if (!new_conn_state->crtc)
 			continue;
 
 		if (WARN_ON(!new_conn_state->best_encoder))
 			continue;
@@ -1323,19 +1323,19 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 		connector->encoder = new_conn_state->best_encoder;
 		connector->encoder->crtc = new_conn_state->crtc;
 	}
 
 	/* set legacy state in the crtc structure */
-	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) {
 		struct drm_plane *primary = crtc->primary;
 		struct drm_plane_state *new_plane_state;
 
 		crtc->mode = new_crtc_state->mode;
 		crtc->enabled = new_crtc_state->enable;
 
 		new_plane_state =
-			drm_atomic_get_new_plane_state(old_state, primary);
+			drm_atomic_get_new_plane_state(state, primary);
 
 		if (new_plane_state && new_plane_state->crtc == crtc) {
 			crtc->x = new_plane_state->src_x >> 16;
 			crtc->y = new_plane_state->src_y >> 16;
 		}

-- 
2.47.1

[PATCH 10/29] drm/atomic-helper: Change parameter name of crtc_set_mode()

[Maxime Ripard]

crtc_set_mode() deals with calling the modeset related hooks for CRTC,
connectors and bridges if and when a new commit changes them. It takes
the drm_atomic_state being committed as a parameter.

However, that parameter name is called as old_state, which is pretty
confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 50b3fdb033eaae8e852a4fd298d2e958be1201f3..55c91f2821d550c8af52b71d8f452e0fdee997e4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1363,19 +1363,19 @@ void drm_atomic_helper_calc_timestamping_constants(struct drm_atomic_state *stat
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_calc_timestamping_constants);
 
 static void
-crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
+crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state;
 	struct drm_connector *connector;
 	struct drm_connector_state *new_conn_state;
 	int i;
 
-	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) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		if (!new_crtc_state->mode_changed)
 			continue;
 
@@ -1387,11 +1387,11 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 			funcs->mode_set_nofb(crtc);
 		}
 	}
 
-	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 		struct drm_display_mode *mode, *adjusted_mode;
 		struct drm_bridge *bridge;
 

-- 
2.47.1

[PATCH 11/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_planes()

[Maxime Ripard]

drm_atomic_helper_commit_planes() updates all planes affected by a new
commit. It takes the drm_atomic_state being committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 55c91f2821d550c8af52b71d8f452e0fdee997e4..9f3cf5df0dd810e3725bf44a02a9574f676f17de 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2719,19 +2719,19 @@ static bool plane_crtc_active(const struct drm_plane_state *state)
 }
 
 /**
  * drm_atomic_helper_commit_planes - commit plane state
  * @dev: DRM device
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  * @flags: flags for committing plane state
  *
  * This function commits the new plane state using the plane and atomic helper
  * functions for planes and CRTCs. It assumes that the atomic state has already
  * been pushed into the relevant object state pointers, since this step can no
  * longer fail.
  *
- * It still requires the global state object @old_state to know which planes and
+ * It still requires the global state object @state to know which planes and
  * crtcs need to be updated though.
  *
  * Note that this function does all plane updates across all CRTCs in one step.
  * If the hardware can't support this approach look at
  * drm_atomic_helper_commit_planes_on_crtc() instead.
@@ -2758,36 +2758,36 @@ static bool plane_crtc_active(const struct drm_plane_state *state)
  * The drm_atomic_helper_commit() default implementation doesn't set the
  * ACTIVE_ONLY flag to most closely match the behaviour of the legacy helpers.
  * This should not be copied blindly by drivers.
  */
 void drm_atomic_helper_commit_planes(struct drm_device *dev,
-				     struct drm_atomic_state *old_state,
+				     struct drm_atomic_state *state,
 				     uint32_t flags)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	int i;
 	bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
 	bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
 
-	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		funcs = crtc->helper_private;
 
 		if (!funcs || !funcs->atomic_begin)
 			continue;
 
 		if (active_only && !new_crtc_state->active)
 			continue;
 
-		funcs->atomic_begin(crtc, old_state);
+		funcs->atomic_begin(crtc, state);
 	}
 
-	for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) {
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 		bool disabling;
 
 		funcs = plane->helper_private;
 
@@ -2821,40 +2821,40 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 
 			if (drm_atomic_crtc_needs_modeset(crtc_state) &&
 			    no_disable)
 				continue;
 
-			funcs->atomic_disable(plane, old_state);
+			funcs->atomic_disable(plane, state);
 		} else if (new_plane_state->crtc || disabling) {
-			funcs->atomic_update(plane, old_state);
+			funcs->atomic_update(plane, state);
 
 			if (!disabling && funcs->atomic_enable) {
 				if (drm_atomic_plane_enabling(old_plane_state, new_plane_state))
-					funcs->atomic_enable(plane, old_state);
+					funcs->atomic_enable(plane, state);
 			}
 		}
 	}
 
-	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		funcs = crtc->helper_private;
 
 		if (!funcs || !funcs->atomic_flush)
 			continue;
 
 		if (active_only && !new_crtc_state->active)
 			continue;
 
-		funcs->atomic_flush(crtc, old_state);
+		funcs->atomic_flush(crtc, state);
 	}
 
 	/*
 	 * Signal end of framebuffer access here before hw_done. After hw_done,
 	 * a later commit might have already released the plane state.
 	 */
-	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs = plane->helper_private;
 
 		if (funcs->end_fb_access)
 			funcs->end_fb_access(plane, old_plane_state);
 	}

-- 
2.47.1

[PATCH 12/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_modeset_enables()

[Maxime Ripard]

drm_atomic_helper_commit_modeset_enables() enables all outputs affected
by a new commit. It takes the drm_atomic_state being committed as a
parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9f3cf5df0dd810e3725bf44a02a9574f676f17de..b95e30dee1b4e2ba8991affd1e280a7a36171afd 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1474,11 +1474,11 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 }
 
 /**
  * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
  * @dev: DRM device
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This function enables all the outputs with the new configuration which had to
  * be turned off for the update.
  *
  * For compatibility with legacy CRTC helpers this should be called after
@@ -1486,20 +1486,20 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
  * does. But drivers with different needs can group the modeset commits together
  * and do the plane commits at the end. This is useful for drivers doing runtime
  * PM since planes updates then only happen when the CRTC is actually enabled.
  */
 void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
-					      struct drm_atomic_state *old_state)
+					      struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
 	struct drm_connector *connector;
 	struct drm_connector_state *new_conn_state;
 	int i;
 
-	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
 		/* Need to filter out CRTCs where only planes change. */
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			continue;
@@ -1511,17 +1511,17 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		if (new_crtc_state->enable) {
 			drm_dbg_atomic(dev, "enabling [CRTC:%d:%s]\n",
 				       crtc->base.id, crtc->name);
 			if (funcs->atomic_enable)
-				funcs->atomic_enable(crtc, old_state);
+				funcs->atomic_enable(crtc, state);
 			else if (funcs->commit)
 				funcs->commit(crtc);
 		}
 	}
 
-	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
 		struct drm_bridge *bridge;
 
 		if (!new_conn_state->best_encoder)
@@ -1540,25 +1540,25 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call enable hooks twice.
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
+		drm_atomic_bridge_chain_pre_enable(bridge, state);
 
 		if (funcs) {
 			if (funcs->atomic_enable)
-				funcs->atomic_enable(encoder, old_state);
+				funcs->atomic_enable(encoder, state);
 			else if (funcs->enable)
 				funcs->enable(encoder);
 			else if (funcs->commit)
 				funcs->commit(encoder);
 		}
 
-		drm_atomic_bridge_chain_enable(bridge, old_state);
+		drm_atomic_bridge_chain_enable(bridge, state);
 	}
 
-	drm_atomic_helper_commit_writebacks(dev, old_state);
+	drm_atomic_helper_commit_writebacks(dev, state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 
 /*
  * For atomic updates which touch just a single CRTC, calculate the time of the

-- 
2.47.1

[PATCH 13/29] drm/bridge: Change parameter name of drm_atomic_bridge_chain_pre_enable()

[Maxime Ripard]

drm_atomic_bridge_chain_pre_enable() enables all bridges affected by
a new commit. It takes the drm_atomic_state being committed as a
parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_bridge.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 9755761d0d0f2659cf8d71e1734aee9772cc8c0f..b060b699e059f0d4bd47dcc4aba49399ce45b332 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -690,17 +690,17 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
 
 static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
-					      struct drm_atomic_state *old_state)
+					      struct drm_atomic_state *state)
 {
-	if (old_state && bridge->funcs->atomic_pre_enable) {
+	if (state && bridge->funcs->atomic_pre_enable) {
 		struct drm_bridge_state *old_bridge_state;
 
 		old_bridge_state =
-			drm_atomic_get_old_bridge_state(old_state,
+			drm_atomic_get_old_bridge_state(state,
 							bridge);
 		if (WARN_ON(!old_bridge_state))
 			return;
 
 		bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
@@ -711,11 +711,11 @@ static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
 
 /**
  * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
  *					the encoder chain
  * @bridge: bridge control structure
- * @old_state: old atomic state
+ * @state: atomic state being committed
  *
  * Calls &drm_bridge_funcs.atomic_pre_enable (falls back on
  * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
  * starting from the last bridge to the first. These are called before calling
  * &drm_encoder_helper_funcs.atomic_enable
@@ -731,11 +731,11 @@ static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
  * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
-					struct drm_atomic_state *old_state)
+					struct drm_atomic_state *state)
 {
 	struct drm_encoder *encoder;
 	struct drm_bridge *iter, *next, *limit;
 
 	if (!bridge)
@@ -770,15 +770,15 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 					/* At the first bridge to request prev
 					 * bridges called first.
 					 */
 					break;
 
-				drm_atomic_bridge_call_pre_enable(next, old_state);
+				drm_atomic_bridge_call_pre_enable(next, state);
 			}
 		}
 
-		drm_atomic_bridge_call_pre_enable(iter, old_state);
+		drm_atomic_bridge_call_pre_enable(iter, state);
 
 		if (iter->pre_enable_prev_first)
 			/* Jump all bridges that we have already pre_enabled */
 			iter = limit;
 

-- 
2.47.1

[PATCH 14/29] drm/bridge: Change parameter name of drm_atomic_bridge_chain_enable()

[Maxime Ripard]

drm_atomic_bridge_chain_enable() enables all bridges affected by a new
commit. It takes the drm_atomic_state being committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_bridge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index b060b699e059f0d4bd47dcc4aba49399ce45b332..c937980d6591fd98e33e37d799ebf84e7e6c5529 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -789,21 +789,21 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
 
 /**
  * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
  * @bridge: bridge control structure
- * @old_state: old atomic state
+ * @state: atomic state being committed
  *
  * Calls &drm_bridge_funcs.atomic_enable (falls back on
  * &drm_bridge_funcs.enable) op for all the bridges in the encoder chain,
  * starting from the first bridge to the last. These are called after completing
  * &drm_encoder_helper_funcs.atomic_enable
  *
  * Note: the bridge passed should be the one closest to the encoder
  */
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
-				    struct drm_atomic_state *old_state)
+				    struct drm_atomic_state *state)
 {
 	struct drm_encoder *encoder;
 
 	if (!bridge)
 		return;
@@ -812,11 +812,11 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
 		if (bridge->funcs->atomic_enable) {
 			struct drm_bridge_state *old_bridge_state;
 
 			old_bridge_state =
-				drm_atomic_get_old_bridge_state(old_state,
+				drm_atomic_get_old_bridge_state(state,
 								bridge);
 			if (WARN_ON(!old_bridge_state))
 				return;
 
 			bridge->funcs->atomic_enable(bridge, old_bridge_state);

-- 
2.47.1

[PATCH 15/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_writebacks()

[Maxime Ripard]

drm_atomic_helper_commit_writebacks() updates all writeback connectors
affected by a new commit. It takes the drm_atomic_state being committed
as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b95e30dee1b4e2ba8991affd1e280a7a36171afd..cf859fa0110ad1154021e970dc262387cf3eb0c8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1451,26 +1451,26 @@ void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev,
 	crtc_set_mode(dev, state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_disables);
 
 static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
-						struct drm_atomic_state *old_state)
+						struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *new_conn_state;
 	int i;
 
-	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
 		const struct drm_connector_helper_funcs *funcs;
 
 		funcs = connector->helper_private;
 		if (!funcs->atomic_commit)
 			continue;
 
 		if (new_conn_state->writeback_job && new_conn_state->writeback_job->fb) {
 			WARN_ON(connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
-			funcs->atomic_commit(connector, old_state);
+			funcs->atomic_commit(connector, state);
 		}
 	}
 }
 
 /**

-- 
2.47.1

[PATCH 16/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_fake_vblank()

[Maxime Ripard]

drm_atomic_helper_fake_vblank() fake a vblank event if needed when a new
commit is being applied. It takes the drm_atomic_state being committed
as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index cf859fa0110ad1154021e970dc262387cf3eb0c8..78bc540158b7c64129eb728d8f8d67a6797bd0b3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2460,11 +2460,11 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 
 /**
  * drm_atomic_helper_fake_vblank - fake VBLANK events if needed
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This function walks all CRTCs and fakes VBLANK events on those with
  * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL.
  * The primary use of this function is writeback connectors working in oneshot
  * mode and faking VBLANK events. In this case they only fake the VBLANK event
@@ -2476,29 +2476,29 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
  * VBLANK interrupt.
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state)
+void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state)
 {
 	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i;
 
-	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) {
 		unsigned long flags;
 
 		if (!new_crtc_state->no_vblank)
 			continue;
 
-		spin_lock_irqsave(&old_state->dev->event_lock, flags);
+		spin_lock_irqsave(&state->dev->event_lock, flags);
 		if (new_crtc_state->event) {
 			drm_crtc_send_vblank_event(crtc,
 						   new_crtc_state->event);
 			new_crtc_state->event = NULL;
 		}
-		spin_unlock_irqrestore(&old_state->dev->event_lock, flags);
+		spin_unlock_irqrestore(&state->dev->event_lock, flags);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
 
 /**

-- 
2.47.1

[PATCH 17/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_hw_done()

[Maxime Ripard]

drm_atomic_helper_commit_hw_done() signals hardware completion of a
given commit. It takes the drm_atomic_state being committed as a
parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 78bc540158b7c64129eb728d8f8d67a6797bd0b3..d642bbc074bd364256409c91649f8d8aa10f5023 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2501,11 +2501,11 @@ void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state)
 }
 EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
 
 /**
  * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This function is used to signal completion of the hardware commit step. After
  * this step the driver is not allowed to read or change any permanent software
  * or hardware modeset state. The only exception is state protected by other
  * means than &drm_modeset_lock locks.
@@ -2514,18 +2514,18 @@ EXPORT_SYMBOL(drm_atomic_helper_fake_vblank);
  * this function is called.
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
+void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 
-	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		commit = new_crtc_state->commit;
 		if (!commit)
 			continue;
 
 		/*

-- 
2.47.1

[PATCH 18/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_wait_for_vblanks()

[Maxime Ripard]

drm_atomic_helper_wait_for_vblanks() waits for vblank events on all the
CRTCs affected by a commit. It takes the drm_atomic_state being
committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d642bbc074bd364256409c91649f8d8aa10f5023..581fd0f14ba0aa2a9d0334cad49575de3fb5fb20 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1656,11 +1656,11 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_fences);
 
 /**
  * drm_atomic_helper_wait_for_vblanks - wait for vblank on CRTCs
  * @dev: DRM device
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * Helper to, after atomic commit, wait for vblanks on all affected
  * CRTCs (ie. before cleaning up old framebuffers using
  * drm_atomic_helper_cleanup_planes()). It will only wait on CRTCs where the
  * framebuffers have actually changed to optimize for the legacy cursor and
@@ -1670,44 +1670,44 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_fences);
  * drm_atomic_helper_setup_commit() should look at
  * drm_atomic_helper_wait_for_flip_done() as an alternative.
  */
 void
 drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
-		struct drm_atomic_state *old_state)
+				   struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i, ret;
 	unsigned int crtc_mask = 0;
 
 	 /*
 	  * Legacy cursor ioctls are completely unsynced, and userspace
 	  * relies on that (by doing tons of cursor updates).
 	  */
-	if (old_state->legacy_cursor_update)
+	if (state->legacy_cursor_update)
 		return;
 
-	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		if (!new_crtc_state->active)
 			continue;
 
 		ret = drm_crtc_vblank_get(crtc);
 		if (ret != 0)
 			continue;
 
 		crtc_mask |= drm_crtc_mask(crtc);
-		old_state->crtcs[i].last_vblank_count = drm_crtc_vblank_count(crtc);
+		state->crtcs[i].last_vblank_count = drm_crtc_vblank_count(crtc);
 	}
 
-	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		if (!(crtc_mask & drm_crtc_mask(crtc)))
 			continue;
 
 		ret = wait_event_timeout(dev->vblank[i].queue,
-				old_state->crtcs[i].last_vblank_count !=
-					drm_crtc_vblank_count(crtc),
-				msecs_to_jiffies(100));
+					 state->crtcs[i].last_vblank_count !=
+						drm_crtc_vblank_count(crtc),
+					 msecs_to_jiffies(100));
 
 		WARN(!ret, "[CRTC:%d:%s] vblank wait timed out\n",
 		     crtc->base.id, crtc->name);
 
 		drm_crtc_vblank_put(crtc);

-- 
2.47.1

[PATCH 19/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_cleanup_planes()

[Maxime Ripard]

drm_atomic_helper_cleanup_planes() is one of the final part of a commit,
and will free up all plane resources used in the previous commit.  It
takes the drm_atomic_state being committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 581fd0f14ba0aa2a9d0334cad49575de3fb5fb20..000ae2d4ded286458ee386e34336d650907a8073 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2977,28 +2977,28 @@ drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state *old_crtc_state,
 EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc);
 
 /**
  * drm_atomic_helper_cleanup_planes - cleanup plane resources after commit
  * @dev: DRM device
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * This function cleans up plane state, specifically framebuffers, from the old
- * configuration. Hence the old configuration must be perserved in @old_state to
+ * configuration. Hence the old configuration must be perserved in @state to
  * be able to call this function.
  *
  * This function may not be called on the new state when the atomic update
  * fails at any point after calling drm_atomic_helper_prepare_planes(). Use
  * drm_atomic_helper_unprepare_planes() in this case.
  */
 void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
-				      struct drm_atomic_state *old_state)
+				      struct drm_atomic_state *state)
 {
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state;
 	int i;
 
-	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
 			funcs->cleanup_fb(plane, old_plane_state);
 	}

-- 
2.47.1

[PATCH 20/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_commit_cleanup_done()

[Maxime Ripard]

drm_atomic_helper_wait_for_dependencies() is the final part of a commit
and signals it completion. It takes the drm_atomic_state being committed
as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.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 000ae2d4ded286458ee386e34336d650907a8073..baa32b10c4a08252b736927fedd36c3fccbe6020 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2541,36 +2541,36 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
 		/* backend must have consumed any event by now */
 		WARN_ON(new_crtc_state->event);
 		complete_all(&commit->hw_done);
 	}
 
-	if (old_state->fake_commit) {
-		complete_all(&old_state->fake_commit->hw_done);
-		complete_all(&old_state->fake_commit->flip_done);
+	if (state->fake_commit) {
+		complete_all(&state->fake_commit->hw_done);
+		complete_all(&state->fake_commit->flip_done);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 
 /**
  * drm_atomic_helper_commit_cleanup_done - signal completion of commit
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
- * This signals completion of the atomic update @old_state, including any
+ * This signals completion of the atomic update @state, including any
  * cleanup work. If used, it must be called right before calling
  * drm_atomic_state_put().
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
  */
-void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
+void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 
-	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		commit = old_crtc_state->commit;
 		if (WARN_ON(!commit))
 			continue;
 
 		complete_all(&commit->cleanup_done);
@@ -2579,13 +2579,13 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 		spin_lock(&crtc->commit_lock);
 		list_del(&commit->commit_entry);
 		spin_unlock(&crtc->commit_lock);
 	}
 
-	if (old_state->fake_commit) {
-		complete_all(&old_state->fake_commit->cleanup_done);
-		WARN_ON(!try_wait_for_completion(&old_state->fake_commit->hw_done));
+	if (state->fake_commit) {
+		complete_all(&state->fake_commit->cleanup_done);
+		WARN_ON(!try_wait_for_completion(&state->fake_commit->hw_done));
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
 
 /**

-- 
2.47.1

[PATCH 21/29] drm/atomic-helper: Change parameter name of drm_atomic_helper_wait_for_flip_done()

[Maxime Ripard]

drm_atomic_helper_wait_for_flip_done() will wait for pages flips on all
CRTCs affected by a given commit. It takes the drm_atomic_state being
committed as a parameter.

However, that parameter name is called (and documented) as old_state,
which is pretty confusing. Let's rename that variable as state.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index baa32b10c4a08252b736927fedd36c3fccbe6020..cb24e590a39f80c0cc5feb8b6a9a4fd41a651211 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1716,11 +1716,11 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 
 /**
  * drm_atomic_helper_wait_for_flip_done - wait for all page flips to be done
  * @dev: DRM device
- * @old_state: atomic state object with old state structures
+ * @state: atomic state object being committed
  *
  * Helper to, after atomic commit, wait for page flips on all affected
  * crtcs (ie. before cleaning up old framebuffers using
  * drm_atomic_helper_cleanup_planes()). Compared to
  * drm_atomic_helper_wait_for_vblanks() this waits for the completion on all
@@ -1729,32 +1729,32 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
  *
  * This requires that drivers use the nonblocking commit tracking support
  * initialized using drm_atomic_helper_setup_commit().
  */
 void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
-					  struct drm_atomic_state *old_state)
+					  struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	int i;
 
 	for (i = 0; i < dev->mode_config.num_crtc; i++) {
-		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+		struct drm_crtc_commit *commit = state->crtcs[i].commit;
 		int ret;
 
-		crtc = old_state->crtcs[i].ptr;
+		crtc = state->crtcs[i].ptr;
 
 		if (!crtc || !commit)
 			continue;
 
 		ret = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
 		if (ret == 0)
 			drm_err(dev, "[CRTC:%d:%s] flip_done timed out\n",
 				crtc->base.id, crtc->name);
 	}
 
-	if (old_state->fake_commit)
-		complete_all(&old_state->fake_commit->flip_done);
+	if (state->fake_commit)
+		complete_all(&state->fake_commit->flip_done);
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done);
 
 /**
  * drm_atomic_helper_commit_tail - commit atomic update to hardware

-- 
2.47.1

[PATCH 22/29] drm/bridge: Rename atomic hooks parameters to drop old prefix

[Maxime Ripard]

All the bridge atomic hooks were using the old_bridge_state name for
their drm_bridge_state parameter. However, this state is the current
state being committed for all of them, which ends up being confusing.

Let's rename it to bridge_state for all of them.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 include/drm/drm_bridge.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4b84faf14e368310dd20aa964e8178ec80aa6fa7..8e18130be8bb85fc2463917dde9bf1d281934184 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -303,11 +303,11 @@ struct drm_bridge_funcs {
 	 * there is one) when this callback is called.
 	 *
 	 * The @atomic_pre_enable callback is optional.
 	 */
 	void (*atomic_pre_enable)(struct drm_bridge *bridge,
-				  struct drm_bridge_state *old_bridge_state);
+				  struct drm_bridge_state *bridge_state);
 
 	/**
 	 * @atomic_enable:
 	 *
 	 * This callback should enable the bridge. It is called right after
@@ -323,11 +323,11 @@ struct drm_bridge_funcs {
 	 * chain if there is one.
 	 *
 	 * The @atomic_enable callback is optional.
 	 */
 	void (*atomic_enable)(struct drm_bridge *bridge,
-			      struct drm_bridge_state *old_bridge_state);
+			      struct drm_bridge_state *bridge_state);
 	/**
 	 * @atomic_disable:
 	 *
 	 * This callback should disable the bridge. It is called right before
 	 * the preceding element in the display pipe is disabled. If the
@@ -340,11 +340,11 @@ struct drm_bridge_funcs {
 	 * signals) feeding it is still running when this callback is called.
 	 *
 	 * The @atomic_disable callback is optional.
 	 */
 	void (*atomic_disable)(struct drm_bridge *bridge,
-			       struct drm_bridge_state *old_bridge_state);
+			       struct drm_bridge_state *bridge_state);
 
 	/**
 	 * @atomic_post_disable:
 	 *
 	 * This callback should disable the bridge. It is called right after the
@@ -359,11 +359,11 @@ struct drm_bridge_funcs {
 	 * called.
 	 *
 	 * The @atomic_post_disable callback is optional.
 	 */
 	void (*atomic_post_disable)(struct drm_bridge *bridge,
-				    struct drm_bridge_state *old_bridge_state);
+				    struct drm_bridge_state *bridge_state);
 
 	/**
 	 * @atomic_duplicate_state:
 	 *
 	 * Duplicate the current bridge state object (which is guaranteed to be

-- 
2.47.1

Re: [PATCH 22/29] drm/bridge: Rename atomic hooks parameters to drop old prefix

[Simona Vetter]

On Wed, Jan 15, 2025 at 10:05:29PM +0100, Maxime Ripard wrote:
> All the bridge atomic hooks were using the old_bridge_state name for
> their drm_bridge_state parameter. However, this state is the current
> state being committed for all of them, which ends up being confusing.
> 
> Let's rename it to bridge_state for all of them.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  include/drm/drm_bridge.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4b84faf14e368310dd20aa964e8178ec80aa6fa7..8e18130be8bb85fc2463917dde9bf1d281934184 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -303,11 +303,11 @@ struct drm_bridge_funcs {
>  	 * there is one) when this callback is called.
>  	 *
>  	 * The @atomic_pre_enable callback is optional.
>  	 */
>  	void (*atomic_pre_enable)(struct drm_bridge *bridge,
> -				  struct drm_bridge_state *old_bridge_state);
> +				  struct drm_bridge_state *bridge_state);
>  
>  	/**
>  	 * @atomic_enable:
>  	 *
>  	 * This callback should enable the bridge. It is called right after
> @@ -323,11 +323,11 @@ struct drm_bridge_funcs {
>  	 * chain if there is one.
>  	 *
>  	 * The @atomic_enable callback is optional.
>  	 */
>  	void (*atomic_enable)(struct drm_bridge *bridge,
> -			      struct drm_bridge_state *old_bridge_state);
> +			      struct drm_bridge_state *bridge_state);

Checked this one, and it very clearly passes the old state. Because the
new state you can get by looking at bridge->state. So this looks very
wrong.

If you want to fully update the pattern, pass the drm_atomic_state
instead, and let callbacks lookup any additional states they use as
needed.
-Sima

>  	/**
>  	 * @atomic_disable:
>  	 *
>  	 * This callback should disable the bridge. It is called right before
>  	 * the preceding element in the display pipe is disabled. If the
> @@ -340,11 +340,11 @@ struct drm_bridge_funcs {
>  	 * signals) feeding it is still running when this callback is called.
>  	 *
>  	 * The @atomic_disable callback is optional.
>  	 */
>  	void (*atomic_disable)(struct drm_bridge *bridge,
> -			       struct drm_bridge_state *old_bridge_state);
> +			       struct drm_bridge_state *bridge_state);
>  
>  	/**
>  	 * @atomic_post_disable:
>  	 *
>  	 * This callback should disable the bridge. It is called right after the
> @@ -359,11 +359,11 @@ struct drm_bridge_funcs {
>  	 * called.
>  	 *
>  	 * The @atomic_post_disable callback is optional.
>  	 */
>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
> -				    struct drm_bridge_state *old_bridge_state);
> +				    struct drm_bridge_state *bridge_state);
>  
>  	/**
>  	 * @atomic_duplicate_state:
>  	 *
>  	 * Duplicate the current bridge state object (which is guaranteed to be
> 
> -- 
> 2.47.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 22/29] drm/bridge: Rename atomic hooks parameters to drop old prefix

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

On Thu, Jan 16, 2025 at 12:34:12PM +0100, Simona Vetter wrote:
> On Wed, Jan 15, 2025 at 10:05:29PM +0100, Maxime Ripard wrote:
> > All the bridge atomic hooks were using the old_bridge_state name for
> > their drm_bridge_state parameter. However, this state is the current
> > state being committed for all of them, which ends up being confusing.
> > 
> > Let's rename it to bridge_state for all of them.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  include/drm/drm_bridge.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 4b84faf14e368310dd20aa964e8178ec80aa6fa7..8e18130be8bb85fc2463917dde9bf1d281934184 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -303,11 +303,11 @@ struct drm_bridge_funcs {
> >  	 * there is one) when this callback is called.
> >  	 *
> >  	 * The @atomic_pre_enable callback is optional.
> >  	 */
> >  	void (*atomic_pre_enable)(struct drm_bridge *bridge,
> > -				  struct drm_bridge_state *old_bridge_state);
> > +				  struct drm_bridge_state *bridge_state);
> >  
> >  	/**
> >  	 * @atomic_enable:
> >  	 *
> >  	 * This callback should enable the bridge. It is called right after
> > @@ -323,11 +323,11 @@ struct drm_bridge_funcs {
> >  	 * chain if there is one.
> >  	 *
> >  	 * The @atomic_enable callback is optional.
> >  	 */
> >  	void (*atomic_enable)(struct drm_bridge *bridge,
> > -			      struct drm_bridge_state *old_bridge_state);
> > +			      struct drm_bridge_state *bridge_state);
> 
> Checked this one, and it very clearly passes the old state.

Urgh, you're right

> Because the new state you can get by looking at bridge->state.

Bridge->state doesn't exist though.

> So this looks very wrong.
> 
> If you want to fully update the pattern, pass the drm_atomic_state
> instead, and let callbacks lookup any additional states they use as
> needed.

Yeah, that's probably the best option. I think I still have the
coccinelle scripts I used for the others somewhere.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 22/29] drm/bridge: Rename atomic hooks parameters to drop old prefix

[Simona Vetter]

On Fri, Jan 17, 2025 at 03:50:09PM +0100, Maxime Ripard wrote:
> On Thu, Jan 16, 2025 at 12:34:12PM +0100, Simona Vetter wrote:
> > On Wed, Jan 15, 2025 at 10:05:29PM +0100, Maxime Ripard wrote:
> > > All the bridge atomic hooks were using the old_bridge_state name for
> > > their drm_bridge_state parameter. However, this state is the current
> > > state being committed for all of them, which ends up being confusing.
> > > 
> > > Let's rename it to bridge_state for all of them.
> > > 
> > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > ---
> > >  include/drm/drm_bridge.h | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index 4b84faf14e368310dd20aa964e8178ec80aa6fa7..8e18130be8bb85fc2463917dde9bf1d281934184 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -303,11 +303,11 @@ struct drm_bridge_funcs {
> > >  	 * there is one) when this callback is called.
> > >  	 *
> > >  	 * The @atomic_pre_enable callback is optional.
> > >  	 */
> > >  	void (*atomic_pre_enable)(struct drm_bridge *bridge,
> > > -				  struct drm_bridge_state *old_bridge_state);
> > > +				  struct drm_bridge_state *bridge_state);
> > >  
> > >  	/**
> > >  	 * @atomic_enable:
> > >  	 *
> > >  	 * This callback should enable the bridge. It is called right after
> > > @@ -323,11 +323,11 @@ struct drm_bridge_funcs {
> > >  	 * chain if there is one.
> > >  	 *
> > >  	 * The @atomic_enable callback is optional.
> > >  	 */
> > >  	void (*atomic_enable)(struct drm_bridge *bridge,
> > > -			      struct drm_bridge_state *old_bridge_state);
> > > +			      struct drm_bridge_state *bridge_state);
> > 
> > Checked this one, and it very clearly passes the old state.
> 
> Urgh, you're right
> 
> > Because the new state you can get by looking at bridge->state.
> 
> Bridge->state doesn't exist though.

Yeah it's defacto your helper to go find the private object, deref the
->state pointer in there and then cast to the right type. But that again
has a bit the locking issue since sometimes you must hold the modeset
lock, sometimes not.

If we instead always go through the drm_atomic_state lookup then we can
require the modeset lock in the other helper for the bridge ->
bridge_state lookup, and things become much harder for drivers to get
wrong.

> > So this looks very wrong.
> > 
> > If you want to fully update the pattern, pass the drm_atomic_state
> > instead, and let callbacks lookup any additional states they use as
> > needed.
> 
> Yeah, that's probably the best option. I think I still have the
> coccinelle scripts I used for the others somewhere.

Yeah cocci would help here.
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 23/29] drm/bridge: Provide a helper to retrieve current bridge state

[Maxime Ripard]

The current bridge state is accessible from the drm_bridge structure,
but since it's fairly indirect it's not easy to figure out.

Provide a helper to retrieve it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 include/drm/drm_bridge.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 8e18130be8bb85fc2463917dde9bf1d281934184..95c5037a6335e4c1be511e6c31308202015c7754 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -955,10 +955,27 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 {
 	return NULL;
 }
 #endif
 
+/**
+ * @drm_bridge_get_current_state() - Get the current bridge state
+ * @bridge: bridge object
+ *
+ * RETURNS:
+ *
+ * The current bridge state, or NULL if there is none.
+ */
+static inline struct drm_bridge_state *
+drm_bridge_get_current_state(struct drm_bridge *bridge)
+{
+	if (!bridge)
+		return NULL;
+
+	return drm_priv_to_bridge_state(bridge->base.state);
+}
+
 /**
  * drm_bridge_get_next_bridge() - Get the next bridge in the chain
  * @bridge: bridge object
  *
  * RETURNS:

-- 
2.47.1

Re: [PATCH 23/29] drm/bridge: Provide a helper to retrieve current bridge state

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 10:05:30PM +0100, Maxime Ripard wrote:
> The current bridge state is accessible from the drm_bridge structure,
> but since it's fairly indirect it's not easy to figure out.
> 
> Provide a helper to retrieve it.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  include/drm/drm_bridge.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 8e18130be8bb85fc2463917dde9bf1d281934184..95c5037a6335e4c1be511e6c31308202015c7754 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -955,10 +955,27 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  {
>  	return NULL;
>  }
>  #endif
>  
> +/**
> + * @drm_bridge_get_current_state() - Get the current bridge state
> + * @bridge: bridge object
> + *
> + * RETURNS:
> + *
> + * The current bridge state, or NULL if there is none.

Are there any kind of limitations on when and how this function can be
used? I don't think we can be accessing the state randomly, as the
framework can change it at some points. E.g. what if the driver uses
this state from audio or cec callbacks, while the DRM framework performs
atomic commit and changes / frees the state right concurrently?

> + */
> +static inline struct drm_bridge_state *
> +drm_bridge_get_current_state(struct drm_bridge *bridge)
> +{
> +	if (!bridge)
> +		return NULL;
> +
> +	return drm_priv_to_bridge_state(bridge->base.state);
> +}
> +
>  /**
>   * drm_bridge_get_next_bridge() - Get the next bridge in the chain
>   * @bridge: bridge object
>   *
>   * RETURNS:
> 
> -- 
> 2.47.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 23/29] drm/bridge: Provide a helper to retrieve current bridge state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi,

On Thu, Jan 16, 2025 at 02:43:37AM +0200, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 10:05:30PM +0100, Maxime Ripard wrote:
> > The current bridge state is accessible from the drm_bridge structure,
> > but since it's fairly indirect it's not easy to figure out.
> > 
> > Provide a helper to retrieve it.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  include/drm/drm_bridge.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 8e18130be8bb85fc2463917dde9bf1d281934184..95c5037a6335e4c1be511e6c31308202015c7754 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -955,10 +955,27 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> >  {
> >  	return NULL;
> >  }
> >  #endif
> >  
> > +/**
> > + * @drm_bridge_get_current_state() - Get the current bridge state
> > + * @bridge: bridge object
> > + *
> > + * RETURNS:
> > + *
> > + * The current bridge state, or NULL if there is none.
> 
> Are there any kind of limitations on when and how this function can be
> used? I don't think we can be accessing the state randomly, as the
> framework can change it at some points. E.g. what if the driver uses
> this state from audio or cec callbacks, while the DRM framework performs
> atomic commit and changes / frees the state right concurrently?

The semantics are equivalent to drm_connector->state, drm_crtc->state,
etc, but I'm not sure we ever defined it clearly.

Also, it looks like I can never remember what the locking rules are :)
It looks like a good occasion to ask Sima and write some more
documentation.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 23/29] drm/bridge: Provide a helper to retrieve current bridge state

[Simona Vetter]

On Thu, Jan 16, 2025 at 02:43:37AM +0200, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 10:05:30PM +0100, Maxime Ripard wrote:
> > The current bridge state is accessible from the drm_bridge structure,
> > but since it's fairly indirect it's not easy to figure out.
> > 
> > Provide a helper to retrieve it.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  include/drm/drm_bridge.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 8e18130be8bb85fc2463917dde9bf1d281934184..95c5037a6335e4c1be511e6c31308202015c7754 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -955,10 +955,27 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> >  {
> >  	return NULL;
> >  }
> >  #endif
> >  
> > +/**
> > + * @drm_bridge_get_current_state() - Get the current bridge state
> > + * @bridge: bridge object
> > + *
> > + * RETURNS:
> > + *
> > + * The current bridge state, or NULL if there is none.
> 
> Are there any kind of limitations on when and how this function can be
> used? I don't think we can be accessing the state randomly, as the
> framework can change it at some points. E.g. what if the driver uses
> this state from audio or cec callbacks, while the DRM framework performs
> atomic commit and changes / frees the state right concurrently?

Yeah you can only look at this when either holding the corresponding
modesetlock, or in atomic commit. But in the latter it's a much cleaner
design pattern to instead look up the state from the drm_atomic_state,
since that pointer is the magic thing which guarantees lifetim/ownership
for lockless access in atomic commit code.
-Sima

> 
> > + */
> > +static inline struct drm_bridge_state *
> > +drm_bridge_get_current_state(struct drm_bridge *bridge)
> > +{
> > +	if (!bridge)
> > +		return NULL;
> > +
> > +	return drm_priv_to_bridge_state(bridge->base.state);
> > +}
> > +
> >  /**
> >   * drm_bridge_get_next_bridge() - Get the next bridge in the chain
> >   * @bridge: bridge object
> >   *
> >   * RETURNS:
> > 
> > -- 
> > 2.47.1
> > 
> 
> -- 
> With best wishes
> Dmitry

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 24/29] drm/bridge: Provide a helper to get the global state from a bridge state

[Maxime Ripard]

We have access to the global drm_atomic_state from a drm_bridge_state,
but since it's fairly indirect it's not as obvious as it can be for
other KMS entities.

Provide a helper to make it easier to figure out.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 include/drm/drm_atomic.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..bd7959ae312c99c0a0034d36378ae44f04f6a374 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -1183,10 +1183,26 @@ static inline struct drm_bridge_state *
 drm_priv_to_bridge_state(struct drm_private_state *priv)
 {
 	return container_of(priv, struct drm_bridge_state, base);
 }
 
+/**
+ * @drm_bridge_state_get_atomic_state() - Get the atomic state from a bridge state
+ * @bridge_state: bridge state object
+ *
+ * RETURNS:
+ * The global atomic state @bridge_state is a part of, or NULL if there is none.
+ */
+static inline struct drm_atomic_state *
+drm_bridge_state_get_atomic_state(struct drm_bridge_state *bridge_state)
+{
+	if (!bridge_state)
+		return NULL;
+
+	return bridge_state->base.state;
+}
+
 struct drm_bridge_state *
 drm_atomic_get_bridge_state(struct drm_atomic_state *state,
 			    struct drm_bridge *bridge);
 struct drm_bridge_state *
 drm_atomic_get_old_bridge_state(const struct drm_atomic_state *state,

-- 
2.47.1

Re: [PATCH 24/29] drm/bridge: Provide a helper to get the global state from a bridge state

[Simona Vetter]

On Wed, Jan 15, 2025 at 10:05:31PM +0100, Maxime Ripard wrote:
> We have access to the global drm_atomic_state from a drm_bridge_state,
> but since it's fairly indirect it's not as obvious as it can be for
> other KMS entities.
> 
> Provide a helper to make it easier to figure out.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  include/drm/drm_atomic.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..bd7959ae312c99c0a0034d36378ae44f04f6a374 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1183,10 +1183,26 @@ static inline struct drm_bridge_state *
>  drm_priv_to_bridge_state(struct drm_private_state *priv)
>  {
>  	return container_of(priv, struct drm_bridge_state, base);
>  }
>  
> +/**
> + * @drm_bridge_state_get_atomic_state() - Get the atomic state from a bridge state
> + * @bridge_state: bridge state object
> + *
> + * RETURNS:
> + * The global atomic state @bridge_state is a part of, or NULL if there is none.
> + */
> +static inline struct drm_atomic_state *
> +drm_bridge_state_get_atomic_state(struct drm_bridge_state *bridge_state)

So this one is nasty, because we clear out these backpointers once we push
the states into obj->state (because they can then outlive the
drm_atomic_state). Which means you can't use this in commit callbacks. Or
the bridge code has a really bad use-after-free when it doesn't clear out
these backpointers when we swap in the new states in
drm_atomic_helper_swap_state().

The better pattern is to just ditch passing individual states to callbacks
and just pass the entire drm_atomic_state container, and let callbacks
fish out what exactly they need. And also provide all necessary helpers to
find the right states and all that stuff.

We should probably also document that design approach in the kerneldoc for
drm_atomic_state, or wherever there's a good place for that.

See also my other reply for some of the history of why we have this mess.

Cheers, Sima

> +{
> +	if (!bridge_state)
> +		return NULL;
> +
> +	return bridge_state->base.state;
> +}
> +
>  struct drm_bridge_state *
>  drm_atomic_get_bridge_state(struct drm_atomic_state *state,
>  			    struct drm_bridge *bridge);
>  struct drm_bridge_state *
>  drm_atomic_get_old_bridge_state(const struct drm_atomic_state *state,
> 
> -- 
> 2.47.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 25/29] drm/bridge: Provide pointers to the connector and crtc in bridge state

[Maxime Ripard]

Now that connectors are no longer necessarily created by the bridges
drivers themselves but might be created by drm_bridge_connector, it's
pretty hard for bridge drivers to retrieve pointers to the connector and
CRTC they are attached to.

Indeed, the only way to retrieve the CRTC is to follow the drm_bridge
encoder field, and then the drm_encoder crtc field, both of them being
deprecated.

And for the connector, since we can have multiple connectors attached to
a CRTC, we don't really have a reliable way to get it.

Let's provide both pointers in the drm_bridge_state structure so we
don't have to follow deprecated, non-atomic, pointers, and be more
consistent with the other KMS entities.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  5 +++++
 drivers/gpu/drm/drm_bridge.c              | 21 +++++++++++++--------
 include/drm/drm_atomic.h                  | 14 ++++++++++++++
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 519228eb109533d2596e899a57b571fa0995824f..66661dca077215b78dffca7bc1712f56d35e3918 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -777,10 +777,15 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);
  * that don't subclass the bridge state.
  */
 void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,
 					    struct drm_bridge_state *state)
 {
+	if (state->connector) {
+		drm_connector_put(state->connector);
+		state->connector = NULL;
+	}
+
 	kfree(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
 
 /**
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
 
 static int drm_atomic_bridge_check(struct drm_bridge *bridge,
 				   struct drm_crtc_state *crtc_state,
 				   struct drm_connector_state *conn_state)
 {
+	struct drm_bridge_state *bridge_state;
+	int ret;
+
+	bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
+						       bridge);
+	if (WARN_ON(!bridge_state))
+		return -EINVAL;
+
+	bridge_state->crtc = crtc_state->crtc;
+
+	drm_connector_get(conn_state->connector);
+	bridge_state->connector = conn_state->connector;
+
 	if (bridge->funcs->atomic_check) {
-		struct drm_bridge_state *bridge_state;
-		int ret;
-
-		bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
-							       bridge);
-		if (WARN_ON(!bridge_state))
-			return -EINVAL;
-
 		ret = bridge->funcs->atomic_check(bridge, bridge_state,
 						  crtc_state, conn_state);
 		if (ret)
 			return ret;
 	} else if (bridge->funcs->mode_fixup) {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index bd7959ae312c99c0a0034d36378ae44f04f6a374..b2c5868a3a66280ffc7437fa7a8613079402facd 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -1166,10 +1166,24 @@ struct drm_bridge_state {
 	/**
 	 * @bridge: the bridge this state refers to
 	 */
 	struct drm_bridge *bridge;
 
+	/**
+	 * @crtc: CRTC the bridge is connected to, NULL if disabled.
+	 *
+	 * Do not change this directly.
+	 */
+	struct drm_crtc *crtc;
+
+	/**
+	 * @connector: The connector the bridge is connected to, NULL if disabled.
+	 *
+	 * Do not change this directly.
+	 */
+	struct drm_connector *connector;
+
 	/**
 	 * @input_bus_cfg: input bus configuration
 	 */
 	struct drm_bus_cfg input_bus_cfg;
 

-- 
2.47.1

Re: [PATCH 25/29] drm/bridge: Provide pointers to the connector and crtc in bridge state

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 10:05:32PM +0100, Maxime Ripard wrote:
> Now that connectors are no longer necessarily created by the bridges
> drivers themselves but might be created by drm_bridge_connector, it's
> pretty hard for bridge drivers to retrieve pointers to the connector and
> CRTC they are attached to.
> 
> Indeed, the only way to retrieve the CRTC is to follow the drm_bridge
> encoder field, and then the drm_encoder crtc field, both of them being
> deprecated.
> 
> And for the connector, since we can have multiple connectors attached to
> a CRTC, we don't really have a reliable way to get it.
> 
> Let's provide both pointers in the drm_bridge_state structure so we
> don't have to follow deprecated, non-atomic, pointers, and be more
> consistent with the other KMS entities.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c |  5 +++++
>  drivers/gpu/drm/drm_bridge.c              | 21 +++++++++++++--------
>  include/drm/drm_atomic.h                  | 14 ++++++++++++++
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 519228eb109533d2596e899a57b571fa0995824f..66661dca077215b78dffca7bc1712f56d35e3918 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -777,10 +777,15 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);
>   * that don't subclass the bridge state.
>   */
>  void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,
>  					    struct drm_bridge_state *state)
>  {
> +	if (state->connector) {
> +		drm_connector_put(state->connector);
> +		state->connector = NULL;
> +	}
> +
>  	kfree(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
>  
>  static int drm_atomic_bridge_check(struct drm_bridge *bridge,
>  				   struct drm_crtc_state *crtc_state,
>  				   struct drm_connector_state *conn_state)
>  {
> +	struct drm_bridge_state *bridge_state;
> +	int ret;
> +
> +	bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> +						       bridge);

It felt like an error to me to call this function for a non-atomic
bridges, until I fully followed the code path to find that it will
return NULL if the bridge isn't registered as a private object.

BTW: if my grep-foo isn't deceiving me, we currently have 34 non-atomic
bridges out of 90. Should we start forcebly updating them to use atomic
interface in attempt to drop the mode_fixup() and other non-atomic
callbacks?

> +	if (WARN_ON(!bridge_state))
> +		return -EINVAL;
> +
> +	bridge_state->crtc = crtc_state->crtc;
> +
> +	drm_connector_get(conn_state->connector);
> +	bridge_state->connector = conn_state->connector;
> +
>  	if (bridge->funcs->atomic_check) {
> -		struct drm_bridge_state *bridge_state;
> -		int ret;
> -
> -		bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> -							       bridge);
> -		if (WARN_ON(!bridge_state))
> -			return -EINVAL;
> -
>  		ret = bridge->funcs->atomic_check(bridge, bridge_state,
>  						  crtc_state, conn_state);
>  		if (ret)
>  			return ret;
>  	} else if (bridge->funcs->mode_fixup) {

-- 
With best wishes
Dmitry

Re: [PATCH 25/29] drm/bridge: Provide pointers to the connector and crtc in bridge state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3853 bytes --]

Hi,

On Thu, Jan 16, 2025 at 03:04:19AM +0200, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 10:05:32PM +0100, Maxime Ripard wrote:
> > Now that connectors are no longer necessarily created by the bridges
> > drivers themselves but might be created by drm_bridge_connector, it's
> > pretty hard for bridge drivers to retrieve pointers to the connector and
> > CRTC they are attached to.
> > 
> > Indeed, the only way to retrieve the CRTC is to follow the drm_bridge
> > encoder field, and then the drm_encoder crtc field, both of them being
> > deprecated.
> > 
> > And for the connector, since we can have multiple connectors attached to
> > a CRTC, we don't really have a reliable way to get it.
> > 
> > Let's provide both pointers in the drm_bridge_state structure so we
> > don't have to follow deprecated, non-atomic, pointers, and be more
> > consistent with the other KMS entities.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_state_helper.c |  5 +++++
> >  drivers/gpu/drm/drm_bridge.c              | 21 +++++++++++++--------
> >  include/drm/drm_atomic.h                  | 14 ++++++++++++++
> >  3 files changed, 32 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 519228eb109533d2596e899a57b571fa0995824f..66661dca077215b78dffca7bc1712f56d35e3918 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -777,10 +777,15 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);
> >   * that don't subclass the bridge state.
> >   */
> >  void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,
> >  					    struct drm_bridge_state *state)
> >  {
> > +	if (state->connector) {
> > +		drm_connector_put(state->connector);
> > +		state->connector = NULL;
> > +	}
> > +
> >  	kfree(state);
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
> >  
> >  static int drm_atomic_bridge_check(struct drm_bridge *bridge,
> >  				   struct drm_crtc_state *crtc_state,
> >  				   struct drm_connector_state *conn_state)
> >  {
> > +	struct drm_bridge_state *bridge_state;
> > +	int ret;
> > +
> > +	bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> > +						       bridge);
> 
> It felt like an error to me to call this function for a non-atomic
> bridges, until I fully followed the code path to find that it will
> return NULL if the bridge isn't registered as a private object.

Yeah.. I wasn't too sure what to do about this one either. I think it
would be more consistent to always have a state properly filled, even if
we have !atomic drivers. It's what happens with the rest of the
framework.

But also, I have no idea what the side-effects might be.

One thing though: a driver having an atomic_check callback is not an
indication of whether it supports atomic mode-setting or not.
atomic_check is optional, so we can have atomic drivers without
atomic_check.

> BTW: if my grep-foo isn't deceiving me, we currently have 34 non-atomic
> bridges out of 90. Should we start forcebly updating them to use atomic
> interface in attempt to drop the mode_fixup() and other non-atomic
> callbacks?

Maybe? I'm not sure forcing anyone to anything really helps. sii8620 for
example is going to be a fun one, and I'd rather stay away from it :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 25/29] drm/bridge: Provide pointers to the connector and crtc in bridge state

[Dmitry Baryshkov]

On Thu, Jan 16, 2025 at 09:42:54AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Jan 16, 2025 at 03:04:19AM +0200, Dmitry Baryshkov wrote:
> > On Wed, Jan 15, 2025 at 10:05:32PM +0100, Maxime Ripard wrote:
> > > Now that connectors are no longer necessarily created by the bridges
> > > drivers themselves but might be created by drm_bridge_connector, it's
> > > pretty hard for bridge drivers to retrieve pointers to the connector and
> > > CRTC they are attached to.
> > > 
> > > Indeed, the only way to retrieve the CRTC is to follow the drm_bridge
> > > encoder field, and then the drm_encoder crtc field, both of them being
> > > deprecated.
> > > 
> > > And for the connector, since we can have multiple connectors attached to
> > > a CRTC, we don't really have a reliable way to get it.
> > > 
> > > Let's provide both pointers in the drm_bridge_state structure so we
> > > don't have to follow deprecated, non-atomic, pointers, and be more
> > > consistent with the other KMS entities.
> > > 
> > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_state_helper.c |  5 +++++
> > >  drivers/gpu/drm/drm_bridge.c              | 21 +++++++++++++--------
> > >  include/drm/drm_atomic.h                  | 14 ++++++++++++++
> > >  3 files changed, 32 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > index 519228eb109533d2596e899a57b571fa0995824f..66661dca077215b78dffca7bc1712f56d35e3918 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -777,10 +777,15 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);
> > >   * that don't subclass the bridge state.
> > >   */
> > >  void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,
> > >  					    struct drm_bridge_state *state)
> > >  {
> > > +	if (state->connector) {
> > > +		drm_connector_put(state->connector);
> > > +		state->connector = NULL;
> > > +	}
> > > +
> > >  	kfree(state);
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
> > >  
> > >  static int drm_atomic_bridge_check(struct drm_bridge *bridge,
> > >  				   struct drm_crtc_state *crtc_state,
> > >  				   struct drm_connector_state *conn_state)
> > >  {
> > > +	struct drm_bridge_state *bridge_state;
> > > +	int ret;
> > > +
> > > +	bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> > > +						       bridge);
> > 
> > It felt like an error to me to call this function for a non-atomic
> > bridges, until I fully followed the code path to find that it will
> > return NULL if the bridge isn't registered as a private object.
> 
> Yeah.. I wasn't too sure what to do about this one either. I think it
> would be more consistent to always have a state properly filled, even if
> we have !atomic drivers. It's what happens with the rest of the
> framework.

Well... Unlike other parts of the framework there is no state for
non-atomic bridges. Of course we can probably fix that by using default
helpers if the callbacks are not provided.

> But also, I have no idea what the side-effects might be.
> 
> One thing though: a driver having an atomic_check callback is not an
> indication of whether it supports atomic mode-setting or not.
> atomic_check is optional, so we can have atomic drivers without
> atomic_check.

Yeah. The framework uses the presence of the .atomic_reset() callback in
order to register the bridge as a private object, maning state
management & co.

-- 
With best wishes
Dmitry

[PATCH 26/29] drm/bridge: cdns-csi: Switch to atomic helpers

[Maxime Ripard]

The Cadence DSI driver follows the drm_encoder->crtc pointer that is
deprecated and shouldn't be used by atomic drivers.

This was due to the fact that we did't have any other alternative to
retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
in the bridge state, so we can move to atomic callbacks and drop that
deprecated pointer usage.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index c7a0247e06adf3e532f830363b3422a8058513ef..aab4959a6f80c3502f717e0881eef30a2c7d1f18 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -653,11 +653,12 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 		return MODE_BAD;
 
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	u32 val;
 
@@ -673,11 +674,12 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
 		dsi->platform_ops->disable(dsi);
 
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 
 	pm_runtime_put(dsi->base.dev);
@@ -750,11 +752,12 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	writel(val, dsi->regs + MCTL_MAIN_EN);
 
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	struct cdns_dsi_output *output = &dsi->output;
 	struct drm_display_mode *mode;
@@ -768,11 +771,11 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 		return;
 
 	if (dsi->platform_ops && dsi->platform_ops->enable)
 		dsi->platform_ops->enable(dsi);
 
-	mode = &bridge->encoder->crtc->state->adjusted_mode;
+	mode = &bridge_state->crtc->state->adjusted_mode;
 	nlanes = output->dev->lanes;
 
 	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
 
 	cdns_dsi_hs_init(dsi);
@@ -890,11 +893,12 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 
 	tmp = readl(dsi->regs + MCTL_MAIN_EN) | IF_EN(input->id);
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
@@ -905,14 +909,14 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
 }
 
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.disable = cdns_dsi_bridge_disable,
-	.pre_enable = cdns_dsi_bridge_pre_enable,
-	.enable = cdns_dsi_bridge_enable,
-	.post_disable = cdns_dsi_bridge_post_disable,
+	.atomic_disable = cdns_dsi_bridge_atomic_disable,
+	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
+	.atomic_enable = cdns_dsi_bridge_atomic_enable,
+	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
 };
 
 static int cdns_dsi_attach(struct mipi_dsi_host *host,
 			   struct mipi_dsi_device *dev)
 {

-- 
2.47.1

Re: [PATCH 26/29] drm/bridge: cdns-csi: Switch to atomic helpers

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 10:05:33PM +0100, Maxime Ripard wrote:
> The Cadence DSI driver follows the drm_encoder->crtc pointer that is
> deprecated and shouldn't be used by atomic drivers.
> 
> This was due to the fact that we did't have any other alternative to
> retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
> in the bridge state, so we can move to atomic callbacks and drop that
> deprecated pointer usage.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index c7a0247e06adf3e532f830363b3422a8058513ef..aab4959a6f80c3502f717e0881eef30a2c7d1f18 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -653,11 +653,12 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  		return MODE_BAD;
>  
>  	return MODE_OK;
>  }
>  
> -static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
> +					   struct drm_bridge_state *bridge_state)
>  {
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
>  	u32 val;
>  
> @@ -673,11 +674,12 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
>  		dsi->platform_ops->disable(dsi);
>  
>  	pm_runtime_put(dsi->base.dev);
>  }
>  
> -static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +						struct drm_bridge_state *bridge_state)
>  {
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
>  
>  	pm_runtime_put(dsi->base.dev);
> @@ -750,11 +752,12 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
>  	writel(val, dsi->regs + MCTL_MAIN_EN);
>  
>  	dsi->link_initialized = true;
>  }
>  
> -static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
> +					  struct drm_bridge_state *bridge_state)
>  {
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
>  	struct cdns_dsi_output *output = &dsi->output;
>  	struct drm_display_mode *mode;
> @@ -768,11 +771,11 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
>  		return;
>  
>  	if (dsi->platform_ops && dsi->platform_ops->enable)
>  		dsi->platform_ops->enable(dsi);
>  
> -	mode = &bridge->encoder->crtc->state->adjusted_mode;
> +	mode = &bridge_state->crtc->state->adjusted_mode;
>  	nlanes = output->dev->lanes;
>  
>  	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
>  
>  	cdns_dsi_hs_init(dsi);
> @@ -890,11 +893,12 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
>  
>  	tmp = readl(dsi->regs + MCTL_MAIN_EN) | IF_EN(input->id);
>  	writel(tmp, dsi->regs + MCTL_MAIN_EN);
>  }
>  
> -static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					      struct drm_bridge_state *bridge_state)
>  {
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
>  
>  	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> @@ -905,14 +909,14 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
>  }
>  
>  static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>  	.attach = cdns_dsi_bridge_attach,
>  	.mode_valid = cdns_dsi_bridge_mode_valid,
> -	.disable = cdns_dsi_bridge_disable,
> -	.pre_enable = cdns_dsi_bridge_pre_enable,
> -	.enable = cdns_dsi_bridge_enable,
> -	.post_disable = cdns_dsi_bridge_post_disable,
> +	.atomic_disable = cdns_dsi_bridge_atomic_disable,
> +	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
> +	.atomic_enable = cdns_dsi_bridge_atomic_enable,
> +	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,

Don't we also need to provide the .atomic_reset /
.atomic_duplicate_state callbacks? Otherwise the bridge isn't registered
as a private object and thus the state will not be managed properly.

>  };
>  
>  static int cdns_dsi_attach(struct mipi_dsi_host *host,
>  			   struct mipi_dsi_device *dev)
>  {
> 
> -- 
> 2.47.1
> 

-- 
With best wishes
Dmitry

[PATCH 27/29] drm/bridge: tc358775: Switch to atomic commit

[Maxime Ripard]

The tc358775 driver follows the drm_encoder->crtc pointer that is
deprecated and shouldn't be used by atomic drivers.

This was due to the fact that we did't have any other alternative to
retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
in the bridge state, so we can move to atomic callbacks and drop that
deprecated pointer usage.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/bridge/tc358775.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
index 0b4efaca6d682320b76ce09ed41824ae7f84ca2d..8f8ed8dc033daf001fc188d919fb38918673bd72 100644
--- a/drivers/gpu/drm/bridge/tc358775.c
+++ b/drivers/gpu/drm/bridge/tc358775.c
@@ -285,11 +285,12 @@ struct tc_data {
 static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
 {
 	return container_of(b, struct tc_data, bridge);
 }
 
-static void tc_bridge_pre_enable(struct drm_bridge *bridge)
+static void tc_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *bridge_state)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	struct device *dev = &tc->dsi->dev;
 	int ret;
 
@@ -308,11 +309,12 @@ static void tc_bridge_pre_enable(struct drm_bridge *bridge)
 
 	gpiod_set_value(tc->reset_gpio, 0);
 	usleep_range(10, 20);
 }
 
-static void tc_bridge_post_disable(struct drm_bridge *bridge)
+static void tc_bridge_atomic_post_disable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *bridge_state)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	struct device *dev = &tc->dsi->dev;
 	int ret;
 
@@ -367,34 +369,22 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
 	if (ret < 0)
 		dev_err(&i2c->dev, "Error %d writing to subaddress 0x%x\n",
 			ret, addr);
 }
 
-/* helper function to access bus_formats */
-static struct drm_connector *get_connector(struct drm_encoder *encoder)
-{
-	struct drm_device *dev = encoder->dev;
-	struct drm_connector *connector;
-
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
-		if (connector->encoder == encoder)
-			return connector;
-
-	return NULL;
-}
-
-static void tc_bridge_enable(struct drm_bridge *bridge)
+static void tc_bridge_atomic_enable(struct drm_bridge *bridge,
+				    struct drm_bridge_state *bridge_state)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	u32 hback_porch, hsync_len, hfront_porch, hactive, htime1, htime2;
 	u32 vback_porch, vsync_len, vfront_porch, vactive, vtime1, vtime2;
 	u32 val = 0;
 	u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
 	struct drm_display_mode *mode;
-	struct drm_connector *connector = get_connector(bridge->encoder);
+	struct drm_connector *connector = bridge_state->connector;
 
-	mode = &bridge->encoder->crtc->state->adjusted_mode;
+	mode = &bridge_state->crtc->state->adjusted_mode;
 
 	hback_porch = mode->htotal - mode->hsync_end;
 	hsync_len  = mode->hsync_end - mode->hsync_start;
 	vback_porch = mode->vtotal - mode->vsync_end;
 	vsync_len  = mode->vsync_end - mode->vsync_start;
@@ -599,14 +589,14 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 				 &tc->bridge, flags);
 }
 
 static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.attach = tc_bridge_attach,
-	.pre_enable = tc_bridge_pre_enable,
-	.enable = tc_bridge_enable,
+	.atomic_pre_enable = tc_bridge_atomic_pre_enable,
+	.atomic_enable = tc_bridge_atomic_enable,
 	.mode_valid = tc_mode_valid,
-	.post_disable = tc_bridge_post_disable,
+	.atomic_post_disable = tc_bridge_atomic_post_disable,
 };
 
 static int tc_attach_host(struct tc_data *tc)
 {
 	struct device *dev = &tc->i2c->dev;

-- 
2.47.1

Re: [PATCH 27/29] drm/bridge: tc358775: Switch to atomic commit

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 10:05:34PM +0100, Maxime Ripard wrote:
> The tc358775 driver follows the drm_encoder->crtc pointer that is
> deprecated and shouldn't be used by atomic drivers.
> 
> This was due to the fact that we did't have any other alternative to
> retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
> in the bridge state, so we can move to atomic callbacks and drop that
> deprecated pointer usage.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 32 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> index 0b4efaca6d682320b76ce09ed41824ae7f84ca2d..8f8ed8dc033daf001fc188d919fb38918673bd72 100644
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -285,11 +285,12 @@ struct tc_data {
>  static inline struct tc_data *bridge_to_tc(struct drm_bridge *b)
>  {
>  	return container_of(b, struct tc_data, bridge);
>  }
>  
> -static void tc_bridge_pre_enable(struct drm_bridge *bridge)
> +static void tc_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					struct drm_bridge_state *bridge_state)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	struct device *dev = &tc->dsi->dev;
>  	int ret;
>  
> @@ -308,11 +309,12 @@ static void tc_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  	gpiod_set_value(tc->reset_gpio, 0);
>  	usleep_range(10, 20);
>  }
>  
> -static void tc_bridge_post_disable(struct drm_bridge *bridge)
> +static void tc_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +					  struct drm_bridge_state *bridge_state)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	struct device *dev = &tc->dsi->dev;
>  	int ret;
>  
> @@ -367,34 +369,22 @@ static void d2l_write(struct i2c_client *i2c, u16 addr, u32 val)
>  	if (ret < 0)
>  		dev_err(&i2c->dev, "Error %d writing to subaddress 0x%x\n",
>  			ret, addr);
>  }
>  
> -/* helper function to access bus_formats */
> -static struct drm_connector *get_connector(struct drm_encoder *encoder)
> -{
> -	struct drm_device *dev = encoder->dev;
> -	struct drm_connector *connector;
> -
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> -		if (connector->encoder == encoder)
> -			return connector;
> -
> -	return NULL;
> -}
> -
> -static void tc_bridge_enable(struct drm_bridge *bridge)
> +static void tc_bridge_atomic_enable(struct drm_bridge *bridge,
> +				    struct drm_bridge_state *bridge_state)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	u32 hback_porch, hsync_len, hfront_porch, hactive, htime1, htime2;
>  	u32 vback_porch, vsync_len, vfront_porch, vactive, vtime1, vtime2;
>  	u32 val = 0;
>  	u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
>  	struct drm_display_mode *mode;
> -	struct drm_connector *connector = get_connector(bridge->encoder);
> +	struct drm_connector *connector = bridge_state->connector;
>  
> -	mode = &bridge->encoder->crtc->state->adjusted_mode;
> +	mode = &bridge_state->crtc->state->adjusted_mode;
>  
>  	hback_porch = mode->htotal - mode->hsync_end;
>  	hsync_len  = mode->hsync_end - mode->hsync_start;
>  	vback_porch = mode->vtotal - mode->vsync_end;
>  	vsync_len  = mode->vsync_end - mode->vsync_start;
> @@ -599,14 +589,14 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  				 &tc->bridge, flags);
>  }
>  
>  static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.attach = tc_bridge_attach,
> -	.pre_enable = tc_bridge_pre_enable,
> -	.enable = tc_bridge_enable,
> +	.atomic_pre_enable = tc_bridge_atomic_pre_enable,
> +	.atomic_enable = tc_bridge_atomic_enable,
>  	.mode_valid = tc_mode_valid,
> -	.post_disable = tc_bridge_post_disable,
> +	.atomic_post_disable = tc_bridge_atomic_post_disable,

Same comment: we have to provide state-management callbacks.

>  };
>  
>  static int tc_attach_host(struct tc_data *tc)
>  {
>  	struct device *dev = &tc->i2c->dev;
> 
> -- 
> 2.47.1
> 

-- 
With best wishes
Dmitry

[PATCH 28/29] drm/bridge: tc358768: Convert to atomic helpers

[Maxime Ripard]

The tc358768 driver follows the drm_encoder->crtc pointer that is
deprecated and shouldn't be used by atomic drivers.

This was due to the fact that we did't have any other alternative to
retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
in the bridge state, so we can move to atomic callbacks and drop that
deprecated pointer usage.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/bridge/tc358768.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index ec79b0dd0e2cd6dfbdd8aee54d6a8d9cf8bfb060..043ecfa5f9bac49c30a2fe9f708b5fba9d235bdc 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -600,17 +600,29 @@ static void tc358768_bridge_disable(struct drm_bridge *bridge)
 	ret = tc358768_clear_error(priv);
 	if (ret)
 		dev_warn(priv->dev, "Software disable failed: %d\n", ret);
 }
 
+static void tc358768_bridge_atomic_disable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *bridge_state)
+{
+	tc358768_bridge_disable(bridge);
+}
+
 static void tc358768_bridge_post_disable(struct drm_bridge *bridge)
 {
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
 
 	tc358768_hw_disable(priv);
 }
 
+static void tc358768_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *bridge_state)
+{
+	tc358768_bridge_post_disable(bridge);
+}
+
 static int tc358768_setup_pll(struct tc358768_priv *priv,
 			      const struct drm_display_mode *mode)
 {
 	u32 fbd, prd, frs;
 	int ret;
@@ -680,11 +692,12 @@ static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val)
 	u64 n = priv->dsiclk / 4 * priv->dsi_lanes;
 
 	return (u32)div_u64(m, n);
 }
 
-static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
+static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *bridge_state)
 {
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
 	struct mipi_dsi_device *dsi_dev = priv->output.dev;
 	unsigned long mode_flags = dsi_dev->mode_flags;
 	u32 val, val2, lptxcnt, hact, data_type;
@@ -717,11 +730,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		dev_err(dev, "Software reset failed: %d\n", ret);
 		tc358768_hw_disable(priv);
 		return;
 	}
 
-	mode = &bridge->encoder->crtc->state->adjusted_mode;
+	mode = &bridge_state->crtc->state->adjusted_mode;
 	ret = tc358768_setup_pll(priv, mode);
 	if (ret) {
 		dev_err(dev, "PLL setup failed: %d\n", ret);
 		tc358768_hw_disable(priv);
 		return;
@@ -1081,11 +1094,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		tc358768_bridge_disable(bridge);
 		tc358768_bridge_post_disable(bridge);
 	}
 }
 
-static void tc358768_bridge_enable(struct drm_bridge *bridge)
+static void tc358768_bridge_atomic_enable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *bridge_state)
 {
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
 	int ret;
 
 	if (!priv->enabled) {
@@ -1164,14 +1178,14 @@ static bool tc358768_mode_fixup(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs tc358768_bridge_funcs = {
 	.attach = tc358768_bridge_attach,
 	.mode_valid = tc358768_bridge_mode_valid,
 	.mode_fixup = tc358768_mode_fixup,
-	.pre_enable = tc358768_bridge_pre_enable,
-	.enable = tc358768_bridge_enable,
-	.disable = tc358768_bridge_disable,
-	.post_disable = tc358768_bridge_post_disable,
+	.atomic_pre_enable = tc358768_bridge_atomic_pre_enable,
+	.atomic_enable = tc358768_bridge_atomic_enable,
+	.atomic_disable = tc358768_bridge_atomic_disable,
+	.atomic_post_disable = tc358768_bridge_atomic_post_disable,
 
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.atomic_get_input_bus_fmts = tc358768_atomic_get_input_bus_fmts,

-- 
2.47.1

[PATCH 29/29] drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

[Maxime Ripard]

The TI sn65dsi86 driver follows the drm_encoder->crtc pointer that is
deprecated and shouldn't be used by atomic drivers.

This was due to the fact that we did't have any other alternative to
retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
in the bridge state, so we can move to atomic callbacks and drop that
deprecated pointer usage.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 41 ++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index b3d617505dda7d22b38c000fb79de46376adf3f1..c17d9486cf5c36d61eb00af2bdf9ba1b6f890ffd 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -242,15 +242,16 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
 	u8 buf[2] = { val & 0xff, val >> 8 };
 
 	regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf));
 }
 
-static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
+static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata,
+				     struct drm_bridge_state *bridge_state)
 {
 	u32 bit_rate_khz, clk_freq_khz;
 	struct drm_display_mode *mode =
-		&pdata->bridge.encoder->crtc->state->adjusted_mode;
+		&bridge_state->crtc->state->adjusted_mode;
 
 	bit_rate_khz = mode->clock *
 			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
 	clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
 
@@ -273,11 +274,12 @@ static const u32 ti_sn_bridge_dsiclk_lut[] = {
 	416000000,
 	486000000,
 	460800000,
 };
 
-static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
+static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata,
+					 struct drm_bridge_state *bridge_state)
 {
 	int i;
 	u32 refclk_rate;
 	const u32 *refclk_lut;
 	size_t refclk_lut_size;
@@ -286,11 +288,11 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
 		refclk_rate = clk_get_rate(pdata->refclk);
 		refclk_lut = ti_sn_bridge_refclk_lut;
 		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
 		clk_prepare_enable(pdata->refclk);
 	} else {
-		refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
+		refclk_rate = ti_sn_bridge_get_dsi_freq(pdata, bridge_state) * 1000;
 		refclk_lut = ti_sn_bridge_dsiclk_lut;
 		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
 	}
 
 	/* for i equals to refclk_lut_size means default frequency */
@@ -310,16 +312,17 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata)
 	 * regardless of its actual sourcing.
 	 */
 	pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i];
 }
 
-static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata)
+static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata,
+				      struct drm_bridge_state *bridge_state)
 {
 	mutex_lock(&pdata->comms_mutex);
 
 	/* configure bridge ref_clk */
-	ti_sn_bridge_set_refclk_freq(pdata);
+	ti_sn_bridge_set_refclk_freq(pdata, bridge_state);
 
 	/*
 	 * HPD on this bridge chip is a bit useless.  This is an eDP bridge
 	 * so the HPD is an internal signal that's only there to signal that
 	 * the panel is done powering up.  ...but the bridge chip debounces
@@ -375,11 +378,11 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
 	 * so we can do it in resume which lets us read the EDID before
 	 * pre_enable(). Without a reference clock we need the MIPI reference
 	 * clock so reading early doesn't work.
 	 */
 	if (pdata->refclk)
-		ti_sn65dsi86_enable_comms(pdata);
+		ti_sn65dsi86_enable_comms(pdata, drm_bridge_get_current_state(&pdata->bridge));
 
 	return ret;
 }
 
 static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev)
@@ -820,16 +823,17 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
 
 	/* disable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
 }
 
-static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
+static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata,
+				      struct drm_bridge_state *bridge_state)
 {
 	unsigned int bit_rate_mhz, clk_freq_mhz;
 	unsigned int val;
 	struct drm_display_mode *mode =
-		&pdata->bridge.encoder->crtc->state->adjusted_mode;
+		&bridge_state->crtc->state->adjusted_mode;
 
 	/* set DSIA clk frequency */
 	bit_rate_mhz = (mode->clock / 1000) *
 			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
 	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
@@ -855,16 +859,18 @@ static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
  */
 static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
 	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
 };
 
-static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
+static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata,
+					     struct drm_bridge_state *bridge_state,
+					     unsigned int bpp)
 {
 	unsigned int bit_rate_khz, dp_rate_mhz;
 	unsigned int i;
 	struct drm_display_mode *mode =
-		&pdata->bridge.encoder->crtc->state->adjusted_mode;
+		&bridge_state->crtc->state->adjusted_mode;
 
 	/* Calculate minimum bit rate based on our pixel clock. */
 	bit_rate_khz = mode->clock * bpp;
 
 	/* Calculate minimum DP data rate, taking 80% as per DP spec */
@@ -959,14 +965,15 @@ static unsigned int ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata)
 	}
 
 	return valid_rates;
 }
 
-static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata)
+static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata,
+					   struct drm_bridge_state *bridge_state)
 {
 	struct drm_display_mode *mode =
-		&pdata->bridge.encoder->crtc->state->adjusted_mode;
+		&bridge_state->crtc->state->adjusted_mode;
 	u8 hsync_polarity = 0, vsync_polarity = 0;
 
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
 		hsync_polarity = CHA_HSYNC_POLARITY;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
@@ -1104,11 +1111,11 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 	regmap_write(pdata->regmap, SN_LN_ASSIGN_REG, pdata->ln_assign);
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, LN_POLRS_MASK,
 			   pdata->ln_polrs << LN_POLRS_OFFSET);
 
 	/* set dsi clk frequency value */
-	ti_sn_bridge_set_dsi_rate(pdata);
+	ti_sn_bridge_set_dsi_rate(pdata, old_bridge_state);
 
 	/*
 	 * The SN65DSI86 only supports ASSR Display Authentication method and
 	 * this method is enabled for eDP panels. An eDP panel must support this
 	 * authentication method. We need to enable this method in the eDP panel
@@ -1139,11 +1146,11 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 			   val);
 
 	valid_rates = ti_sn_bridge_read_valid_rates(pdata);
 
 	/* Train until we run out of rates */
-	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
+	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, old_bridge_state, bpp);
 	     dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
 	     dp_rate_idx++) {
 		if (!(valid_rates & BIT(dp_rate_idx)))
 			continue;
 
@@ -1155,11 +1162,11 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 		DRM_DEV_ERROR(pdata->dev, "%s (%d)\n", last_err_str, ret);
 		return;
 	}
 
 	/* config video parameters */
-	ti_sn_bridge_set_video_timings(pdata);
+	ti_sn_bridge_set_video_timings(pdata, old_bridge_state);
 
 	/* enable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE,
 			   VSTREAM_ENABLE);
 }
@@ -1170,11 +1177,11 @@ static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
 	pm_runtime_get_sync(pdata->dev);
 
 	if (!pdata->refclk)
-		ti_sn65dsi86_enable_comms(pdata);
+		ti_sn65dsi86_enable_comms(pdata, old_bridge_state);
 
 	/* td7: min 100 us after enable before DSI data */
 	usleep_range(100, 110);
 }
 

-- 
2.47.1

Re: [PATCH 29/29] drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

[Dmitry Baryshkov]

On Wed, Jan 15, 2025 at 10:05:36PM +0100, Maxime Ripard wrote:
> The TI sn65dsi86 driver follows the drm_encoder->crtc pointer that is
> deprecated and shouldn't be used by atomic drivers.
> 
> This was due to the fact that we did't have any other alternative to
> retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
> in the bridge state, so we can move to atomic callbacks and drop that
> deprecated pointer usage.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 41 ++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index b3d617505dda7d22b38c000fb79de46376adf3f1..c17d9486cf5c36d61eb00af2bdf9ba1b6f890ffd 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -242,15 +242,16 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
>  	u8 buf[2] = { val & 0xff, val >> 8 };
>  
>  	regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf));
>  }
>  
> -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
> +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata,
> +				     struct drm_bridge_state *bridge_state)
>  {
>  	u32 bit_rate_khz, clk_freq_khz;
>  	struct drm_display_mode *mode =
> -		&pdata->bridge.encoder->crtc->state->adjusted_mode;
> +		&bridge_state->crtc->state->adjusted_mode;

At least we should document why is it safe to follow the crtc->state.

-- 
With best wishes
Dmitry

Re: [PATCH 29/29] drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

Hi Dmitry,

On Thu, Jan 16, 2025 at 03:08:00AM +0200, Dmitry Baryshkov wrote:
> On Wed, Jan 15, 2025 at 10:05:36PM +0100, Maxime Ripard wrote:
> > The TI sn65dsi86 driver follows the drm_encoder->crtc pointer that is
> > deprecated and shouldn't be used by atomic drivers.
> > 
> > This was due to the fact that we did't have any other alternative to
> > retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
> > in the bridge state, so we can move to atomic callbacks and drop that
> > deprecated pointer usage.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 41 ++++++++++++++++++++---------------
> >  1 file changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index b3d617505dda7d22b38c000fb79de46376adf3f1..c17d9486cf5c36d61eb00af2bdf9ba1b6f890ffd 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -242,15 +242,16 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> >  	u8 buf[2] = { val & 0xff, val >> 8 };
> >  
> >  	regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf));
> >  }
> >  
> > -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
> > +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata,
> > +				     struct drm_bridge_state *bridge_state)
> >  {
> >  	u32 bit_rate_khz, clk_freq_khz;
> >  	struct drm_display_mode *mode =
> > -		&pdata->bridge.encoder->crtc->state->adjusted_mode;
> > +		&bridge_state->crtc->state->adjusted_mode;
> 
> At least we should document why is it safe to follow the crtc->state.

What do you have in mind there? crtc->state is a pointer that is widely
used, what is there to document?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 29/29] drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

[Dmitry Baryshkov]

On Mon, Feb 03, 2025 at 11:01:28AM +0100, Maxime Ripard wrote:
> Hi Dmitry,
> 
> On Thu, Jan 16, 2025 at 03:08:00AM +0200, Dmitry Baryshkov wrote:
> > On Wed, Jan 15, 2025 at 10:05:36PM +0100, Maxime Ripard wrote:
> > > The TI sn65dsi86 driver follows the drm_encoder->crtc pointer that is
> > > deprecated and shouldn't be used by atomic drivers.
> > > 
> > > This was due to the fact that we did't have any other alternative to
> > > retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
> > > in the bridge state, so we can move to atomic callbacks and drop that
> > > deprecated pointer usage.
> > > 
> > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 41 ++++++++++++++++++++---------------
> > >  1 file changed, 24 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index b3d617505dda7d22b38c000fb79de46376adf3f1..c17d9486cf5c36d61eb00af2bdf9ba1b6f890ffd 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -242,15 +242,16 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> > >  	u8 buf[2] = { val & 0xff, val >> 8 };
> > >  
> > >  	regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf));
> > >  }
> > >  
> > > -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
> > > +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata,
> > > +				     struct drm_bridge_state *bridge_state)
> > >  {
> > >  	u32 bit_rate_khz, clk_freq_khz;
> > >  	struct drm_display_mode *mode =
> > > -		&pdata->bridge.encoder->crtc->state->adjusted_mode;
> > > +		&bridge_state->crtc->state->adjusted_mode;
> > 
> > At least we should document why is it safe to follow the crtc->state.
> 
> What do you have in mind there? crtc->state is a pointer that is widely
> used, what is there to document?

If I understand correctly, crtc->state is safe to be used during atomic
callbacks only or if the mutex is being held. However this function is
also being called from ti_sn65dsi86_enable_comms(), which is in turn is
used in ti_sn65dsi86_resume(). Is it safe? Why?

-- 
With best wishes
Dmitry

Re: [PATCH 29/29] drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

On Mon, Feb 03, 2025 at 07:49:05PM +0200, Dmitry Baryshkov wrote:
> On Mon, Feb 03, 2025 at 11:01:28AM +0100, Maxime Ripard wrote:
> > Hi Dmitry,
> > 
> > On Thu, Jan 16, 2025 at 03:08:00AM +0200, Dmitry Baryshkov wrote:
> > > On Wed, Jan 15, 2025 at 10:05:36PM +0100, Maxime Ripard wrote:
> > > > The TI sn65dsi86 driver follows the drm_encoder->crtc pointer that is
> > > > deprecated and shouldn't be used by atomic drivers.
> > > > 
> > > > This was due to the fact that we did't have any other alternative to
> > > > retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
> > > > in the bridge state, so we can move to atomic callbacks and drop that
> > > > deprecated pointer usage.
> > > > 
> > > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > > ---
> > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 41 ++++++++++++++++++++---------------
> > > >  1 file changed, 24 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > index b3d617505dda7d22b38c000fb79de46376adf3f1..c17d9486cf5c36d61eb00af2bdf9ba1b6f890ffd 100644
> > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > @@ -242,15 +242,16 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata,
> > > >  	u8 buf[2] = { val & 0xff, val >> 8 };
> > > >  
> > > >  	regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf));
> > > >  }
> > > >  
> > > > -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata)
> > > > +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata,
> > > > +				     struct drm_bridge_state *bridge_state)
> > > >  {
> > > >  	u32 bit_rate_khz, clk_freq_khz;
> > > >  	struct drm_display_mode *mode =
> > > > -		&pdata->bridge.encoder->crtc->state->adjusted_mode;
> > > > +		&bridge_state->crtc->state->adjusted_mode;
> > > 
> > > At least we should document why is it safe to follow the crtc->state.
> > 
> > What do you have in mind there? crtc->state is a pointer that is widely
> > used, what is there to document?
> 
> If I understand correctly, crtc->state is safe to be used during atomic
> callbacks only or if the mutex is being held. However this function is
> also being called from ti_sn65dsi86_enable_comms(), which is in turn is
> used in ti_sn65dsi86_resume(). Is it safe? Why?

It's not safe, and it wasn't before this series. I'll send a patch
trying to fix it. However, I can't test it and fixing individual locking
issues isn't really the point of this series either, so I might as well
drop it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]