[1/5] drm/i915: Fix post-fastset modeset check for port sync
diff mbox series

Message ID 20200115190813.17971-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • [1/5] drm/i915: Fix post-fastset modeset check for port sync
Related show

Commit Message

Ville Syrjälä Jan. 15, 2020, 7:08 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The post-fastset "does anyone still need a full modeset?" for
port sync looks busted. The outer loop bails out of a full modeset
is still needed by the current crtc which, and then we skip forcing
a full modeset on the related crtcs. That's totally the opposite
of what we want.

The MST path has the logic mostly the other way around so it
looks correct. To fix the port sync case let's follow the MST
logic for both. So, if the current crtc already needs a modeset
we do nothing. otherwise we check if any of the related crtcs
needs a modeset, and if so we force a full modeset for the
current crtc.

And while at let's change the else if to a plain if to so
we don't have needless coupling between the MST and port sync
checks.

Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency loop for port sync")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++------------
 1 file changed, 17 insertions(+), 26 deletions(-)

Comments

José Roberto de Souza Jan. 16, 2020, 12:05 a.m. UTC | #1
On Wed, 2020-01-15 at 21:08 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The post-fastset "does anyone still need a full modeset?" for
> port sync looks busted. The outer loop bails out of a full modeset
> is still needed by the current crtc which, and then we skip forcing
> a full modeset on the related crtcs. That's totally the opposite
> of what we want.

Ops yeah, it is always doing a full modeset.

