Re: [Intel-gfx] [PATCH 10/13] drm/i915: Fix port sync code to work with >2 pipes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Apr 03, 2020 at 12:32:20AM +0000, Souza, Jose wrote:
> On Fri, 2020-03-13 at 18:48 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Don't assume there is just one port sync slave. We might have
> > several.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 98 ++++++++++------
> > ----
> >  1 file changed, 49 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index b56a5a49418f..33f38c8a5da4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15009,18 +15009,6 @@ static void intel_update_crtc(struct
> > intel_atomic_state *state,
> >  		intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
> >  }
> >  
> > -static struct intel_crtc *intel_get_slave_crtc(const struct
> > intel_crtc_state *new_crtc_state)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(new_crtc_state-
> > >uapi.crtc->dev);
> > -	enum transcoder slave_transcoder;
> > -
> > -	drm_WARN_ON(&dev_priv->drm,
> > -		    !is_power_of_2(new_crtc_state-
> > >sync_mode_slaves_mask));
> > -
> > -	slave_transcoder = ffs(new_crtc_state->sync_mode_slaves_mask) -
> > 1;
> > -	return intel_get_crtc_for_pipe(dev_priv,
> > -				       (enum pipe)slave_transcoder);
> > -}
> >  
> >  static void intel_old_crtc_state_disables(struct intel_atomic_state
> > *state,
> >  					  struct intel_crtc_state
> > *old_crtc_state,
> > @@ -15109,8 +15097,8 @@ static void
> > intel_commit_modeset_enables(struct intel_atomic_state *state)
> >  	}
> >  }
> >  
> > -static void intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
> > -				       struct intel_atomic_state
> > *state)
> > +static void intel_set_dp_tp_ctl_normal(struct intel_atomic_state
> > *state,
> > +				       struct intel_crtc *crtc)
> >  {
> >  	struct drm_connector *uninitialized_var(conn);
> >  	struct drm_connector_state *conn_state;
> > @@ -15125,45 +15113,55 @@ static void
> > intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
> >  	intel_dp_stop_link_train(intel_dp);
> >  }
> >  
> > -static void intel_update_trans_port_sync_crtcs(struct intel_crtc
> > *crtc,
> > -					       struct
> > intel_atomic_state *state,
> > -					       struct intel_crtc_state
> > *old_crtc_state,
> > -					       struct intel_crtc_state
> > *new_crtc_state)
> > +static void intel_update_trans_port_sync_crtcs(struct
> > intel_atomic_state *state,
> > +					       struct intel_crtc *crtc)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > -	struct intel_crtc *slave_crtc =
> > intel_get_slave_crtc(new_crtc_state);
> > -	struct intel_crtc_state *new_slave_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, slave_crtc);
> > -	struct intel_crtc_state *old_slave_crtc_state =
> > -		intel_atomic_get_old_crtc_state(state, slave_crtc);
> > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > +	const struct intel_crtc_state *new_slave_crtc_state;
> > +	const struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *slave_crtc;
> > +	int i;
> >  
> > -	drm_WARN_ON(&i915->drm, !slave_crtc || !new_slave_crtc_state ||
> > -		    !old_slave_crtc_state);
> > +	for_each_new_intel_crtc_in_state(state, slave_crtc,
> > +					 new_slave_crtc_state, i) {
> > +		if (new_slave_crtc_state->master_transcoder !=
> > +		    new_crtc_state->cpu_transcoder)
> 
> Missing new_crtc_state initialization.

Whoops. Fixed.

> 
> With that:
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>
<snip>
> > -			/* TODO: update entries[] of slave */
> > -			modeset_pipes &= ~BIT(slave_crtc->pipe);
> > +			for_each_new_intel_crtc_in_state(state,
> > slave_crtc,
> > +							 new_slave_crtc
> > _state, i) {
> >  
> > +				/* TODO: update entries[] of slave */
> > +				modeset_pipes &= ~BIT(slave_crtc-
> > >pipe);

Noticed another problem here. Instead of clearing modeset_pipes for
the slaves of the current crtc we clear it for all crtcs. Fixed to do
the same master_transcoder check as above.

> > +			}
> >  		} else {
> >  			intel_enable_crtc(state, crtc);
> >  			intel_update_crtc(state, crtc);

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx