Re: [PATCH 5/8] drm/i915: Enable AUX power earlier

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

On Wed, Oct 31, 2018 at 02:17:36AM +0200, Souza, Jose wrote:
> On Wed, 2018-10-31 at 02:04 +0200, Imre Deak wrote:
> > On Wed, Oct 31, 2018 at 01:52:58AM +0200, Souza, Jose wrote:
> > > On Wed, 2018-10-31 at 01:28 +0200, Imre Deak wrote:
> > > > On Wed, Oct 31, 2018 at 01:18:09AM +0200, Souza, Jose wrote:
> > > > > On Wed, 2018-10-31 at 01:12 +0200, Imre Deak wrote:
> > > > > > On Wed, Oct 31, 2018 at 01:07:04AM +0200, Souza, Jose wrote:
> > > > > > > On Tue, 2018-10-30 at 17:40 +0200, Imre Deak wrote:
> > > > > > > > For DDI/TypeC ports the AUX power domain needs to be
> > > > > > > > enabled
> > > > > > > > before
> > > > > > > > the
> > > > > > > > port's PLL is enabled, so move the enabling earlier
> > > > > > > > accordingly.
> > > > > > > 
> > > > > > > Could you just pointed out where did you got this
> > > > > > > information?
> > > > > > > I
> > > > > > > checked in this Bspec pages 20845, 20598 and 21750 and I
> > > > > > > did
> > > > > > > not
> > > > > > > found.
> > > > > > 
> > > > > > It's at 21750, connect/disconnect flows.
> > > > > 
> > > > > What piece specifically?
> > > > > The only related that I can find is this one:
> > > > > "Aux power must be enabled while using Aux channel or the main
> > > > > link
> > > > > (including HDMI). It should be disabled to save power when not
> > > > > using
> > > > > Aux channel or main link."
> > > > 
> > > > We can do AUX transfers even before enabling the main link, where
> > > > we
> > > > enable already AUX power. In fact all the connect flows have:
> > > > 
> > > > "Display software issues AUX reads for EDID/DPCD."
> > > > 
> > > > So now if we enabled it after PLL programming we'd have the two
> > > > things
> > > > happen in both orders, which I'd rather not do, if not necessary.
> > > 
> > > I still don't get your point.
> > > 
> > > The reads for EDID/DPCD is executed way before PLL programming, so
> > > it
> > > will still enable and disable power aux power wells.
> > 
> > Yes, so here PLL is not yet programmed and we enable AUX power. I
> > want
> > to have only this sequence and avoid having also the opposite, where
> > we
> > first program the PLL then enable AUX power.
> 
> In what case aux power will be enabled when doing the PLL programming?

That case doesn't have to exist for my point to be valid. But nothing
prevents us doing delayed AUX power disabling in the future.

> If this cases exits I agree in going forward with this change if the
> commit message is updated, reading it looks like is a hardware
> requirement.

In fact I found another point at 22243:

1. Enable Power Wells
a.    Based on the resources to be used, enable the appropriate power wells following the Sequences for Power Wel
b.  Type-C: Note that AUX power is required for running main link.

...

4. Enable Port PLL

