On Wed, Dec 18, 2019 at 10:59:07AM -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 > > v4: > - added check for modeset_pipes too to decide if is necessary for a > wait a vblank > - added DDB allocation overlap check for pipes that needs a modeset > > Cc: Ville Syrjälä <[email protected]> > Cc: Maarten Lankhorst <[email protected]> > Cc: Matt Roper <[email protected]> > Cc: Manasi Navare <[email protected]> > Signed-off-by: José Roberto de Souza <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_display.c | 108 ++++++++++++++----- > 1 file changed, 83 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 59b3bfe8b721..a4f252d05b37 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -14517,15 +14517,21 @@ 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; > + const u8 num_pipes = INTEL_NUM_PIPES(dev_priv); > + 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 */ > @@ -14535,38 +14541,29 @@ 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, > - entries, > - > INTEL_NUM_PIPES(dev_priv), i)) > + entries, num_pipes, i)) > 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, > @@ -14576,11 +14573,72 @@ 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 | modeset_pipes)) > intel_wait_for_vblank(dev_priv, pipe); > } > } > > + /* > + * 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; > + > + WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > + entries, num_pipes, i)); > + > + entries[i] = new_crtc_state->wm.skl.ddb; > + modeset_pipes &= ~BIT(pipe); > + > + if (is_trans_port_sync_mode(new_crtc_state)) { > + struct intel_crtc *slave_crtc; > + > + intel_update_trans_port_sync_crtcs(crtc, state, > + old_crtc_state, > + new_crtc_state); > + > + slave_crtc = intel_get_slave_crtc(new_crtc_state); > + /* TODO: update entries[] of slave */ > + modeset_pipes &= ~BIT(slave_crtc->pipe);
OK. So we clear the bit for both master+slave at the same time. Should work and not trip un the WARN at the end (assuming both have their modeset properly flagged). Reviewed-by: Ville Syrjälä <[email protected]> > + > + } 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; > + > + WARN_ON(skl_ddb_allocation_overlaps(&new_crtc_state->wm.skl.ddb, > + entries, num_pipes, i)); > + > + entries[i] = new_crtc_state->wm.skl.ddb; > + modeset_pipes &= ~BIT(pipe); > + > + intel_update_crtc(crtc, state, old_crtc_state, new_crtc_state); > + } > + > + WARN_ON(modeset_pipes); > + > /* 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
