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 <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.
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ä <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
