Re: [PATCH v3 6/8] drm/i915/display/icl: Enable master-slaves in trans port sync mode in correct order

[Manasi Navare <manasi.d.navare@intel.com>]

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