On Wed, Oct 23, 2024 at 03:15:11PM +0300, Ville Syrjälä wrote: > 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 <[email protected]> > > --- > > 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.
Yes, it's waiting for the crtc only if the connector is enabled. I can rename it to wait_for_connector_hw_done(). > > 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? Ok, will do that. I think it should be backported only after it's been tested and actual problems related to this were reported (I don't know of any). > Otherwise > Reviewed-by: Ville Syrjälä <[email protected]> > > > +{ > > + 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
