On Wed, Oct 16, 2024 at 04:23:58PM +0300, Imre Deak wrote:
> Make sure that a DP connector detection doesn't happen in parallel
> with an ongoing modeset on the connector. The reasons for this are:
>
> - Besides reading the capabilities, EDID etc. the detection may change
> the state of the sink (via the AUX bus), for instance by setting the
> LTTPR mode or the source OUI (the latter introduced by an upcoming
> patch). It's better to avoid such changes affecting an onging modeset
> in any way.
>
> - During a modeset's link training any access to DPCD registers, besides
> the registers used for link training should be avoided, at least in
> the LTTPR non-transparent and transparent link training modes.
>
> Such asynchronous accesses - besides connector detection - can also
> happen via the AUX device node for instance, for those a parallel
> modeset will have to be avoided in a similar way to the change in this
> patch. (A topic for a follow-up change.)
>
> - The source OUI written to an eDP sink is valid only while the panel
> power is enabled. A modeset on eDP will enable/disable the panel power
> synchronously; this should be prevented in the middle of the connector
> detection, to ensure a consistent sink state (which depends on the
> source OUI) for the whole duration of detection. The panel power could
> still get disabled during detection after an idle period (1 sec), this
> will be prevented by the next patch.
>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 28 ++++++++++++++++++---
> drivers/gpu/drm/i915/display/intel_dp.h | 1 +
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++
> 3 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6b27fabd61c37..977ff2ce18eeb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5035,6 +5035,16 @@ bool intel_dp_has_connector(struct intel_dp *intel_dp,
> return false;
> }
>
> +static void wait_for_crtc_hw_done(
This doesn't really do anything with the crtc, so the name feels
a bit off.
> struct drm_i915_private *i915, struct drm_crtc_commit *commit)
'struct intel_display' everywhere? Or are you looking to backport this
so far back that we don't have that?
Otherwise
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> +{
> + if (!commit)
> + return;
> +
> + drm_WARN_ON(&i915->drm,
> + !wait_for_completion_timeout(&commit->hw_done,
> + msecs_to_jiffies(5000)));
> +}
> +
> int intel_dp_get_active_pipes(struct intel_dp *intel_dp,
> struct drm_modeset_acquire_ctx *ctx,
> u8 *pipe_mask)
> @@ -5071,10 +5081,7 @@ int intel_dp_get_active_pipes(struct intel_dp *intel_dp,
> if (!crtc_state->hw.active)
> continue;
>
> - if (conn_state->commit)
> - drm_WARN_ON(&i915->drm,
> - !wait_for_completion_timeout(&conn_state->commit->hw_done,
> - msecs_to_jiffies(5000)));
> + wait_for_crtc_hw_done(i915, conn_state->commit);
>
> *pipe_mask |= BIT(crtc->pipe);
> }
> @@ -5083,6 +5090,17 @@ int intel_dp_get_active_pipes(struct intel_dp *intel_dp,
> return ret;
> }
>
> +void intel_dp_flush_connector_commits(struct intel_connector *connector)
> +{
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> + const struct drm_connector_state *conn_state =
> + connector->base.state;
> +
> + drm_modeset_lock_assert_held(&i915->drm.mode_config.connection_mutex);
> +
> + return wait_for_crtc_hw_done(i915, conn_state->commit);
> +}
> +
> static bool intel_dp_is_connected(struct intel_dp *intel_dp)
> {
> struct intel_connector *connector = intel_dp->attached_connector;
> @@ -5596,6 +5614,8 @@ intel_dp_detect(struct drm_connector *connector,
> if (!intel_display_driver_check_access(dev_priv))
> return connector->status;
>
> + intel_dp_flush_connector_commits(intel_connector);
> +
> /* Can't disconnect eDP */
> if (intel_dp_is_edp(intel_dp))
> status = edp_detect(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 60baf4072dc9d..4efb9605a50e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -54,6 +54,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
> int intel_dp_get_active_pipes(struct intel_dp *intel_dp,
> struct drm_modeset_acquire_ctx *ctx,
> u8 *pipe_mask);
> +void intel_dp_flush_connector_commits(struct intel_connector *connector);
> void intel_dp_link_check(struct intel_encoder *encoder);
> void intel_dp_check_link_state(struct intel_dp *intel_dp);
> void intel_dp_set_power(struct intel_dp *intel_dp, u8 mode);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 1a2ff3e1cb68f..5bba078c00d89 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1573,6 +1573,8 @@ intel_dp_mst_detect(struct drm_connector *connector,
> if (!intel_display_driver_check_access(i915))
> return connector->status;
>
> + intel_dp_flush_connector_commits(intel_connector);
> +
> return drm_dp_mst_detect_port(connector, ctx, &intel_dp->mst_mgr,
> intel_connector->port);
> }
> --
> 2.44.2
--
Ville Syrjälä
Intel
