diff mbox series

[v4,26/30] drm/i915: Factor out function to clear pipe update flags

Message ID 20231030155843.2251023-27-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Improve BW management on MST links | expand

Commit Message

Imre Deak Oct. 30, 2023, 3:58 p.m. UTC 
Factor out a helper to clear the pipe update flags, used by a follow-up
patch to modeset an MST topology.

v2:
- Move the intel_crtc_needs_modeset() check to the callers. (Ville)

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> (v1)
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 52 ++++++++++----------
 1 file changed, 27 insertions(+), 25 deletions(-)

Comments

Ville Syrjälä Nov. 1, 2023, 10:17 a.m. UTC | #1 
On Mon, Oct 30, 2023 at 05:58:39PM +0200, Imre Deak wrote:
> Factor out a helper to clear the pipe update flags, used by a follow-up
> patch to modeset an MST topology.
> 
> v2:
> - Move the intel_crtc_needs_modeset() check to the callers. (Ville)
> 
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> (v1)
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 52 ++++++++++----------
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 1133c2893bb38..d27afda0cfaa6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5551,6 +5551,14 @@ int intel_modeset_pipes_in_mask_early(struct intel_atomic_state *state,
>  	return 0;
>  }
>  
> +static void
> +clear_pipe_update_flags_on_modeset_crtc(struct intel_crtc_state *crtc_state)

The 'on_modeset_crtc()' part doesn't make sense anymore.

Hmm. Another idea would be to move the mode_changed=true assignment
here and call this something like intel_crtc_flag_fmodeset(), which
might be a bit more descriptive.

