Re: [PATCH 2/3] drm/i915/icl: Fix TypeC legacy HDMI HPD handling

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

 



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? or could simply

intel_dp->reset_link_params = true;

instead?! 

> +}
> +
> +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?

> +	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




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

  Powered by Linux