On Thu, 2020-01-16 at 12:56 +0200, Ville Syrjälä wrote:
> On Thu, Jan 16, 2020 at 12:05:43AM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-15 at 21:08 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > The post-fastset "does anyone still need a full modeset?" for
> > > port sync looks busted. The outer loop bails out of a full
> > > modeset
> > > is still needed by the current crtc which, and then we skip
> > > forcing
> > > a full modeset on the related crtcs. That's totally the opposite
> > > of what we want.
> >
> > Ops yeah, it is always doing a full modeset.
> >
> > > The MST path has the logic mostly the other way around so it
> > > looks correct. To fix the port sync case let's follow the MST
> > > logic for both. So, if the current crtc already needs a modeset
> > > we do nothing. otherwise we check if any of the related crtcs
> > > needs a modeset, and if so we force a full modeset for the
> > > current crtc.
> > >
> > > And while at let's change the else if to a plain if to so
> > > we don't have needless coupling between the MST and port sync
> > > checks.
> > >
> > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > Fixes: 05a8e45136ca ("drm/i915/display: Use external dependency
> > > loop
> > > for port sync")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++----
> > > ----
> > > ----
> > > 1 file changed, 17 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index dd03987cc24f..b397816ce253 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14469,37 +14469,23 @@ static int
> > > intel_atomic_check_crtcs(struct
> > > intel_atomic_state *state)
> > > return 0;
> > > }
> > >
> > > -static bool intel_cpu_transcoder_needs_modeset(struct
> > > intel_atomic_state *state,
> > > - enum transcoder
> > > transcoder)
> > > +static bool intel_cpu_transcoders_need_modeset(struct
> > > intel_atomic_state *state,
> > > + u8 transcoders)
> > > {
> > > - struct intel_crtc_state *new_crtc_state;
> > > + const struct intel_crtc_state *new_crtc_state;
> > > struct intel_crtc *crtc;
> > > int i;
> > >
> > > - for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i)
> > > - if (new_crtc_state->cpu_transcoder == transcoder)
> > > - return needs_modeset(new_crtc_state);
> > > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > > i) {
> > > + if (new_crtc_state->hw.enable &&
> >
> > The intel_pipe_config_compare() will force a modeset for MST and
> > port
> > sync but this hw.enable could skip modeset in future features when
> > one
> > of the pipes are going from enabled to disabled.
>
> We can't trust what's in .cpu_transcoder when enable==false. It can
> be some stale junk left over from the previous state. Well, until
> the next patch at least.
>
> If the disabled pipe was the master then the mst/port sync master
> transcoder will change and the fastset check won't pass on any of the
> slaves. So the enable check is fine for that case.
>
> If the disabled pipe was one of the port sync slaves the relevant
> bitmask will change for the master, again making the fastset check
> fail. And thus this code here will force a modeset for all the
> other slaves.
>
> So can't see how having the check could be a problem.
I know this will not affect MST and port sync, I was thinking in future
users of this function but okay, we can leave that to be worked when
such users shows up.
Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
>
> > Removing it:
> >
> > Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> >
> > > + transcoders & BIT(new_crtc_state->cpu_transcoder)
> > > &&
> > > + needs_modeset(new_crtc_state))
> > > + return true;
> > > + }
> > >
> > > return false;
> > > }
> > >
> > > -static void
> > > -intel_modeset_synced_crtcs(struct intel_atomic_state *state,
> > > - u8 transcoders)
> > > -{
> > > - struct intel_crtc_state *new_crtc_state;
> > > - struct intel_crtc *crtc;
> > > - int i;
> > > -
> > > - for_each_new_intel_crtc_in_state(state, crtc,
> > > - new_crtc_state, i) {
> > > - if (transcoders & BIT(new_crtc_state->cpu_transcoder))
> > > {
> > > - new_crtc_state->uapi.mode_changed = true;
> > > - new_crtc_state->update_pipe = false;
> > > - }
> > > - }
> > > -}
> > > -
> > > static int
> > > intel_modeset_all_tiles(struct intel_atomic_state *state, int
> > > tile_grp_id)
> > > {
> > > @@ -14655,15 +14641,20 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > > if (intel_dp_mst_is_slave_trans(new_crtc_state)) {
> > > enum transcoder master = new_crtc_state-
> > > > mst_master_transcoder;
> > >
> > > - if (intel_cpu_transcoder_needs_modeset(state,
> > > master)) {
> > > + if (intel_cpu_transcoders_need_modeset(state,
> > > BIT(master))) {
> > > new_crtc_state->uapi.mode_changed =
> > > true;
> > > new_crtc_state->update_pipe = false;
> > > }
> > > - } else if (is_trans_port_sync_mode(new_crtc_state)) {
> > > + }
> > > +
> > > + if (is_trans_port_sync_mode(new_crtc_state)) {
> > > u8 trans = new_crtc_state-
> > > > sync_mode_slaves_mask |
> > > BIT(new_crtc_state-
> > > > master_transcoder);
> > >
> > > - intel_modeset_synced_crtcs(state, trans);
> > > + if (intel_cpu_transcoders_need_modeset(state,
> > > trans)) {
> > > + new_crtc_state->uapi.mode_changed =
> > > true;
> > > + new_crtc_state->update_pipe = false;
> > > + }
> > > }
> > > }
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
