[PATCH 2/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset

Ville Syrjälä ville.syrjala at linux.intel.com
Fri Mar 1 15:26:19 UTC 2024


On Fri, Mar 01, 2024 at 05:17:41PM +0200, Lisovskiy, Stanislav wrote:
> On Fri, Mar 01, 2024 at 04:40:28PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 01, 2024 at 02:29:28PM +0200, Lisovskiy, Stanislav wrote:
> > > On Fri, Mar 01, 2024 at 12:43:46PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Mar 01, 2024 at 12:27:18PM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Fri, Mar 01, 2024 at 12:10:52PM +0200, Ville Syrjälä wrote:
> > > > > > On Wed, Feb 21, 2024 at 09:20:09PM +0200, Stanislav Lisovskiy wrote:
> > > > > > > Handle only bigjoiner masters in skl_commit_modeset_enables/disables,
> > > > > > > slave crtcs should be handled by master hooks. Same for encoders.
> > > > > > > That way we can also remove a bunch of checks like intel_crtc_is_bigjoiner_slave.
> > > > > > > 
> > > > > > > v2: Get rid of master vs slave checks and separation in crtc enable/disable hooks.
> > > > > > >     Use unified iteration cycle for all of those, while enabling/disabling
> > > > > > >     transcoder only for those pipes where its needed(Ville Syrjälä)
> > > > > > > 
> > > > > > > v3: Move all the intel_encoder_* calls under transcoder code path(Ville Syrjälä)
> > > > > > > 
> > > > > > > v4:  - Call intel_crtc_vblank_on from hsw_crtc_enable only for non-transcoder path
> > > > > > >        (for master pipe that will be called from intel_encoders_enable/intel_enable_ddi)
> > > > > > >      - Fix stupid mistake with using crtc->pipe for the mask, instead of BIT(crtc->pipe)
> > > > > > > 
> > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/display/intel_ddi.c     |  21 +--
> > > > > > >  drivers/gpu/drm/i915/display/intel_display.c | 183 ++++++++++++-------
> > > > > > >  drivers/gpu/drm/i915/display/intel_display.h |   6 +
> > > > > > >  3 files changed, 121 insertions(+), 89 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > index bea4415902044..6071e9f500871 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > @@ -3100,7 +3100,6 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
> > > > > > >  				   const struct drm_connector_state *old_conn_state)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > > > > -	struct intel_crtc *slave_crtc;
> > > > > > >  
> > > > > > >  	if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) {
> > > > > > >  		intel_crtc_vblank_off(old_crtc_state);
> > > > > > > @@ -3117,17 +3116,6 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,
> > > > > > >  			ilk_pfit_disable(old_crtc_state);
> > > > > > >  	}
> > > > > > 
> > > > > > The master pipe stuff is right here ^ ...
> > > > > > 
> > > > > > >  
> > > > > > > -	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, slave_crtc,
> > > > > > > -					 intel_crtc_bigjoiner_slave_pipes(old_crtc_state)) {
> > > > > > > -		const struct intel_crtc_state *old_slave_crtc_state =
> > > > > > > -			intel_atomic_get_old_crtc_state(state, slave_crtc);
> > > > > > > -
> > > > > > > -		intel_crtc_vblank_off(old_slave_crtc_state);
> > > > > > > -
> > > > > > > -		intel_dsc_disable(old_slave_crtc_state);
> > > > > > > -		skl_scaler_disable(old_slave_crtc_state);
> > > > > > > -	}
> > > > > > 
> > > > > > .. but now you're moving the slave pipe stuff somewhere else?
> > > > > > 
> > > > > > We should be just iterating the pipes here (assuming this 
> > > > > > is the correct spot to do these steps).
> > > > > > 
> > > > > > > -
> > > > > > >  	/*
> > > > > > >  	 * When called from DP MST code:
> > > > > > >  	 * - old_conn_state will be NULL
> > > > > > > @@ -3363,8 +3351,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state,
> > > > > > >  {
> > > > > > >  	drm_WARN_ON(state->base.dev, crtc_state->has_pch_encoder);
> > > > > > >  
> > > > > > > -	if (!intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > > > > -		intel_ddi_enable_transcoder_func(encoder, crtc_state);
> > > > > > > +	intel_ddi_enable_transcoder_func(encoder, crtc_state);
> > > > > > >  
> > > > > > >  	/* Enable/Disable DP2.0 SDP split config before transcoder */
> > > > > > >  	intel_audio_sdp_split_update(crtc_state);
> > > > > > > @@ -3469,9 +3456,6 @@ void intel_ddi_update_active_dpll(struct intel_atomic_state *state,
> > > > > > >  				  struct intel_crtc *crtc)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > > > > > > -	struct intel_crtc_state *crtc_state =
> > > > > > > -		intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > > -	struct intel_crtc *slave_crtc;
> > > > > > >  	enum phy phy = intel_port_to_phy(i915, encoder->port);
> > > > > > >  
> > > > > > >  	/* FIXME: Add MTL pll_mgr */
> > > > > > > @@ -3479,9 +3463,6 @@ void intel_ddi_update_active_dpll(struct intel_atomic_state *state,
> > > > > > >  		return;
> > > > > > >  
> > > > > > >  	intel_update_active_dpll(state, crtc, encoder);
> > > > > > > -	for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc,
> > > > > > > -					 intel_crtc_bigjoiner_slave_pipes(crtc_state))
> > > > > > > -		intel_update_active_dpll(state, slave_crtc, encoder);
> > > > > > >  }
> > > > > > >  
> > > > > > >  static void
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > index 916c13a149fd5..e1ea53fd6a288 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > > > @@ -1631,31 +1631,12 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> > > > > > >  	hsw_set_transconf(crtc_state);
> > > > > > >  }
> > > > > > >  
> > > > > > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > > > > -			    struct intel_crtc *crtc)
> > > > > > > +static void hsw_crtc_enable_pre_transcoder(struct intel_atomic_state *state,
> > > > > > > +					   struct intel_crtc *crtc)
> > > > > > >  {
> > > > > > >  	const struct intel_crtc_state *new_crtc_state =
> > > > > > >  		intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > > > > -	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > > > > -	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > > > > > -	bool psl_clkgate_wa;
> > > > > > > -
> > > > > > > -	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > > > > -		return;
> > > > > > > -
> > > > > > > -	intel_dmc_enable_pipe(dev_priv, crtc->pipe);
> > > > > > > -
> > > > > > > -	if (!new_crtc_state->bigjoiner_pipes) {
> > > > > > > -		intel_encoders_pre_pll_enable(state, crtc);
> > > > > > > -
> > > > > > > -		if (new_crtc_state->shared_dpll)
> > > > > > > -			intel_enable_shared_dpll(new_crtc_state);
> > > > > > > -
> > > > > > > -		intel_encoders_pre_enable(state, crtc);
> > > > > > > -	} else {
> > > > > > > -		icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > > > > > -	}
> > > > > > >  
> > > > > > >  	intel_dsc_enable(new_crtc_state);
> > > > > > >  
> > > > > > > @@ -1665,19 +1646,17 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > > > >  	intel_set_pipe_src_size(new_crtc_state);
> > > > > > >  	if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > > > > >  		bdw_set_pipe_misc(new_crtc_state);
> > > > > > > +}
> > > > > > >  
> > > > > > > -	if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > > > > > > -	    !transcoder_is_dsi(cpu_transcoder))
> > > > > > > -		hsw_configure_cpu_transcoder(new_crtc_state);
> > > > > > > +static void hsw_crtc_enable_post_transcoder(struct intel_atomic_state *state,
> > > > > > > +					    struct intel_crtc *crtc)
> > > > > > > +{
> > > > > > > +	const struct intel_crtc_state *new_crtc_state =
> > > > > > > +		intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > > > >  
> > > > > > >  	crtc->active = true;
> > > > > > >  
> > > > > > > -	/* Display WA #1180: WaDisableScalarClockGating: glk */
> > > > > > > -	psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> > > > > > > -		new_crtc_state->pch_pfit.enabled;
> > > > > > > -	if (psl_clkgate_wa)
> > > > > > > -		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > > > > > > -
> > > > > > >  	if (DISPLAY_VER(dev_priv) >= 9)
> > > > > > >  		skl_pfit_enable(new_crtc_state);
> > > > > > >  	else
> > > > > > > @@ -1700,27 +1679,84 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > > > >  		icl_set_pipe_chicken(new_crtc_state);
> > > > > > >  
> > > > > > >  	intel_initial_watermarks(state, crtc);
> > > > > > > +}
> > > > > > >  
> > > > > > > -	if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> > > > > > > -		intel_crtc_vblank_on(new_crtc_state);
> > > > > > > +static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > > > > +			    struct intel_crtc *crtc)
> > > > > > > +{
> > > > > > > +	const struct intel_crtc_state *new_crtc_state =
> > > > > > > +		intel_atomic_get_new_crtc_state(state, crtc);
> > > > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > > > > +	enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > > > > > +	struct intel_crtc *_crtc;
> > > > > > > +	int slave_pipe_mask = intel_crtc_bigjoiner_slave_pipes(new_crtc_state);
> > > > > > > +	int pipe_mask = slave_pipe_mask | BIT(crtc->pipe);
> > > > > > > +	bool psl_clkgate_wa;
> > > > > > > +	enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > > > >  
> > > > > > > -	intel_encoders_enable(state, crtc);
> > > > > > > +	if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> > > > > > > +		return;
> > > > > > >  
> > > > > > > -	if (psl_clkgate_wa) {
> > > > > > > -		intel_crtc_wait_for_next_vblank(crtc);
> > > > > > > -		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, false);
> > > > > > > -	}
> > > > > > > +	/*
> > > > > > > +	 * Use reverse iterator to go through slave pipes first.
> > > > > > > +	 * TODO: We might need smarter iterator here
> > > > > > > +	 */
> > > > > > > +	for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->drm, _crtc,
> > > > > > > +						 pipe_mask) {
> > > > > > > +		const struct intel_crtc_state *_new_crtc_state =
> > > > > > > +			intel_atomic_get_new_crtc_state(state, _crtc);
> > > > > > > +		bool needs_transcoder = ((slave_pipe_mask & BIT(_crtc->pipe)) == 0) &&
> > > > > > > +					!transcoder_is_dsi(cpu_transcoder);
> > > > > > > +
> > > > > > > +		intel_dmc_enable_pipe(dev_priv, crtc->pipe);
> > > > > > > +
> > > > > > > +		if (!new_crtc_state->bigjoiner_pipes) {
> > > > > > > +			if (needs_transcoder)
> > > > > > > +				intel_encoders_pre_pll_enable(state, crtc);
> > > > > > > +
> > > > > > > +			if (new_crtc_state->shared_dpll)
> > > > > > > +				intel_enable_shared_dpll(new_crtc_state);
> > > > > > > +
> > > > > > > +			if (needs_transcoder)
> > > > > > > +				intel_encoders_pre_enable(state, crtc);
> > > > > > > +		} else {
> > > > > > > +			icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > > > > > +		}
> > > > > > 
> > > > > > That mess needs to be eliminated entirely.
> > > > > 
> > > > > Yeah, was thinking about this too, was just a bit unsure how..
> > > > > 
> > > > > > 
> > > > > > > +
> > > > > > > +		hsw_crtc_enable_pre_transcoder(state, _crtc);
> > > > > > > +
> > > > > > > +		if (needs_transcoder)
> > > > > > > +			hsw_configure_cpu_transcoder(_new_crtc_state);
> > > > > > 
> > > > > > These transcoder things should not be within any pipe loop at all.
> > > > > 
> > > > > I didn't want to split the loop, which I would have to do otherwise,
> > > > > but may be it makes sense, since transcoder path is needed only for master
> > > > > pipe. However what if _hypothetically_ :) we would have more than one master
> > > > > pipe?
> > > > 
> > > > Doesn't matter how many pipes there are. There is always just one
> > > > transcoder.
> > > 
> > > How I see it now, the sequence for example for master hsw_crtc_enable is:
> > > 
> > > pipe1 stuff before transcoder
> > > (transcoder programming not needed)
> > > pipe1 stuff after transcoder
> > > 
> > > pipe0 stuff before transcoder
> > > transcoder programmed
> > > pipe0 stuff after transcoder
> > > 
> > > if we want to have a single unified loop for all pipes, I guess we have to
> > > have a transcoder check inside a loop, because we cant do the 
> > > "pipe0 stuff after transcoder" thing, before the transcoder is programmed.
> > > 
> > > I could of course split it this way:
> > > 
> > > for (...)
> > >   pipe stuff before transcoder
> > > 
> > > transcoder programmed
> > > 
> > > for (...)
> > >   pipe stuff after transcoder
> > > 
> > > but then the sequence still will be different from original, it will look like:
> > > pipe1 stuff before transcoder
> > > 
> > > pipe0 stuff before transcoder
> > > 
> > > transcoder programmed
> > > 
> > > pipe1 stuff after transcoder
> > > 
> > > pipe0 stuff after transcoder
> > > 
> > > which is different from original sequence, because we in fact
> > > want that:
> > > program pipe1(slave)
> > > 
> > > program pipe0 stuff before transcoder(master)
> > > program transcoder
> > > program pipe0 stuff after transcoder(master)
> > > 
> > > So do you think that splitting won't harm or you see some other way to do that?
> > 
> > The current code is mostly nonsense I think. Probably only work
> > through the power of prayer. I think we need to be able to control
> > the per-pipe vs. per-transcoder steps more freely to make it actually
> > correct.
> 
> 
> Ville, could you communicate to me next time, if you decide to do everything
> yourself or you see it some other way.
> Basically I was trying to discuss how you see things here and then it just ends
> up that way. 
> If you didn't like the whole approach why I hear this only now, was following
> all your comments and asking questions.
> You could either communicate your ideas to me or at least communicate that you
> plan to send own series..
> Now we had a discussion, some particular things were discussed, then suddenly
> you say that everything is crap and send own series.
> 
> Is that really professional team work?
> 
> Stan
> 
> > 
> > I fired off a quick attempt at converting the disable side,
> > since that is a bit more straightforwad. The end result looks
> > fairly reasonable to me at least.
> > https://patchwork.freedesktop.org/series/130619/
> 
> Ville, could you communicate to me next time, if you decide to do everything

I didn't do everything. Just enough to demonstrate the direction
where I think we should go.

-- 
Ville Syrjälä
Intel


More information about the Intel-gfx mailing list