| Message ID | 20190624210850.17223-7-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable Transcoder Port Sync feature for tiled displays | expand |
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@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > 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 > + > + 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)) ? 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. - 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. - Also ensure that the slave crtc is marked as updated, and update both entries to correct values in the entries again. You should be able to get the slave crtc with intel_get_crtc_for_pipe(dev_priv, intel_crtc->pipe + 1); 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()
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@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > 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() > >
Op 01-08-2019 om 01:24 schreef Manasi Navare: > 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@ffwll.ch> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Matt Roper <matthew.d.roper@intel.com> >>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >>> --- >>> 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? Instead of 2 separate allocations, only have a single allocation that contains the slave and master ddb during modeset/fastset. This will allow it to be updated as a single crtc. This is useful for modeset enable/disable as a single sequence, and could potentiallybe useful for normal page flips as well to reduce tearing. >>> + >>> + 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? It's not really needed, a minor optimization. If needs_modeset is true, we can be guaranteed that we are always enabling, so the initial DDB allocation is always zero. > >> 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? That's the idea. :) > >> - 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. > Yeah but with the ddb allocations you make sure that we can always use the correct order. Because the master + slave ddb are sequential, if an allocation that encompasses both doesn't overlap, we know for sure we can just do both. :) ~Maarten
On Thu, Aug 01, 2019 at 05:07:48PM +0200, Maarten Lankhorst wrote: > Op 01-08-2019 om 01:24 schreef Manasi Navare: > > 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@ffwll.ch> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Cc: Matt Roper <matthew.d.roper@intel.com> > >>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > >>> --- > >>> 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? > > Instead of 2 separate allocations, only have a single allocation that contains the slave and master > ddb during modeset/fastset. So I will call this master_slave_entries[I915_MAX_PIPES] and have a separate for loop for ddb allocations of the master and slaves that involved in the current modeset correct? if (new_crtc_state->active && needs_modeset() && master_or_slave) Add to master_slave_entries Sounds good? Now this call if (skl_ddb_allocation_overlaps(&cstate->wm.skl.ddb)) will have to be done for if( is_trans_port_sync_mode(cstate)) Now the overlap will check if each non modeset crtc entries overlap with master+slave together, if they do just continue if not then we call all the 4 for loops as is correct? That should fix the allocations issue? updated and entries should then add mask and entries for both master +slave correct? Also the last part of the loop where we wait for a vblank is not clear, do we need that at all? Manasi > > This will allow it to be updated as a single crtc. This is useful for modeset enable/disable as a single sequence, and could > potentiallybe useful for normal page flips as well to reduce tearing. > > >>> + > >>> + 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? > > It's not really needed, a minor optimization. > > If needs_modeset is true, we can be guaranteed that we are always enabling, so the initial DDB allocation is always zero. > > > > >> 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? > That's the idea. :) > > > >> - 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. > > > Yeah but with the ddb allocations you make sure that we can always use the correct order. Because the master + slave ddb are sequential, if an allocation that encompasses both doesn't overlap, we know for sure we can just do both. :) > > ~Maarten >
Hi Maarten,
For this patch, you want me to modify it such that if (slave && needs_modeset) then
dont do anything since the slave update crtc and pipe an dplane updates will happen with master.
So if(master && needs_modeset) {
obtain slaves from slave_mask
obtain corresponding slave crtc state
Now update crtc and link train for slaves first
then master
then upstae planes, pipe_start, pipe_end
}
Is this is the correct logic that addresses your review comments and aligns
with the 8K 2p1p approach?
Manasi
On Thu, Aug 01, 2019 at 05:07:48PM +0200, Maarten Lankhorst wrote:
> Op 01-08-2019 om 01:24 schreef Manasi Navare:
> > 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@ffwll.ch>
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Cc: Matt Roper <matthew.d.roper@intel.com>
> >>> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >>> ---
> >>> 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?
>
> Instead of 2 separate allocations, only have a single allocation that contains the slave and master
> ddb during modeset/fastset.
>
> This will allow it to be updated as a single crtc. This is useful for modeset enable/disable as a single sequence, and could
> potentiallybe useful for normal page flips as well to reduce tearing.
>
> >>> +
> >>> + 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?
>
> It's not really needed, a minor optimization.
>
> If needs_modeset is true, we can be guaranteed that we are always enabling, so the initial DDB allocation is always zero.
>
> >
> >> 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?
> That's the idea. :)
> >
> >> - 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.
> >
> Yeah but with the ddb allocations you make sure that we can always use the correct order. Because the master + slave ddb are sequential, if an allocation that encompasses both doesn't overlap, we know for sure we can just do both. :)
>
> ~Maarten
>
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] = {}; + + 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; + + /* 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); + + /* Separate loop for updating Slave CRTCs that need modeset. + * We need to loop through all slaves of tiled display first and + * follow enable sequence with DP_TP_CTL left Idle until the master + * is ready. + */ + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + bool modeset = needs_modeset(new_crtc_state); + 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; + + update_scanline_offset(pipe_config); + dev_priv->display.crtc_enable(pipe_config, state); + intel_crtc_enable_pipe_crc(intel_crtc); + } + /* Now do the display enable sequence for master CRTC */ + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + bool modeset = needs_modeset(new_crtc_state); + 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_master(pipe_config)) + continue; + + update_scanline_offset(pipe_config); + dev_priv->display.crtc_enable(pipe_config, state); + intel_crtc_enable_pipe_crc(intel_crtc); + } + /* 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); + intel_dp_stop_link_train(intel_dp); + } + /* Set Master'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_master(pipe_config)) + continue; + + /* Wait for 200us before setting the master DP_TP_TCL to Normal */ + usleep_range(200, 400); + 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); + intel_dp_stop_link_train(intel_dp); + } + /* Now do the post crtc enable for all master and slaves */ + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + bool modeset = needs_modeset(new_crtc_state); + struct intel_plane_state *new_plane_state; + 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_mode(pipe_config)) + continue; + + 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); + } /* If 2nd DBuf slice is no more required disable it */ - if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices) + if (required_slices < hw_enabled_slices) icl_dbuf_slices_update(dev_priv, required_slices); } @@ -15926,7 +16137,9 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv) dev_priv->display.fdi_link_train = hsw_fdi_link_train; } - if (INTEL_GEN(dev_priv) >= 9) + if (INTEL_GEN(dev_priv) >= 11) + dev_priv->display.commit_modeset_enables = icl_commit_modeset_enables; + else if (INTEL_GEN(dev_priv) >= 9) dev_priv->display.commit_modeset_enables = skl_commit_modeset_enables; else dev_priv->display.commit_modeset_enables = intel_commit_modeset_enables; diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index 40054fbec82c..29f6c4c91d53 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -28,8 +28,11 @@ #include <drm/drm_util.h> #include <drm/i915_drm.h> +#include "intel_dp_link_training.h" + struct drm_i915_private; struct intel_plane_state; +struct intel_crtc_state; enum i915_gpio { GPIOA, @@ -358,5 +361,6 @@ void lpt_disable_clkout_dp(struct drm_i915_private *dev_priv); u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv, u32 pixel_format, u64 modifier); bool intel_plane_can_remap(const struct intel_plane_state *plane_state); +bool is_trans_port_sync_mode(const struct intel_crtc_state *state); #endif
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@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- 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(-)