From: Lyude Paul <lyude@redhat.com>
To: Imre Deak <imre.deak@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 16/17] drm/i915/dp_mst: Add workaround for a DELL P2715Q payload allocation problem
Date: Tue, 31 Jan 2023 17:47:11 -0500 [thread overview]
Message-ID: <4add821aebae4f796a808d9dfe9ad85e83cac128.camel@redhat.com> (raw)
In-Reply-To: <20230131150548.1614458-17-imre.deak@intel.com>
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@redhat.com>
>
> 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@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> 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
next prev parent reply other threads:[~2023-01-31 22:47 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-31 15:05 [Intel-gfx] [PATCH v2 00/17] drm/i915: drm/i915/dp_mst: Fix MST payload removal during output disabling Imre Deak
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 01/17] drm/i915/dp_mst: Add the MST topology state for modesetted CRTCs Imre Deak
2023-01-31 15:05 ` Imre Deak
2023-01-31 15:05 ` [PATCH v2 02/17] drm/display/dp_mst: Handle old/new payload states in drm_dp_remove_payload() Imre Deak
2023-01-31 15:05 ` Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 23:13 ` Lyude Paul
2023-01-31 23:13 ` Lyude Paul
2023-01-31 23:13 ` [Intel-gfx] " Lyude Paul
2023-02-01 15:04 ` Imre Deak
2023-02-01 15:04 ` Imre Deak
2023-02-01 15:04 ` [Intel-gfx] " Imre Deak
2023-02-07 0:42 ` Lyude Paul
2023-02-07 0:42 ` Lyude Paul
2023-02-07 0:42 ` Lyude Paul
2023-02-07 12:11 ` Imre Deak
2023-02-07 12:11 ` [Intel-gfx] " Imre Deak
2023-02-07 12:11 ` Imre Deak
2023-02-08 0:21 ` [Intel-gfx] " Lyude Paul
2023-02-08 0:21 ` Lyude Paul
2023-02-08 0:21 ` Lyude Paul
2023-02-08 7:41 ` Imre Deak
2023-02-08 7:41 ` [Intel-gfx] " Imre Deak
2023-02-08 7:41 ` Imre Deak
2023-02-09 21:43 ` Lyude Paul
2023-02-09 21:43 ` [Intel-gfx] " Lyude Paul
2023-02-09 21:43 ` Lyude Paul
2023-01-31 15:05 ` [PATCH v2 03/17] drm/display/dp_mst: Add drm_atomic_get_old_mst_topology_state() Imre Deak
2023-01-31 15:05 ` Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 04/17] drm/i915/dp_mst: Fix payload removal during output disabling Imre Deak
2023-01-31 15:05 ` Imre Deak
2023-01-31 15:05 ` [PATCH v2 05/17] drm/display/dp_mst: Fix the payload VCPI check in drm_dp_mst_dump_topology() Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 06/17] drm/display/dp_mst: Sanitize payload iteration " Imre Deak
2023-01-31 23:14 ` Lyude Paul
2023-02-03 12:22 ` Ville Syrjälä
2023-02-03 13:12 ` Imre Deak
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 07/17] drm/i915: Factor out helpers for modesetting CRTCs and connectors Imre Deak
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 08/17] drm/i915/dp_mst: Move getting the MST topology state earlier to connector check Imre Deak
2023-01-31 15:05 ` [PATCH v2 09/17] drm/display/dp_mst: Add a helper to verify the MST payload state Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 10/17] drm/i915/dp_mst: Verify the MST state of modesetted outputs Imre Deak
2023-01-31 15:05 ` [PATCH v2 11/17] drm/display/dp_mst: Add helpers to query for payload allocation errors Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-02-02 10:13 ` kernel test robot
2023-02-02 12:15 ` [Intel-gfx] " Dan Carpenter
2023-02-02 12:15 ` Dan Carpenter
2023-02-02 12:15 ` Dan Carpenter
2023-02-02 12:35 ` Imre Deak
2023-02-02 12:35 ` [Intel-gfx] " Imre Deak
2023-02-02 12:35 ` Imre Deak
2023-01-31 15:05 ` [PATCH v2 12/17] drm/display/dp_mst: Add helpers to query payload allocation properties Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 15:05 ` [PATCH v2 13/17] drm/display/dp_mst: Export the DP_PAYLOAD_TABLE_SIZE definition Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 15:05 ` [PATCH v2 14/17] drm/display/dp_mst: Factor out a helper to reset the payload table Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 15:05 ` [PATCH v2 15/17] drm/dp: Add a quirk for a DELL P2715Q MST payload allocation problem Imre Deak
2023-01-31 15:05 ` [Intel-gfx] " Imre Deak
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 16/17] drm/i915/dp_mst: Add workaround for a DELL P2715Q " Imre Deak
2023-01-31 22:47 ` Lyude Paul [this message]
2023-02-01 14:41 ` Imre Deak
2023-01-31 23:43 ` Lyude Paul
2023-01-31 15:05 ` [Intel-gfx] [PATCH v2 17/17] drm/i915/dp_mst: Verify the HW state of MST encoders Imre Deak
2023-02-01 9:41 ` Jani Nikula
2023-02-01 14:03 ` Imre Deak
2023-01-31 15:27 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: drm/i915/dp_mst: Fix MST payload removal during output disabling Patchwork
2023-01-31 15:59 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-01-31 20:42 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4add821aebae4f796a808d9dfe9ad85e83cac128.camel@redhat.com \
--to=lyude@redhat.com \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.