On Fri, Dec 14, 2018 at 08:27:02PM +0200, Imre Deak wrote: > Atm HPD disconnect events on TypeC ports will break things, since we'll > switch the TypeC mode (between legacy and disconnected modes as well as > among USB DP alternate, Thunderbolt alternate and disconnected modes) on > the fly from the HPD disconnect interrupt work while the port may be > still active. > > Even if the port happens to be not active during the disconnect we'd > still have a problem during a subsequent modeset or AUX transfer that > could happen regardless of the port's connected state. For instance the > system resume display mode restore code and userspace could perform a > modeset on the port or userspace could start an AUX transfer even if the > port is in disconnected state. > > To fix this keep TypeC legacy ports in legacy mode whenever we're not > suspended. This mode is a static configuration as opposed to the > Thunderbolt and USB DP alternate modes between which we can switch > dynamically. > > We determine if a TypeC port is legacy (wired to a legacy HDMI or a > legacy DP connector) via the VBT DDI port specific USB-TypeC and > Thunderbolt flags. If both these flags are cleared then the port is > configured for legacy mode. > > On such legacy ports we'll run the TypeC PHY connect sequence explicitly > during driver loading and system resume (vs. running the sequence during > HPD processing). The connect will succeed even if the display is not > connected to begin with (or disappears during the suspended state) since > for legacy ports the PORT_TX_DFLEXDPPMS / DP_PHY_MODE_STATUS_COMPLETED > flag is always set (as opposed to the USB DP alternate mode where it > gets set only when a display is connected). > > Correspondingly run the TypeC PHY disconnect sequence during system > suspend and driver unloading. For the unloading case I had to split > up intel_dp_encoder_destroy() to be able to have the 1. flush any > pending encoder work, 2. disconnect TC PHY, 3. call DRM core cleanup and > kfree on the encoder object. > > For now run the PHY disconnect during suspend only for TypeC legacy > ports. We will need to disconnect even in USB DP alternate mode in the > future, but atm we don't have a way to reconnect the port in this mode > during resume if the display disappears while being suspended. So for > now punt on this case. > > Note that we do not disconnect the port during runtime suspend; in > legacy mode there are no shared HW resources (PHY lanes) with other HW > blocks (USB), so no need to release / reacquire these resources as with > USB DP alternate mode. The only reason to disconnect legacy ports during > system suspend is that the PORT_TX_DFLEXDPPMS / > DP_PHY_MODE_STATUS_COMPLETED flag must be rechecked and the port must be > connected again during system resume. We'll also have to turn the check > for this flag into a poll, after figuring out what's the proper timeout > value for it. > > v2: > - Remove the redundant special casing of legacy mode when doing a > disconnect in icl_tc_port_connected(). It's guaranteed already that we > won't disconnect legacy ports in that function. > - Add a note about the new intel_ddi_encoder_destroy() hook. > - Reword the commit message after switching to the VBT based detection. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> (just because you asked about what I feel non-symmetric I will write here, but I do believe your versions is good and optimal and my "symmetric" one would be just bad OCD. the asymmetric part I see is: ddi_reset -> dp_reset ddi_destroy -> flush dp_destroy -> flush a more symmetric way would be ddi_reset -> dp_reset ddi_destroy -> dp_destroy or ddi_reset -> some_common_reset dp_reset -> some_common_reset ddi_destroy -> flush dp_destroy -> flush ) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108070 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108924 > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_ddi.c | 63 +++++++++++++++++++++++++++++++++++----- > drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++----- > drivers/gpu/drm/i915/intel_drv.h | 5 +++- > 3 files changed, 73 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index f3e1d6a0b7dd..295d75c50688 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3901,9 +3901,50 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder, > > } > > +static void intel_ddi_encoder_suspend(struct intel_encoder *encoder) > +{ > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > + > + intel_dp_encoder_suspend(encoder); > + > + /* > + * TODO: disconnect also from USB DP alternate mode once we have a > + * way to handle the modeset restore in that mode during resume > + * even if the sink has disappeared while being suspended. > + */ > + if (dig_port->tc_legacy_port) > + icl_tc_phy_disconnect(i915, dig_port); > +} > + > +static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder) > +{ > + struct intel_digital_port *dig_port = enc_to_dig_port(drm_encoder); > + struct drm_i915_private *i915 = to_i915(drm_encoder->dev); > + > + if (intel_port_is_tc(i915, dig_port->base.port)) > + intel_digital_port_connected(&dig_port->base); > + > + intel_dp_encoder_reset(drm_encoder); > +} > + > +static void intel_ddi_encoder_destroy(struct drm_encoder *encoder) > +{ > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > + struct drm_i915_private *i915 = to_i915(encoder->dev); > + > + intel_dp_encoder_flush_work(encoder); > + > + if (intel_port_is_tc(i915, dig_port->base.port)) > + icl_tc_phy_disconnect(i915, dig_port); > + > + drm_encoder_cleanup(encoder); > + kfree(dig_port); > +} > + > static const struct drm_encoder_funcs intel_ddi_funcs = { > - .reset = intel_dp_encoder_reset, > - .destroy = intel_dp_encoder_destroy, > + .reset = intel_ddi_encoder_reset, > + .destroy = intel_ddi_encoder_destroy, > }; > > static struct intel_connector * > @@ -4147,16 +4188,16 @@ intel_ddi_max_lanes(struct intel_digital_port *intel_dport) > > void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > { > + struct ddi_vbt_port_info *port_info = > + &dev_priv->vbt.ddi_port_info[port]; > struct intel_digital_port *intel_dig_port; > struct intel_encoder *intel_encoder; > struct drm_encoder *encoder; > bool init_hdmi, init_dp, init_lspcon = false; > enum pipe pipe; > > - > - init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || > - dev_priv->vbt.ddi_port_info[port].supports_hdmi); > - init_dp = dev_priv->vbt.ddi_port_info[port].supports_dp; > + init_hdmi = port_info->supports_dvi || port_info->supports_hdmi; > + init_dp = port_info->supports_dp; > > if (intel_bios_is_lspcon_present(dev_priv, port)) { > /* > @@ -4197,7 +4238,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > intel_encoder->post_disable = intel_ddi_post_disable; > intel_encoder->get_hw_state = intel_ddi_get_hw_state; > intel_encoder->get_config = intel_ddi_get_config; > - intel_encoder->suspend = intel_dp_encoder_suspend; > + intel_encoder->suspend = intel_ddi_encoder_suspend; > intel_encoder->get_power_domains = intel_ddi_get_power_domains; > intel_encoder->type = INTEL_OUTPUT_DDI; > intel_encoder->power_domain = intel_port_to_power_domain(port); > @@ -4216,6 +4257,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > intel_dig_port->max_lanes = intel_ddi_max_lanes(intel_dig_port); > intel_dig_port->aux_ch = intel_bios_port_aux_ch(dev_priv, port); > > + intel_dig_port->tc_legacy_port = intel_port_is_tc(dev_priv, port) && > + !port_info->supports_typec_usb && > + !port_info->supports_tbt; > + > switch (port) { > case PORT_A: > intel_dig_port->ddi_io_power_domain = > @@ -4274,6 +4319,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > } > > intel_infoframe_init(intel_dig_port); > + > + if (intel_port_is_tc(dev_priv, port)) > + intel_digital_port_connected(intel_encoder); > + > return; > > err: > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 082594bb65a7..b2a3012478ca 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5108,9 +5108,6 @@ static void icl_update_tc_port_type(struct drm_i915_private *dev_priv, > tc_type_name(intel_dig_port->tc_type)); > } > > -static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv, > - struct intel_digital_port *dig_port); > - > /* > * This function implements the first part of the Connect Flow described by our > * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading > @@ -5145,6 +5142,7 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv, > val = I915_READ(PORT_TX_DFLEXDPPMS); > if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) { > DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port); > + WARN_ON(dig_port->tc_legacy_port); > return false; > } > > @@ -5176,8 +5174,8 @@ static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv, > * See the comment at the connect function. This implements the Disconnect > * Flow. > */ > -static void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv, > - struct intel_digital_port *dig_port) > +void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv, > + struct intel_digital_port *dig_port) > { > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port); > > @@ -5222,7 +5220,8 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, > bool is_legacy, is_typec, is_tbt; > u32 dpsp; > > - is_legacy = I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port); > + is_legacy = intel_dig_port->tc_legacy_port || > + I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port); > > /* > * The spec says we shouldn't be using the ISR bits for detecting > @@ -5234,6 +5233,7 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, > > if (!is_legacy && !is_typec && !is_tbt) { > icl_tc_phy_disconnect(dev_priv, intel_dig_port); > + > return false; > } > > @@ -5542,7 +5542,7 @@ intel_dp_connector_unregister(struct drm_connector *connector) > intel_connector_unregister(connector); > } > > -void intel_dp_encoder_destroy(struct drm_encoder *encoder) > +void intel_dp_encoder_flush_work(struct drm_encoder *encoder) > { > struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); > struct intel_dp *intel_dp = &intel_dig_port->dp; > @@ -5565,9 +5565,14 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) > } > > intel_dp_aux_fini(intel_dp); > +} > + > +static void intel_dp_encoder_destroy(struct drm_encoder *encoder) > +{ > + intel_dp_encoder_flush_work(encoder); > > drm_encoder_cleanup(encoder); > - kfree(intel_dig_port); > + kfree(enc_to_dig_port(encoder)); > } > > void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d08f08f607dd..97c422bea7fb 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1234,6 +1234,7 @@ struct intel_digital_port { > /* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */ > enum aux_ch aux_ch; > enum intel_display_power_domain ddi_io_power_domain; > + bool tc_legacy_port:1; > enum tc_port_type tc_type; > > void (*write_infoframe)(struct intel_encoder *encoder, > @@ -1806,7 +1807,7 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp, > bool enable); > void intel_dp_encoder_reset(struct drm_encoder *encoder); > void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); > -void intel_dp_encoder_destroy(struct drm_encoder *encoder); > +void intel_dp_encoder_flush_work(struct drm_encoder *encoder); > bool intel_dp_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > struct drm_connector_state *conn_state); > @@ -1874,6 +1875,8 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp); > int intel_dp_link_required(int pixel_clock, int bpp); > int intel_dp_max_data_rate(int max_link_clock, int max_lanes); > bool intel_digital_port_connected(struct intel_encoder *encoder); > +void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv, > + struct intel_digital_port *dig_port); > > /* intel_dp_aux_backlight.c */ > int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector); > -- > 2.13.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
