[Intel-gfx] [PATCH v2 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Dec 20 18:41:12 UTC 2019
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 at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Signed-off-by: Manasi Navare <manasi.d.navare at 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
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list