| Message ID | 20191219215117.929-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset | expand |
On Thu, Dec 19, 2019 at 01:51:15PM -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. > > v2: > * Change crtc_state scope, remove tile_grp_id (Ville) > * Use intel_connector_needs_modeset() (Ville) > * Add modeset_synced_crtcs (Ville) > * Make sure synced crtcs are forced full modeset > after fastset check (Ville) > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > 1 file changed, 131 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index a3f9430493ae..00608d8cef50 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > new_crtc_state->uapi.mode_changed = false; > new_crtc_state->update_pipe = true; > > - /* > - * If we're not doing the full modeset we want to > - * keep the current M/N values as they may be > - * sufficiently different to the computed values > - * to cause problems. > - * > - * FIXME: should really copy more fuzzy state here > - */ > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > } The check vs. copy should really be a separate patch. Otherwise we risk having to revert the whole thing if something goes wrong. > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > return 0; > } > > +static void > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) I would remove "dp" from the names of all these functions. The fact that we only enable port sync on DP outputs is just an implementation detail we don't have/want to care about in higher level code like this. > +{ > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > + new_crtc_state->uapi.mode_changed = true; > + new_crtc_state->update_pipe = false; > + } > + } > +} > + > +static void > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > +{ > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > + !needs_modeset(new_crtc_state)) > + continue; > + > + intel_dp_modeset_synced_crtcs(state); > + } This is not sufficient for the pre-compute_config() check. The point is that we don't yet necessarily have the synced crtcs in the atomic state so we have to add them. Please have a look in my branch for what I mean exactly. For the check between the fastset check vs. copy this here should work. > +} > + > +static int > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct drm_connector *connector; > + struct drm_connector_list_iter conn_list_iter; 'conn_iter' is the customary name. > + > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > + drm_for_each_connector_iter(connector, &conn_list_iter) { > + struct drm_connector_state *conn_iter_state; 'conn_state' > + struct drm_crtc_state *crtc_state; > + > + if (!(connector->has_tile && > + connector->tile_group->id == tile_grp_id)) > + continue; or maybe !has_tile || tile_grrp != tile_grp I find that form a bit easier on the eyes. But either will work. > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > + connector); > + if (IS_ERR(conn_iter_state)) { > + drm_connector_list_iter_end(&conn_list_iter); > + return PTR_ERR(conn_iter_state); nit: could just do 'ret = PTR_ERR(); break;' and 'int ret=0;' + 'return ret' at the fars ends of the function. > + } > + > + if (!conn_iter_state->crtc) > + 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; Missing drm_atomic_add_affected_planes(). We also need that in the pre-compute_config() check_synced_crtcs(). > + } > + drm_connector_list_iter_end(&conn_list_iter); > + > + return 0; > +} > + > +static int > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct drm_connector *connector; > + struct drm_connector_state *old_conn_state, *new_conn_state; > + int i, ret; > + > + if (INTEL_GEN(dev_priv) < 11) > + return 0; > + > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > + for_each_oldnew_connector_in_state(&state->base, connector, > + old_conn_state, new_conn_state, i) { > + if (!connector->has_tile) > + continue; > + if (!intel_connector_needs_modeset(state, old_conn_state, > + new_conn_state)) > + continue; > + > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > /** > * intel_atomic_check - validate state object > * @dev: drm device > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > if (ret) > goto fail; > > + ret = intel_dp_atomic_check_tiled_conns(state); > + if (ret) > + goto fail; > + > + intel_dp_atomic_check_synced_crtcs(state); > + > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > new_crtc_state, i) { > if (!needs_modeset(new_crtc_state)) { > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > any_ms = true; > } > > + /* > + * In case of port synced crtcs, if one of the synced crtcs > + * needs a full modeset, all other synced crtcs should be > + * forced a full modeset. > + */ > + intel_dp_atomic_check_synced_crtcs(state); > + > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > + new_crtc_state, i) { > + if (needs_modeset(new_crtc_state)) > + continue; > + > + /* > + * If we're not doing the full modeset we want to > + * keep the current M/N values as they may be > + * sufficiently different to the computed values > + * to cause problems. > + * > + * FIXME: should really copy more fuzzy state here > + */ > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; Same comment I gave in Jose's patch: Pls keep this in a separate function. My branch has an example. > + } > + > if (any_ms && !check_digital_port_conflicts(state)) { > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > ret = EINVAL; > -- > 2.19.1
On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote: > On Thu, Dec 19, 2019 at 01:51:15PM -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. > > > > v2: > > * Change crtc_state scope, remove tile_grp_id (Ville) > > * Use intel_connector_needs_modeset() (Ville) > > * Add modeset_synced_crtcs (Ville) > > * Make sure synced crtcs are forced full modeset > > after fastset check (Ville) > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > > 1 file changed, 131 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index a3f9430493ae..00608d8cef50 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > > new_crtc_state->uapi.mode_changed = false; > > new_crtc_state->update_pipe = true; > > > > - /* > > - * If we're not doing the full modeset we want to > > - * keep the current M/N values as they may be > > - * sufficiently different to the computed values > > - * to cause problems. > > - * > > - * FIXME: should really copy more fuzzy state here > > - */ > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > } > > The check vs. copy should really be a separate patch. Otherwise we risk > having to revert the whole thing if something goes wrong. > Yes will sepaarte it out , also I think Jose's series might get merged before mine so.. > > > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > return 0; > > } > > > > +static void > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > > I would remove "dp" from the names of all these functions. The fact > that we only enable port sync on DP outputs is just an implementation > detail we don't have/want to care about in higher level code like this. > Okay will rename > > +{ > > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > > + new_crtc_state->uapi.mode_changed = true; > > + new_crtc_state->update_pipe = false; > > + } > > + } > > +} > > + > > +static void > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > > +{ > > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > > + !needs_modeset(new_crtc_state)) > > + continue; > > + > > + intel_dp_modeset_synced_crtcs(state); > > + } > > This is not sufficient for the pre-compute_config() check. The point is > that we don't yet necessarily have the synced crtcs in the atomic state > so we have to add them. Please have a look in my branch for what I mean > exactly. > But if I use old crtc state or new crtc state here, I do see that since we havent cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn, I do see both conns in trans port sync mode and i can set the mode changed to true for both. I dont understand why what you mean by we would beed to add them? > For the check between the fastset check vs. copy this here should work. > > > +} > > + > > +static int > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct drm_connector *connector; > > + struct drm_connector_list_iter conn_list_iter; > > 'conn_iter' is the customary name. > ok > > + > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > > + drm_for_each_connector_iter(connector, &conn_list_iter) { > > + struct drm_connector_state *conn_iter_state; > > 'conn_state' > ok > > + struct drm_crtc_state *crtc_state; > > + > > + if (!(connector->has_tile && > > + connector->tile_group->id == tile_grp_id)) > > + continue; > > or maybe !has_tile || tile_grrp != tile_grp > > I find that form a bit easier on the eyes. But either will work. > Ok > > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > > + connector); > > + if (IS_ERR(conn_iter_state)) { > > + drm_connector_list_iter_end(&conn_list_iter); > > + return PTR_ERR(conn_iter_state); > > nit: could just do 'ret = PTR_ERR(); break;' > and 'int ret=0;' + 'return ret' at the fars ends of the function. > ok > > + } > > + > > + if (!conn_iter_state->crtc) > > + 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; > > Missing drm_atomic_add_affected_planes(). We also need that in the > pre-compute_config() check_synced_crtcs(). > Manasi Oh we need to call this function along with mode_changed = true? > > + } > > + drm_connector_list_iter_end(&conn_list_iter); > > + > > + return 0; > > +} > > + > > +static int > > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct drm_connector *connector; > > + struct drm_connector_state *old_conn_state, *new_conn_state; > > + int i, ret; > > + > > + if (INTEL_GEN(dev_priv) < 11) > > + return 0; > > + > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > > + for_each_oldnew_connector_in_state(&state->base, connector, > > + old_conn_state, new_conn_state, i) { > > + if (!connector->has_tile) > > + continue; > > + if (!intel_connector_needs_modeset(state, old_conn_state, > > + new_conn_state)) > > + continue; > > + > > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > /** > > * intel_atomic_check - validate state object > > * @dev: drm device > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > > if (ret) > > goto fail; > > > > + ret = intel_dp_atomic_check_tiled_conns(state); > > + if (ret) > > + goto fail; > > + > > + intel_dp_atomic_check_synced_crtcs(state); > > + > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > new_crtc_state, i) { > > if (!needs_modeset(new_crtc_state)) { > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > > any_ms = true; > > } > > > > + /* > > + * In case of port synced crtcs, if one of the synced crtcs > > + * needs a full modeset, all other synced crtcs should be > > + * forced a full modeset. > > + */ > > + intel_dp_atomic_check_synced_crtcs(state); > > + > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > + new_crtc_state, i) { > > + if (needs_modeset(new_crtc_state)) > > + continue; > > + > > + /* > > + * If we're not doing the full modeset we want to > > + * keep the current M/N values as they may be > > + * sufficiently different to the computed values > > + * to cause problems. > > + * > > + * FIXME: should really copy more fuzzy state here > > + */ > > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > Same comment I gave in Jose's patch: Pls keep this in a separate > function. My branch has an example. > > > + } > > + > > if (any_ms && !check_digital_port_conflicts(state)) { > > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > > ret = EINVAL; > > -- > > 2.19.1 > > -- > Ville Syrjälä > Intel
On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote: > On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote: > > On Thu, Dec 19, 2019 at 01:51:15PM -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. > > > > > > v2: > > > * Change crtc_state scope, remove tile_grp_id (Ville) > > > * Use intel_connector_needs_modeset() (Ville) > > > * Add modeset_synced_crtcs (Ville) > > > * Make sure synced crtcs are forced full modeset > > > after fastset check (Ville) > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > > > 1 file changed, 131 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index a3f9430493ae..00608d8cef50 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > > > new_crtc_state->uapi.mode_changed = false; > > > new_crtc_state->update_pipe = true; > > > > > > - /* > > > - * If we're not doing the full modeset we want to > > > - * keep the current M/N values as they may be > > > - * sufficiently different to the computed values > > > - * to cause problems. > > > - * > > > - * FIXME: should really copy more fuzzy state here > > > - */ > > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > } > > > > The check vs. copy should really be a separate patch. Otherwise we risk > > having to revert the whole thing if something goes wrong. > > > > Yes will sepaarte it out , also I think Jose's series might get merged before mine so.. > > > > > > > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > > return 0; > > > } > > > > > > +static void > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > > > > I would remove "dp" from the names of all these functions. The fact > > that we only enable port sync on DP outputs is just an implementation > > detail we don't have/want to care about in higher level code like this. > > > > Okay will rename > > > > +{ > > > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > > > + new_crtc_state->uapi.mode_changed = true; > > > + new_crtc_state->update_pipe = false; > > > + } > > > + } > > > +} > > > + > > > +static void > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > > > +{ > > > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > > > + !needs_modeset(new_crtc_state)) > > > + continue; > > > + > > > + intel_dp_modeset_synced_crtcs(state); > > > + } > > > > This is not sufficient for the pre-compute_config() check. The point is > > that we don't yet necessarily have the synced crtcs in the atomic state > > so we have to add them. Please have a look in my branch for what I mean > > exactly. > > > > But if I use old crtc state or new crtc state here, I do see that since we havent > cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn, > I do see both conns in trans port sync mode and i can set the mode changed to true for both. > > I dont understand why what you mean by we would beed to add them? for_each_new_intel_crtc_in_state() only iterates the crtcs we've already added to the state, not all crtcs. The whole point of the pre-compute_config() check is that we add the ones that were previously synced but weren't yet added to the state. This for the case where the tile info has gone poof so the function that adds stuff based on the tile info will have missed them. > > > For the check between the fastset check vs. copy this here should work. > > > > > +} > > > + > > > +static int > > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > + struct drm_connector *connector; > > > + struct drm_connector_list_iter conn_list_iter; > > > > 'conn_iter' is the customary name. > > > > ok > > > > + > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > > > + drm_for_each_connector_iter(connector, &conn_list_iter) { > > > + struct drm_connector_state *conn_iter_state; > > > > 'conn_state' > > > > ok > > > > + struct drm_crtc_state *crtc_state; > > > + > > > + if (!(connector->has_tile && > > > + connector->tile_group->id == tile_grp_id)) > > > + continue; > > > > or maybe !has_tile || tile_grrp != tile_grp > > > > I find that form a bit easier on the eyes. But either will work. > > > > Ok > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > > > + connector); > > > + if (IS_ERR(conn_iter_state)) { > > > + drm_connector_list_iter_end(&conn_list_iter); > > > + return PTR_ERR(conn_iter_state); > > > > nit: could just do 'ret = PTR_ERR(); break;' > > and 'int ret=0;' + 'return ret' at the fars ends of the function. > > > > ok > > > > + } > > > + > > > + if (!conn_iter_state->crtc) > > > + 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; > > > > Missing drm_atomic_add_affected_planes(). We also need that in the > > pre-compute_config() check_synced_crtcs(). > > > > Manasi > > > Oh we need to call this function along with mode_changed = true? > > > > + } > > > + drm_connector_list_iter_end(&conn_list_iter); > > > + > > > + return 0; > > > +} > > > + > > > +static int > > > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > + struct drm_connector *connector; > > > + struct drm_connector_state *old_conn_state, *new_conn_state; > > > + int i, ret; > > > + > > > + if (INTEL_GEN(dev_priv) < 11) > > > + return 0; > > > + > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > > > + for_each_oldnew_connector_in_state(&state->base, connector, > > > + old_conn_state, new_conn_state, i) { > > > + if (!connector->has_tile) > > > + continue; > > > + if (!intel_connector_needs_modeset(state, old_conn_state, > > > + new_conn_state)) > > > + continue; > > > + > > > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * intel_atomic_check - validate state object > > > * @dev: drm device > > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > > > if (ret) > > > goto fail; > > > > > > + ret = intel_dp_atomic_check_tiled_conns(state); > > > + if (ret) > > > + goto fail; > > > + > > > + intel_dp_atomic_check_synced_crtcs(state); > > > + > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > new_crtc_state, i) { > > > if (!needs_modeset(new_crtc_state)) { > > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > > > any_ms = true; > > > } > > > > > > + /* > > > + * In case of port synced crtcs, if one of the synced crtcs > > > + * needs a full modeset, all other synced crtcs should be > > > + * forced a full modeset. > > > + */ > > > + intel_dp_atomic_check_synced_crtcs(state); > > > + > > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > + new_crtc_state, i) { > > > + if (needs_modeset(new_crtc_state)) > > > + continue; > > > + > > > + /* > > > + * If we're not doing the full modeset we want to > > > + * keep the current M/N values as they may be > > > + * sufficiently different to the computed values > > > + * to cause problems. > > > + * > > > + * FIXME: should really copy more fuzzy state here > > > + */ > > > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > > Same comment I gave in Jose's patch: Pls keep this in a separate > > function. My branch has an example. > > > > > + } > > > + > > > if (any_ms && !check_digital_port_conflicts(state)) { > > > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > > > ret = EINVAL; > > > -- > > > 2.19.1 > > > > -- > > Ville Syrjälä > > Intel
On Fri, Dec 20, 2019 at 06:47:46PM +0200, Ville Syrjälä wrote: > On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote: > > On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote: > > > On Thu, Dec 19, 2019 at 01:51:15PM -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. > > > > > > > > v2: > > > > * Change crtc_state scope, remove tile_grp_id (Ville) > > > > * Use intel_connector_needs_modeset() (Ville) > > > > * Add modeset_synced_crtcs (Ville) > > > > * Make sure synced crtcs are forced full modeset > > > > after fastset check (Ville) > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > > > > 1 file changed, 131 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > index a3f9430493ae..00608d8cef50 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > > > > new_crtc_state->uapi.mode_changed = false; > > > > new_crtc_state->update_pipe = true; > > > > > > > > - /* > > > > - * If we're not doing the full modeset we want to > > > > - * keep the current M/N values as they may be > > > > - * sufficiently different to the computed values > > > > - * to cause problems. > > > > - * > > > > - * FIXME: should really copy more fuzzy state here > > > > - */ > > > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > > } > > > > > > The check vs. copy should really be a separate patch. Otherwise we risk > > > having to revert the whole thing if something goes wrong. > > > > > > > Yes will sepaarte it out , also I think Jose's series might get merged before mine so.. > > > > > > > > > > > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > > > return 0; > > > > } > > > > > > > > +static void > > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > > > > > > I would remove "dp" from the names of all these functions. The fact > > > that we only enable port sync on DP outputs is just an implementation > > > detail we don't have/want to care about in higher level code like this. > > > > > > > Okay will rename > > > > > > +{ > > > > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > > > > + new_crtc_state->uapi.mode_changed = true; > > > > + new_crtc_state->update_pipe = false; > > > > + } > > > > + } > > > > +} > > > > + > > > > +static void > > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > > > > +{ > > > > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > > > > + !needs_modeset(new_crtc_state)) > > > > + continue; > > > > + > > > > + intel_dp_modeset_synced_crtcs(state); > > > > + } > > > > > > This is not sufficient for the pre-compute_config() check. The point is > > > that we don't yet necessarily have the synced crtcs in the atomic state > > > so we have to add them. Please have a look in my branch for what I mean > > > exactly. > > > > > > > But if I use old crtc state or new crtc state here, I do see that since we havent > > cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn, > > I do see both conns in trans port sync mode and i can set the mode changed to true for both. > > > > I dont understand why what you mean by we would beed to add them? > > for_each_new_intel_crtc_in_state() only iterates the crtcs we've already > added to the state, not all crtcs. The whole point of the > pre-compute_config() check is that we add the ones that were previously > synced but weren't yet added to the state. This for the case where the > tile info has gone poof so the function that adds stuff based on the > tile info will have missed them. > But it would have been previously synced and that info would still be in new_crtc_state->master_trans and slve_mask since this is before intel_crtc_prepare_cleared_state() Manasi > > > > > For the check between the fastset check vs. copy this here should work. > > > > > > > +} > > > > + > > > > +static int > > > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > + struct drm_connector *connector; > > > > + struct drm_connector_list_iter conn_list_iter; > > > > > > 'conn_iter' is the customary name. > > > > > > > ok > > > > > > + > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > > > > + drm_for_each_connector_iter(connector, &conn_list_iter) { > > > > + struct drm_connector_state *conn_iter_state; > > > > > > 'conn_state' > > > > > > > ok > > > > > > + struct drm_crtc_state *crtc_state; > > > > + > > > > + if (!(connector->has_tile && > > > > + connector->tile_group->id == tile_grp_id)) > > > > + continue; > > > > > > or maybe !has_tile || tile_grrp != tile_grp > > > > > > I find that form a bit easier on the eyes. But either will work. > > > > > > > Ok > > > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > > > > + connector); > > > > + if (IS_ERR(conn_iter_state)) { > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > + return PTR_ERR(conn_iter_state); > > > > > > nit: could just do 'ret = PTR_ERR(); break;' > > > and 'int ret=0;' + 'return ret' at the fars ends of the function. > > > > > > > ok > > > > > > + } > > > > + > > > > + if (!conn_iter_state->crtc) > > > > + 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; > > > > > > Missing drm_atomic_add_affected_planes(). We also need that in the > > > pre-compute_config() check_synced_crtcs(). > > > > > > > Manasi > > > > > > Oh we need to call this function along with mode_changed = true? > > > > > > + } > > > > + drm_connector_list_iter_end(&conn_list_iter); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int > > > > +intel_dp_atomic_check_tiled_conns(struct intel_atomic_state *state) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > + struct drm_connector *connector; > > > > + struct drm_connector_state *old_conn_state, *new_conn_state; > > > > + int i, ret; > > > > + > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > + return 0; > > > > + > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > > > > + for_each_oldnew_connector_in_state(&state->base, connector, > > > > + old_conn_state, new_conn_state, i) { > > > > + if (!connector->has_tile) > > > > + continue; > > > > + if (!intel_connector_needs_modeset(state, old_conn_state, > > > > + new_conn_state)) > > > > + continue; > > > > + > > > > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /** > > > > * intel_atomic_check - validate state object > > > > * @dev: drm device > > > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > > > > if (ret) > > > > goto fail; > > > > > > > > + ret = intel_dp_atomic_check_tiled_conns(state); > > > > + if (ret) > > > > + goto fail; > > > > + > > > > + intel_dp_atomic_check_synced_crtcs(state); > > > > + > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > > new_crtc_state, i) { > > > > if (!needs_modeset(new_crtc_state)) { > > > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > > > > any_ms = true; > > > > } > > > > > > > > + /* > > > > + * In case of port synced crtcs, if one of the synced crtcs > > > > + * needs a full modeset, all other synced crtcs should be > > > > + * forced a full modeset. > > > > + */ > > > > + intel_dp_atomic_check_synced_crtcs(state); > > > > + > > > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > > + new_crtc_state, i) { > > > > + if (needs_modeset(new_crtc_state)) > > > > + continue; > > > > + > > > > + /* > > > > + * If we're not doing the full modeset we want to > > > > + * keep the current M/N values as they may be > > > > + * sufficiently different to the computed values > > > > + * to cause problems. > > > > + * > > > > + * FIXME: should really copy more fuzzy state here > > > > + */ > > > > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > > > > Same comment I gave in Jose's patch: Pls keep this in a separate > > > function. My branch has an example. > > > > > > > + } > > > > + > > > > if (any_ms && !check_digital_port_conflicts(state)) { > > > > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > > > > ret = EINVAL; > > > > -- > > > > 2.19.1 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Fri, Dec 20, 2019 at 09:04:00AM -0800, Manasi Navare wrote: > On Fri, Dec 20, 2019 at 06:47:46PM +0200, Ville Syrjälä wrote: > > On Fri, Dec 20, 2019 at 08:35:58AM -0800, Manasi Navare wrote: > > > On Fri, Dec 20, 2019 at 03:17:27PM +0200, Ville Syrjälä wrote: > > > > On Thu, Dec 19, 2019 at 01:51:15PM -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. > > > > > > > > > > v2: > > > > > * Change crtc_state scope, remove tile_grp_id (Ville) > > > > > * Use intel_connector_needs_modeset() (Ville) > > > > > * Add modeset_synced_crtcs (Ville) > > > > > * Make sure synced crtcs are forced full modeset > > > > > after fastset check (Ville) > > > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > > > > > 1 file changed, 131 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > index a3f9430493ae..00608d8cef50 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > > > > > new_crtc_state->uapi.mode_changed = false; > > > > > new_crtc_state->update_pipe = true; > > > > > > > > > > - /* > > > > > - * If we're not doing the full modeset we want to > > > > > - * keep the current M/N values as they may be > > > > > - * sufficiently different to the computed values > > > > > - * to cause problems. > > > > > - * > > > > > - * FIXME: should really copy more fuzzy state here > > > > > - */ > > > > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > > > } > > > > > > > > The check vs. copy should really be a separate patch. Otherwise we risk > > > > having to revert the whole thing if something goes wrong. > > > > > > > > > > Yes will sepaarte it out , also I think Jose's series might get merged before mine so.. > > > > > > > > > > > > > > > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > > > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > > > > return 0; > > > > > } > > > > > > > > > > +static void > > > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > > > > > > > > I would remove "dp" from the names of all these functions. The fact > > > > that we only enable port sync on DP outputs is just an implementation > > > > detail we don't have/want to care about in higher level code like this. > > > > > > > > > > Okay will rename > > > > > > > > +{ > > > > > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > > > > > + new_crtc_state->uapi.mode_changed = true; > > > > > + new_crtc_state->update_pipe = false; > > > > > + } > > > > > + } > > > > > +} > > > > > + > > > > > +static void > > > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > > > > > +{ > > > > > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > > > > > + !needs_modeset(new_crtc_state)) > > > > > + continue; > > > > > + > > > > > + intel_dp_modeset_synced_crtcs(state); > > > > > + } > > > > > > > > This is not sufficient for the pre-compute_config() check. The point is > > > > that we don't yet necessarily have the synced crtcs in the atomic state > > > > so we have to add them. Please have a look in my branch for what I mean > > > > exactly. > > > > > > > > > > But if I use old crtc state or new crtc state here, I do see that since we havent > > > cleared the state yet, we do have all synced crtcs in the state , like here after I disconnect 1 conn, > > > I do see both conns in trans port sync mode and i can set the mode changed to true for both. > > > > > > I dont understand why what you mean by we would beed to add them? > > > > for_each_new_intel_crtc_in_state() only iterates the crtcs we've already > > added to the state, not all crtcs. The whole point of the > > pre-compute_config() check is that we add the ones that were previously > > synced but weren't yet added to the state. This for the case where the > > tile info has gone poof so the function that adds stuff based on the > > tile info will have missed them. > > > > But it would have been previously synced and that info would still be in new_crtc_state->master_trans and slve_mask > since this is before intel_crtc_prepare_cleared_state() You don't even have that new_crtc_state yet for the crtcs that were synced previously but whose tile info vanished in the meantime. That's the whole point here.
On Thu, Dec 19, 2019 at 01:51:15PM -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. > > v2: > * Change crtc_state scope, remove tile_grp_id (Ville) > * Use intel_connector_needs_modeset() (Ville) > * Add modeset_synced_crtcs (Ville) > * Make sure synced crtcs are forced full modeset > after fastset check (Ville) > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > 1 file changed, 131 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index a3f9430493ae..00608d8cef50 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > new_crtc_state->uapi.mode_changed = false; > new_crtc_state->update_pipe = true; > > - /* > - * If we're not doing the full modeset we want to > - * keep the current M/N values as they may be > - * sufficiently different to the computed values > - * to cause problems. > - * > - * FIXME: should really copy more fuzzy state here > - */ > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > } > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > return 0; > } > > +static void > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > +{ > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > + new_crtc_state->uapi.mode_changed = true; > + new_crtc_state->update_pipe = false; > + } Upon further thinking this is not quite right even for the post fastset check part. You're not checking at all whether these crtcs here are actually synced to the crtc that still had its needs_modeset() flagged. So this can end up forcing a modeset on an entirely unrelated crtc. We don't want that. You could fix that by passing in a transcoder bitmask, and then only flagging a modeset on crtcs whose cpu_transcoder matches the bitmask. (see intel_modeset_transcoders() in my branch). > + } > +} > + > +static void > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > +{ > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > + !needs_modeset(new_crtc_state)) > + continue; > + > + intel_dp_modeset_synced_crtcs(state); > + } > +} > + > +static int > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct drm_connector *connector; > + struct drm_connector_list_iter conn_list_iter; > + > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > + drm_for_each_connector_iter(connector, &conn_list_iter) { > + struct drm_connector_state *conn_iter_state; > + struct drm_crtc_state *crtc_state; > + > + if (!(connector->has_tile && > + connector->tile_group->id == tile_grp_id)) > + continue; > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > + connector); > + 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; > + > + 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_check_tiled_conns(struct intel_atomic_state *state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct drm_connector *connector; > + struct drm_connector_state *old_conn_state, *new_conn_state; > + int i, ret; > + > + if (INTEL_GEN(dev_priv) < 11) > + return 0; > + > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > + for_each_oldnew_connector_in_state(&state->base, connector, > + old_conn_state, new_conn_state, i) { > + if (!connector->has_tile) > + continue; > + if (!intel_connector_needs_modeset(state, old_conn_state, > + new_conn_state)) > + continue; > + > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > /** > * intel_atomic_check - validate state object > * @dev: drm device > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > if (ret) > goto fail; > > + ret = intel_dp_atomic_check_tiled_conns(state); > + if (ret) > + goto fail; Since this is not sufficient for the pre-compute_config() check I think we should drop this part for now. It's a somewhat unlikely scenario after all so we can do it as a followup. > + > + intel_dp_atomic_check_synced_crtcs(state); > + > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > new_crtc_state, i) { > if (!needs_modeset(new_crtc_state)) { > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > any_ms = true; > } > > + /* > + * In case of port synced crtcs, if one of the synced crtcs > + * needs a full modeset, all other synced crtcs should be > + * forced a full modeset. > + */ > + intel_dp_atomic_check_synced_crtcs(state); > + > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > + new_crtc_state, i) { > + if (needs_modeset(new_crtc_state)) > + continue; > + > + /* > + * If we're not doing the full modeset we want to > + * keep the current M/N values as they may be > + * sufficiently different to the computed values > + * to cause problems. > + * > + * FIXME: should really copy more fuzzy state here > + */ > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; > + } > + > if (any_ms && !check_digital_port_conflicts(state)) { > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > ret = EINVAL; > -- > 2.19.1
On Fri, Dec 20, 2019 at 08:41:12PM +0200, Ville Syrjälä wrote: > On Thu, Dec 19, 2019 at 01:51:15PM -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. > > > > v2: > > * Change crtc_state scope, remove tile_grp_id (Ville) > > * Use intel_connector_needs_modeset() (Ville) > > * Add modeset_synced_crtcs (Ville) > > * Make sure synced crtcs are forced full modeset > > after fastset check (Ville) > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > > 1 file changed, 131 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index a3f9430493ae..00608d8cef50 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > > new_crtc_state->uapi.mode_changed = false; > > new_crtc_state->update_pipe = true; > > > > - /* > > - * If we're not doing the full modeset we want to > > - * keep the current M/N values as they may be > > - * sufficiently different to the computed values > > - * to cause problems. > > - * > > - * FIXME: should really copy more fuzzy state here > > - */ > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > } > > > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > return 0; > > } > > > > +static void > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > > +{ > > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > > + new_crtc_state->uapi.mode_changed = true; > > + new_crtc_state->update_pipe = false; > > + } > > Upon further thinking this is not quite right even for the post fastset > check part. You're not checking at all whether these crtcs here are > actually synced to the crtc that still had its needs_modeset() flagged. > So this can end up forcing a modeset on an entirely unrelated crtc. We > don't want that. > > You could fix that by passing in a transcoder bitmask, and then only > flagging a modeset on crtcs whose cpu_transcoder matches the bitmask. > (see intel_modeset_transcoders() in my branch). > > > + } > > +} > > + > > +static void > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > > +{ > > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > > + !needs_modeset(new_crtc_state)) > > + continue; > > + > > + intel_dp_modeset_synced_crtcs(state); I guess in this call I can add master trans and sync_mode_slaves_bitmask And then if the crtcs in state are in sync mode and their cpu_trans matches the master trans or transcoders in the slave mask, add them to the modeset? > > + } > > +} > > + > > +static int > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct drm_connector *connector; > > + struct drm_connector_list_iter conn_list_iter; > > + > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > > + drm_for_each_connector_iter(connector, &conn_list_iter) { > > + struct drm_connector_state *conn_iter_state; > > + struct drm_crtc_state *crtc_state; > > + > > + if (!(connector->has_tile && > > + connector->tile_group->id == tile_grp_id)) > > + continue; > > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > > + connector); > > + 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; > > + > > + 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_check_tiled_conns(struct intel_atomic_state *state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct drm_connector *connector; > > + struct drm_connector_state *old_conn_state, *new_conn_state; > > + int i, ret; > > + > > + if (INTEL_GEN(dev_priv) < 11) > > + return 0; > > + > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > > + for_each_oldnew_connector_in_state(&state->base, connector, > > + old_conn_state, new_conn_state, i) { > > + if (!connector->has_tile) > > + continue; > > + if (!intel_connector_needs_modeset(state, old_conn_state, > > + new_conn_state)) > > + continue; > > + > > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > /** > > * intel_atomic_check - validate state object > > * @dev: drm device > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > > if (ret) > > goto fail; > > > > + ret = intel_dp_atomic_check_tiled_conns(state); > > + if (ret) > > + goto fail; > > Since this is not sufficient for the pre-compute_config() check I think > we should drop this part for now. It's a somewhat unlikely scenario > after all so we can do it as a followup. > Well we do need this for handling the hotplug cases correctly so that the connector disconnected are added to the modeset which is working well with this function. Manasi > > + > > + intel_dp_atomic_check_synced_crtcs(state); > > + > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > new_crtc_state, i) { > > if (!needs_modeset(new_crtc_state)) { > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > > any_ms = true; > > } > > > > + /* > > + * In case of port synced crtcs, if one of the synced crtcs > > + * needs a full modeset, all other synced crtcs should be > > + * forced a full modeset. > > + */ > > + intel_dp_atomic_check_synced_crtcs(state); > > + > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > + new_crtc_state, i) { > > + if (needs_modeset(new_crtc_state)) > > + continue; > > + > > + /* > > + * If we're not doing the full modeset we want to > > + * keep the current M/N values as they may be > > + * sufficiently different to the computed values > > + * to cause problems. > > + * > > + * FIXME: should really copy more fuzzy state here > > + */ > > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > + } > > + > > if (any_ms && !check_digital_port_conflicts(state)) { > > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > > ret = EINVAL; > > -- > > 2.19.1 > > -- > Ville Syrjälä > Intel
On Fri, Dec 20, 2019 at 11:05:31AM -0800, Manasi Navare wrote: > On Fri, Dec 20, 2019 at 08:41:12PM +0200, Ville Syrjälä wrote: > > On Thu, Dec 19, 2019 at 01:51:15PM -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. > > > > > > v2: > > > * Change crtc_state scope, remove tile_grp_id (Ville) > > > * Use intel_connector_needs_modeset() (Ville) > > > * Add modeset_synced_crtcs (Ville) > > > * Make sure synced crtcs are forced full modeset > > > after fastset check (Ville) > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > > > 1 file changed, 131 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index a3f9430493ae..00608d8cef50 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > > > new_crtc_state->uapi.mode_changed = false; > > > new_crtc_state->update_pipe = true; > > > > > > - /* > > > - * If we're not doing the full modeset we want to > > > - * keep the current M/N values as they may be > > > - * sufficiently different to the computed values > > > - * to cause problems. > > > - * > > > - * FIXME: should really copy more fuzzy state here > > > - */ > > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > } > > > > > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > > return 0; > > > } > > > > > > +static void > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > > > +{ > > > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > > > + new_crtc_state->uapi.mode_changed = true; > > > + new_crtc_state->update_pipe = false; > > > + } > > > > Upon further thinking this is not quite right even for the post fastset > > check part. You're not checking at all whether these crtcs here are > > actually synced to the crtc that still had its needs_modeset() flagged. > > So this can end up forcing a modeset on an entirely unrelated crtc. We > > don't want that. > > > > You could fix that by passing in a transcoder bitmask, and then only > > flagging a modeset on crtcs whose cpu_transcoder matches the bitmask. > > (see intel_modeset_transcoders() in my branch). > > > > > + } > > > +} > > > + > > > +static void > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > > > +{ > > > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > > > + !needs_modeset(new_crtc_state)) > > > + continue; > > > + > > > + intel_dp_modeset_synced_crtcs(state); > > I guess in this call I can add master trans and sync_mode_slaves_bitmask > > And then if the crtcs in state are in sync mode and their cpu_trans matches the master trans > or transcoders in the slave mask, add them to the modeset? It can be as simple as: modeset_transcoders(state, transcoders) { for_each_crtc_in() { if (transcoders & BIT(cpu_transcoder)) modeset = true; } } intel_dp_atomic_check_synced_crtcs() { for_each_crtc_in() { if (is_port_sync_maseter() && needs_modeset()) modeset_transcoders(state, slave_mask); else if (is_port_sync_slave() && needs_modeset()) modeset_transcoders(state, BIT(master_transcoder)); } } > > > > > > + } > > > +} > > > + > > > +static int > > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > + struct drm_connector *connector; > > > + struct drm_connector_list_iter conn_list_iter; > > > + > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > > > + drm_for_each_connector_iter(connector, &conn_list_iter) { > > > + struct drm_connector_state *conn_iter_state; > > > + struct drm_crtc_state *crtc_state; > > > + > > > + if (!(connector->has_tile && > > > + connector->tile_group->id == tile_grp_id)) > > > + continue; > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > > > + connector); > > > + 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; > > > + > > > + 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_check_tiled_conns(struct intel_atomic_state *state) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > + struct drm_connector *connector; > > > + struct drm_connector_state *old_conn_state, *new_conn_state; > > > + int i, ret; > > > + > > > + if (INTEL_GEN(dev_priv) < 11) > > > + return 0; > > > + > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > > > + for_each_oldnew_connector_in_state(&state->base, connector, > > > + old_conn_state, new_conn_state, i) { > > > + if (!connector->has_tile) > > > + continue; > > > + if (!intel_connector_needs_modeset(state, old_conn_state, > > > + new_conn_state)) > > > + continue; > > > + > > > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * intel_atomic_check - validate state object > > > * @dev: drm device > > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > > > if (ret) > > > goto fail; > > > > > > + ret = intel_dp_atomic_check_tiled_conns(state); > > > + if (ret) > > > + goto fail; > > > > Since this is not sufficient for the pre-compute_config() check I think > > we should drop this part for now. It's a somewhat unlikely scenario > > after all so we can do it as a followup. > > > > Well we do need this for handling the hotplug cases correctly so that the connector disconnected > are added to the modeset which is working well with this function.a Did I reply here? > > Manasi > > > > + > > > + intel_dp_atomic_check_synced_crtcs(state); I meant to reply here. Ie. we should keep intel_dp_atomic_check_tiled_conns() but drop the intel_dp_atomic_check_synced_crtcs() call here (we will want to keep the other callsite after the fastset check). > > > + > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > new_crtc_state, i) { > > > if (!needs_modeset(new_crtc_state)) { > > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > > > any_ms = true; > > > } > > > > > > + /* > > > + * In case of port synced crtcs, if one of the synced crtcs > > > + * needs a full modeset, all other synced crtcs should be > > > + * forced a full modeset. > > > + */ > > > + intel_dp_atomic_check_synced_crtcs(state); > > > + > > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > + new_crtc_state, i) { > > > + if (needs_modeset(new_crtc_state)) > > > + continue; > > > + > > > + /* > > > + * If we're not doing the full modeset we want to > > > + * keep the current M/N values as they may be > > > + * sufficiently different to the computed values > > > + * to cause problems. > > > + * > > > + * FIXME: should really copy more fuzzy state here > > > + */ > > > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > + } > > > + > > > if (any_ms && !check_digital_port_conflicts(state)) { > > > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > > > ret = EINVAL; > > > -- > > > 2.19.1 > > > > -- > > Ville Syrjälä > > Intel
On Fri, Dec 20, 2019 at 09:35:43PM +0200, Ville Syrjälä wrote: > On Fri, Dec 20, 2019 at 11:05:31AM -0800, Manasi Navare wrote: > > On Fri, Dec 20, 2019 at 08:41:12PM +0200, Ville Syrjälä wrote: > > > On Thu, Dec 19, 2019 at 01:51:15PM -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. > > > > > > > > v2: > > > > * Change crtc_state scope, remove tile_grp_id (Ville) > > > > * Use intel_connector_needs_modeset() (Ville) > > > > * Add modeset_synced_crtcs (Ville) > > > > * Make sure synced crtcs are forced full modeset > > > > after fastset check (Ville) > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- > > > > 1 file changed, 131 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > index a3f9430493ae..00608d8cef50 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta > > > > new_crtc_state->uapi.mode_changed = false; > > > > new_crtc_state->update_pipe = true; > > > > > > > > - /* > > > > - * If we're not doing the full modeset we want to > > > > - * keep the current M/N values as they may be > > > > - * sufficiently different to the computed values > > > > - * to cause problems. > > > > - * > > > > - * FIXME: should really copy more fuzzy state here > > > > - */ > > > > - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > > - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > > - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > > - new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > > } > > > > > > > > static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, > > > > @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) > > > > return 0; > > > > } > > > > > > > > +static void > > > > +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) > > > > +{ > > > > + 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 (is_trans_port_sync_mode(new_crtc_state)) { > > > > + new_crtc_state->uapi.mode_changed = true; > > > > + new_crtc_state->update_pipe = false; > > > > + } > > > > > > Upon further thinking this is not quite right even for the post fastset > > > check part. You're not checking at all whether these crtcs here are > > > actually synced to the crtc that still had its needs_modeset() flagged. > > > So this can end up forcing a modeset on an entirely unrelated crtc. We > > > don't want that. > > > > > > You could fix that by passing in a transcoder bitmask, and then only > > > flagging a modeset on crtcs whose cpu_transcoder matches the bitmask. > > > (see intel_modeset_transcoders() in my branch). > > > > > > > + } > > > > +} > > > > + > > > > +static void > > > > +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) > > > > +{ > > > > + 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 (!is_trans_port_sync_mode(new_crtc_state) || > > > > + !needs_modeset(new_crtc_state)) > > > > + continue; > > > > + > > > > + intel_dp_modeset_synced_crtcs(state); > > > > I guess in this call I can add master trans and sync_mode_slaves_bitmask > > > > And then if the crtcs in state are in sync mode and their cpu_trans matches the master trans > > or transcoders in the slave mask, add them to the modeset? > > It can be as simple as: > > modeset_transcoders(state, transcoders) > { > for_each_crtc_in() { > if (transcoders & BIT(cpu_transcoder)) > modeset = true; > } > } > > intel_dp_atomic_check_synced_crtcs() > { > for_each_crtc_in() { > if (is_port_sync_maseter() && needs_modeset()) > modeset_transcoders(state, slave_mask); > else if (is_port_sync_slave() && needs_modeset()) > modeset_transcoders(state, BIT(master_transcoder)); > } > } > > > > > > > > > > > + } > > > > +} > > > > + > > > > +static int > > > > +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > + struct drm_connector *connector; > > > > + struct drm_connector_list_iter conn_list_iter; > > > > + > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); > > > > + drm_for_each_connector_iter(connector, &conn_list_iter) { > > > > + struct drm_connector_state *conn_iter_state; > > > > + struct drm_crtc_state *crtc_state; > > > > + > > > > + if (!(connector->has_tile && > > > > + connector->tile_group->id == tile_grp_id)) > > > > + continue; > > > > + conn_iter_state = drm_atomic_get_connector_state(&state->base, > > > > + connector); > > > > + 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; > > > > + > > > > + 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_check_tiled_conns(struct intel_atomic_state *state) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > + struct drm_connector *connector; > > > > + struct drm_connector_state *old_conn_state, *new_conn_state; > > > > + int i, ret; > > > > + > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > + return 0; > > > > + > > > > + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ > > > > + for_each_oldnew_connector_in_state(&state->base, connector, > > > > + old_conn_state, new_conn_state, i) { > > > > + if (!connector->has_tile) > > > > + continue; > > > > + if (!intel_connector_needs_modeset(state, old_conn_state, > > > > + new_conn_state)) > > > > + continue; > > > > + > > > > + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /** > > > > * intel_atomic_check - validate state object > > > > * @dev: drm device > > > > @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, > > > > if (ret) > > > > goto fail; > > > > > > > > + ret = intel_dp_atomic_check_tiled_conns(state); > > > > + if (ret) > > > > + goto fail; > > > > > > Since this is not sufficient for the pre-compute_config() check I think > > > we should drop this part for now. It's a somewhat unlikely scenario > > > after all so we can do it as a followup. > > > > > > > Well we do need this for handling the hotplug cases correctly so that the connector disconnected > > are added to the modeset which is working well with this function.a > > Did I reply here? > > > > > Manasi > > > > > > + > > > > + intel_dp_atomic_check_synced_crtcs(state); > > I meant to reply here. Ie. we should keep intel_dp_atomic_check_tiled_conns() > but drop the intel_dp_atomic_check_synced_crtcs() call here (we will want to > keep the other callsite after the fastset check). > Yes got it thank you so much, I will now resubmit this with these fixes just in a bit Manasi > > > > + > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > > new_crtc_state, i) { > > > > if (!needs_modeset(new_crtc_state)) { > > > > @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, > > > > any_ms = true; > > > > } > > > > > > > > + /* > > > > + * In case of port synced crtcs, if one of the synced crtcs > > > > + * needs a full modeset, all other synced crtcs should be > > > > + * forced a full modeset. > > > > + */ > > > > + intel_dp_atomic_check_synced_crtcs(state); > > > > + > > > > + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > > + new_crtc_state, i) { > > > > + if (needs_modeset(new_crtc_state)) > > > > + continue; > > > > + > > > > + /* > > > > + * If we're not doing the full modeset we want to > > > > + * keep the current M/N values as they may be > > > > + * sufficiently different to the computed values > > > > + * to cause problems. > > > > + * > > > > + * FIXME: should really copy more fuzzy state here > > > > + */ > > > > + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; > > > > + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; > > > > + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; > > > > + new_crtc_state->has_drrs = old_crtc_state->has_drrs; > > > > + } > > > > + > > > > if (any_ms && !check_digital_port_conflicts(state)) { > > > > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > > > > ret = EINVAL; > > > > -- > > > > 2.19.1 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index a3f9430493ae..00608d8cef50 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -13910,18 +13910,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta new_crtc_state->uapi.mode_changed = false; new_crtc_state->update_pipe = true; - /* - * If we're not doing the full modeset we want to - * keep the current M/N values as they may be - * sufficiently different to the computed values - * to cause problems. - * - * FIXME: should really copy more fuzzy state here - */ - new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; - new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; - new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; - new_crtc_state->has_drrs = old_crtc_state->has_drrs; } static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state, @@ -14032,6 +14020,105 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state) return 0; } +static void +intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state) +{ + 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 (is_trans_port_sync_mode(new_crtc_state)) { + new_crtc_state->uapi.mode_changed = true; + new_crtc_state->update_pipe = false; + } + } +} + +static void +intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state) +{ + 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 (!is_trans_port_sync_mode(new_crtc_state) || + !needs_modeset(new_crtc_state)) + continue; + + intel_dp_modeset_synced_crtcs(state); + } +} + +static int +intel_dp_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + struct drm_connector *connector; + struct drm_connector_list_iter conn_list_iter; + + drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter); + drm_for_each_connector_iter(connector, &conn_list_iter) { + struct drm_connector_state *conn_iter_state; + struct drm_crtc_state *crtc_state; + + if (!(connector->has_tile && + connector->tile_group->id == tile_grp_id)) + continue; + conn_iter_state = drm_atomic_get_connector_state(&state->base, + connector); + 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; + + 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_check_tiled_conns(struct intel_atomic_state *state) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + struct drm_connector *connector; + struct drm_connector_state *old_conn_state, *new_conn_state; + int i, ret; + + if (INTEL_GEN(dev_priv) < 11) + return 0; + + /* Is tiled, mark all other tiled CRTCs as needing a modeset */ + for_each_oldnew_connector_in_state(&state->base, connector, + old_conn_state, new_conn_state, i) { + if (!connector->has_tile) + continue; + if (!intel_connector_needs_modeset(state, old_conn_state, + new_conn_state)) + continue; + + ret = intel_dp_modeset_all_tiles(state, connector->tile_group->id); + if (ret) + return ret; + } + + return 0; +} + /** * intel_atomic_check - validate state object * @dev: drm device @@ -14059,6 +14146,12 @@ static int intel_atomic_check(struct drm_device *dev, if (ret) goto fail; + ret = intel_dp_atomic_check_tiled_conns(state); + if (ret) + goto fail; + + intel_dp_atomic_check_synced_crtcs(state); + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { if (!needs_modeset(new_crtc_state)) { @@ -14089,6 +14182,32 @@ static int intel_atomic_check(struct drm_device *dev, any_ms = true; } + /* + * In case of port synced crtcs, if one of the synced crtcs + * needs a full modeset, all other synced crtcs should be + * forced a full modeset. + */ + intel_dp_atomic_check_synced_crtcs(state); + + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, + new_crtc_state, i) { + if (needs_modeset(new_crtc_state)) + continue; + + /* + * If we're not doing the full modeset we want to + * keep the current M/N values as they may be + * sufficiently different to the computed values + * to cause problems. + * + * FIXME: should really copy more fuzzy state here + */ + new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n; + new_crtc_state->dp_m_n = old_crtc_state->dp_m_n; + new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2; + new_crtc_state->has_drrs = old_crtc_state->has_drrs; + } + if (any_ms && !check_digital_port_conflicts(state)) { DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); ret = EINVAL;
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. v2: * Change crtc_state scope, remove tile_grp_id (Ville) * Use intel_connector_needs_modeset() (Ville) * Add modeset_synced_crtcs (Ville) * Make sure synced crtcs are forced full modeset after fastset check (Ville) Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5 Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 143 +++++++++++++++++-- 1 file changed, 131 insertions(+), 12 deletions(-)