[PATCH v3 1/8] drm/i915/dp: Flush modeset commits during connector detection

[Imre Deak]

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 at intel.com>
> > ---
> >  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 at linux.intel.com>
> 
> > +{
> > +	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