On Thu, Dec 13, 2018 at 01:06:51PM -0800, Rodrigo Vivi wrote: > On Thu, Dec 13, 2018 at 09:48:49PM +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 legacy TypeC ports in TypeC legacy mode whenever we're > > not suspended. The legacy mode is a static configuration as opposed to > > the Thunderbolt and USB DP alternate modes between which we can switch > > dynamically. > > > > We don't have yet an explicit way to detect TypeC legacy ports, but we > > can imply that at least in case of HDMI enabled ports, since HDMI can > > only be provided in legacy mode. So mark TypeC HDMI enabled ports as > > legacy and run the TypeC PHY connect sequence explicitly during driver > > loading and system resume. 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, but disconnect during suspend only for > > legacy TypeC mode. 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 may also need to turn the check > > for this flag into a poll, depending on further tests and clarifications > > we are expecting from HW/FW people. > > > > If VBT says that the port provides only DP functionality then we can't > > assume that it's a legacy port as for HDMI (since it could as well be > > a TBT/DP Alt connector), so we'll solve HPD handling for the DP case > > with a different method in the next patch. > > > > Eventually - instead of the above method - we'll depend on an explicit > > detection method provided either via a VBT flag or a FW/HW register both > > for the HDMI and DP case. > > > > 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 | 54 +++++++++++++++++++++++++++++++++++++--- > > drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++------- > > drivers/gpu/drm/i915/intel_drv.h | 5 +++- > > 3 files changed, 75 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index f3e1d6a0b7dd..2e47ffa1c95a 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); > > do we need all of that? You mean if we need to call intel_dp_encoder_reset()? That was the original state, as before this change we had .reset = intel_dp_encoder_reset in intel_ddi_funcs. > or could simply > > intel_dp->reset_link_params = true; > > instead?! Not sure how we could avoid calling intel_dp_encoder_reset() here, it has required LSPCON and eDP specific parts. > > > +} > > + > > +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); > > and here don't we need stuff on intel_dp_encoder destroy here like > intel_dp_aux_fini? Yes we do, that's part of the new intel_dp_encoder_flush_work() func. Had to split up intel_dp_encoder_destroy(), since we want to flush any pending work on the encoder first (for instance MST specific stuff) then disconnect the TC PHY, then do the DRM core cleanup and kfree. Probably should've been in the commit log. > > > + 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 * > > @@ -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); > > @@ -4257,6 +4298,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > > if (intel_encoder->type != INTEL_OUTPUT_EDP && init_hdmi) { > > if (!intel_ddi_init_hdmi_connector(intel_dig_port)) > > goto err; > > + > > + if (intel_port_is_tc(dev_priv, port)) > > + intel_dig_port->tc_legacy_port = true; > > } > > > > if (init_lspcon) { > > @@ -4274,6 +4318,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..19e49adab548 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 > > @@ -5233,7 +5232,14 @@ static bool icl_tc_port_connected(struct drm_i915_private *dev_priv, > > is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port); > > > > if (!is_legacy && !is_typec && !is_tbt) { > > - icl_tc_phy_disconnect(dev_priv, intel_dig_port); > > + /* > > + * We disconnect from legacy mode only during system > > + * suspend and driver unloading, otherwise we keep the port in > > + * legacy mode even after an HPD disconnect event. > > + */ > > + if (intel_dig_port->tc_type != TC_PORT_LEGACY) > > + icl_tc_phy_disconnect(dev_priv, intel_dig_port); > > + > > return false; > > } > > > > @@ -5542,7 +5548,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 +5571,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
