On Mon, Dec 16, 2019 at 02:07:39PM -0800, José Roberto de Souza wrote:
> Due to DDB overlaps the pipe enabling sequence is not always crescent.
> As the previous patch selects the smallest pipe/transcoder in the MST
> stream to be master and it needs to be enabled first this changes
> were needed to guarantee that.
>
> So first lets enable all pipes that did not needed a fullmodeset so
> it don't have any external dependency, this ones can overlap with
> each other ddb allocations.
>
> Then on the second loop it will enable all the pipes that needs a
> modeset and don't depends on other pipes like MST master
> pipe/transcoder.
>
> Then finally all the pipes that needs a modeset and have dependency
> on other pipes.
>
> v3: rebased
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 88 +++++++++++++++-----
> 1 file changed, 65 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e976f82a0db7..176e31de68e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14553,15 +14553,20 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> u8 required_slices = state->wm_results.ddb.enabled_slices;
> struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> - u8 dirty_pipes = 0;
> + u8 update_pipes = 0, modeset_pipes = 0;
> int i;
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + if (!new_crtc_state->hw.active)
> + continue;
> +
> /* ignore allocations for crtc's that have been turned off. */
> - if (!needs_modeset(new_crtc_state) && new_crtc_state->hw.active)
> + if (!needs_modeset(new_crtc_state)) {
> entries[i] = old_crtc_state->wm.skl.ddb;
> - if (new_crtc_state->hw.active)
> - dirty_pipes |= BIT(crtc->pipe);
> + update_pipes |= BIT(crtc->pipe);
> + } else {
> + modeset_pipes |= BIT(modeset_pipes);
> + }
> }
>
> /* If 2nd DBuf slice required, enable it here */
> @@ -14571,16 +14576,18 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> /*
> * Whenever the number of active pipes changes, we need to make sure we
> * update the pipes in the right order so that their ddb allocations
> - * never overlap with eachother inbetween CRTC updates. Otherwise we'll
> + * never overlap with each other between CRTC updates. Otherwise we'll
> * cause pipe underruns and other bad stuff.
> + *
> + * So first lets enable all pipes that did not needed a fullmodeset so
> + * it don't have any external dependency
> */
> - while (dirty_pipes) {
> + while (update_pipes) {
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> enum pipe pipe = crtc->pipe;
> - bool modeset = needs_modeset(new_crtc_state);
>
> - if ((dirty_pipes & BIT(pipe)) == 0)
> + if ((update_pipes & BIT(pipe)) == 0)
> continue;
>
> if (skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb,
> @@ -14589,20 +14596,10 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> continue;
>
> entries[i] = new_crtc_state->wm.skl.ddb;
> - dirty_pipes &= ~BIT(pipe);
> -
> - if (modeset && is_trans_port_sync_mode(new_crtc_state)) {
> - if (is_trans_port_sync_master(new_crtc_state))
> - intel_update_trans_port_sync_crtcs(crtc,
> - state,
> - old_crtc_state,
> - new_crtc_state);
> - else
> - continue;
> - } else {
> - intel_update_crtc(crtc, state, old_crtc_state,
> - new_crtc_state);
> - }
> + update_pipes &= ~BIT(pipe);
> +
> + intel_update_crtc(crtc, state, old_crtc_state,
> + new_crtc_state);
>
> /*
> * If this is an already active pipe, it's DDB changed,
> @@ -14612,11 +14609,56 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> */
> if (!skl_ddb_entry_equal(&new_crtc_state->wm.skl.ddb,
> &old_crtc_state->wm.skl.ddb) &&
> - !modeset && dirty_pipes)
> + update_pipes)
Needs to be update_pipes|modeset_pipes.
> intel_wait_for_vblank(dev_priv, pipe);
> +
Extra newline.
> }
> }
>
> + /*
> + * Enabling all pipes that needs a modeset and do not depends on other
> + * pipes
> + */
> + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> + new_crtc_state, i) {
> + enum pipe pipe = crtc->pipe;
> +
> + if ((modeset_pipes & BIT(pipe)) == 0)
> + continue;
> +
> + if (intel_dp_mst_is_slave_trans(new_crtc_state) ||
> + is_trans_port_sync_slave(new_crtc_state))
> + continue;
> +
> + modeset_pipes &= ~BIT(pipe);
> +
> + if (is_trans_port_sync_mode(new_crtc_state))
> + intel_update_trans_port_sync_crtcs(crtc, state,
> + old_crtc_state,
> + new_crtc_state);
> + else
> + intel_update_crtc(crtc, state, old_crtc_state,
> + new_crtc_state);
> + }
> +
> + /*
> + * Finally enable all pipes that needs a modeset and depends on
> + * other pipes, right now it is only MST slaves as both port sync slave
> + * and master are enabled together
> + */
> + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> + new_crtc_state, i) {
> + enum pipe pipe = crtc->pipe;
> +
> + if ((modeset_pipes & BIT(pipe)) == 0)
> + continue;
> +
> + if (is_trans_port_sync_slave(new_crtc_state))
> + continue;
If we clear the modeset_pipes bit for these in the previous loop we
could avoid this check here entirely.
Still missing the DDB overlap WARNs that I think would be good to
have in these two enable loops.
Looks nice otherwise.
> +
> + intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state);
> + }
> +
> /* If 2nd DBuf slice is no more required disable it */
> if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
> icl_dbuf_slices_update(dev_priv, required_slices);
> --
> 2.24.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