> 
> The MST path has the logic mostly the other way around so it
> looks correct. To fix the port sync case let's follow the MST
> logic for both. So, if the current crtc already needs a modeset
> we do nothing. otherwise we check if any of the related crtcs
> needs a modeset, and if so we force a full modeset for the
> current crtc.
> 
> And while at let's change the else if to a plain if to so
> we don't have needless coupling between the MST and port sync
> checks.
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency loop
> for port sync")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++--------
> ----
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index dd03987cc24f..b397816ce253 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14469,37 +14469,23 @@ static int intel_atomic_check_crtcs(struct
> intel_atomic_state *state)
>  	return 0;
>  }
>  
> -static bool intel_cpu_transcoder_needs_modeset(struct
> intel_atomic_state *state,
> -					       enum transcoder
> transcoder)
> +static bool intel_cpu_transcoders_need_modeset(struct
> intel_atomic_state *state,
> +					       u8 transcoders)
>  {
> -	struct intel_crtc_state *new_crtc_state;
> +	const struct intel_crtc_state *new_crtc_state;
>  	struct intel_crtc *crtc;
>  	int i;
>  
> -	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> i)
> -		if (new_crtc_state->cpu_transcoder == transcoder)
> -			return needs_modeset(new_crtc_state);
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> i) {
> +		if (new_crtc_state->hw.enable &&

The intel_pipe_config_compare() will force a modeset for MST and port
sync but this hw.enable could skip modeset in future features when one
of the pipes are going from enabled to disabled.

Removing it:

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> +		    transcoders & BIT(new_crtc_state->cpu_transcoder)
> &&
> +		    needs_modeset(new_crtc_state))
> +			return true;
> +	}
>  
>  	return false;
>  }
>  
> -static void
> -intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> -			   u8 transcoders)
> -{
> -	struct intel_crtc_state *new_crtc_state;
> -	struct intel_crtc *crtc;
> -	int i;
> -
> -	for_each_new_intel_crtc_in_state(state, crtc,
> -					 new_crtc_state, i) {
> -		if (transcoders & BIT(new_crtc_state->cpu_transcoder))
> {
> -			new_crtc_state->uapi.mode_changed = true;
> -			new_crtc_state->update_pipe = false;
> -		}
> -	}
> -}
> -
>  static int
>  intel_modeset_all_tiles(struct intel_atomic_state *state, int
> tile_grp_id)
>  {
> @@ -14655,15 +14641,20 @@ static 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_transcoder_needs_modeset(state,
> master)) {
> +			if (intel_cpu_transcoders_need_modeset(state,
> BIT(master))) {
>  				new_crtc_state->uapi.mode_changed =
> true;
>  				new_crtc_state->update_pipe = false;
>  			}
> -		} else if (is_trans_port_sync_mode(new_crtc_state)) {
> +		}
> +
> +		if (is_trans_port_sync_mode(new_crtc_state)) {
>  			u8 trans = new_crtc_state-
> >sync_mode_slaves_mask |
>  				   BIT(new_crtc_state-
> >master_transcoder);
>  
> -			intel_modeset_synced_crtcs(state, trans);
> +			if (intel_cpu_transcoders_need_modeset(state,
> trans)) {
> +				new_crtc_state->uapi.mode_changed =
> true;
> +				new_crtc_state->update_pipe = false;
> +			}
>  		}
>  	}
>
Ville Syrjälä Jan. 16, 2020, 10:56 a.m. UTC | #2
On Thu, Jan 16, 2020 at 12:05:43AM +0000, Souza, Jose wrote:
> On Wed, 2020-01-15 at 21:08 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The post-fastset "does anyone still need a full modeset?" for
> > port sync looks busted. The outer loop bails out of a full modeset
> > is still needed by the current crtc which, and then we skip forcing
> > a full modeset on the related crtcs. That's totally the opposite
> > of what we want.
> 
> Ops yeah, it is always doing a full modeset.
> 
> > 
> > The MST path has the logic mostly the other way around so it
> > looks correct. To fix the port sync case let's follow the MST
> > logic for both. So, if the current crtc already needs a modeset
> > we do nothing. otherwise we check if any of the related crtcs
> > needs a modeset, and if so we force a full modeset for the
> > current crtc.
> > 
> > And while at let's change the else if to a plain if to so
> > we don't have needless coupling between the MST and port sync
> > checks.
> > 
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency loop
> > for port sync")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++--------
> > ----
> >  1 file changed, 17 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index dd03987cc24f..b397816ce253 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14469,37 +14469,23 @@ static int intel_atomic_check_crtcs(struct
> > intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > -static bool intel_cpu_transcoder_needs_modeset(struct
> > intel_atomic_state *state,
> > -					       enum transcoder
> > transcoder)
> > +static bool intel_cpu_transcoders_need_modeset(struct
> > intel_atomic_state *state,
> > +					       u8 transcoders)
> >  {
> > -	struct intel_crtc_state *new_crtc_state;
> > +	const struct intel_crtc_state *new_crtc_state;
> >  	struct intel_crtc *crtc;
> >  	int i;
> >  
> > -	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i)
> > -		if (new_crtc_state->cpu_transcoder == transcoder)
> > -			return needs_modeset(new_crtc_state);
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i) {
> > +		if (new_crtc_state->hw.enable &&
> 
> The intel_pipe_config_compare() will force a modeset for MST and port
> sync but this hw.enable could skip modeset in future features when one
> of the pipes are going from enabled to disabled.

We can't trust what's in .cpu_transcoder when enable==false. It can
be some stale junk left over from the previous state. Well, until
the next patch at least.

If the disabled pipe was the master then the mst/port sync master
transcoder will change and the fastset check won't pass on any of the
slaves. So the enable check is fine for that case.

If the disabled pipe was one of the port sync slaves the relevant
bitmask will change for the master, again making the fastset check
fail. And thus this code here will force a modeset for all the
other slaves.

So can't see how having the check could be a problem.

> 
> Removing it:
> 
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > +		    transcoders & BIT(new_crtc_state->cpu_transcoder)
> > &&
> > +		    needs_modeset(new_crtc_state))
> > +			return true;
> > +	}
> >  
> >  	return false;
> >  }
> >  
> > -static void
> > -intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > -			   u8 transcoders)
> > -{
> > -	struct intel_crtc_state *new_crtc_state;
> > -	struct intel_crtc *crtc;
> > -	int i;
> > -
> > -	for_each_new_intel_crtc_in_state(state, crtc,
> > -					 new_crtc_state, i) {
> > -		if (transcoders & BIT(new_crtc_state->cpu_transcoder))
> > {
> > -			new_crtc_state->uapi.mode_changed = true;
> > -			new_crtc_state->update_pipe = false;
> > -		}
> > -	}
> > -}
> > -
> >  static int
> >  intel_modeset_all_tiles(struct intel_atomic_state *state, int
> > tile_grp_id)
> >  {
> > @@ -14655,15 +14641,20 @@ static 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_transcoder_needs_modeset(state,
> > master)) {
> > +			if (intel_cpu_transcoders_need_modeset(state,
> > BIT(master))) {
> >  				new_crtc_state->uapi.mode_changed =
> > true;
> >  				new_crtc_state->update_pipe = false;
> >  			}
> > -		} else if (is_trans_port_sync_mode(new_crtc_state)) {
> > +		}
> > +
> > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> >  			u8 trans = new_crtc_state-
> > >sync_mode_slaves_mask |
> >  				   BIT(new_crtc_state-
> > >master_transcoder);
> >  
> > -			intel_modeset_synced_crtcs(state, trans);
> > +			if (intel_cpu_transcoders_need_modeset(state,
> > trans)) {
> > +				new_crtc_state->uapi.mode_changed =
> > true;
> > +				new_crtc_state->update_pipe = false;
> > +			}
> >  		}
> >  	}
> >
José Roberto de Souza Jan. 16, 2020, 6:15 p.m. UTC | #3
On Thu, 2020-01-16 at 12:56 +0200, Ville Syrjälä wrote:
> On Thu, Jan 16, 2020 at 12:05:43AM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-15 at 21:08 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The post-fastset "does anyone still need a full modeset?" for
> > > port sync looks busted. The outer loop bails out of a full
> > > modeset
> > > is still needed by the current crtc which, and then we skip
> > > forcing
> > > a full modeset on the related crtcs. That's totally the opposite
> > > of what we want.
> > 
> > Ops yeah, it is always doing a full modeset.
> > 
> > > The MST path has the logic mostly the other way around so it
> > > looks correct. To fix the port sync case let's follow the MST
> > > logic for both. So, if the current crtc already needs a modeset
> > > we do nothing. otherwise we check if any of the related crtcs
> > > needs a modeset, and if so we force a full modeset for the
> > > current crtc.
> > > 
> > > And while at let's change the else if to a plain if to so
> > > we don't have needless coupling between the MST and port sync
> > > checks.
> > > 
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency
> > > loop
> > > for port sync")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++----
> > > ----
> > > ----
> > >  1 file changed, 17 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index dd03987cc24f..b397816ce253 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14469,37 +14469,23 @@ static int
> > > intel_atomic_check_crtcs(struct
> > > intel_atomic_state *state)
> > >  	return 0;
> > >  }
> > >  
> > > -static bool intel_cpu_transcoder_needs_modeset(struct
> > > intel_atomic_state *state,
> > > -					       enum transcoder
> > > transcoder)
> > > +static bool intel_cpu_transcoders_need_modeset(struct
> > > intel_atomic_state *state,
> > > +					       u8 transcoders)
> > >  {
> > > -	struct intel_crtc_state *new_crtc_state;
> > > +	const struct intel_crtc_state *new_crtc_state;
> > >  	struct intel_crtc *crtc;
> > >  	int i;
> > >  
> > > -	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i)
> > > -		if (new_crtc_state->cpu_transcoder == transcoder)
> > > -			return needs_modeset(new_crtc_state);
> > > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i) {
> > > +		if (new_crtc_state->hw.enable &&
> > 
> > The intel_pipe_config_compare() will force a modeset for MST and
> > port
> > sync but this hw.enable could skip modeset in future features when
> > one
> > of the pipes are going from enabled to disabled.
> 
> We can't trust what's in .cpu_transcoder when enable==false. It can
> be some stale junk left over from the previous state. Well, until
> the next patch at least.
> 
> If the disabled pipe was the master then the mst/port sync master
> transcoder will change and the fastset check won't pass on any of the
> slaves. So the enable check is fine for that case.
> 
> If the disabled pipe was one of the port sync slaves the relevant
> bitmask will change for the master, again making the fastset check
> fail. And thus this code here will force a modeset for all the
> other slaves.
> 
> So can't see how having the check could be a problem.

I know this will not affect MST and port sync, I was thinking in future
users of this function but okay, we can leave that to be worked when
such users shows up.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> > Removing it:
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > > +		    transcoders & BIT(new_crtc_state->cpu_transcoder)
> > > &&
> > > +		    needs_modeset(new_crtc_state))
> > > +			return true;
> > > +	}
> > >  
> > >  	return false;
> > >  }
> > >  
> > > -static void
> > > -intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > -			   u8 transcoders)
> > > -{
> > > -	struct intel_crtc_state *new_crtc_state;
> > > -	struct intel_crtc *crtc;
> > > -	int i;
> > > -
> > > -	for_each_new_intel_crtc_in_state(state, crtc,
> > > -					 new_crtc_state, i) {
> > > -		if (transcoders & BIT(new_crtc_state->cpu_transcoder))
> > > {
> > > -			new_crtc_state->uapi.mode_changed = true;
> > > -			new_crtc_state->update_pipe = false;
> > > -		}
> > > -	}
> > > -}
> > > -
> > >  static int
> > >  intel_modeset_all_tiles(struct intel_atomic_state *state, int
> > > tile_grp_id)
> > >  {
> > > @@ -14655,15 +14641,20 @@ static 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_transcoder_needs_modeset(state,
> > > master)) {
> > > +			if (intel_cpu_transcoders_need_modeset(state,
> > > BIT(master))) {
> > >  				new_crtc_state->uapi.mode_changed =
> > > true;
> > >  				new_crtc_state->update_pipe = false;
> > >  			}
> > > -		} else if (is_trans_port_sync_mode(new_crtc_state)) {
> > > +		}
> > > +
> > > +		if (is_trans_port_sync_mode(new_crtc_state)) {
> > >  			u8 trans = new_crtc_state-
> > > > sync_mode_slaves_mask |
> > >  				   BIT(new_crtc_state-
> > > > master_transcoder);
> > >  
> > > -			intel_modeset_synced_crtcs(state, trans);
> > > +			if (intel_cpu_transcoders_need_modeset(state,
> > > trans)) {
> > > +				new_crtc_state->uapi.mode_changed =
> > > true;
> > > +				new_crtc_state->update_pipe = false;
> > > +			}
> > >  		}
> > >  	}
> > >

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index dd03987cc24f..b397816ce253 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14469,37 +14469,23 @@  static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
 	return 0;
 }
 
