On Tue, 2023-01-31 at 17:05 +0200, Imre Deak wrote:
> The DELL P2715Q monitor's MST hub has a payload allocation problem,
LMAO hello yet again, Dell P2715Q. It's been a while.
> where the VCPI used to reserve the last two time slots (at position
> 0x3e, 0x3f) in the hub's payload table, this VCPI can't be reused for
> later payload configurations.
>
> The problem results at least in streams reusing older VCPIs to stay
> blank on the screen and the payload table containing bogus VCPIs
> (repeating the one earlier used to reserve the 0x3e, 0x3f slots) after
> the last reservered slot.
WOW. you know I've been trying for ages to figure out what is up with this
exact monitor and I think I just gave up because it's the only monitor I've
ever seen do this.
(Also yes, I do have two of this exact monitor. I think we even have this
model mentioned in our testcases. I looked up a google photo of it just to
confirm. I think ours is the P2715Qb, but it looks identical and the problem
you're describing sounds identical as well).
This patch looks fine to me, we could probably also put this in the MST
helpers as well if you can think of a way to do that and I can handle testing
it on nouveau/amdgpu, but this is basically the only monitor I've ever seen do
this - so I don't think it's a big deal either way.
either way:
Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
>
> To work around the problem detect this monitor and the condition for the
> problem (when the last two slots get allocated in a commit), force a
> full modeset of the MST topology in the subsequent commit and reset the
> payload table during the latter commit after all payloads have been
> freed.
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_atomic.c | 13 +++--
> drivers/gpu/drm/i915/display/intel_atomic.h | 3 +-
> .../drm/i915/display/intel_display_types.h | 1 +
> drivers/gpu/drm/i915/display/intel_dp.c | 5 +-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 48 +++++++++++++++++--
> 5 files changed, 61 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 0f1c5b9c9a826..04e5f0e0fffa6 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -616,7 +616,8 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
> }
>
> static int modeset_pipe(struct intel_atomic_state *state,
> - struct intel_crtc *crtc, const char *reason)
> + struct intel_crtc *crtc, const char *reason,
> + bool allow_fastset)
> {
> struct drm_i915_private *i915 = to_i915(state->base.dev);
> struct intel_crtc_state *crtc_state;
> @@ -629,6 +630,8 @@ static int modeset_pipe(struct intel_atomic_state *state,
> return PTR_ERR(crtc_state);
>
> crtc_state->uapi.mode_changed = true;
> + if (!allow_fastset)
> + crtc_state->uapi.connectors_changed = true;
> crtc_state->update_pipe = false;
>
> return intel_atomic_add_affected_planes(state, crtc);
> @@ -639,6 +642,7 @@ static int modeset_pipe(struct intel_atomic_state *state,
> * @state: atomic state
> * @connector: connector to add the state for
> * @reason: the reason why the connector needs to be added
> + * @allow_fastset: allow a fastset
> *
> * Add the @connector to the atomic state with its CRTC state and force a modeset
> * on the CRTC if any.
> @@ -648,7 +652,8 @@ static int modeset_pipe(struct intel_atomic_state *state,
> * Returns 0 in case of success, a negative error code on failure.
> */
> int intel_atomic_modeset_connector(struct intel_atomic_state *state,
> - struct intel_connector *connector, const char *reason)
> + struct intel_connector *connector, const char *reason,
> + bool allow_fastset)
> {
> struct drm_i915_private *i915 = to_i915(state->base.dev);
> struct drm_connector_state *conn_state;
> @@ -671,7 +676,7 @@ int intel_atomic_modeset_connector(struct intel_atomic_state *state,
> if (ret)
> return ret;
>
> - return modeset_pipe(state, crtc, reason);
> + return modeset_pipe(state, crtc, reason, allow_fastset);
> }
>
> /**
> @@ -700,7 +705,7 @@ int intel_atomic_modeset_pipe(struct intel_atomic_state *state,
> if (ret)
> return ret;
>
> - return modeset_pipe(state, crtc, reason);
> + return modeset_pipe(state, crtc, reason, true);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index 84295d388e3cb..7778aea8a09fe 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -59,7 +59,8 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> int intel_atomic_modeset_pipe(struct intel_atomic_state *state,
> struct intel_crtc *crtc, const char *reason);
> int intel_atomic_modeset_connector(struct intel_atomic_state *state,
> - struct intel_connector *connector, const char *reason);
> + struct intel_connector *connector, const char *reason,
> + bool allow_fastset);
> int intel_atomic_modeset_all_pipes(struct intel_atomic_state *state,
> const char *reason);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 9ccae7a460200..06d51d2b5e0d6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1656,6 +1656,7 @@ struct intel_dp {
> bool has_audio;
> bool reset_link_params;
> bool use_max_params;
> + bool mst_reset_payload_table;
> u8 dpcd[DP_RECEIVER_CAP_SIZE];
> u8 psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
> u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index d6b0ef38f6563..c157bcd976103 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4689,6 +4689,8 @@ intel_dp_detect(struct drm_connector *connector,
> memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
> memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
>
> + intel_dp->mst_reset_payload_table = false;
> +
> if (intel_dp->is_mst) {
> drm_dbg_kms(&dev_priv->drm,
> "MST device may have disappeared %d vs %d\n",
> @@ -4924,7 +4926,8 @@ static int intel_modeset_tile_group(struct intel_atomic_state *state,
> continue;
>
> ret = intel_atomic_modeset_connector(state, connector,
> - "connector tile group");
> + "connector tile group",
> + true);
> if (ret)
> break;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 08222fc6c5ecd..a9bb339e41987 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -422,9 +422,10 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct drm_connector_list_iter connector_list_iter;
> struct intel_connector *connector_iter;
> + bool reset_payload_table = connector->mst_port->mst_reset_payload_table;
> int ret = 0;
>
> - if (DISPLAY_VER(dev_priv) < 12)
> + if (DISPLAY_VER(dev_priv) < 12 && !reset_payload_table)
> return 0;
>
> if (!intel_connector_needs_modeset(state, &connector->base))
> @@ -437,7 +438,8 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> continue;
>
> ret = intel_atomic_modeset_connector(state, connector_iter,
> - "MST master transcoder");
> + "MST master transcoder",
> + !reset_payload_table);
> if (ret)
> break;
> }
> @@ -531,6 +533,41 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
> intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state);
> }
>
> +static void detect_payload_allocation_bug(const struct drm_dp_mst_topology_state *mst_state,
> + const struct intel_connector *connector,
> + struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> + if (!drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_MST_PAYLOAD_TABLE_RESET_WA))
> + return;
> +
> + if (drm_dp_mst_allocated_time_slots(mst_state) < DP_PAYLOAD_TABLE_SIZE - 2)
> + return;
> +
> + drm_dbg(&i915->drm,
> + "[CONNECTOR:%d:%s] Payload table allocation bug detected\n",
> + connector->base.base.id, connector->base.name);
> +
> + intel_dp->mst_reset_payload_table = true;
> +}
> +
> +static void payload_allocation_bug_wa(const struct intel_connector *connector,
> + struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> + if (!intel_dp->mst_reset_payload_table)
> + return;
> +
> + drm_dbg(&i915->drm,
> + "[CONNECTOR:%d:%s] Resetting payload table due to allocation bug\n",
> + connector->base.base.id, connector->base.name);
> +
> + drm_dp_mst_reset_payload_table(&intel_dp->mst_mgr);
> + intel_dp->mst_reset_payload_table = false;
> +}
> +
> static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
> struct intel_encoder *encoder,
> const struct intel_crtc_state *old_crtc_state,
> @@ -594,10 +631,13 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
>
>
> intel_mst->connector = NULL;
> - if (last_mst_stream)
> + if (last_mst_stream) {
> dig_port->base.post_disable(state, &dig_port->base,
> old_crtc_state, NULL);
>
> + payload_allocation_bug_wa(connector, intel_dp);
> + }
> +
> drm_dbg_kms(&dev_priv->drm, "active links %d\n",
> intel_dp->active_mst_links);
> }
> @@ -662,6 +702,8 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,
> drm_err(&dev_priv->drm, "Failed to create MST payload for %s: %d\n",
> connector->base.name, ret);
>
> + detect_payload_allocation_bug(mst_state, connector, intel_dp);
> +
> /*
> * Before Gen 12 this is not done as part of
> * dig_port->base.pre_enable() and should be done here. For
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
