Re: [PATCH 13/23] drm/i915: Fix the TypeC port mode sanitization during loading/resume

[Imre Deak <imre.deak@intel.com>]

On Sat, Jun 08, 2019 at 01:39:36AM +0300, Souza, Jose wrote:
> On Tue, 2019-06-04 at 17:58 +0300, Imre Deak wrote:
> > For using the correct AUX power domains we have to sanitize the TypeC
> > port mode early, so move that before encoder sanitization. To do this
> > properly read out the actual port mode instead of just relying on the
> > VBT legacy port flag (which can be incorrect).
> > 
> > We also verify that the PHY is connected as expected if the port is
> > active. In case the port is inactive we connect the PHY in case of a
> > legacy port - as we did so far. The PHY will be connected during
> > detection for DP-alt mode - as it was done so far. For TBT-alt mode
> > nothing needs to be done to connect the PHY.
> > 
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c     | 16 +-----
> >  drivers/gpu/drm/i915/intel_display.c | 10 ++++
> >  drivers/gpu/drm/i915/intel_dp_mst.h  |  8 ++-
> >  drivers/gpu/drm/i915/intel_tc.c      | 83
> > ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_tc.h      |  2 +
> >  5 files changed, 103 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index d236839bee19..2bc3b4f2c9a5 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3918,17 +3918,6 @@ static void intel_ddi_encoder_suspend(struct
> > intel_encoder *encoder)
> >  		icl_tc_phy_disconnect(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);
> > @@ -3944,7 +3933,7 @@ static void intel_ddi_encoder_destroy(struct
> > drm_encoder *encoder)
> >  }
> >  
> >  static const struct drm_encoder_funcs intel_ddi_funcs = {
> > -	.reset = intel_ddi_encoder_reset,
> > +	.reset = intel_dp_encoder_reset,
> >  	.destroy = intel_ddi_encoder_destroy,
> >  };
> >  
> > @@ -4309,9 +4298,6 @@ 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_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 0e425a6eebf3..91d18cd0371c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -75,6 +75,7 @@
> >  #include "intel_sdvo.h"
> >  #include "intel_sideband.h"
> >  #include "intel_sprite.h"
> > +#include "intel_tc.h"
> >  #include "intel_tv.h"
> >  #include "intel_vdsc.h"
> >  
> > @@ -16683,6 +16684,15 @@ intel_modeset_setup_hw_state(struct
> > drm_device *dev,
> >  	intel_modeset_readout_hw_state(dev);
> >  
> >  	/* HW state is read out, now we need to sanitize this mess. */
> > +
> > +	/* Sanitize the TypeC port mode upfront, encoders depend on
> > this */
> > +	for_each_intel_encoder(dev, encoder) {
> > +		/* We need to sanitize only the MST primary port. */
> > +		if (encoder->type != INTEL_OUTPUT_DP_MST &&
> > +		    intel_port_is_tc(dev_priv, encoder->port))
> > +			intel_tc_port_sanitize(enc_to_dig_port(&encoder
> > ->base));
> > +	}
> > +
> >  	get_encoder_power_domains(dev_priv);
> >  
> >  	if (HAS_PCH_IBX(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.h
> > b/drivers/gpu/drm/i915/intel_dp_mst.h
> > index 1470c6e0514b..6754c211205a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.h
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.h
> > @@ -6,9 +6,15 @@
> >  #ifndef __INTEL_DP_MST_H__
> >  #define __INTEL_DP_MST_H__
> >  
> > -struct intel_digital_port;
> > +#include "intel_drv.h"
> >  
> >  int intel_dp_mst_encoder_init(struct intel_digital_port
> > *intel_dig_port, int conn_id);
> >  void intel_dp_mst_encoder_cleanup(struct intel_digital_port
> > *intel_dig_port);
> > +static inline int
> > +intel_dp_mst_encoder_active_links(struct intel_digital_port
> > *intel_dig_port)
> > +{
> > +	return intel_dig_port->dp.active_mst_links;
> > +}
> > +
> >  
> >  #endif /* __INTEL_DP_MST_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_tc.c
> > b/drivers/gpu/drm/i915/intel_tc.c
> > index 9832e2ddb92e..9ebf25d7931c 100644
> > --- a/drivers/gpu/drm/i915/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/intel_tc.c
> > @@ -3,6 +3,7 @@
> >   * Copyright © 2019 Intel Corporation
> >   */
> >  #include "intel_display.h"
> > +#include "intel_dp_mst.h"
> >  #include "i915_drv.h"
> >  #include "intel_tc.h"
> >  
> > @@ -167,6 +168,15 @@ static bool icl_tc_phy_set_safe_mode(struct
> > intel_digital_port *dig_port,
> >  	return true;
> >  }
> >  
> > +static bool icl_tc_phy_is_in_safe_mode(struct intel_digital_port
> > *dig_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port-
> > >base.base.dev);
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-
> > >base.port);
> > +
> > +	return !(I915_READ(PORT_TX_DFLEXDPCSSS) &
> > +		 DP_PHY_MODE_STATUS_NOT_SAFE(tc_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
> > @@ -250,6 +260,49 @@ void icl_tc_phy_disconnect(struct
> > intel_digital_port *dig_port)
> >  	}
> >  }
> >  
> > +static bool icl_tc_phy_is_connected(struct intel_digital_port
> > *dig_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port-
> > >base.base.dev);
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-
> > >base.port);
> > +
> > +	if (!icl_tc_phy_status_complete(dig_port)) {
> > +		DRM_DEBUG_KMS("Port %s: PHY status not complete\n",
> > +			      tc_port_name(dev_priv, tc_port));
> > +		return dig_port->tc_mode == TC_PORT_TBT_ALT;
> > +	}
> > +
> > +	if (icl_tc_phy_is_in_safe_mode(dig_port)) {
> > +		DRM_DEBUG_KMS("Port %s: PHY still in safe mode\n",
> > +			      tc_port_name(dev_priv, tc_port));
> > +
> > +		return false;
> > +	}
> > +
> > +	return dig_port->tc_mode == TC_PORT_DP_ALT ||
> > +	       dig_port->tc_mode == TC_PORT_LEGACY;
> > +}
> > +
> > +static enum tc_port_mode
> > +intel_tc_port_get_current_mode(struct intel_digital_port *dig_port)
> > +{
> > +	u32 live_status_mask = tc_port_live_status_mask(dig_port);
> > +	bool in_safe_mode = icl_tc_phy_is_in_safe_mode(dig_port);
> > +	enum tc_port_mode mode;
> > +
> > +	if (in_safe_mode ||
> > WARN_ON(!icl_tc_phy_status_complete(dig_port)))
> > +		return TC_PORT_TBT_ALT;
> > +
> > +	mode = dig_port->tc_legacy_port ? TC_PORT_LEGACY :
> > TC_PORT_DP_ALT;
> > +	if (live_status_mask) {
> > +		enum tc_port_mode live_mode = fls(live_status_mask) -
> > 1;
> > +
> > +		if (!WARN_ON(live_mode == TC_PORT_TBT_ALT))
> > +			mode = live_mode;
> > +	}
> > +
> > +	return mode;
> > +}
> > +
> >  static enum tc_port_mode
> >  intel_tc_port_get_target_mode(struct intel_digital_port *dig_port)
> >  {
> > @@ -278,6 +331,36 @@ static void intel_tc_port_reset_mode(struct
> > intel_digital_port *dig_port)
> >  		      tc_port_mode_name(dig_port->tc_mode));
> >  }
> >  
> > +void intel_tc_port_sanitize(struct intel_digital_port *dig_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port-
> > >base.base.dev);
> > +	struct intel_encoder *encoder = &dig_port->base;
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, encoder-
> > >port);
> > +	int active_links = 0;
> > +
> > +	dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port);
> > +	if (dig_port->dp.is_mst)
> > +		active_links =
> > intel_dp_mst_encoder_active_links(dig_port);
> > +	else if (encoder->base.crtc)
> > +		active_links = to_intel_crtc(encoder->base.crtc)-
> > >active;
> > +
> > +	if (active_links) {
> > +		if (!icl_tc_phy_is_connected(dig_port))
> > +			DRM_DEBUG_DRIVER("Port %s: PHY disconnected
> > with %d active link(s)\n",
> > +					 tc_port_name(dev_priv,
> > tc_port),
> > +					 active_links);
> > +		goto out;
> 
> And then probe sequences will take care of call
> intel_digital_port_connected() to reset TC state. 

Right, once the active mode (we detected here) gets disabled, since
before that you can't reset the mode. Btw, we'll also check if a mode
reset is needed before AUX transfers (if the port is not active).

> 
> > +	}
> > +
> > +	if (dig_port->tc_legacy_port)
> > +		icl_tc_phy_connect(dig_port);
> > +
> > +out:
> > +	DRM_DEBUG_DRIVER("Port %s: sanitize mode (%s)\n",
> > +			 tc_port_name(dev_priv, tc_port),
> > +			 tc_port_mode_name(dig_port->tc_mode));
> 
> Should be:
> 
> DRM_DEBUG_DRIVER()
> 
> I guess I missed this on previous patches too.

Ah right, DRM_DEBUG_KMS().

> 
> 
> With that:
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> 
> > +}
> > +
> >  static bool
> >  intel_tc_port_needs_reset(struct intel_digital_port *dig_port)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_tc.h
> > b/drivers/gpu/drm/i915/intel_tc.h
> > index e937f5326959..10f0f0d81ee4 100644
> > --- a/drivers/gpu/drm/i915/intel_tc.h
> > +++ b/drivers/gpu/drm/i915/intel_tc.h
> > @@ -11,4 +11,6 @@ bool intel_tc_port_connected(struct
> > intel_digital_port *dig_port);
> >  u32 intel_tc_port_get_lane_info(struct intel_digital_port
> > *dig_port);
> >  int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> > *dig_port);
> >  
> > +void intel_tc_port_sanitize(struct intel_digital_port *dig_port);
> > +
> >  #endif /* __INTEL_TC_H__ */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx