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@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> ---
> 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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
