On Tue, Dec 17, 2019 at 12:50:31PM +0200, Ville Syrjälä wrote: > On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote: > > On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote: > > > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote: > > > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote: > > > > > On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote: > > > > > > On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote: > > > > > > > In case of tiled displays, all the tiles are linke dto each other > > > > > > > for transcoder port sync. So in intel_atomic_check() we need to make > > > > > > > sure that we add all the tiles to the modeset and if one of the > > > > > > > tiles needs a full modeset then mark all other tiles for a full modeset. > > > > > > > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx> > > > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++ > > > > > > > 1 file changed, 78 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > index 803993a01ca7..7263eaa66cda 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +static int > > > > > > > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv, > > > > > > > + struct intel_atomic_state *state, int tile_grp_id) > > > > > > > +{ > > > > > > > + struct drm_connector *conn_iter; > > > > > > > + struct drm_connector_list_iter conn_list_iter; > > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > > + > > > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > > > > > > > + drm_for_each_connector_iter(conn_iter, &conn_list_iter) { > > > > > > > + struct drm_connector_state *conn_iter_state; > > > > > > > + > > > > > > > + if (!conn_iter->has_tile) > > > > > > > + continue; > > > > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > > > > > > > + conn_iter); > > > > > > > + if (IS_ERR(conn_iter_state)) { > > > > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > > > > + return PTR_ERR(conn_iter_state); > > > > > > > + } > > > > > > > + > > > > > > > + if (!conn_iter_state->crtc) > > > > > > > + continue; > > > > > > > + > > > > > > > + if (conn_iter->tile_group->id != tile_grp_id) > > > > > > > + continue; > > > > > > > + > > > > > > > + crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc); > > > > > > > + if (IS_ERR(crtc_state)) { > > > > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > > > > + return PTR_ERR(conn_iter_state); > > > > > > > + } > > > > > > > + crtc_state->mode_changed = true; > > > > > > > + } > > > > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > +static int > > > > > > > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv, > > > > > > > + struct intel_atomic_state *state) > > > > > > > +{ > > > > > > > + struct drm_connector *connector; > > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > > + struct drm_connector_state *connector_state; > > > > > > > + int i, ret, tile_grp_id = 0; > > > > > > > + > > > > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > > > > + return 0; > > > > > > > + > > > > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > > > > > > > + for_each_new_connector_in_state(&state->base, connector, connector_state, i) { > > > > > > > + if (!connector->has_tile) > > > > > > > + continue; > > > > > > > + if (connector_state->crtc && > > > > > > > + tile_grp_id != connector->tile_group->id) { > > > > > > > + crtc_state = drm_atomic_get_new_crtc_state(&state->base, > > > > > > > + connector_state->crtc); > > > > > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state)) > > > > > > > + continue; > > > > > > > + > > > > > > > + tile_grp_id = connector->tile_group->id; > > > > > > > + } else > > > > > > > + continue; > > > > > > > + > > > > > > > + ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + } > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > > > > > > BTW after some more pondering I don't think this alone is sufficient. > > > > > > The tile information may have already disppeared so I believe we also > > > > > > need to make sure we mark all currently synced crtcs as needing a > > > > > > modeset if any of them need a modeset. And I guess that's pretty much > > > > > > the same function we'll need to handle fastset correctly. > > > > > > > > > > > > > > > > The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call > > > > > So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset > > > > > to true for all synced crtcs if one of them needs modeset? > > > > > > > > I think it should look something like: > > > > > > > > modeset_tiled_things(); > > > > modeset_synced_things(); > > > > > > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called > > > from within modeset_pipe_config just before compute_config() call. > > > > > > > > > > > for_each() { > > > > > > This is the for_each_crtc loop in intel_atomic_check() right? > > > > > > > modeset_pipes_config(); > > > > > > So have icl_add_sync_crtcs outside of modeset_pipe_config()? > > > > > > > if (can_fastset()) { > > > > modeset=false; > > > > fastset=true; > > > > } > > > > } > > > > > > > > modeset_synced_things(); > > > > > > > > for_each() { > > > > if (!modeset && fastset) > > > > copy_state(); > > > > } > > > We already do this in the code right? > > > > > > Manasi > > > > > > > > > > > > > > > > > And why would the tile information be disappeared? > > > > > > > > It'll get updated whenever someone does a getconnector() or whatever. > > > > > > > > Example: > > > > 1. sync pipe A and B, pipe A is master > > > > 2. swap pipe B display for something else > > > > If we disconnect and connect other display for pipe B, port sync mode is off and > > Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile > > Port sync will stay enabled until we do a modeset to disable it. In this > example there is never any modeset on pipe B until we add soemthing to > force one in step 4. seems to be working with the current intel_dp_add_modeset_tiles() since the moment we unplug, it goes through the disable sequence where all the associated tiles get disabled and port sync mode gets disabled. This happens because the Pipe A which is still connected now indicates a mode change since it fallsback to a lower non tiled mode. But to be on safer side, i can check if say Pipe A is a master or slave (in port sync mode) , check its crtc_state which is still showing the old master slave links since we havent cleared those yet and then add all other synced crtcs to the modeset? So the order of calls can still be : intel_atomic_check() { intel_dp_atomic_tiled_check() { modeset_all_tiles modeset_synced_crtcs} intel_modeset_pipe_Config() icl_add_sync_crtcs compute_config fastsetcheck() modeset_synced_things (here it looks at the new master slave assignments) Does this look correct, I want send this patch out today Manasi > > > > > so why we wold need to add both pipes to modeset in this case at all > > > > Manasi > > > > > > 3. getconnector() -> tile info goes poof > > > > 4. do something on pipe A that needs a modeset > > > > no tile info so we miss that pipe B also needs a modeset > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
