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

[Maarten Lankhorst]

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 at ffwll.ch>
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>> Signed-off-by: Manasi Navare <manasi.d.navare at 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