Re: [Intel-gfx] [PATCH 15/16] drm/i915: Reduce bigjoiner special casing

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Mon, Sep 13, 2021 at 05:44:39PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Try to make bigjoiner pipes less special.
> 
> The main things here are that each pipe now does full
> clock computation/readout with its own shared_dpll reference.
> Also every pipe's cpu_transcoder always points correctly
> at the master transcoder.
> 
> Due to the above changes state readout is now complete
> and all the related hacks can go away. The actual modeset
> sequence code is still a mess, but I think in order to clean
> that up properly we're probably going to have to redesign
> the modeset logic to treat transcoders vs. pipes separately.
> That is going to require significant amounts of work.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

This cleanup and making both slave and master generic makes sense and lot cleaner

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c     |  16 +--
>  drivers/gpu/drm/i915/display/intel_display.c | 132 ++++++-------------
>  2 files changed, 40 insertions(+), 108 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index f51c5d732d41..6a068a57f6d2 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3615,18 +3615,7 @@ static void intel_ddi_get_config(struct intel_encoder *encoder,
>  	if (drm_WARN_ON(&dev_priv->drm, transcoder_is_dsi(cpu_transcoder)))
>  		return;
>  
> -	if (pipe_config->bigjoiner_slave) {
> -		/* read out pipe settings from master */
> -		enum transcoder save = pipe_config->cpu_transcoder;
> -
> -		/* Our own transcoder needs to be disabled when reading it in intel_ddi_read_func_ctl() */
> -		WARN_ON(pipe_config->output_types);
> -		pipe_config->cpu_transcoder = (enum transcoder)pipe_config->bigjoiner_linked_crtc->pipe;
> -		intel_ddi_read_func_ctl(encoder, pipe_config);
> -		pipe_config->cpu_transcoder = save;
> -	} else {
> -		intel_ddi_read_func_ctl(encoder, pipe_config);
> -	}
> +	intel_ddi_read_func_ctl(encoder, pipe_config);
>  
>  	intel_ddi_mso_get_config(encoder, pipe_config);
>  
> @@ -3654,8 +3643,7 @@ static void intel_ddi_get_config(struct intel_encoder *encoder,
>  		dev_priv->vbt.edp.bpp = pipe_config->pipe_bpp;
>  	}
>  
> -	if (!pipe_config->bigjoiner_slave)
> -		ddi_dotclock_get(pipe_config);
> +	ddi_dotclock_get(pipe_config);
>  
>  	if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
>  		pipe_config->lane_lat_optim_mask =
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 25ae9e4f6b66..17d12d12bc0a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2027,15 +2027,17 @@ struct intel_encoder *
>  intel_get_crtc_new_encoder(const struct intel_atomic_state *state,
>  			   const struct intel_crtc_state *crtc_state)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	const struct drm_connector_state *connector_state;
>  	const struct drm_connector *connector;
>  	struct intel_encoder *encoder = NULL;
> +	struct intel_crtc *master_crtc;
>  	int num_encoders = 0;
>  	int i;
>  
> +	master_crtc = intel_master_crtc(crtc_state);
> +
>  	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> -		if (connector_state->crtc != &crtc->base)
> +		if (connector_state->crtc != &master_crtc->base)
>  			continue;
>  
>  		encoder = to_intel_encoder(connector_state->best_encoder);
> @@ -2044,7 +2046,7 @@ intel_get_crtc_new_encoder(const struct intel_atomic_state *state,
>  
>  	drm_WARN(encoder->base.dev, num_encoders != 1,
>  		 "%d encoders for pipe %c\n",
> -		 num_encoders, pipe_name(crtc->pipe));
> +		 num_encoders, pipe_name(master_crtc->pipe));
>  
>  	return encoder;
>  }
> @@ -3005,20 +3007,20 @@ static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
>  		break;
>  	}
>  
> -	if (!crtc_state->bigjoiner_slave) {
> -		/* need to enable VDSC, which we skipped in pre-enable */
> -		intel_dsc_enable(crtc_state);
> -	} else {
> -		/*
> -		 * Enable sequence steps 1-7 on bigjoiner master
> -		 */
> +	/*
> +	 * Enable sequence steps 1-7 on bigjoiner master
> +	 */
> +	if (crtc_state->bigjoiner_slave)
>  		intel_encoders_pre_pll_enable(state, master_crtc);
> -		if (master_crtc_state->shared_dpll)
> -			intel_enable_shared_dpll(master_crtc_state);
> +
> +	if (crtc_state->shared_dpll)
> +		intel_enable_shared_dpll(crtc_state);
> +
> +	if (crtc_state->bigjoiner_slave)
>  		intel_encoders_pre_enable(state, master_crtc);
>  
> -		intel_dsc_enable(crtc_state);
> -	}
> +	/* need to enable VDSC, which we skipped in pre-enable */
> +	intel_dsc_enable(crtc_state);
>  
>  	if (DISPLAY_VER(dev_priv) >= 13)
>  		intel_uncompressed_joiner_enable(crtc_state);
> @@ -3201,12 +3203,17 @@ static void ilk_crtc_disable(struct intel_atomic_state *state,
>  static void hsw_crtc_disable(struct intel_atomic_state *state,
>  			     struct intel_crtc *crtc)
>  {
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +
>  	/*
>  	 * FIXME collapse everything to one hook.
>  	 * Need care with mst->ddi interactions.
>  	 */
> -	intel_encoders_disable(state, crtc);
> -	intel_encoders_post_disable(state, crtc);
> +	if (!old_crtc_state->bigjoiner_slave) {
> +		intel_encoders_disable(state, crtc);
> +		intel_encoders_post_disable(state, crtc);
> +	}
>  }
>  
>  static void i9xx_pfit_enable(const struct intel_crtc_state *crtc_state)
> @@ -5912,17 +5919,10 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  	if (DISPLAY_VER(dev_priv) >= 13 && !pipe_config->dsc.compression_enable)
>  		intel_uncompressed_joiner_get_config(pipe_config);
>  
> -	if (!active) {
> -		/* bigjoiner slave doesn't enable transcoder */
> -		if (!pipe_config->bigjoiner_slave)
> -			goto out;
> +	if (!active)
> +		goto out;
>  
> -		active = true;
> -		pipe_config->pixel_multiplier = 1;
> -
> -		/* we cannot read out most state, so don't bother.. */
> -		pipe_config->quirks |= PIPE_CONFIG_QUIRK_BIGJOINER_SLAVE;
> -	} else if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
> +	if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
>  	    DISPLAY_VER(dev_priv) >= 11) {
>  		hsw_get_ddi_port_state(crtc, pipe_config);
>  		intel_get_transcoder_timings(crtc, pipe_config);
> @@ -5994,10 +5994,7 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  		}
>  	}
>  
> -	if (pipe_config->bigjoiner_slave) {
> -		/* Cannot be read out as a slave, set to 0. */
> -		pipe_config->pixel_multiplier = 0;
> -	} else if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
> +	if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
>  	    !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
>  		pipe_config->pixel_multiplier =
>  			intel_de_read(dev_priv,
> @@ -6845,7 +6842,6 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
>  
>  	if (mode_changed && crtc_state->hw.enable &&
>  	    dev_priv->display.crtc_compute_clock &&
> -	    !crtc_state->bigjoiner_slave &&
>  	    !drm_WARN_ON(&dev_priv->drm, crtc_state->shared_dpll)) {
>  		ret = dev_priv->display.crtc_compute_clock(crtc_state);
>  		if (ret)
> @@ -7463,7 +7459,6 @@ copy_bigjoiner_crtc_state(struct intel_crtc_state *crtc_state,
>  			  const struct intel_crtc_state *from_crtc_state)
>  {
>  	struct intel_crtc_state *saved_state;
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
>  	saved_state = kmemdup(from_crtc_state, sizeof(*saved_state), GFP_KERNEL);
>  	if (!saved_state)
> @@ -7493,8 +7488,8 @@ copy_bigjoiner_crtc_state(struct intel_crtc_state *crtc_state,
>  	crtc_state->nv12_planes = crtc_state->c8_planes = crtc_state->update_planes = 0;
>  	crtc_state->bigjoiner_linked_crtc = to_intel_crtc(from_crtc_state->uapi.crtc);
>  	crtc_state->bigjoiner_slave = true;
> -	crtc_state->cpu_transcoder = (enum transcoder)crtc->pipe;
> -	crtc_state->has_audio = false;
> +	crtc_state->cpu_transcoder = from_crtc_state->cpu_transcoder;
> +	crtc_state->has_audio = from_crtc_state->has_audio;
>  
>  	return 0;
>  }
> @@ -8581,10 +8576,6 @@ verify_crtc_state(struct intel_crtc *crtc,
>  	if (!new_crtc_state->hw.active)
>  		return;
>  
> -	if (new_crtc_state->bigjoiner_slave)
> -		/* No PLLs set for slave */
> -		pipe_config->shared_dpll = NULL;
> -
>  	intel_pipe_config_sanity_check(dev_priv, pipe_config);
>  
>  	if (!intel_pipe_config_compare(new_crtc_state,
> @@ -8703,9 +8694,6 @@ verify_mpllb_state(struct intel_atomic_state *state,
>  	if (!new_crtc_state->hw.active)
>  		return;
>  
> -	if (new_crtc_state->bigjoiner_slave)
> -		return;
> -
>  	encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
>  	intel_mpllb_readout_hw_state(encoder, &mpllb_hw_state);
>  
> @@ -9915,16 +9903,6 @@ static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  
> -	drm_WARN_ON(&dev_priv->drm, old_crtc_state->bigjoiner_slave);
> -
> -	/*
> -	 * We still need special handling for disabling bigjoiner master
> -	 * and slaves since for slave we do not have encoder or plls
> -	 * so we dont need to disable those.
> -	 */
> -	if (old_crtc_state->bigjoiner)
> -		old_crtc_state->bigjoiner_linked_crtc->active = false;
> -
>  	/*
>  	 * We need to disable pipe CRC before disabling the pipe,
>  	 * or we race against vblank off.
> @@ -9965,7 +9943,7 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  	/* Only disable port sync and MST slaves */
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
> -		if (!intel_crtc_needs_modeset(new_crtc_state) || old_crtc_state->bigjoiner)
> +		if (!intel_crtc_needs_modeset(new_crtc_state))
>  			continue;
>  
>  		if (!old_crtc_state->hw.active)
> @@ -9977,7 +9955,8 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  		 * Slave vblanks are masked till Master Vblanks.
>  		 */
>  		if (!is_trans_port_sync_slave(old_crtc_state) &&
> -		    !intel_dp_mst_is_slave_trans(old_crtc_state))
> +		    !intel_dp_mst_is_slave_trans(old_crtc_state) &&
> +		    !old_crtc_state->bigjoiner_slave)
>  			continue;
>  
>  		intel_old_crtc_state_disables(state, old_crtc_state,
> @@ -9989,13 +9968,14 @@ static void intel_commit_modeset_disables(struct intel_atomic_state *state)
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!intel_crtc_needs_modeset(new_crtc_state) ||
> -		    (handled & BIT(crtc->pipe)) ||
> -		    old_crtc_state->bigjoiner_slave)
> +		    (handled & BIT(crtc->pipe)))
>  			continue;
>  
> -		if (old_crtc_state->hw.active)
> -			intel_old_crtc_state_disables(state, old_crtc_state,
> -						      new_crtc_state, crtc);
> +		if (!old_crtc_state->hw.active)
> +			continue;
> +
> +		intel_old_crtc_state_disables(state, old_crtc_state,
> +					      new_crtc_state, crtc);
>  	}
>  }
>  
> @@ -12373,9 +12353,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		struct intel_plane *plane;
>  		int min_cdclk = 0;
>  
> -		if (crtc_state->bigjoiner_slave)
> -			continue;
> -
>  		if (crtc_state->hw.active) {
>  			/*
>  			 * The initial mode needs to be set in order to keep
> @@ -12435,39 +12412,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		intel_bw_crtc_update(bw_state, crtc_state);
>  
>  		intel_pipe_config_sanity_check(dev_priv, crtc_state);
> -
> -		/* discard our incomplete slave state, copy it from master */
> -		if (crtc_state->bigjoiner && crtc_state->hw.active) {
> -			struct intel_crtc *slave = crtc_state->bigjoiner_linked_crtc;
> -			struct intel_crtc_state *slave_crtc_state =
> -				to_intel_crtc_state(slave->base.state);
> -
> -			copy_bigjoiner_crtc_state(slave_crtc_state, crtc_state);
> -			slave->base.mode = crtc->base.mode;
> -
> -			cdclk_state->min_cdclk[slave->pipe] = min_cdclk;
> -			cdclk_state->min_voltage_level[slave->pipe] =
> -				crtc_state->min_voltage_level;
> -
> -			for_each_intel_plane_on_crtc(&dev_priv->drm, slave, plane) {
> -				const struct intel_plane_state *plane_state =
> -					to_intel_plane_state(plane->base.state);
> -
> -				/*
> -				 * FIXME don't have the fb yet, so can't
> -				 * use intel_plane_data_rate() :(
> -				 */
> -				if (plane_state->uapi.visible)
> -					crtc_state->data_rate[plane->id] =
> -						4 * crtc_state->pixel_rate;
> -				else
> -					crtc_state->data_rate[plane->id] = 0;
> -			}
> -
> -			intel_bw_crtc_update(bw_state, slave_crtc_state);
> -			drm_calc_timestamping_constants(&slave->base,
> -							&slave_crtc_state->hw.adjusted_mode);
> -		}
>  	}
>  }
>  
> -- 
> 2.32.0
>