| Message ID | 20241016132405.2231744-2-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Write source OUI for non-eDP sinks | expand |
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@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. > 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@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
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@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@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
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(struct drm_i915_private *i915, struct drm_crtc_commit *commit) +{ + 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); }
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@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(-)