| Message ID | 20200131171547.25938-3-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Introduce encoder->compute_config_late() | expand |
On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote: > If one of the synced crtcs needs a full modeset, we need > to make sure all the synced crtcs are forced a full > modeset. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 88 +------------ > drivers/gpu/drm/i915/display/intel_dp.c | 131 ++++++++++++++++++- > 2 files changed, 131 insertions(+), 88 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index e638543f5f87..709a737638b6 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config) > struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev); > struct drm_connector *connector; > struct drm_connector_state *connector_state; > - int base_bpp, ret; > - int i, tile_group_id = -1, num_tiled_conns = 0; > + int base_bpp, ret, i; > bool retry = true; > > pipe_config->cpu_transcoder = > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state, > return false; > } > > -static int > -intel_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_iter; > - int ret = 0; > - > - drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > - drm_for_each_connector_iter(connector, &conn_iter) { > - struct drm_connector_state *conn_state; > - struct drm_crtc_state *crtc_state; > - > - if (!connector->has_tile || > - connector->tile_group->id != tile_grp_id) > - continue; > - conn_state = drm_atomic_get_connector_state(&state->base, > - connector); > - if (IS_ERR(conn_state)) { > - ret = PTR_ERR(conn_state); > - break; > - } > - > - if (!conn_state->crtc) > - continue; > - > - crtc_state = drm_atomic_get_crtc_state(&state->base, > - conn_state->crtc); > - if (IS_ERR(crtc_state)) { > - ret = PTR_ERR(crtc_state); > - break; > - } > - crtc_state->mode_changed = true; > - ret = drm_atomic_add_affected_connectors(&state->base, > - conn_state->crtc); > - if (ret) > - break; > - } > - drm_connector_list_iter_end(&conn_iter); > - > - return ret; > -} > - > -static int > -intel_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, connector)) > - continue; > - > - ret = intel_modeset_all_tiles(state, connector->tile_group->id); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > /** > * intel_atomic_check - validate state object > * @dev: drm device > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev, > if (ret) > goto fail; > > - /** > - * This check adds all the connectors in current state that belong to > - * the same tile group to a full modeset. > - * This function directly sets the mode_changed to true and we also call > - * drm_atomic_add_affected_connectors(). Hence we are not explicitly > - * calling drm_atomic_helper_check_modeset() after this. > - * > - * Fixme: Handle some corner cases where one of the > - * tiled connectors gets disconnected and tile info is lost but since it > - * was previously synced to other conn, we need to add that to the modeset. > - */ > - ret = intel_atomic_check_tiled_conns(state); > - if (ret) > - goto fail; > - > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > new_crtc_state, i) { > if (!needs_modeset(new_crtc_state)) { > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index f4dede6253f8..7eb4b3dbbcb3 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > } > } > > +static int intel_modeset_tile_group(struct intel_atomic_state *state, > + int tile_group_id) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct drm_connector_list_iter conn_iter; > + struct drm_connector *connector; > + int ret = 0; > + > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + struct drm_connector_state *conn_state; > + struct intel_crtc_state *crtc_state; > + struct intel_crtc *crtc; > + > + if (!connector->has_tile || > + connector->tile_group->id != tile_group_id) > + continue; > + > + conn_state = drm_atomic_get_connector_state(&state->base, > + connector); > + if (IS_ERR(conn_state)) { > + ret = PTR_ERR(conn_state); > + break; > + } > + > + crtc = to_intel_crtc(conn_state->crtc); > + > + if (!crtc) > + continue; > + > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > + if (IS_ERR(crtc_state)) { > + ret = PTR_ERR(crtc_state); > + break; > + } > + > + crtc_state->uapi.mode_changed = true; > + > + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base); > + if (ret) > + break; > + } > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > + > + return ret; > +} > + > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct intel_crtc *crtc; > + > + if (transcoders == 0) > + return 0; > + > + for_each_intel_crtc(&dev_priv->drm, crtc) { > + struct intel_crtc_state *crtc_state; > + int ret; > + > + if ((transcoders & BIT(crtc->pipe)) == 0) > + continue; Dropping the EDP transcoder on the floor here. I think we should just do the guaranteed correct thing and look at the cpu_transcoder instead. Yes, that does mean we more or less end up adding all crtcs to the state whenever modesetting any synced crtc, but so be it. We can think of ways to optimize that later. > + > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + > + if (!crtc_state->hw.enable) > + continue; > + > + crtc_state->uapi.mode_changed = true; > + > + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base); > + if (ret) > + return ret; Missing add_affected_planes() here I think. Or was that guaranteed to be done by the helper? Can't recall. > + > + WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder); > + > + transcoders &= ~BIT(crtc_state->cpu_transcoder); > + } > + > + WARN_ON(transcoders != 0); > + > + return 0; > + > +} > + > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state, > + struct drm_connector *connector) > +{ > + const struct drm_connector_state *old_conn_state = > + drm_atomic_get_old_connector_state(&state->base, connector); > + const struct intel_crtc_state *old_crtc_state; > + struct intel_crtc *crtc; > + > + crtc = to_intel_crtc(old_conn_state->crtc); > + if (!crtc) > + return 0; > + > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > + > + if (!old_crtc_state->hw.active) > + return 0; > + > + return intel_modeset_affected_transcoders(state, > + (old_crtc_state->sync_mode_slaves_mask | > + BIT(old_crtc_state->master_transcoder)) & This seems to have the same master==INVALID problem that we faced elsewhere already. > + ~BIT(old_crtc_state->cpu_transcoder)); I guess this part is redundant. Or can we somehow have our own transcoder be included in sync_mode_slaves_mask/master_transcoder? > +} > + > +static int intel_dp_connector_atomic_check(struct drm_connector *conn, > + struct drm_atomic_state *_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(conn->dev); > + struct intel_atomic_state *state = to_intel_atomic_state(_state); > + int ret; > + > + ret = intel_digital_connector_atomic_check(conn, &state->base); > + if (ret) > + return ret; > + > + if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) { > + ret = intel_modeset_tile_group(state, conn->tile_group->id); > + if (ret) > + return ret; > + } > + > + return intel_modeset_synced_crtcs(state, conn); No gen check... Ah, yeah we don't need it because the port sync state will be INVALID/0. > +} > + > static const struct drm_connector_funcs intel_dp_connector_funcs = { > .force = intel_dp_force, > .fill_modes = drm_helper_probe_single_connector_modes, > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = > .detect_ctx = intel_dp_detect, > .get_modes = intel_dp_get_modes, > .mode_valid = intel_dp_mode_valid, > - .atomic_check = intel_digital_connector_atomic_check, > + .atomic_check = intel_dp_connector_atomic_check, > }; > > static const struct drm_encoder_funcs intel_dp_enc_funcs = { > -- > 2.19.1
On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote: > On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote: > > If one of the synced crtcs needs a full modeset, we need > > to make sure all the synced crtcs are forced a full > > modeset. > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 88 +------------ > > drivers/gpu/drm/i915/display/intel_dp.c | 131 ++++++++++++++++++- > > 2 files changed, 131 insertions(+), 88 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index e638543f5f87..709a737638b6 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config) > > struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev); > > struct drm_connector *connector; > > struct drm_connector_state *connector_state; > > - int base_bpp, ret; > > - int i, tile_group_id = -1, num_tiled_conns = 0; > > + int base_bpp, ret, i; > > bool retry = true; > > > > pipe_config->cpu_transcoder = > > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state, > > return false; > > } > > > > -static int > > -intel_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_iter; > > - int ret = 0; > > - > > - drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > - drm_for_each_connector_iter(connector, &conn_iter) { > > - struct drm_connector_state *conn_state; > > - struct drm_crtc_state *crtc_state; > > - > > - if (!connector->has_tile || > > - connector->tile_group->id != tile_grp_id) > > - continue; > > - conn_state = drm_atomic_get_connector_state(&state->base, > > - connector); > > - if (IS_ERR(conn_state)) { > > - ret = PTR_ERR(conn_state); > > - break; > > - } > > - > > - if (!conn_state->crtc) > > - continue; > > - > > - crtc_state = drm_atomic_get_crtc_state(&state->base, > > - conn_state->crtc); > > - if (IS_ERR(crtc_state)) { > > - ret = PTR_ERR(crtc_state); > > - break; > > - } > > - crtc_state->mode_changed = true; > > - ret = drm_atomic_add_affected_connectors(&state->base, > > - conn_state->crtc); > > - if (ret) > > - break; > > - } > > - drm_connector_list_iter_end(&conn_iter); > > - > > - return ret; > > -} > > - > > -static int > > -intel_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, connector)) > > - continue; > > - > > - ret = intel_modeset_all_tiles(state, connector->tile_group->id); > > - if (ret) > > - return ret; > > - } > > - > > - return 0; > > -} > > - > > /** > > * intel_atomic_check - validate state object > > * @dev: drm device > > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev, > > if (ret) > > goto fail; > > > > - /** > > - * This check adds all the connectors in current state that belong to > > - * the same tile group to a full modeset. > > - * This function directly sets the mode_changed to true and we also call > > - * drm_atomic_add_affected_connectors(). Hence we are not explicitly > > - * calling drm_atomic_helper_check_modeset() after this. > > - * > > - * Fixme: Handle some corner cases where one of the > > - * tiled connectors gets disconnected and tile info is lost but since it > > - * was previously synced to other conn, we need to add that to the modeset. > > - */ > > - ret = intel_atomic_check_tiled_conns(state); > > - if (ret) > > - goto fail; > > - > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > new_crtc_state, i) { > > if (!needs_modeset(new_crtc_state)) { > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index f4dede6253f8..7eb4b3dbbcb3 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > > } > > } > > > > +static int intel_modeset_tile_group(struct intel_atomic_state *state, > > + int tile_group_id) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct drm_connector_list_iter conn_iter; > > + struct drm_connector *connector; > > + int ret = 0; > > + > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > + drm_for_each_connector_iter(connector, &conn_iter) { > > + struct drm_connector_state *conn_state; > > + struct intel_crtc_state *crtc_state; > > + struct intel_crtc *crtc; > > + > > + if (!connector->has_tile || > > + connector->tile_group->id != tile_group_id) > > + continue; > > + > > + conn_state = drm_atomic_get_connector_state(&state->base, > > + connector); > > + if (IS_ERR(conn_state)) { > > + ret = PTR_ERR(conn_state); > > + break; > > + } > > + > > + crtc = to_intel_crtc(conn_state->crtc); > > + > > + if (!crtc) > > + continue; > > + > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > + if (IS_ERR(crtc_state)) { > > + ret = PTR_ERR(crtc_state); > > + break; > > + } > > + > > + crtc_state->uapi.mode_changed = true; > > + > > + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base); > > + if (ret) > > + break; > > + } > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > + > > + return ret; > > +} > > + > > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct intel_crtc *crtc; > > + > > + if (transcoders == 0) > > + return 0; > > + > > + for_each_intel_crtc(&dev_priv->drm, crtc) { > > + struct intel_crtc_state *crtc_state; > > + int ret; > > + > > + if ((transcoders & BIT(crtc->pipe)) == 0) > > + continue; > > Dropping the EDP transcoder on the floor here. I think we should just do > the guaranteed correct thing and look at the cpu_transcoder instead. > Yes, that does mean we more or less end up adding all crtcs to the state > whenever modesetting any synced crtc, but so be it. We can think of ways > to optimize that later. > So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ? Or should I keep this as is and then add the second loop like in your branch? > > + > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > + if (IS_ERR(crtc_state)) > > + return PTR_ERR(crtc_state); > > + > > + if (!crtc_state->hw.enable) > > + continue; > > + > > + crtc_state->uapi.mode_changed = true; > > + > > + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base); > > + if (ret) > > + return ret; > > Missing add_affected_planes() here I think. Or was that guaranteed to be > done by the helper? Can't recall. > No i think i missed it, will add that > > + > > + WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder); > > + > > + transcoders &= ~BIT(crtc_state->cpu_transcoder); > > + } > > + > > + WARN_ON(transcoders != 0); > > + > > + return 0; > > + > > +} > > + > > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state, > > + struct drm_connector *connector) > > +{ > > + const struct drm_connector_state *old_conn_state = > > + drm_atomic_get_old_connector_state(&state->base, connector); > > + const struct intel_crtc_state *old_crtc_state; > > + struct intel_crtc *crtc; > > + > > + crtc = to_intel_crtc(old_conn_state->crtc); > > + if (!crtc) > > + return 0; > > + > > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > + > > + if (!old_crtc_state->hw.active) > > + return 0; > > + > > + return intel_modeset_affected_transcoders(state, > > + (old_crtc_state->sync_mode_slaves_mask | > > + BIT(old_crtc_state->master_transcoder)) & > > This seems to have the same master==INVALID problem that we faced > elsewhere already. > Yes will have to add the same check and add it only for master_trans != INVALID, will add that > > + ~BIT(old_crtc_state->cpu_transcoder)); > > I guess this part is redundant. Or can we somehow have our own > transcoder be included in sync_mode_slaves_mask/master_transcoder? > Actually shouldnt it be: if old_crtc_state->needs_modeset() { then call intel_modeset_affected_transcoders(state, (old_crtc_state->sync_mode_slaves_mask | BIT(old_crtc_state->master_transcoder)) & ~BIT(old_crtc_state->cpu_transcoder)); dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to force modeset there if we add this needs_modeset check? Manasi > > +} > > + > > +static int intel_dp_connector_atomic_check(struct drm_connector *conn, > > + struct drm_atomic_state *_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(conn->dev); > > + struct intel_atomic_state *state = to_intel_atomic_state(_state); > > + int ret; > > + > > + ret = intel_digital_connector_atomic_check(conn, &state->base); > > + if (ret) > > + return ret; > > + > > + if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) { > > + ret = intel_modeset_tile_group(state, conn->tile_group->id); > > + if (ret) > > + return ret; > > + } > > + > > + return intel_modeset_synced_crtcs(state, conn); > > No gen check... Ah, yeah we don't need it because the port sync state > will be INVALID/0. > > > +} > > + > > static const struct drm_connector_funcs intel_dp_connector_funcs = { > > .force = intel_dp_force, > > .fill_modes = drm_helper_probe_single_connector_modes, > > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = > > .detect_ctx = intel_dp_detect, > > .get_modes = intel_dp_get_modes, > > .mode_valid = intel_dp_mode_valid, > > - .atomic_check = intel_digital_connector_atomic_check, > > + .atomic_check = intel_dp_connector_atomic_check, > > }; > > > > static const struct drm_encoder_funcs intel_dp_enc_funcs = { > > -- > > 2.19.1 > > -- > Ville Syrjälä > Intel
On Fri, Jan 31, 2020 at 11:46:25AM -0800, Manasi Navare wrote: > On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote: > > > If one of the synced crtcs needs a full modeset, we need > > > to make sure all the synced crtcs are forced a full > > > modeset. > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 88 +------------ > > > drivers/gpu/drm/i915/display/intel_dp.c | 131 ++++++++++++++++++- > > > 2 files changed, 131 insertions(+), 88 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index e638543f5f87..709a737638b6 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config) > > > struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev); > > > struct drm_connector *connector; > > > struct drm_connector_state *connector_state; > > > - int base_bpp, ret; > > > - int i, tile_group_id = -1, num_tiled_conns = 0; > > > + int base_bpp, ret, i; > > > bool retry = true; > > > > > > pipe_config->cpu_transcoder = > > > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state, > > > return false; > > > } > > > > > > -static int > > > -intel_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_iter; > > > - int ret = 0; > > > - > > > - drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > - drm_for_each_connector_iter(connector, &conn_iter) { > > > - struct drm_connector_state *conn_state; > > > - struct drm_crtc_state *crtc_state; > > > - > > > - if (!connector->has_tile || > > > - connector->tile_group->id != tile_grp_id) > > > - continue; > > > - conn_state = drm_atomic_get_connector_state(&state->base, > > > - connector); > > > - if (IS_ERR(conn_state)) { > > > - ret = PTR_ERR(conn_state); > > > - break; > > > - } > > > - > > > - if (!conn_state->crtc) > > > - continue; > > > - > > > - crtc_state = drm_atomic_get_crtc_state(&state->base, > > > - conn_state->crtc); > > > - if (IS_ERR(crtc_state)) { > > > - ret = PTR_ERR(crtc_state); > > > - break; > > > - } > > > - crtc_state->mode_changed = true; > > > - ret = drm_atomic_add_affected_connectors(&state->base, > > > - conn_state->crtc); > > > - if (ret) > > > - break; > > > - } > > > - drm_connector_list_iter_end(&conn_iter); > > > - > > > - return ret; > > > -} > > > - > > > -static int > > > -intel_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, connector)) > > > - continue; > > > - > > > - ret = intel_modeset_all_tiles(state, connector->tile_group->id); > > > - if (ret) > > > - return ret; > > > - } > > > - > > > - return 0; > > > -} > > > - > > > /** > > > * intel_atomic_check - validate state object > > > * @dev: drm device > > > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev, > > > if (ret) > > > goto fail; > > > > > > - /** > > > - * This check adds all the connectors in current state that belong to > > > - * the same tile group to a full modeset. > > > - * This function directly sets the mode_changed to true and we also call > > > - * drm_atomic_add_affected_connectors(). Hence we are not explicitly > > > - * calling drm_atomic_helper_check_modeset() after this. > > > - * > > > - * Fixme: Handle some corner cases where one of the > > > - * tiled connectors gets disconnected and tile info is lost but since it > > > - * was previously synced to other conn, we need to add that to the modeset. > > > - */ > > > - ret = intel_atomic_check_tiled_conns(state); > > > - if (ret) > > > - goto fail; > > > - > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > new_crtc_state, i) { > > > if (!needs_modeset(new_crtc_state)) { > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > index f4dede6253f8..7eb4b3dbbcb3 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > > > } > > > } > > > > > > +static int intel_modeset_tile_group(struct intel_atomic_state *state, > > > + int tile_group_id) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > + struct drm_connector_list_iter conn_iter; > > > + struct drm_connector *connector; > > > + int ret = 0; > > > + > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > + drm_for_each_connector_iter(connector, &conn_iter) { > > > + struct drm_connector_state *conn_state; > > > + struct intel_crtc_state *crtc_state; > > > + struct intel_crtc *crtc; > > > + > > > + if (!connector->has_tile || > > > + connector->tile_group->id != tile_group_id) > > > + continue; > > > + > > > + conn_state = drm_atomic_get_connector_state(&state->base, > > > + connector); > > > + if (IS_ERR(conn_state)) { > > > + ret = PTR_ERR(conn_state); > > > + break; > > > + } > > > + > > > + crtc = to_intel_crtc(conn_state->crtc); > > > + > > > + if (!crtc) > > > + continue; > > > + > > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > > + if (IS_ERR(crtc_state)) { > > > + ret = PTR_ERR(crtc_state); > > > + break; > > > + } > > > + > > > + crtc_state->uapi.mode_changed = true; > > > + > > > + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base); > > > + if (ret) > > > + break; > > > + } > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > + > > > + return ret; > > > +} > > > + > > > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > + struct intel_crtc *crtc; > > > + > > > + if (transcoders == 0) > > > + return 0; > > > + > > > + for_each_intel_crtc(&dev_priv->drm, crtc) { > > > + struct intel_crtc_state *crtc_state; > > > + int ret; > > > + > > > + if ((transcoders & BIT(crtc->pipe)) == 0) > > > + continue; > > > > Dropping the EDP transcoder on the floor here. I think we should just do > > the guaranteed correct thing and look at the cpu_transcoder instead. > > Yes, that does mean we more or less end up adding all crtcs to the state > > whenever modesetting any synced crtc, but so be it. We can think of ways > > to optimize that later. > > > > So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed > to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ? Yeah, I think that's fine for now. We can try to optimize it later if needed. > > Or should I keep this as is and then add the second loop like in your branch? > > > > + > > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > > + if (IS_ERR(crtc_state)) > > > + return PTR_ERR(crtc_state); > > > + > > > + if (!crtc_state->hw.enable) > > > + continue; > > > + > > > + crtc_state->uapi.mode_changed = true; > > > + > > > + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base); > > > + if (ret) > > > + return ret; > > > > Missing add_affected_planes() here I think. Or was that guaranteed to be > > done by the helper? Can't recall. > > > > No i think i missed it, will add that > > > > + > > > + WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder); > > > + > > > + transcoders &= ~BIT(crtc_state->cpu_transcoder); > > > + } > > > + > > > + WARN_ON(transcoders != 0); > > > + > > > + return 0; > > > + > > > +} > > > + > > > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state, > > > + struct drm_connector *connector) > > > +{ > > > + const struct drm_connector_state *old_conn_state = > > > + drm_atomic_get_old_connector_state(&state->base, connector); > > > + const struct intel_crtc_state *old_crtc_state; > > > + struct intel_crtc *crtc; > > > + > > > + crtc = to_intel_crtc(old_conn_state->crtc); > > > + if (!crtc) > > > + return 0; > > > + > > > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > > + > > > + if (!old_crtc_state->hw.active) > > > + return 0; > > > + > > > + return intel_modeset_affected_transcoders(state, > > > + (old_crtc_state->sync_mode_slaves_mask | > > > + BIT(old_crtc_state->master_transcoder)) & > > > > This seems to have the same master==INVALID problem that we faced > > elsewhere already. > > > > Yes will have to add the same check and add it only for master_trans != INVALID, will add that > > > > + ~BIT(old_crtc_state->cpu_transcoder)); > > > > I guess this part is redundant. Or can we somehow have our own > > transcoder be included in sync_mode_slaves_mask/master_transcoder? > > > > Actually shouldnt it be: > > if old_crtc_state->needs_modeset() { That would need to be new_crtc_state. And yes we should skip this (and also intel_modeset_tile_group()) when there's no modeset. So the whole thing could probably look something like: { intel_digital_connector_atomic_check(); if (!intel_connector_needs_modeset(connector)) return 0; if (tile) intel_modeset_tile_group() intel_modeset_synced_crtcs(); } > then call intel_modeset_affected_transcoders(state, > (old_crtc_state->sync_mode_slaves_mask | > BIT(old_crtc_state->master_transcoder)) & > ~BIT(old_crtc_state->cpu_transcoder)); > > dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to > force modeset there if we add this needs_modeset check? > > Manasi > > > > +} > > > + > > > +static int intel_dp_connector_atomic_check(struct drm_connector *conn, > > > + struct drm_atomic_state *_state) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(conn->dev); > > > + struct intel_atomic_state *state = to_intel_atomic_state(_state); > > > + int ret; > > > + > > > + ret = intel_digital_connector_atomic_check(conn, &state->base); > > > + if (ret) > > > + return ret; > > > + > > > + if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) { > > > + ret = intel_modeset_tile_group(state, conn->tile_group->id); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return intel_modeset_synced_crtcs(state, conn); > > > > No gen check... Ah, yeah we don't need it because the port sync state > > will be INVALID/0. > > > > > +} > > > + > > > static const struct drm_connector_funcs intel_dp_connector_funcs = { > > > .force = intel_dp_force, > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = > > > .detect_ctx = intel_dp_detect, > > > .get_modes = intel_dp_get_modes, > > > .mode_valid = intel_dp_mode_valid, > > > - .atomic_check = intel_digital_connector_atomic_check, > > > + .atomic_check = intel_dp_connector_atomic_check, > > > }; > > > > > > static const struct drm_encoder_funcs intel_dp_enc_funcs = { > > > -- > > > 2.19.1 > > > > -- > > Ville Syrjälä > > Intel
On Fri, Jan 31, 2020 at 10:05:03PM +0200, Ville Syrjälä wrote: > On Fri, Jan 31, 2020 at 11:46:25AM -0800, Manasi Navare wrote: > > On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote: > > > On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote: > > > > If one of the synced crtcs needs a full modeset, we need > > > > to make sure all the synced crtcs are forced a full > > > > modeset. > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 88 +------------ > > > > drivers/gpu/drm/i915/display/intel_dp.c | 131 ++++++++++++++++++- > > > > 2 files changed, 131 insertions(+), 88 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > index e638543f5f87..709a737638b6 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config) > > > > struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev); > > > > struct drm_connector *connector; > > > > struct drm_connector_state *connector_state; > > > > - int base_bpp, ret; > > > > - int i, tile_group_id = -1, num_tiled_conns = 0; > > > > + int base_bpp, ret, i; > > > > bool retry = true; > > > > > > > > pipe_config->cpu_transcoder = > > > > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state, > > > > return false; > > > > } > > > > > > > > -static int > > > > -intel_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_iter; > > > > - int ret = 0; > > > > - > > > > - drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > > - drm_for_each_connector_iter(connector, &conn_iter) { > > > > - struct drm_connector_state *conn_state; > > > > - struct drm_crtc_state *crtc_state; > > > > - > > > > - if (!connector->has_tile || > > > > - connector->tile_group->id != tile_grp_id) > > > > - continue; > > > > - conn_state = drm_atomic_get_connector_state(&state->base, > > > > - connector); > > > > - if (IS_ERR(conn_state)) { > > > > - ret = PTR_ERR(conn_state); > > > > - break; > > > > - } > > > > - > > > > - if (!conn_state->crtc) > > > > - continue; > > > > - > > > > - crtc_state = drm_atomic_get_crtc_state(&state->base, > > > > - conn_state->crtc); > > > > - if (IS_ERR(crtc_state)) { > > > > - ret = PTR_ERR(crtc_state); > > > > - break; > > > > - } > > > > - crtc_state->mode_changed = true; > > > > - ret = drm_atomic_add_affected_connectors(&state->base, > > > > - conn_state->crtc); > > > > - if (ret) > > > > - break; > > > > - } > > > > - drm_connector_list_iter_end(&conn_iter); > > > > - > > > > - return ret; > > > > -} > > > > - > > > > -static int > > > > -intel_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, connector)) > > > > - continue; > > > > - > > > > - ret = intel_modeset_all_tiles(state, connector->tile_group->id); > > > > - if (ret) > > > > - return ret; > > > > - } > > > > - > > > > - return 0; > > > > -} > > > > - > > > > /** > > > > * intel_atomic_check - validate state object > > > > * @dev: drm device > > > > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev, > > > > if (ret) > > > > goto fail; > > > > > > > > - /** > > > > - * This check adds all the connectors in current state that belong to > > > > - * the same tile group to a full modeset. > > > > - * This function directly sets the mode_changed to true and we also call > > > > - * drm_atomic_add_affected_connectors(). Hence we are not explicitly > > > > - * calling drm_atomic_helper_check_modeset() after this. > > > > - * > > > > - * Fixme: Handle some corner cases where one of the > > > > - * tiled connectors gets disconnected and tile info is lost but since it > > > > - * was previously synced to other conn, we need to add that to the modeset. > > > > - */ > > > > - ret = intel_atomic_check_tiled_conns(state); > > > > - if (ret) > > > > - goto fail; > > > > - > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > > new_crtc_state, i) { > > > > if (!needs_modeset(new_crtc_state)) { > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index f4dede6253f8..7eb4b3dbbcb3 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > > > > } > > > > } > > > > > > > > +static int intel_modeset_tile_group(struct intel_atomic_state *state, > > > > + int tile_group_id) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > + struct drm_connector_list_iter conn_iter; > > > > + struct drm_connector *connector; > > > > + int ret = 0; > > > > + > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > > + drm_for_each_connector_iter(connector, &conn_iter) { > > > > + struct drm_connector_state *conn_state; > > > > + struct intel_crtc_state *crtc_state; > > > > + struct intel_crtc *crtc; > > > > + > > > > + if (!connector->has_tile || > > > > + connector->tile_group->id != tile_group_id) > > > > + continue; > > > > + > > > > + conn_state = drm_atomic_get_connector_state(&state->base, > > > > + connector); > > > > + if (IS_ERR(conn_state)) { > > > > + ret = PTR_ERR(conn_state); > > > > + break; > > > > + } > > > > + > > > > + crtc = to_intel_crtc(conn_state->crtc); > > > > + > > > > + if (!crtc) > > > > + continue; > > > > + > > > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > > > + if (IS_ERR(crtc_state)) { > > > > + ret = PTR_ERR(crtc_state); > > > > + break; > > > > + } > > > > + > > > > + crtc_state->uapi.mode_changed = true; > > > > + > > > > + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base); > > > > + if (ret) > > > > + break; > > > > + } > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > + struct intel_crtc *crtc; > > > > + > > > > + if (transcoders == 0) > > > > + return 0; > > > > + > > > > + for_each_intel_crtc(&dev_priv->drm, crtc) { > > > > + struct intel_crtc_state *crtc_state; > > > > + int ret; > > > > + > > > > + if ((transcoders & BIT(crtc->pipe)) == 0) > > > > + continue; > > > > > > Dropping the EDP transcoder on the floor here. I think we should just do > > > the guaranteed correct thing and look at the cpu_transcoder instead. > > > Yes, that does mean we more or less end up adding all crtcs to the state > > > whenever modesetting any synced crtc, but so be it. We can think of ways > > > to optimize that later. > > > > > > > So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed > > to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ? > > Yeah, I think that's fine for now. We can try to optimize it later if > needed. > > > > > Or should I keep this as is and then add the second loop like in your branch? > > > > > > + > > > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > > > + if (IS_ERR(crtc_state)) > > > > + return PTR_ERR(crtc_state); > > > > + > > > > + if (!crtc_state->hw.enable) > > > > + continue; > > > > + > > > > + crtc_state->uapi.mode_changed = true; > > > > + > > > > + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base); > > > > + if (ret) > > > > + return ret; > > > > > > Missing add_affected_planes() here I think. Or was that guaranteed to be > > > done by the helper? Can't recall. > > > > > > > No i think i missed it, will add that > > > > > > + > > > > + WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder); > > > > + > > > > + transcoders &= ~BIT(crtc_state->cpu_transcoder); > > > > + } > > > > + > > > > + WARN_ON(transcoders != 0); > > > > + > > > > + return 0; > > > > + > > > > +} > > > > + > > > > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state, > > > > + struct drm_connector *connector) > > > > +{ > > > > + const struct drm_connector_state *old_conn_state = > > > > + drm_atomic_get_old_connector_state(&state->base, connector); > > > > + const struct intel_crtc_state *old_crtc_state; > > > > + struct intel_crtc *crtc; > > > > + > > > > + crtc = to_intel_crtc(old_conn_state->crtc); > > > > + if (!crtc) > > > > + return 0; > > > > + > > > > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > > > + > > > > + if (!old_crtc_state->hw.active) > > > > + return 0; > > > > + > > > > + return intel_modeset_affected_transcoders(state, > > > > + (old_crtc_state->sync_mode_slaves_mask | > > > > + BIT(old_crtc_state->master_transcoder)) & > > > > > > This seems to have the same master==INVALID problem that we faced > > > elsewhere already. > > > > > > > Yes will have to add the same check and add it only for master_trans != INVALID, will add that > > > > > > + ~BIT(old_crtc_state->cpu_transcoder)); > > > > > > I guess this part is redundant. Or can we somehow have our own > > > transcoder be included in sync_mode_slaves_mask/master_transcoder? > > > > > > > Actually shouldnt it be: > > > > if old_crtc_state->needs_modeset() { > > That would need to be new_crtc_state. And yes we should skip this > (and also intel_modeset_tile_group()) when there's no modeset. > > So the whole thing could probably look something like: > { > intel_digital_connector_atomic_check(); > > if (!intel_connector_needs_modeset(connector)) > return 0; > > if (tile) > intel_modeset_tile_group() > > intel_modeset_synced_crtcs(); > } Great yea this makes sense. So then keep the ~BIT(old_crtc_state->cpu_transcoder)); right? so we dont forec modeset on the current crtc? Manasi > > > then call intel_modeset_affected_transcoders(state, > > (old_crtc_state->sync_mode_slaves_mask | > > BIT(old_crtc_state->master_transcoder)) & > > ~BIT(old_crtc_state->cpu_transcoder)); > > > > dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to > > force modeset there if we add this needs_modeset check? > > > > Manasi > > > > > > +} > > > > + > > > > +static int intel_dp_connector_atomic_check(struct drm_connector *conn, > > > > + struct drm_atomic_state *_state) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(conn->dev); > > > > + struct intel_atomic_state *state = to_intel_atomic_state(_state); > > > > + int ret; > > > > + > > > > + ret = intel_digital_connector_atomic_check(conn, &state->base); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) { > > > > + ret = intel_modeset_tile_group(state, conn->tile_group->id); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + return intel_modeset_synced_crtcs(state, conn); > > > > > > No gen check... Ah, yeah we don't need it because the port sync state > > > will be INVALID/0. > > > > > > > +} > > > > + > > > > static const struct drm_connector_funcs intel_dp_connector_funcs = { > > > > .force = intel_dp_force, > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = > > > > .detect_ctx = intel_dp_detect, > > > > .get_modes = intel_dp_get_modes, > > > > .mode_valid = intel_dp_mode_valid, > > > > - .atomic_check = intel_digital_connector_atomic_check, > > > > + .atomic_check = intel_dp_connector_atomic_check, > > > > }; > > > > > > > > static const struct drm_encoder_funcs intel_dp_enc_funcs = { > > > > -- > > > > 2.19.1 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Fri, Jan 31, 2020 at 12:54:52PM -0800, Manasi Navare wrote: > On Fri, Jan 31, 2020 at 10:05:03PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 31, 2020 at 11:46:25AM -0800, Manasi Navare wrote: > > > On Fri, Jan 31, 2020 at 07:53:23PM +0200, Ville Syrjälä wrote: > > > > On Fri, Jan 31, 2020 at 09:15:47AM -0800, Manasi Navare wrote: > > > > > If one of the synced crtcs needs a full modeset, we need > > > > > to make sure all the synced crtcs are forced a full > > > > > modeset. > > > > > > > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 88 +------------ > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 131 ++++++++++++++++++- > > > > > 2 files changed, 131 insertions(+), 88 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > index e638543f5f87..709a737638b6 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config) > > > > > struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev); > > > > > struct drm_connector *connector; > > > > > struct drm_connector_state *connector_state; > > > > > - int base_bpp, ret; > > > > > - int i, tile_group_id = -1, num_tiled_conns = 0; > > > > > + int base_bpp, ret, i; > > > > > bool retry = true; > > > > > > > > > > pipe_config->cpu_transcoder = > > > > > @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state, > > > > > return false; > > > > > } > > > > > > > > > > -static int > > > > > -intel_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_iter; > > > > > - int ret = 0; > > > > > - > > > > > - drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > > > - drm_for_each_connector_iter(connector, &conn_iter) { > > > > > - struct drm_connector_state *conn_state; > > > > > - struct drm_crtc_state *crtc_state; > > > > > - > > > > > - if (!connector->has_tile || > > > > > - connector->tile_group->id != tile_grp_id) > > > > > - continue; > > > > > - conn_state = drm_atomic_get_connector_state(&state->base, > > > > > - connector); > > > > > - if (IS_ERR(conn_state)) { > > > > > - ret = PTR_ERR(conn_state); > > > > > - break; > > > > > - } > > > > > - > > > > > - if (!conn_state->crtc) > > > > > - continue; > > > > > - > > > > > - crtc_state = drm_atomic_get_crtc_state(&state->base, > > > > > - conn_state->crtc); > > > > > - if (IS_ERR(crtc_state)) { > > > > > - ret = PTR_ERR(crtc_state); > > > > > - break; > > > > > - } > > > > > - crtc_state->mode_changed = true; > > > > > - ret = drm_atomic_add_affected_connectors(&state->base, > > > > > - conn_state->crtc); > > > > > - if (ret) > > > > > - break; > > > > > - } > > > > > - drm_connector_list_iter_end(&conn_iter); > > > > > - > > > > > - return ret; > > > > > -} > > > > > - > > > > > -static int > > > > > -intel_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, connector)) > > > > > - continue; > > > > > - > > > > > - ret = intel_modeset_all_tiles(state, connector->tile_group->id); > > > > > - if (ret) > > > > > - return ret; > > > > > - } > > > > > - > > > > > - return 0; > > > > > -} > > > > > - > > > > > /** > > > > > * intel_atomic_check - validate state object > > > > > * @dev: drm device > > > > > @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev, > > > > > if (ret) > > > > > goto fail; > > > > > > > > > > - /** > > > > > - * This check adds all the connectors in current state that belong to > > > > > - * the same tile group to a full modeset. > > > > > - * This function directly sets the mode_changed to true and we also call > > > > > - * drm_atomic_add_affected_connectors(). Hence we are not explicitly > > > > > - * calling drm_atomic_helper_check_modeset() after this. > > > > > - * > > > > > - * Fixme: Handle some corner cases where one of the > > > > > - * tiled connectors gets disconnected and tile info is lost but since it > > > > > - * was previously synced to other conn, we need to add that to the modeset. > > > > > - */ > > > > > - ret = intel_atomic_check_tiled_conns(state); > > > > > - if (ret) > > > > > - goto fail; > > > > > - > > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > > > > new_crtc_state, i) { > > > > > if (!needs_modeset(new_crtc_state)) { > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > index f4dede6253f8..7eb4b3dbbcb3 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) > > > > > } > > > > > } > > > > > > > > > > +static int intel_modeset_tile_group(struct intel_atomic_state *state, > > > > > + int tile_group_id) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > + struct drm_connector_list_iter conn_iter; > > > > > + struct drm_connector *connector; > > > > > + int ret = 0; > > > > > + > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > > > + drm_for_each_connector_iter(connector, &conn_iter) { > > > > > + struct drm_connector_state *conn_state; > > > > > + struct intel_crtc_state *crtc_state; > > > > > + struct intel_crtc *crtc; > > > > > + > > > > > + if (!connector->has_tile || > > > > > + connector->tile_group->id != tile_group_id) > > > > > + continue; > > > > > + > > > > > + conn_state = drm_atomic_get_connector_state(&state->base, > > > > > + connector); > > > > > + if (IS_ERR(conn_state)) { > > > > > + ret = PTR_ERR(conn_state); > > > > > + break; > > > > > + } > > > > > + > > > > > + crtc = to_intel_crtc(conn_state->crtc); > > > > > + > > > > > + if (!crtc) > > > > > + continue; > > > > > + > > > > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > > > > + if (IS_ERR(crtc_state)) { > > > > > + ret = PTR_ERR(crtc_state); > > > > > + break; > > > > > + } > > > > > + > > > > > + crtc_state->uapi.mode_changed = true; > > > > > + > > > > > + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base); > > > > > + if (ret) > > > > > + break; > > > > > + } > > > > > + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > + struct intel_crtc *crtc; > > > > > + > > > > > + if (transcoders == 0) > > > > > + return 0; > > > > > + > > > > > + for_each_intel_crtc(&dev_priv->drm, crtc) { > > > > > + struct intel_crtc_state *crtc_state; > > > > > + int ret; > > > > > + > > > > > + if ((transcoders & BIT(crtc->pipe)) == 0) > > > > > + continue; > > > > > > > > Dropping the EDP transcoder on the floor here. I think we should just do > > > > the guaranteed correct thing and look at the cpu_transcoder instead. > > > > Yes, that does mean we more or less end up adding all crtcs to the state > > > > whenever modesetting any synced crtc, but so be it. We can think of ways > > > > to optimize that later. > > > > > > > > > > So should i not even do this check of trans & BIT(crtc->pipe) ? Instead just proceed > > > to go get the crtc state and then check if trans & BIT(crtc_state->cpu_trans) ? > > > > Yeah, I think that's fine for now. We can try to optimize it later if > > needed. > > > > > > > > Or should I keep this as is and then add the second loop like in your branch? > > > > > > > > + > > > > > + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > > > > > + if (IS_ERR(crtc_state)) > > > > > + return PTR_ERR(crtc_state); > > > > > + > > > > > + if (!crtc_state->hw.enable) > > > > > + continue; > > > > > + > > > > > + crtc_state->uapi.mode_changed = true; > > > > > + > > > > > + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > Missing add_affected_planes() here I think. Or was that guaranteed to be > > > > done by the helper? Can't recall. > > > > > > > > > > No i think i missed it, will add that > > > > > > > > + > > > > > + WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder); > > > > > + > > > > > + transcoders &= ~BIT(crtc_state->cpu_transcoder); > > > > > + } > > > > > + > > > > > + WARN_ON(transcoders != 0); > > > > > + > > > > > + return 0; > > > > > + > > > > > +} > > > > > + > > > > > +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state, > > > > > + struct drm_connector *connector) > > > > > +{ > > > > > + const struct drm_connector_state *old_conn_state = > > > > > + drm_atomic_get_old_connector_state(&state->base, connector); > > > > > + const struct intel_crtc_state *old_crtc_state; > > > > > + struct intel_crtc *crtc; > > > > > + > > > > > + crtc = to_intel_crtc(old_conn_state->crtc); > > > > > + if (!crtc) > > > > > + return 0; > > > > > + > > > > > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); > > > > > + > > > > > + if (!old_crtc_state->hw.active) > > > > > + return 0; > > > > > + > > > > > + return intel_modeset_affected_transcoders(state, > > > > > + (old_crtc_state->sync_mode_slaves_mask | > > > > > + BIT(old_crtc_state->master_transcoder)) & > > > > > > > > This seems to have the same master==INVALID problem that we faced > > > > elsewhere already. > > > > > > > > > > Yes will have to add the same check and add it only for master_trans != INVALID, will add that > > > > > > > > + ~BIT(old_crtc_state->cpu_transcoder)); > > > > > > > > I guess this part is redundant. Or can we somehow have our own > > > > transcoder be included in sync_mode_slaves_mask/master_transcoder? > > > > > > > > > > Actually shouldnt it be: > > > > > > if old_crtc_state->needs_modeset() { > > > > That would need to be new_crtc_state. And yes we should skip this > > (and also intel_modeset_tile_group()) when there's no modeset. > > > > So the whole thing could probably look something like: > > { > > intel_digital_connector_atomic_check(); > > > > if (!intel_connector_needs_modeset(connector)) > > return 0; > > > > if (tile) > > intel_modeset_tile_group() > > > > intel_modeset_synced_crtcs(); > > } > > Great yea this makes sense. > So then keep the ~BIT(old_crtc_state->cpu_transcoder)); right? so we dont forec modeset > on the current crtc? intel_connector_needs_modeset() already told us the old crtc (if any) needs a modeset. But even if it didn't cpu_transcoder still can't be part of master|slaves so still redundant. > > Manasi > > > > > > then call intel_modeset_affected_transcoders(state, > > > (old_crtc_state->sync_mode_slaves_mask | > > > BIT(old_crtc_state->master_transcoder)) & > > > ~BIT(old_crtc_state->cpu_transcoder)); > > > > > > dont understand why ~BIT(old_crtc_state->cpu_transcoder) is redundant? why do we need to > > > force modeset there if we add this needs_modeset check? > > > > > > Manasi > > > > > > > > +} > > > > > + > > > > > +static int intel_dp_connector_atomic_check(struct drm_connector *conn, > > > > > + struct drm_atomic_state *_state) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(conn->dev); > > > > > + struct intel_atomic_state *state = to_intel_atomic_state(_state); > > > > > + int ret; > > > > > + > > > > > + ret = intel_digital_connector_atomic_check(conn, &state->base); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) { > > > > > + ret = intel_modeset_tile_group(state, conn->tile_group->id); > > > > > + if (ret) > > > > > + return ret; > > > > > + } > > > > > + > > > > > + return intel_modeset_synced_crtcs(state, conn); > > > > > > > > No gen check... Ah, yeah we don't need it because the port sync state > > > > will be INVALID/0. > > > > > > > > > +} > > > > > + > > > > > static const struct drm_connector_funcs intel_dp_connector_funcs = { > > > > > .force = intel_dp_force, > > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > > @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = > > > > > .detect_ctx = intel_dp_detect, > > > > > .get_modes = intel_dp_get_modes, > > > > > .mode_valid = intel_dp_mode_valid, > > > > > - .atomic_check = intel_digital_connector_atomic_check, > > > > > + .atomic_check = intel_dp_connector_atomic_check, > > > > > }; > > > > > > > > > > static const struct drm_encoder_funcs intel_dp_enc_funcs = { > > > > > -- > > > > > 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 e638543f5f87..709a737638b6 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -13123,8 +13123,7 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config) struct drm_i915_private *i915 = to_i915(pipe_config->uapi.crtc->dev); struct drm_connector *connector; struct drm_connector_state *connector_state; - int base_bpp, ret; - int i, tile_group_id = -1, num_tiled_conns = 0; + int base_bpp, ret, i; bool retry = true; pipe_config->cpu_transcoder = @@ -14559,76 +14558,6 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state, return false; } -static int -intel_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_iter; - int ret = 0; - - drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); - drm_for_each_connector_iter(connector, &conn_iter) { - struct drm_connector_state *conn_state; - struct drm_crtc_state *crtc_state; - - if (!connector->has_tile || - connector->tile_group->id != tile_grp_id) - continue; - conn_state = drm_atomic_get_connector_state(&state->base, - connector); - if (IS_ERR(conn_state)) { - ret = PTR_ERR(conn_state); - break; - } - - if (!conn_state->crtc) - continue; - - crtc_state = drm_atomic_get_crtc_state(&state->base, - conn_state->crtc); - if (IS_ERR(crtc_state)) { - ret = PTR_ERR(crtc_state); - break; - } - crtc_state->mode_changed = true; - ret = drm_atomic_add_affected_connectors(&state->base, - conn_state->crtc); - if (ret) - break; - } - drm_connector_list_iter_end(&conn_iter); - - return ret; -} - -static int -intel_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, connector)) - continue; - - ret = intel_modeset_all_tiles(state, connector->tile_group->id); - if (ret) - return ret; - } - - return 0; -} - /** * intel_atomic_check - validate state object * @dev: drm device @@ -14656,21 +14585,6 @@ static int intel_atomic_check(struct drm_device *dev, if (ret) goto fail; - /** - * This check adds all the connectors in current state that belong to - * the same tile group to a full modeset. - * This function directly sets the mode_changed to true and we also call - * drm_atomic_add_affected_connectors(). Hence we are not explicitly - * calling drm_atomic_helper_check_modeset() after this. - * - * Fixme: Handle some corner cases where one of the - * tiled connectors gets disconnected and tile info is lost but since it - * was previously synced to other conn, we need to add that to the modeset. - */ - ret = intel_atomic_check_tiled_conns(state); - if (ret) - goto fail; - for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { if (!needs_modeset(new_crtc_state)) { diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index f4dede6253f8..7eb4b3dbbcb3 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -6582,6 +6582,135 @@ void intel_dp_encoder_reset(struct drm_encoder *encoder) } } +static int intel_modeset_tile_group(struct intel_atomic_state *state, + int tile_group_id) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + struct drm_connector_list_iter conn_iter; + struct drm_connector *connector; + int ret = 0; + + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); + drm_for_each_connector_iter(connector, &conn_iter) { + struct drm_connector_state *conn_state; + struct intel_crtc_state *crtc_state; + struct intel_crtc *crtc; + + if (!connector->has_tile || + connector->tile_group->id != tile_group_id) + continue; + + conn_state = drm_atomic_get_connector_state(&state->base, + connector); + if (IS_ERR(conn_state)) { + ret = PTR_ERR(conn_state); + break; + } + + crtc = to_intel_crtc(conn_state->crtc); + + if (!crtc) + continue; + + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); + if (IS_ERR(crtc_state)) { + ret = PTR_ERR(crtc_state); + break; + } + + crtc_state->uapi.mode_changed = true; + + ret = drm_atomic_add_affected_planes(&state->base, &crtc->base); + if (ret) + break; + } + drm_connector_list_iter_begin(&dev_priv->drm, &conn_iter); + + return ret; +} + +static int intel_modeset_affected_transcoders(struct intel_atomic_state *state, u8 transcoders) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + struct intel_crtc *crtc; + + if (transcoders == 0) + return 0; + + for_each_intel_crtc(&dev_priv->drm, crtc) { + struct intel_crtc_state *crtc_state; + int ret; + + if ((transcoders & BIT(crtc->pipe)) == 0) + continue; + + crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + if (!crtc_state->hw.enable) + continue; + + crtc_state->uapi.mode_changed = true; + + ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base); + if (ret) + return ret; + + WARN_ON((enum transcoder)crtc->pipe != crtc_state->cpu_transcoder); + + transcoders &= ~BIT(crtc_state->cpu_transcoder); + } + + WARN_ON(transcoders != 0); + + return 0; + +} + +static int intel_modeset_synced_crtcs(struct intel_atomic_state *state, + struct drm_connector *connector) +{ + const struct drm_connector_state *old_conn_state = + drm_atomic_get_old_connector_state(&state->base, connector); + const struct intel_crtc_state *old_crtc_state; + struct intel_crtc *crtc; + + crtc = to_intel_crtc(old_conn_state->crtc); + if (!crtc) + return 0; + + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc); + + if (!old_crtc_state->hw.active) + return 0; + + return intel_modeset_affected_transcoders(state, + (old_crtc_state->sync_mode_slaves_mask | + BIT(old_crtc_state->master_transcoder)) & + ~BIT(old_crtc_state->cpu_transcoder)); +} + +static int intel_dp_connector_atomic_check(struct drm_connector *conn, + struct drm_atomic_state *_state) +{ + struct drm_i915_private *dev_priv = to_i915(conn->dev); + struct intel_atomic_state *state = to_intel_atomic_state(_state); + int ret; + + ret = intel_digital_connector_atomic_check(conn, &state->base); + if (ret) + return ret; + + if (INTEL_GEN(dev_priv) >= 11 && conn->has_tile) { + ret = intel_modeset_tile_group(state, conn->tile_group->id); + if (ret) + return ret; + } + + return intel_modeset_synced_crtcs(state, conn); +} + static const struct drm_connector_funcs intel_dp_connector_funcs = { .force = intel_dp_force, .fill_modes = drm_helper_probe_single_connector_modes, @@ -6598,7 +6727,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = .detect_ctx = intel_dp_detect, .get_modes = intel_dp_get_modes, .mode_valid = intel_dp_mode_valid, - .atomic_check = intel_digital_connector_atomic_check, + .atomic_check = intel_dp_connector_atomic_check, }; static const struct drm_encoder_funcs intel_dp_enc_funcs = {
If one of the synced crtcs needs a full modeset, we need to make sure all the synced crtcs are forced a full modeset. Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 88 +------------ drivers/gpu/drm/i915/display/intel_dp.c | 131 ++++++++++++++++++- 2 files changed, 131 insertions(+), 88 deletions(-)