> +{
> +	crtc_state->update_pipe = false;
> +	crtc_state->update_m_n = false;
> +	crtc_state->update_lrr = false;
> +}
> +
>  /**
>   * intel_modeset_all_pipes_late - force a full modeset on all pipes
>   * @state: intel atomic state
> @@ -5584,9 +5592,8 @@ int intel_modeset_all_pipes_late(struct intel_atomic_state *state,
>  		if (ret)
>  			return ret;
>  
> -		crtc_state->update_pipe = false;
> -		crtc_state->update_m_n = false;
> -		crtc_state->update_lrr = false;
> +		clear_pipe_update_flags_on_modeset_crtc(crtc_state);
> +
>  		crtc_state->update_planes |= crtc_state->active_planes;
>  		crtc_state->async_flip_planes = 0;
>  		crtc_state->do_async_flip = false;
> @@ -5699,13 +5706,14 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  	else
>  		new_crtc_state->uapi.mode_changed = false;
>  
> -	if (intel_crtc_needs_modeset(new_crtc_state) ||
> -	    intel_compare_link_m_n(&old_crtc_state->dp_m_n,
> +	if (intel_crtc_needs_modeset(new_crtc_state))
> +		clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
> +
> +	if (intel_compare_link_m_n(&old_crtc_state->dp_m_n,
>  				   &new_crtc_state->dp_m_n))
>  		new_crtc_state->update_m_n = false;
>  
> -	if (intel_crtc_needs_modeset(new_crtc_state) ||
> -	    (old_crtc_state->hw.adjusted_mode.crtc_vtotal == new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
> +	if ((old_crtc_state->hw.adjusted_mode.crtc_vtotal == new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
>  	     old_crtc_state->hw.adjusted_mode.crtc_vblank_end == new_crtc_state->hw.adjusted_mode.crtc_vblank_end))
>  		new_crtc_state->update_lrr = false;
>  
> @@ -6484,12 +6492,9 @@ int intel_atomic_check(struct drm_device *dev,
>  		if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
>  			enum transcoder master = new_crtc_state->mst_master_transcoder;
>  
> -			if (intel_cpu_transcoders_need_modeset(state, BIT(master))) {
> -				new_crtc_state->uapi.mode_changed = true;
> -				new_crtc_state->update_pipe = false;
> -				new_crtc_state->update_m_n = false;
> -				new_crtc_state->update_lrr = false;
> -			}
> +			if (intel_cpu_transcoders_need_modeset(state, BIT(master)))
> +				intel_modeset_pipes_in_mask_early(state, "MST master transcoder",
> +								  BIT(crtc->pipe));

These changes don't really belong in this refactoring patch.

>  		}
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> @@ -6498,22 +6503,19 @@ int intel_atomic_check(struct drm_device *dev,
>  			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
>  				trans |= BIT(new_crtc_state->master_transcoder);
>  
> -			if (intel_cpu_transcoders_need_modeset(state, trans)) {
> -				new_crtc_state->uapi.mode_changed = true;
> -				new_crtc_state->update_pipe = false;
> -				new_crtc_state->update_m_n = false;
> -				new_crtc_state->update_lrr = false;
> -			}
> +			if (intel_cpu_transcoders_need_modeset(state, trans))
> +				intel_modeset_pipes_in_mask_early(state, "port sync",
> +								  BIT(crtc->pipe));
>  		}
>  
>  		if (new_crtc_state->bigjoiner_pipes) {
> -			if (intel_pipes_need_modeset(state, new_crtc_state->bigjoiner_pipes)) {
> -				new_crtc_state->uapi.mode_changed = true;
> -				new_crtc_state->update_pipe = false;
> -				new_crtc_state->update_m_n = false;
> -				new_crtc_state->update_lrr = false;
> -			}
> +			if (intel_pipes_need_modeset(state, new_crtc_state->bigjoiner_pipes))
> +				intel_modeset_pipes_in_mask_early(state, "bigjoiner pipes",
> +								  BIT(crtc->pipe));
>  		}
> +
> +		if (intel_crtc_needs_modeset(new_crtc_state))
> +			clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
>  	}
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> -- 
> 2.39.2
Imre Deak Nov. 1, 2023, 11:38 a.m. UTC | #2 
On Wed, Nov 01, 2023 at 12:17:30PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 30, 2023 at 05:58:39PM +0200, Imre Deak wrote:
> > Factor out a helper to clear the pipe update flags, used by a follow-up
> > patch to modeset an MST topology.
> > 
> > v2:
> > - Move the intel_crtc_needs_modeset() check to the callers. (Ville)
> > 
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> (v1)
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 52 ++++++++++----------
> >  1 file changed, 27 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1133c2893bb38..d27afda0cfaa6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5551,6 +5551,14 @@ int intel_modeset_pipes_in_mask_early(struct intel_atomic_state *state,
> >  	return 0;
> >  }
> >  
> > +static void
> > +clear_pipe_update_flags_on_modeset_crtc(struct intel_crtc_state *crtc_state)
> 
> The 'on_modeset_crtc()' part doesn't make sense anymore.
>
> Hmm. Another idea would be to move the mode_changed=true assignment
> here and call this something like intel_crtc_flag_fmodeset(), which
> might be a bit more descriptive.

Ok makes sense, will do the above.

In intel_crtc_check_fastset() this will set mode_changed=true where it
wasn't before, but that looks ok.

> > +{
> > +	crtc_state->update_pipe = false;
> > +	crtc_state->update_m_n = false;
> > +	crtc_state->update_lrr = false;
> > +}
> > +
> >  /**
> >   * intel_modeset_all_pipes_late - force a full modeset on all pipes
> >   * @state: intel atomic state
> > @@ -5584,9 +5592,8 @@ int intel_modeset_all_pipes_late(struct intel_atomic_state *state,
> >  		if (ret)
> >  			return ret;
> >  
> > -		crtc_state->update_pipe = false;
> > -		crtc_state->update_m_n = false;
> > -		crtc_state->update_lrr = false;
> > +		clear_pipe_update_flags_on_modeset_crtc(crtc_state);
> > +
> >  		crtc_state->update_planes |= crtc_state->active_planes;
> >  		crtc_state->async_flip_planes = 0;
> >  		crtc_state->do_async_flip = false;
> > @@ -5699,13 +5706,14 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
> >  	else
> >  		new_crtc_state->uapi.mode_changed = false;
> >  
> > -	if (intel_crtc_needs_modeset(new_crtc_state) ||
> > -	    intel_compare_link_m_n(&old_crtc_state->dp_m_n,
> > +	if (intel_crtc_needs_modeset(new_crtc_state))
> > +		clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
> > +
> > +	if (intel_compare_link_m_n(&old_crtc_state->dp_m_n,
> >  				   &new_crtc_state->dp_m_n))
> >  		new_crtc_state->update_m_n = false;
> >  
> > -	if (intel_crtc_needs_modeset(new_crtc_state) ||
> > -	    (old_crtc_state->hw.adjusted_mode.crtc_vtotal == new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
> > +	if ((old_crtc_state->hw.adjusted_mode.crtc_vtotal == new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
> >  	     old_crtc_state->hw.adjusted_mode.crtc_vblank_end == new_crtc_state->hw.adjusted_mode.crtc_vblank_end))
> >  		new_crtc_state->update_lrr = false;
> >  
> > @@ -6484,12 +6492,9 @@ int intel_atomic_check(struct drm_device *dev,
> >  		if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
> >  			enum transcoder master = new_crtc_state->mst_master_transcoder;
> >  
> > -			if (intel_cpu_transcoders_need_modeset(state, BIT(master))) {
> > -				new_crtc_state->uapi.mode_changed = true;
> > -				new_crtc_state->update_pipe = false;
> > -				new_crtc_state->update_m_n = false;
> > -				new_crtc_state->update_lrr = false;
> > -			}
> > +			if (intel_cpu_transcoders_need_modeset(state, BIT(master)))
> > +				intel_modeset_pipes_in_mask_early(state, "MST master transcoder",
> > +								  BIT(crtc->pipe));
> 
> These changes don't really belong in this refactoring patch.

This had the benefit to show the reason for the modeset, but yes,
should've been a separate change. I'll remove it.

> >  		}
> >  
> >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > @@ -6498,22 +6503,19 @@ int intel_atomic_check(struct drm_device *dev,
> >  			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
> >  				trans |= BIT(new_crtc_state->master_transcoder);
> >  
> > -			if (intel_cpu_transcoders_need_modeset(state, trans)) {
> > -				new_crtc_state->uapi.mode_changed = true;
> > -				new_crtc_state->update_pipe = false;
> > -				new_crtc_state->update_m_n = false;
> > -				new_crtc_state->update_lrr = false;
> > -			}
> > +			if (intel_cpu_transcoders_need_modeset(state, trans))
> > +				intel_modeset_pipes_in_mask_early(state, "port sync",
> > +								  BIT(crtc->pipe));
> >  		}
> >  
> >  		if (new_crtc_state->bigjoiner_pipes) {
> > -			if (intel_pipes_need_modeset(state, new_crtc_state->bigjoiner_pipes)) {
> > -				new_crtc_state->uapi.mode_changed = true;
> > -				new_crtc_state->update_pipe = false;
> > -				new_crtc_state->update_m_n = false;
> > -				new_crtc_state->update_lrr = false;
> > -			}
> > +			if (intel_pipes_need_modeset(state, new_crtc_state->bigjoiner_pipes))
> > +				intel_modeset_pipes_in_mask_early(state, "bigjoiner pipes",
> > +								  BIT(crtc->pipe));
> >  		}
> > +
> > +		if (intel_crtc_needs_modeset(new_crtc_state))
> > +			clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
> >  	}
> >  
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > -- 
> > 2.39.2
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 1133c2893bb38..d27afda0cfaa6 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5551,6 +5551,14 @@  int intel_modeset_pipes_in_mask_early(struct intel_atomic_state *state,
 	return 0;
 }
 
+static void
+clear_pipe_update_flags_on_modeset_crtc(struct intel_crtc_state *crtc_state)
+{
+	crtc_state->update_pipe = false;
+	crtc_state->update_m_n = false;
+	crtc_state->update_lrr = false;
+}
+
 /**
  * intel_modeset_all_pipes_late - force a full modeset on all pipes
  * @state: intel atomic state
@@ -5584,9 +5592,8 @@  int intel_modeset_all_pipes_late(struct intel_atomic_state *state,
 		if (ret)
 			return ret;
 
-		crtc_state->update_pipe = false;
-		crtc_state->update_m_n = false;
-		crtc_state->update_lrr = false;
+		clear_pipe_update_flags_on_modeset_crtc(crtc_state);
+
 		crtc_state->update_planes |= crtc_state->active_planes;
 		crtc_state->async_flip_planes = 0;
 		crtc_state->do_async_flip = false;
@@ -5699,13 +5706,14 @@  static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
 	else
 		new_crtc_state->uapi.mode_changed = false;
 
-	if (intel_crtc_needs_modeset(new_crtc_state) ||
-	    intel_compare_link_m_n(&old_crtc_state->dp_m_n,
+	if (intel_crtc_needs_modeset(new_crtc_state))
+		clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
+
+	if (intel_compare_link_m_n(&old_crtc_state->dp_m_n,
 				   &new_crtc_state->dp_m_n))
 		new_crtc_state->update_m_n = false;
 
-	if (intel_crtc_needs_modeset(new_crtc_state) ||
-	    (old_crtc_state->hw.adjusted_mode.crtc_vtotal == new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
+	if ((old_crtc_state->hw.adjusted_mode.crtc_vtotal == new_crtc_state->hw.adjusted_mode.crtc_vtotal &&
 	     old_crtc_state->hw.adjusted_mode.crtc_vblank_end == new_crtc_state->hw.adjusted_mode.crtc_vblank_end))
 		new_crtc_state->update_lrr = false;
 
@@ -6484,12 +6492,9 @@  int intel_atomic_check(struct drm_device *dev,
 		if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
 			enum transcoder master = new_crtc_state->mst_master_transcoder;
 
-			if (intel_cpu_transcoders_need_modeset(state, BIT(master))) {
-				new_crtc_state->uapi.mode_changed = true;
-				new_crtc_state->update_pipe = false;
-				new_crtc_state->update_m_n = false;
-				new_crtc_state->update_lrr = false;
-			}
+			if (intel_cpu_transcoders_need_modeset(state, BIT(master)))
+				intel_modeset_pipes_in_mask_early(state, "MST master transcoder",
+								  BIT(crtc->pipe));
 		}
 
 		if (is_trans_port_sync_mode(new_crtc_state)) {
@@ -6498,22 +6503,19 @@  int intel_atomic_check(struct drm_device *dev,
 			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
 				trans |= BIT(new_crtc_state->master_transcoder);
 
-			if (intel_cpu_transcoders_need_modeset(state, trans)) {
-				new_crtc_state->uapi.mode_changed = true;
-				new_crtc_state->update_pipe = false;
-				new_crtc_state->update_m_n = false;
-				new_crtc_state->update_lrr = false;
-			}
+			if (intel_cpu_transcoders_need_modeset(state, trans))
+				intel_modeset_pipes_in_mask_early(state, "port sync",
+								  BIT(crtc->pipe));
 		}
 
 		if (new_crtc_state->bigjoiner_pipes) {
-			if (intel_pipes_need_modeset(state, new_crtc_state->bigjoiner_pipes)) {
-				new_crtc_state->uapi.mode_changed = true;
-				new_crtc_state->update_pipe = false;
-				new_crtc_state->update_m_n = false;
-				new_crtc_state->update_lrr = false;
-			}
+			if (intel_pipes_need_modeset(state, new_crtc_state->bigjoiner_pipes))
+				intel_modeset_pipes_in_mask_early(state, "bigjoiner pipes",
+								  BIT(crtc->pipe));
 		}
+
+		if (intel_crtc_needs_modeset(new_crtc_state))
+			clear_pipe_update_flags_on_modeset_crtc(new_crtc_state);
 	}
 
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,