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@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?
>
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
