On Sun, Dec 16, 2018 at 11:05:27PM -0800, Rodrigo Vivi wrote: > On Sat, Dec 15, 2018 at 01:25:45AM +0200, Imre Deak wrote: > > On Fri, Dec 14, 2018 at 02:22:09PM -0800, Rodrigo Vivi wrote: > > > On Fri, Dec 14, 2018 at 01:25:07AM +0200, Imre Deak wrote: > > > > 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 > > > > > > yes. > > > > > > > > > > > .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. > > > > > > I had in my mind the type_c only, sorry. > > > > > > But now what I don't understand is why we don't need to do the same > > > and call intel_dp_encoder_destroy from inside intel_ddi_encoder_destroy. > > > it seems that we will loose some LSPCON and eDP there as well... > > > > That stuff moved to intel_dp_encoder_flush_work() as explained below, > > which is called from intel_ddi_encoder_destroy(). > > hmmm, now I saw this part of the move. sorry... > I think the non-symmetric name got a bit confusing, hm, not sure what you mean. After this change we have intel_dp_encoder_destroy() that pre-DDI platforms set as their encoder .destroy hook and we have intel_ddi_encoder_destroy() that DDI platforms set as their encoder .destroy hook. Both intel_dp_encoder_destroy() and intel_ddi_encoder_destroy() will call in turn intel_dp_encoder_flush_work(). So not sure what part you think is non-symmetric. > but the code is indeed right, so: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> Thanks. I sent a v2 patchset with a minor change in this and the last patch, does the r-b still apply on those? --Imre > > > > > > > > > > > > > > > > > > > > > +} > > > > > > + > > > > > > +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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
