Re: [PATCH] drm/i915/dp_mst: Fix MST state after a sink reset

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 24 Jul 2024, Imre Deak <imre.deak@intel.com> wrote:
> In some cases the sink can reset itself after it was configured into MST
> mode, without the driver noticing the disconnected state. For instance
> the reset may happen in the middle of a modeset, or the (long) HPD pulse
> generated may be not long enough for the encoder detect handler to
> observe the HPD's deasserted state. In this case the sink's DPCD
> register programmed to enable MST will be reset, while the driver still
> assumes MST is still enabled. Detect this condition, which will tear
> down and recreate/re-enable the MST topology.
>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11195
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     |  3 +++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 30 +++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.h |  1 +
>  3 files changed, 34 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 1e43e32e05199..c621f6daf8235 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5878,6 +5878,9 @@ intel_dp_detect(struct drm_connector *connector,
>  	else
>  		status = connector_status_disconnected;
>  
> +	if (!intel_dp_mst_verify_dpcd_state(intel_dp))
> +		status = connector_status_disconnected;
> +
>  	if (status == connector_status_disconnected) {
>  		memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance));
>  		memset(intel_connector->dp.dsc_dpcd, 0, sizeof(intel_connector->dp.dsc_dpcd));
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 27ce5c3f5951e..89b147e37b400 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1998,3 +1998,33 @@ bool intel_dp_mst_crtc_needs_modeset(struct intel_atomic_state *state,
>  
>  	return false;
>  }
> +
> +/**
> + * intel_dp_mst_verify_dpcd_state - verify the MST SW enabled state wrt. the DPCD
> + * @intel_dp: DP port object
> + *
> + * Verify if @intel_dp's MST enabled SW state matches the corresponding DPCD
> + * state. A long HPD pulse -not long enough to be detected as a disconnected
> + * state - could've reset the DPCD state, which requires tearing
> + * down/recreating the MST topology.
> + *
> + * Returns %true if the SW MST enabled and DPCD states match, %false
> + * otherwise.
> + */
> +bool intel_dp_mst_verify_dpcd_state(struct intel_dp *intel_dp)
> +{
> +	int ret;
> +	u8 val;
> +
> +	if (!intel_dp->is_mst)
> +		return true;
> +
> +	ret = drm_dp_dpcd_readb(intel_dp->mst_mgr.aux, DP_MSTM_CTRL, &val);
> +	if (ret < 0)
> +		return false;
> +
> +	if (val != (DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC))

Hmm. This is not comparing the value to any software state, but some
hardcoded combo of values which may or may not match. In particular,
this makes it harder to untangle MST from SST-with-sideband, I think.

BR,
Jani.

> +		return false;
> +
> +	return true;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> index 8ca1d599091c6..9e4c7679f1c3a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
> @@ -27,5 +27,6 @@ int intel_dp_mst_atomic_check_link(struct intel_atomic_state *state,
>  				   struct intel_link_bw_limits *limits);
>  bool intel_dp_mst_crtc_needs_modeset(struct intel_atomic_state *state,
>  				     struct intel_crtc *crtc);
> +bool intel_dp_mst_verify_dpcd_state(struct intel_dp *intel_dp);
>  
>  #endif /* __INTEL_DP_MST_H__ */

-- 
Jani Nikula, Intel