> 
> > 
> > > Also I did not found any aux transactions between the current
> > > places
> > > that gets and puts a aux power well reference.
> > > 
> > > haswell_crtc_enable()	
> > > 	intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
> > > 
> > > 	if (pipe_config->shared_dpll)
> > > 		intel_enable_shared_dpll(pipe_config);
> > > 
> > > 	if (INTEL_GEN(dev_priv) >= 11)
> > > 		icl_map_plls_to_ports(crtc, pipe_config, old_state);
> > > 
> > > 	intel_encoders_pre_enable(crtc, pipe_config, old_state);
> > > 
> > > 
> > > haswell_crtc_disable()
> > > 	intel_encoders_post_disable(crtc, old_crtc_state, old_state);
> > > 
> > > 	if (INTEL_GEN(dev_priv) >= 11)
> > > 		icl_unmap_plls_to_ports(crtc, old_crtc_state,
> > > old_state);
> > > 
> > > 	intel_encoders_post_pll_disable(crtc, old_crtc_state,
> > > old_state);
> > > 
> > > So if it is not sequence or requirement I don't see why we should
> > > enable earlier.
> > 
> > To have only one order of AUX enabling wrt. PLL programming.
> > 
> > > > > > > With that:
> > > > > > > Reviewed-by: José Roberto de Souza <jose.souza@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     | 46
> > > > > > > > +++++++++++++++++++++++++-
> > > > > > > > ----------
> > > > > > > >  drivers/gpu/drm/i915/intel_display.c |  2 ++
> > > > > > > >  2 files changed, 34 insertions(+), 14 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > index 5bb459011a49..7731ca704862 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > @@ -2082,10 +2082,8 @@ bool intel_ddi_get_hw_state(struct
> > > > > > > > intel_encoder *encoder,
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static inline enum intel_display_power_domain
> > > > > > > > -intel_ddi_main_link_aux_domain(struct intel_dp
> > > > > > > > *intel_dp)
> > > > > > > > +intel_ddi_main_link_aux_domain(struct intel_digital_port
> > > > > > > > *dig_port)
> > > > > > > >  {
> > > > > > > > -	struct intel_digital_port *dig_port =
> > > > > > > > dp_to_dig_port(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
> > > > > > > > @@ -2120,11 +2118,8 @@ static u64
> > > > > > > > intel_ddi_get_power_domains(struct
> > > > > > > > intel_encoder *encoder,
> > > > > > > >  	domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > > > > > > >  
> > > > > > > >  	/* 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));
> > > > > > > > -	}
> > > > > > > > +	if (intel_crtc_has_dp_encoder(crtc_state))
> > > > > > > > +		domains |=
> > > > > > > > BIT_ULL(intel_ddi_main_link_aux_domain(dig_port));
> > > > > > > >  
> > > > > > > >  	return domains;
> > > > > > > >  }
> > > > > > > > @@ -2891,6 +2886,32 @@ static void
> > > > > > > > intel_ddi_clk_disable(struct
> > > > > > > > intel_encoder *encoder)
> > > > > > > >  	}
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +static void
> > > > > > > > +intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
> > > > > > > > +			 const struct intel_crtc_state
> > > > > > > > *crtc_state,
> > > > > > > > +			 const struct drm_connector_state
> > > > > > > > *conn_state)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > > > > > > base.dev);
> > > > > > > > +	struct intel_digital_port *dig_port =
> > > > > > > > enc_to_dig_port(&encoder-
> > > > > > > > > base);
> > > > > > > > +
> > > > > > > > +	if (intel_crtc_has_dp_encoder(crtc_state))
> > > > > > > > +		intel_display_power_get(dev_priv,
> > > > > > > > +					intel_ddi_main_link_aux
> > > > > > > > _domain(
> > > > > > > > dig_port));
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void
> > > > > > > > +intel_ddi_post_pll_disable(struct intel_encoder
> > > > > > > > *encoder,
> > > > > > > > +			   const struct intel_crtc_state
> > > > > > > > *crtc_state,
> > > > > > > > +			   const struct drm_connector_state
> > > > > > > > *conn_state)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > > > > > > base.dev);
> > > > > > > > +	struct intel_digital_port *dig_port =
> > > > > > > > enc_to_dig_port(&encoder-
> > > > > > > > > base);
> > > > > > > > +
> > > > > > > > +	if (intel_crtc_has_dp_encoder(crtc_state))
> > > > > > > > +		intel_display_power_put(dev_priv,
> > > > > > > > +					intel_ddi_main_link_aux
> > > > > > > > _domain(
> > > > > > > > dig_port));
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void intel_ddi_pre_enable_dp(struct intel_encoder
> > > > > > > > *encoder,
> > > > > > > >  				    const struct
> > > > > > > > intel_crtc_state
> > > > > > > > *crtc_state,
> > > > > > > >  				    const struct
> > > > > > > > drm_connector_state
> > > > > > > > *conn_state)
> > > > > > > > @@ -2904,9 +2925,6 @@ 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);
> > > > > > > >  
> > > > > > > > @@ -3071,9 +3089,6 @@ 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,
> > > > > > > > @@ -3830,6 +3845,9 @@ void intel_ddi_init(struct
> > > > > > > > drm_i915_private
> > > > > > > > *dev_priv, enum port port)
> > > > > > > >  	intel_encoder->enable = intel_enable_ddi;
> > > > > > > >  	if (IS_GEN9_LP(dev_priv))
> > > > > > > >  		intel_encoder->pre_pll_enable =
> > > > > > > > bxt_ddi_pre_pll_enable;
> > > > > > > > +
> > > > > > > > +	intel_encoder->pre_pll_enable =
> > > > > > > > intel_ddi_pre_pll_enable;
> > > > > > > > +	intel_encoder->post_pll_disable =
> > > > > > > > intel_ddi_post_pll_disable;
> > > > > > > >  	intel_encoder->pre_enable = intel_ddi_pre_enable;
> > > > > > > >  	intel_encoder->disable = intel_disable_ddi;
> > > > > > > >  	intel_encoder->post_disable = intel_ddi_post_disable;
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > index 36710a30fb37..12ba2b923e6b 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > @@ -5876,6 +5876,8 @@ static void
> > > > > > > > haswell_crtc_disable(struct
> > > > > > > > intel_crtc_state *old_crtc_state,
> > > > > > > >  
> > > > > > > >  	if (INTEL_GEN(dev_priv) >= 11)
> > > > > > > >  		icl_unmap_plls_to_ports(crtc, old_crtc_state,
> > > > > > > > old_state);
> > > > > > > > +
> > > > > > > > +	intel_encoders_post_pll_disable(crtc, old_crtc_state,
> > > > > > > > old_state);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > >  static void i9xx_pfit_enable(const struct
> > > > > > > > intel_crtc_state
> > > > > > > > *crtc_state)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx