Re: [PATCH v3] drm/i915/ddi: Get AUX power domain for DP main link too

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

 



On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> So far we got an AUX power domain reference only for the duration of
> DP
> AUX transfers. However, the following suggests that we also need
> these
> for main link functionality:
> - The specification doesn't state whether it's needed or not for main
>   link functionality, but suggests that these power wells need to be
>   enabled already during display core initialization (Sequences to
>   Initialize Display).
> - For PSR we need to keep the AUX power well enabled.
> - On ICL combo PHY ports (non-TC) the AUX power well is needed for
>   link training too: while the port is enabled with a DP link
> training
>   test pattern trying to toggle the AUX power well will time out.
> - On ICL MG PHY ports (TC) the AUX power well is needed also for main
>   link functionality (both in DP and HDMI modes).
> - Windows enables these power wells both for main and AUX lane
>   functionality.
> 
> Based on the above take an AUX power reference for main link
> functionality too. This makes a difference only on GEN10+ (GLK+)
> platforms, where we have separate port specific AUX power wells.
> 
> For PSR we still need to distinguish between port A and the other
> ports, since on port A DC states must stay enabled for main link
> functionality, but DC states must be disabled for driver initiated
> AUX transfers. So re-use the corresponding helper from intel_psr.c.
> 
> Since we take now a reference for main link functionality on all DP
> ports we can forgo taking the separate power ref for PSR
> functionality.
> 
> v2:
> - Make sure DC states stay enabled when taking the ref on port A.
>   (Ville)
> 
> v3: (Ville)
> - Fix comment about logic for encoders without a crtc state and
>   add FIXME note for a simplification to avoid calling
> get_power_domains
>   in such cases.
> - Use intel_crtc_has_dp_encoder() instead !intel_crtc_has_type(HDMI).
> 
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>

