Thanks Maarten for your review comments, please see my responses/questions below:
On Tue, Jul 30, 2019 at 12:53:30PM +0200, Maarten Lankhorst wrote:
> Op 24-06-2019 om 23:08 schreef Manasi Navare:
> > As per the display enable sequence, we need to follow the enable sequence
> > for slaves first with DP_TP_CTL set to Idle and configure the transcoder
> > port sync register to select the corersponding master, then follow the
> > enable sequence for master leaving DP_TP_CTL to idle.
> > At this point the transcoder port sync mode is configured and enabled
> > and the Vblanks of both ports are synchronized so then set DP_TP_CTL
> > for the slave and master to Normal and do post crtc enable updates.
> >
> > v2:
> > * Create a icl_update_crtcs hook (Maarten, Danvet)
> > * This sequence only for CRTCs in trans port sync mode (Maarten)
> >
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 3 +-
> > drivers/gpu/drm/i915/display/intel_display.c | 217 ++++++++++++++++++-
> > drivers/gpu/drm/i915/display/intel_display.h | 4 +
> > 3 files changed, 221 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 7925a176f900..bceb7e4b1877 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3154,7 +3154,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > true);
> > intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
> > intel_dp_start_link_train(intel_dp);
> > - if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > + if ((port != PORT_A || INTEL_GEN(dev_priv) >= 9) &&
> > + !is_trans_port_sync_mode(crtc_state))
> > intel_dp_stop_link_train(intel_dp);
> >
> > intel_ddi_enable_fec(encoder, crtc_state);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7156b1b4c6c5..f88d3a929e36 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -520,6 +520,26 @@ needs_modeset(const struct drm_crtc_state *state)
> > return drm_atomic_crtc_needs_modeset(state);
> > }
> >
> > +bool
> > +is_trans_port_sync_mode(const struct intel_crtc_state *state)
> > +{
> > + return (state->master_transcoder != INVALID_TRANSCODER ||
> > + state->sync_mode_slaves_mask);
> > +}
> > +
> > +static bool
> > +is_trans_port_sync_slave(const struct intel_crtc_state *state)
> > +{
> > + return state->master_transcoder != INVALID_TRANSCODER;
> > +}
> > +
> > +static bool
> > +is_trans_port_sync_master(const struct intel_crtc_state *state)
> > +{
> > + return (state->master_transcoder == INVALID_TRANSCODER &&
> > + state->sync_mode_slaves_mask);
> > +}
> > +
> > /*
> > * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> > * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> > @@ -13944,9 +13964,200 @@ static void skl_commit_modeset_enables(struct drm_atomic_state *state)
> > progress = true;
> > }
> > } while (progress);
> > +}
> >
> > +static void icl_commit_modeset_enables(struct drm_atomic_state *state)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(state->dev);
> > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > + struct drm_crtc *crtc;
> > + struct intel_crtc *intel_crtc;
> > + struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > + struct intel_crtc_state *cstate;
> > + unsigned int updated = 0;
> > + bool progress;
> > + enum pipe pipe;
> > + int i;
> > + u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> > + u8 required_slices = intel_state->wm_results.ddb.enabled_slices;
> > + struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> Add old_entries as well, merge master + slave
I didnt understand what you meant by merge master+slaves? You mean add also the
master and slave that are already enabled?
> > +
> > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i)
> > + /* ignore allocations for crtc's that have been turned off. */
> > + if (new_crtc_state->active)
> > + entries[i] = to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
>
> Can be changed to: if (new_crtc_state->active && !needs_modeset(new_crtc_state)) ?
We need !needs_modeset() also? That was not intially there in the original skl_update_crtcs(),
so why do we need it here?
>
> Small refinement to the algorithm in general, I dislike the current handling.
>
> What I would do:
> - Make a new_entries array as well, that contains (for the master crtc, during modeset only) master_ddb.begin + slave_ddb.end,
> and nothing for the slave ddb in that case, the ddb allocation for all other cases including master/slave non-modeset should be the default wm.skl.ddb.
> Use it for comparing instead of the one from crtc_state. This way we know we can always enable both at the same time.
So in the the case of modeset on master or slave, we shd create another entries similar to default one and have the allocations for master + slave and make sure that doesnt overlap with the already active crtc allocations?
> - Ignore the slave crtc when needs_modeset() is true, called from master instead.
> - If a modeset happens on master crtc, do the special enabling dance on both in a separate function.
So you are saying that if it is slave crtc and needs_modeset just skip and dont do anything,
But in case of master crtc modeset, thats where we will need these additional 4 loops and thats where we access the slave crtcs from the slave_sync_mask and then do the correct enabling sequence etc?
> - Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again.
This again I am not 100% clear on how to implement might need to discuss on IRC
>
> You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1);
But the problem is that ther could be multiple slaves not just 1 and IMO accessing the slaves during the master modeset is more complicated
where as the current logic takes care of handling the correct enabling sequence for any number of slaves and master and modesets
in any order.
Will still have to check for proper ddb allocations and ensure no overlap.
Manasi
>
> With these changes you should be able to get rid of the icl special handling, it's confined to a single function for enabling,
> only called when needs_modeset() && is_trans_port_sync_master is true, without the problem of overlapping allocations.
>
>
>
> > + /* If 2nd DBuf slice required, enable it here */
> > + if (required_slices > hw_enabled_slices)
> > + icl_dbuf_slices_update(dev_priv, required_slices);
> > +
> > + /*
> > + * 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
> > + * cause pipe underruns and other bad stuff.
> > + */
> > + do {
> > + progress = false;
> > +
> > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > + bool vbl_wait = false;
> > + unsigned int cmask = drm_crtc_mask(crtc);
> > + bool modeset = needs_modeset(new_crtc_state);
> > +
> > + intel_crtc = to_intel_crtc(crtc);
> > + cstate = to_intel_crtc_state(new_crtc_state);
> > + pipe = intel_crtc->pipe;
> > +
> > + if (updated & cmask || !cstate->base.active)
> > + continue;
> > +
> > + if (modeset && is_trans_port_sync_mode(cstate)) {
> > + DRM_DEBUG_KMS("Pushing the Sync Mode CRTC %d %s that needs update to separate loop\n",
> > + intel_crtc->base.base.id, intel_crtc->base.name);
> > + continue;
> > + }
>
>
>
> > +
> > + if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb,
> > + entries,
> > + INTEL_INFO(dev_priv)->num_pipes, i))
> > + continue;
> > +
> > + updated |= cmask;
> > + entries[i] = cstate->wm.skl.ddb;
> > +
> > + /*
> > + * If this is an already active pipe, it's DDB changed,
> > + * and this isn't the last pipe that needs updating
> > + * then we need to wait for a vblank to pass for the
> > + * new ddb allocation to take effect.
> > + */
> > + if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
> > + &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
> > + !new_crtc_state->active_changed &&
> > + intel_state->wm_results.dirty_pipes != updated)
> > + vbl_wait = true;
> > +
> > + intel_update_crtc(crtc, state, old_crtc_state,
> > + new_crtc_state);
> > +
> > + if (vbl_wait)
> > + intel_wait_for_vblank(dev_priv, pipe);
> > +
> > + progress = true;
> > + }
> > + } while (progress);
> > + /* Set Slave's DP_TP_CTL to Normal */
> > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > + int j;
> > + struct drm_connector_state *conn_state;
> > + struct drm_connector *conn;
> > + bool modeset = needs_modeset(new_crtc_state);
> > + struct intel_dp *intel_dp;
> > + struct intel_crtc_state *pipe_config =
> > + to_intel_crtc_state(new_crtc_state);
> > +
> > + intel_crtc = to_intel_crtc(crtc);
> > +
> > + if (!pipe_config->base.active || !modeset ||
> > + !is_trans_port_sync_slave(pipe_config))
> > + continue;
> > +
> > + for_each_new_connector_in_state(state, conn, conn_state, j) {
> > + if (conn_state->crtc == crtc)
> > + break;
> > + }
> > + intel_dp = enc_to_intel_dp(&intel_attached_encoder(conn)->base);
>
> enc_to_intel_dp(conn_state->best_encoder). :)
>
> Make a small helper here, stop_link_training_for_crtc, call it for slave_crtc, then usleep, then master_crtc in the function described above?
>
>
> > +
> > + new_plane_state =
> > + intel_atomic_get_new_plane_state(to_intel_atomic_state(state),
> > + to_intel_plane(crtc->primary));
> > + if (pipe_config->update_pipe && !pipe_config->enable_fbc)
> > + intel_fbc_disable(intel_crtc);
> > + else if (new_plane_state)
> > + intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> > +
> > + intel_begin_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> > + skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> > + intel_finish_crtc_commit(to_intel_atomic_state(state), intel_crtc);
> > + }
>
> intel_commit_planes()
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
