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