-static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
-					       enum transcoder transcoder)
+static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
+					       u8 transcoders)
 {
-	struct intel_crtc_state *new_crtc_state;
+	const struct intel_crtc_state *new_crtc_state;
 	struct intel_crtc *crtc;
 	int i;
 
-	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
-		if (new_crtc_state->cpu_transcoder == transcoder)
-			return needs_modeset(new_crtc_state);
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+		if (new_crtc_state->hw.enable &&
+		    transcoders & BIT(new_crtc_state->cpu_transcoder) &&
+		    needs_modeset(new_crtc_state))
+			return true;
+	}
 
 	return false;
 }
 
-static void
-intel_modeset_synced_crtcs(struct intel_atomic_state *state,
-			   u8 transcoders)
-{
-	struct intel_crtc_state *new_crtc_state;
-	struct intel_crtc *crtc;
-	int i;
-
-	for_each_new_intel_crtc_in_state(state, crtc,
-					 new_crtc_state, i) {
-		if (transcoders & BIT(new_crtc_state->cpu_transcoder)) {
-			new_crtc_state->uapi.mode_changed = true;
-			new_crtc_state->update_pipe = false;
-		}
-	}
-}
-
 static int
 intel_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
 {
@@ -14655,15 +14641,20 @@  static 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_transcoder_needs_modeset(state, master)) {
+			if (intel_cpu_transcoders_need_modeset(state, BIT(master))) {
 				new_crtc_state->uapi.mode_changed = true;
 				new_crtc_state->update_pipe = false;
 			}
-		} else if (is_trans_port_sync_mode(new_crtc_state)) {
+		}
+
+		if (is_trans_port_sync_mode(new_crtc_state)) {
 			u8 trans = new_crtc_state->sync_mode_slaves_mask |
 				   BIT(new_crtc_state->master_transcoder);
 
-			intel_modeset_synced_crtcs(state, trans);
+			if (intel_cpu_transcoders_need_modeset(state, trans)) {
+				new_crtc_state->uapi.mode_changed = true;
+				new_crtc_state->update_pipe = false;
+			}
 		}
 	}