The spec is not clear but this fix the "aux power well" enable timeouts
that I was getting in aux B so looks like your interpretation is right.

Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c        | 54
> ++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_display.c    | 12 +++++++-
>  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
>  drivers/gpu/drm/i915/intel_psr.c        | 41 ---------------------
> ----
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  1 +
>  5 files changed, 64 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 044fe1fb9872..0319825b725b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> intel_encoder *encoder,
>  	return ret;
>  }
>  
> -static u64 intel_ddi_get_power_domains(struct intel_encoder
> *encoder)
> +static inline enum intel_display_power_domain
> +intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> +{
> +	/* CNL HW requires corresponding AUX IOs to be powered up
> for PSR with
> +	 * DC states enabled at the same time, while for driver
> initiated AUX
> +	 * transfers we need the same AUX IOs to be powered but with
> DC states
> +	 * disabled. Accordingly use the AUX power domain here which
> leaves DC
> +	 * states enabled.
> +	 * However, for non-A AUX ports the corresponding non-EDP
> transcoders
> +	 * would have already enabled power well 2 and DC_OFF. This
> means we can
> +	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> instead of a
> +	 * specific AUX_IO reference without powering up any extra
> wells.
> +	 * Note that PSR is enabled only on Port A even though this
> function
> +	 * returns the correct domain for other ports too.
> +	 */
> +	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> :
> +					      intel_dp-
> >aux_power_domain;
> +}
> +
> +static u64 intel_ddi_get_power_domains(struct intel_encoder
> *encoder,
> +				       struct intel_crtc_state
> *crtc_state)
>  {
>  	struct intel_digital_port *dig_port =
> enc_to_dig_port(&encoder->base);
>  	enum pipe pipe;
> +	u64 domains;
>  
> -	if (intel_ddi_get_hw_state(encoder, &pipe))
> -		return BIT_ULL(dig_port->ddi_io_power_domain);
> +	if (!intel_ddi_get_hw_state(encoder, &pipe))
> +		return 0;
>  
> -	return 0;
> +	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> +	if (!crtc_state)
> +		return domains;
> +
> +	/*
> +	 * TODO: Add support for MST encoders. Atm, the following
> should never
> +	 * happen since this function will be called only for the
> primary
> +	 * encoder which doesn't have its own pipe/crtc_state.
> +	 */
> +	if (WARN_ON(intel_crtc_has_type(crtc_state,
> INTEL_OUTPUT_DP_MST)))
> +		return domains;
> +
> +	/* AUX power is only needed for (e)DP mode, not for HDMI. */
> +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> +		struct intel_dp *intel_dp = &dig_port->dp;
> +
> +		domains |=
> BIT_ULL(intel_ddi_main_link_aux_domain(intel_dp));
> +	}
> +
> +	return domains;
>  }
>  
>  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> *crtc_state)
> @@ -2631,6 +2671,9 @@ static void intel_ddi_pre_enable_dp(struct
> intel_encoder *encoder,
>  
>  	WARN_ON(is_mst && (port == PORT_A || port == PORT_E));
>  
> +	intel_display_power_get(dev_priv,
> +				intel_ddi_main_link_aux_domain(intel
> _dp));
> +
>  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
>  				 crtc_state->lane_count, is_mst);
>  
> @@ -2775,6 +2818,9 @@ static void intel_ddi_post_disable_dp(struct
> intel_encoder *encoder,
>  	intel_display_power_put(dev_priv, dig_port-
> >ddi_io_power_domain);
>  
>  	intel_ddi_clk_disable(encoder);
> +
> +	intel_display_power_put(dev_priv,
> +				intel_ddi_main_link_aux_domain(intel
> _dp));
>  }
>  
>  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2c8fef3ede54..ce615f89b43a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> drm_i915_private *dev_priv)
>  	for_each_intel_encoder(&dev_priv->drm, encoder) {
>  		u64 get_domains;
>  		enum intel_display_power_domain domain;
> +		struct intel_crtc_state *crtc_state;
>  
>  		if (!encoder->get_power_domains)
>  			continue;
>  
> -		get_domains = encoder->get_power_domains(encoder);
> +		/*
> +		 * For MST and inactive encoders we don't have a
> crtc state.
> +		 * FIXME: no need to call get_power_domains in such
> cases, it
> +		 * will always return 0.
> +		 */
> +		crtc_state = encoder->base.crtc ?
> +			     to_intel_crtc_state(encoder->base.crtc-
> >state) :
> +			     NULL;
> +
> +		get_domains = encoder->get_power_domains(encoder,
> crtc_state);
>  		for_each_power_domain(domain, get_domains)
>  			intel_display_power_get(dev_priv, domain);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0c3ac0eafde0..7174429d930a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -254,7 +254,8 @@ struct intel_encoder {
>  			   struct intel_crtc_state *pipe_config);
>  	/* Returns a mask of power domains that need to be
> referenced as part
>  	 * of the hardware state readout code. */
> -	u64 (*get_power_domains)(struct intel_encoder *encoder);
> +	u64 (*get_power_domains)(struct intel_encoder *encoder,
> +				 struct intel_crtc_state
> *crtc_state);
>  	/*
>  	 * Called during system suspend after all pending requests
> for the
>  	 * encoder are flushed (for example for DP AUX transactions)
> and
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index d4cd19fea148..eecdd8b5b5e0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,43 +56,6 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> -static inline enum intel_display_power_domain
> -psr_aux_domain(struct intel_dp *intel_dp)
> -{
> -	/* CNL HW requires corresponding AUX IOs to be powered up
> for PSR.
> -	 * However, for non-A AUX ports the corresponding non-EDP
> transcoders
> -	 * would have already enabled power well 2 and DC_OFF. This
> means we can
> -	 * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference
> instead of a
> -	 * specific AUX_IO reference without powering up any extra
> wells.
> -	 * Note that PSR is enabled only on Port A even though this
> function
> -	 * returns the correct domain for other ports too.
> -	 */
> -	return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A
> :
> -					      intel_dp-
> >aux_power_domain;
> -}
> -
> -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> >base.base.dev);
> -
> -	if (INTEL_GEN(dev_priv) < 10)
> -		return;
> -
> -	intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> -}
> -
> -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
> -	struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> >base.base.dev);
> -
> -	if (INTEL_GEN(dev_priv) < 10)
> -		return;
> -
> -	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> -}
> -
>  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> debug)
>  {
>  	u32 debug_mask, mask;
> @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct intel_dp
> *intel_dp,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  
> -	psr_aux_io_power_get(intel_dp);
> -
>  	/* Only HSW and BDW have PSR AUX registers that need to be
> setup. SKL+
>  	 * use hardcoded values PSR AUX transactions
>  	 */
> @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> *intel_dp,
>  		else
>  			WARN_ON(I915_READ(EDP_PSR_CTL) &
> EDP_PSR_ENABLE);
>  	}
> -
> -	psr_aux_io_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index de3a81034f77..2969787201ef 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define GLK_DISPLAY_AUX_A_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |		\
> +	BIT_ULL(POWER_DOMAIN_AUX_IO_A) |		\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define GLK_DISPLAY_AUX_B_POWER_DOMAINS (		\
>  	BIT_ULL(POWER_DOMAIN_AUX_B) |		\
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




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

  Powered by Linux