Re: [PATCH v2 3/4] drm/i915/icl: Fix HPD handling for TypeC legacy ports

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 17, 2018 at 10:05:40AM -0800, Rodrigo Vivi wrote:
> 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

Ok, I see what you mean now, but not sure if it's really an issue. As I
see the assymetry comes from the fact that intel_dp_encoder_reset() can be
reused by the ddi code as a whole while intel_dp_encoder_destroy() can't be
reused by the ddi code. IOW ..

> 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

.. dp_reset() is just the same as 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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux