On Mon, Sep 13, 2021 at 05:44:39PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
This cleanup and making both slave and master generic makes sense and lot cleaner
Reviewed-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
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
>
