| Message ID | 20181030154051.30851-6-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/icl: Fix HDMI on TypeC static ports | expand |
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. 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)
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. > > 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)
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." > > > 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)
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. > > > > > > 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)
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. 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. > > > > > 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)
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. > 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)
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? 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. > > > 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)
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)
On Wed, 2018-10-31 at 02:33 +0200, Imre Deak wrote: > 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 Nice, please just add some of it to the commit message. Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > 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)
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)
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. 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(-)