Re: [Intel-gfx] [PATCH v2 12/12] drm/i915/tc: Reset TypeC PHYs left enabled in DP-alt mode after the sink disconnects
On Thu, May 04, 2023 at 08:41:49PM +0300, Ville Syrjälä wrote:
> On Thu, May 04, 2023 at 02:10:48AM +0300, Imre Deak wrote:
> > If the output on a DP-alt link with its sink disconnected is kept
> > enabled for too long (about 20 sec), then some IOM/TCSS firmware timeout
> > will cause havoc on the PCI bus, at least for other GFX devices on it
> > which will stop powering up. Since user space is not guaranteed to do a
> > disabling modeset in time, switch such disconnected but active links to
> > TBT mode - which is without such shortcomings - with a 2 second delay.
> >
> > If the above condition is detected already during the driver load/system
> > resume sanitization step disable the output instead, as at that point no
> > user space or kernel client depends on a consistent output state yet and
> > because subsequent atomic modeset on such connectors - without the
> > actual sink capabilities available - can fail.
> >
> > An active/disconnected port as above will also block the HPD status of
> > other active/disconnected ports to get updated (stuck in the connected
> > state), until the former port is disabled, its PHY is disconnected and
> > a ~10 ms delay has elapsed. This means the link state for all TypeC
> > ports/CRTCs must be rechecked after a CRTC is disabled due to the above
> > reason. For this disconnect the PHY synchronously after the CRTC/port is
> > disabled and recheck all CRTCs for the above condition whenever such a
> > port is disabled.
> >
> > To account for a race condition during driver loading where the sink is
> > disconnected after the above sanitization step and before the HPD
> > interrupts get enabled, do an explicit check/link reset if needed from
> > the encoder's late_register hook, which is called after the HPD
> > interrupts are enabled already.
> >
> > v2:
> > - Handle an active/disconnected port blocking the HPD state update of
> > another active/disconnected port.
> > - Cancel the delayed work resetting the link also from the encoder
> > enable/suspend/shutdown hooks.
> > - Rebase on the earlier intel_modeset_lock_ctx_retry() addition,
> > fixing here the missed atomic state reset in case of a retry.
> > - Fix handling of an error return from intel_atomic_get_crtc_state().
> > - Recheck if the port needs to be reset after all the atomic state
> > is locked and async commits are waited on.
> >
> > Cc: Kai-Heng Feng
> > Tested-by: Kai-Heng Feng
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5860
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 68 +--
> > .../drm/i915/display/intel_display_types.h| 2 +
> > drivers/gpu/drm/i915/display/intel_dp.c | 59 +
> > drivers/gpu/drm/i915/display/intel_dp.h | 1 +
> > .../drm/i915/display/intel_modeset_setup.c| 83 ++-
> > drivers/gpu/drm/i915/display/intel_tc.c | 29 +++
> > drivers/gpu/drm/i915/display/intel_tc.h | 1 +
> > 7 files changed, 215 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index eb391fff0f1be..db390b9c824ec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3313,6 +3313,8 @@ static void intel_disable_ddi(struct
> > intel_atomic_state *state,
> > const struct intel_crtc_state *old_crtc_state,
> > const struct drm_connector_state *old_conn_state)
> > {
> > + cancel_delayed_work(_to_dig_port(encoder)->reset_link_work);
> > +
> > intel_hdcp_disable(to_intel_connector(old_conn_state->connector));
> >
> > if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI))
> > @@ -3381,6 +3383,8 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state
> > *state,
> > enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> > bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
> >
> > + cancel_delayed_work(_port->reset_link_work);
> > +
> > if (is_tc_port) {
> > struct intel_crtc *master_crtc =
> > to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -4229,9 +4233,53 @@ static void intel_ddi_encoder_reset(struct
> > drm_encoder *encoder)
> > intel_tc_port_init_mode(dig_port);
> > }
> >
> > +static void intel_ddi_tc_port_reset_link_work(struct work_struct *work)
> > +{
> > + struct intel_digital_port *dig_port =
> > + container_of(work, struct intel_digital_port,
> > reset_link_work.work);
> > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > + struct intel_encoder *encoder = _port->base;
> > +
> > + mutex_lock(>drm.mode_config.mutex);
> > +
> > + if (intel_tc_port_link_needs_reset(dig_port)) {
> > + int ret;
> > +
> > + drm_dbg_kms(>drm,
> > + "[ENCODER:%d:%s] TypeC DP-alt sink disconnected,
> > resetting link\n",
Re: [Intel-gfx] [PATCH v2 12/12] drm/i915/tc: Reset TypeC PHYs left enabled in DP-alt mode after the sink disconnects
On Thu, May 04, 2023 at 02:10:48AM +0300, Imre Deak wrote:
> If the output on a DP-alt link with its sink disconnected is kept
> enabled for too long (about 20 sec), then some IOM/TCSS firmware timeout
> will cause havoc on the PCI bus, at least for other GFX devices on it
> which will stop powering up. Since user space is not guaranteed to do a
> disabling modeset in time, switch such disconnected but active links to
> TBT mode - which is without such shortcomings - with a 2 second delay.
>
> If the above condition is detected already during the driver load/system
> resume sanitization step disable the output instead, as at that point no
> user space or kernel client depends on a consistent output state yet and
> because subsequent atomic modeset on such connectors - without the
> actual sink capabilities available - can fail.
>
> An active/disconnected port as above will also block the HPD status of
> other active/disconnected ports to get updated (stuck in the connected
> state), until the former port is disabled, its PHY is disconnected and
> a ~10 ms delay has elapsed. This means the link state for all TypeC
> ports/CRTCs must be rechecked after a CRTC is disabled due to the above
> reason. For this disconnect the PHY synchronously after the CRTC/port is
> disabled and recheck all CRTCs for the above condition whenever such a
> port is disabled.
>
> To account for a race condition during driver loading where the sink is
> disconnected after the above sanitization step and before the HPD
> interrupts get enabled, do an explicit check/link reset if needed from
> the encoder's late_register hook, which is called after the HPD
> interrupts are enabled already.
>
> v2:
> - Handle an active/disconnected port blocking the HPD state update of
> another active/disconnected port.
> - Cancel the delayed work resetting the link also from the encoder
> enable/suspend/shutdown hooks.
> - Rebase on the earlier intel_modeset_lock_ctx_retry() addition,
> fixing here the missed atomic state reset in case of a retry.
> - Fix handling of an error return from intel_atomic_get_crtc_state().
> - Recheck if the port needs to be reset after all the atomic state
> is locked and async commits are waited on.
>
> Cc: Kai-Heng Feng
> Tested-by: Kai-Heng Feng
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5860
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 68 +--
> .../drm/i915/display/intel_display_types.h| 2 +
> drivers/gpu/drm/i915/display/intel_dp.c | 59 +
> drivers/gpu/drm/i915/display/intel_dp.h | 1 +
> .../drm/i915/display/intel_modeset_setup.c| 83 ++-
> drivers/gpu/drm/i915/display/intel_tc.c | 29 +++
> drivers/gpu/drm/i915/display/intel_tc.h | 1 +
> 7 files changed, 215 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index eb391fff0f1be..db390b9c824ec 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3313,6 +3313,8 @@ static void intel_disable_ddi(struct intel_atomic_state
> *state,
> const struct intel_crtc_state *old_crtc_state,
> const struct drm_connector_state *old_conn_state)
> {
> + cancel_delayed_work(_to_dig_port(encoder)->reset_link_work);
> +
> intel_hdcp_disable(to_intel_connector(old_conn_state->connector));
>
> if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI))
> @@ -3381,6 +3383,8 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state
> *state,
> enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
>
> + cancel_delayed_work(_port->reset_link_work);
> +
> if (is_tc_port) {
> struct intel_crtc *master_crtc =
> to_intel_crtc(crtc_state->uapi.crtc);
> @@ -4229,9 +4233,53 @@ static void intel_ddi_encoder_reset(struct drm_encoder
> *encoder)
> intel_tc_port_init_mode(dig_port);
> }
>
> +static void intel_ddi_tc_port_reset_link_work(struct work_struct *work)
> +{
> + struct intel_digital_port *dig_port =
> + container_of(work, struct intel_digital_port,
> reset_link_work.work);
> + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> + struct intel_encoder *encoder = _port->base;
> +
> + mutex_lock(>drm.mode_config.mutex);
> +
> + if (intel_tc_port_link_needs_reset(dig_port)) {
> + int ret;
> +
> + drm_dbg_kms(>drm,
> + "[ENCODER:%d:%s] TypeC DP-alt sink disconnected,
> resetting link\n",
> + encoder->base.base.id, encoder->base.name);
> + ret = intel_dp_reset_link(encoder);
> + drm_WARN_ON(>drm, ret);
> + }
> +
> +
