| Message ID | 20221102171530.3261282-7-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tgl+: Enable DC power states on all eDP ports | expand |
On Wed, 02 Nov 2022, Imre Deak <imre.deak@intel.com> wrote: > Factor out functions to get/put the AUX_IO power domain for the main > link on DDI ports. > > While at it clarify the corresponding code comment. > > No functional change. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 84 ++++++++++++++---------- > 1 file changed, 51 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 00b577a5b9a76..7453772d2073d 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -846,26 +846,63 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > } > > static enum intel_display_power_domain > -intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port) > +intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port, > + const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > + enum phy phy = intel_port_to_phy(i915, dig_port->base.port); > > - /* ICL+ HW requires corresponding AUX IOs to be powered up for PSR with > + /* > + * ICL+ 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. > + * disabled. Accordingly use the AUX_IO_<port> power domain here which > + * leaves DC states enabled. > + * > + * Before MTL all DP ports and HDMI ports on TypeC PHYs also require > + * AUX IO to be enabled, but all these require DC_OFF to be enabled as > + * well, so we can acquire a wider AUX_<port> power domain reference > + * instead of a specific AUX_IO_<port> reference without powering up any > + * extra wells. > */ > if (intel_dp_is_edp(&dig_port->dp)) > return intel_display_power_aux_io_domain(i915, dig_port->aux_ch); > - else > + else if (intel_crtc_has_dp_encoder(crtc_state) || > + intel_phy_is_tc(i915, phy)) > return intel_aux_power_domain(dig_port); > + else > + return POWER_DOMAIN_INVALID; > +} > + > +static void > +get_aux_power_for_main_link(struct intel_digital_port *dig_port, > + const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > + enum intel_display_power_domain domain = > + intel_ddi_main_link_aux_domain(dig_port, crtc_state); > + > + drm_WARN_ON(&i915->drm, dig_port->aux_wakeref); > + > + if (domain == POWER_DOMAIN_INVALID) > + return; > + > + dig_port->aux_wakeref = intel_display_power_get(i915, domain); > +} > + > +static void > +put_aux_power_for_main_link(struct intel_digital_port *dig_port, > + const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > + intel_wakeref_t wf = fetch_and_zero(&dig_port->aux_wakeref); > + enum intel_display_power_domain domain = > + intel_ddi_main_link_aux_domain(dig_port, crtc_state); > + > + if (!wf) > + return; > + > + intel_display_power_put(i915, domain, wf); > } Nitpick, I think foo_get()/foo_put() are much more common in the driver than get_foo()/put_foo(). > > static void intel_ddi_get_power_domains(struct intel_encoder *encoder, > @@ -873,7 +910,6 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder, > { > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port; > - enum phy phy = intel_port_to_phy(dev_priv, encoder->port); > > /* > * TODO: Add support for MST encoders. Atm, the following should never > @@ -892,17 +928,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder, > dig_port->ddi_io_power_domain); > } > > - /* > - * AUX power is only needed for (e)DP mode, and for HDMI mode on TC > - * ports. > - */ > - if (intel_crtc_has_dp_encoder(crtc_state) || > - intel_phy_is_tc(dev_priv, phy)) { > - drm_WARN_ON(&dev_priv->drm, dig_port->aux_wakeref); > - dig_port->aux_wakeref = > - intel_display_power_get(dev_priv, > - intel_ddi_main_link_aux_domain(dig_port)); > - } > + get_aux_power_for_main_link(dig_port, crtc_state); > } > > void intel_ddi_enable_pipe_clock(struct intel_encoder *encoder, > @@ -2741,10 +2767,7 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state, > intel_ddi_post_disable_dp(state, encoder, old_crtc_state, > old_conn_state); > > - if (intel_crtc_has_dp_encoder(old_crtc_state) || is_tc_port) > - intel_display_power_put(dev_priv, > - intel_ddi_main_link_aux_domain(dig_port), > - fetch_and_zero(&dig_port->aux_wakeref)); > + put_aux_power_for_main_link(dig_port, old_crtc_state); > > if (is_tc_port) > intel_tc_port_put_link(dig_port); > @@ -3065,12 +3088,7 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state *state, > if (is_tc_port) > intel_tc_port_get_link(dig_port, crtc_state->lane_count); > > - if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port) { > - drm_WARN_ON(&dev_priv->drm, dig_port->aux_wakeref); > - dig_port->aux_wakeref = > - intel_display_power_get(dev_priv, > - intel_ddi_main_link_aux_domain(dig_port)); > - } > + get_aux_power_for_main_link(dig_port, crtc_state); > > if (is_tc_port && !intel_tc_port_in_tbt_alt_mode(dig_port)) > /*
On Wed, Nov 02, 2022 at 09:02:41PM +0200, Jani Nikula wrote: > On Wed, 02 Nov 2022, Imre Deak <imre.deak@intel.com> wrote: > > Factor out functions to get/put the AUX_IO power domain for the main > > link on DDI ports. > > > > While at it clarify the corresponding code comment. > > > > No functional change. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 84 ++++++++++++++---------- > > 1 file changed, 51 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 00b577a5b9a76..7453772d2073d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -846,26 +846,63 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > } > > > > static enum intel_display_power_domain > > -intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port) > > +intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port, > > + const struct intel_crtc_state *crtc_state) > > { > > struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > > + enum phy phy = intel_port_to_phy(i915, dig_port->base.port); > > > > - /* ICL+ HW requires corresponding AUX IOs to be powered up for PSR with > > + /* > > + * ICL+ 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. > > + * disabled. Accordingly use the AUX_IO_<port> power domain here which > > + * leaves DC states enabled. > > + * > > + * Before MTL all DP ports and HDMI ports on TypeC PHYs also require > > + * AUX IO to be enabled, but all these require DC_OFF to be enabled as > > + * well, so we can acquire a wider AUX_<port> power domain reference > > + * instead of a specific AUX_IO_<port> reference without powering up any > > + * extra wells. > > */ > > if (intel_dp_is_edp(&dig_port->dp)) > > return intel_display_power_aux_io_domain(i915, dig_port->aux_ch); > > - else > > + else if (intel_crtc_has_dp_encoder(crtc_state) || > > + intel_phy_is_tc(i915, phy)) > > return intel_aux_power_domain(dig_port); > > + else > > + return POWER_DOMAIN_INVALID; > > +} > > + > > +static void > > +get_aux_power_for_main_link(struct intel_digital_port *dig_port, > > + const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > > + enum intel_display_power_domain domain = > > + intel_ddi_main_link_aux_domain(dig_port, crtc_state); > > + > > + drm_WARN_ON(&i915->drm, dig_port->aux_wakeref); > > + > > + if (domain == POWER_DOMAIN_INVALID) > > + return; > > + > > + dig_port->aux_wakeref = intel_display_power_get(i915, domain); > > +} > > + > > +static void > > +put_aux_power_for_main_link(struct intel_digital_port *dig_port, > > + const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > > + intel_wakeref_t wf = fetch_and_zero(&dig_port->aux_wakeref); > > + enum intel_display_power_domain domain = > > + intel_ddi_main_link_aux_domain(dig_port, crtc_state); > > + > > + if (!wf) > > + return; > > + > > + intel_display_power_put(i915, domain, wf); > > } > > Nitpick, I think foo_get()/foo_put() are much more common in the driver > than get_foo()/put_foo(). Ok, can rename these. > > static void intel_ddi_get_power_domains(struct intel_encoder *encoder, > > @@ -873,7 +910,6 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder, > > { > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_digital_port *dig_port; > > - enum phy phy = intel_port_to_phy(dev_priv, encoder->port); > > > > /* > > * TODO: Add support for MST encoders. Atm, the following should never > > @@ -892,17 +928,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder, > > dig_port->ddi_io_power_domain); > > } > > > > - /* > > - * AUX power is only needed for (e)DP mode, and for HDMI mode on TC > > - * ports. > > - */ > > - if (intel_crtc_has_dp_encoder(crtc_state) || > > - intel_phy_is_tc(dev_priv, phy)) { > > - drm_WARN_ON(&dev_priv->drm, dig_port->aux_wakeref); > > - dig_port->aux_wakeref = > > - intel_display_power_get(dev_priv, > > - intel_ddi_main_link_aux_domain(dig_port)); > > - } > > + get_aux_power_for_main_link(dig_port, crtc_state); > > } > > > > void intel_ddi_enable_pipe_clock(struct intel_encoder *encoder, > > @@ -2741,10 +2767,7 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state, > > intel_ddi_post_disable_dp(state, encoder, old_crtc_state, > > old_conn_state); > > > > - if (intel_crtc_has_dp_encoder(old_crtc_state) || is_tc_port) > > - intel_display_power_put(dev_priv, > > - intel_ddi_main_link_aux_domain(dig_port), > > - fetch_and_zero(&dig_port->aux_wakeref)); > > + put_aux_power_for_main_link(dig_port, old_crtc_state); > > > > if (is_tc_port) > > intel_tc_port_put_link(dig_port); > > @@ -3065,12 +3088,7 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state *state, > > if (is_tc_port) > > intel_tc_port_get_link(dig_port, crtc_state->lane_count); > > > > - if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port) { > > - drm_WARN_ON(&dev_priv->drm, dig_port->aux_wakeref); > > - dig_port->aux_wakeref = > > - intel_display_power_get(dev_priv, > > - intel_ddi_main_link_aux_domain(dig_port)); > > - } > > + get_aux_power_for_main_link(dig_port, crtc_state); > > > > if (is_tc_port && !intel_tc_port_in_tbt_alt_mode(dig_port)) > > /* > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 00b577a5b9a76..7453772d2073d 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -846,26 +846,63 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, } static enum intel_display_power_domain -intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port) +intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port, + const struct intel_crtc_state *crtc_state) { struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); + enum phy phy = intel_port_to_phy(i915, dig_port->base.port); - /* ICL+ HW requires corresponding AUX IOs to be powered up for PSR with + /* + * ICL+ 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. + * disabled. Accordingly use the AUX_IO_<port> power domain here which + * leaves DC states enabled. + * + * Before MTL all DP ports and HDMI ports on TypeC PHYs also require + * AUX IO to be enabled, but all these require DC_OFF to be enabled as + * well, so we can acquire a wider AUX_<port> power domain reference + * instead of a specific AUX_IO_<port> reference without powering up any + * extra wells. */ if (intel_dp_is_edp(&dig_port->dp)) return intel_display_power_aux_io_domain(i915, dig_port->aux_ch); - else + else if (intel_crtc_has_dp_encoder(crtc_state) || + intel_phy_is_tc(i915, phy)) return intel_aux_power_domain(dig_port); + else + return POWER_DOMAIN_INVALID; +} + +static void +get_aux_power_for_main_link(struct intel_digital_port *dig_port, + const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); + enum intel_display_power_domain domain = + intel_ddi_main_link_aux_domain(dig_port, crtc_state); + + drm_WARN_ON(&i915->drm, dig_port->aux_wakeref); + + if (domain == POWER_DOMAIN_INVALID) + return; + + dig_port->aux_wakeref = intel_display_power_get(i915, domain); +} + +static void +put_aux_power_for_main_link(struct intel_digital_port *dig_port, + const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); + intel_wakeref_t wf = fetch_and_zero(&dig_port->aux_wakeref); + enum intel_display_power_domain domain = + intel_ddi_main_link_aux_domain(dig_port, crtc_state); + + if (!wf) + return; + + intel_display_power_put(i915, domain, wf); } static void intel_ddi_get_power_domains(struct intel_encoder *encoder, @@ -873,7 +910,6 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder, { struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_digital_port *dig_port; - enum phy phy = intel_port_to_phy(dev_priv, encoder->port); /* * TODO: Add support for MST encoders. Atm, the following should never @@ -892,17 +928,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder, dig_port->ddi_io_power_domain); } - /* - * AUX power is only needed for (e)DP mode, and for HDMI mode on TC - * ports. - */ - if (intel_crtc_has_dp_encoder(crtc_state) || - intel_phy_is_tc(dev_priv, phy)) { - drm_WARN_ON(&dev_priv->drm, dig_port->aux_wakeref); - dig_port->aux_wakeref = - intel_display_power_get(dev_priv, - intel_ddi_main_link_aux_domain(dig_port)); - } + get_aux_power_for_main_link(dig_port, crtc_state); } void intel_ddi_enable_pipe_clock(struct intel_encoder *encoder, @@ -2741,10 +2767,7 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state, intel_ddi_post_disable_dp(state, encoder, old_crtc_state, old_conn_state); - if (intel_crtc_has_dp_encoder(old_crtc_state) || is_tc_port) - intel_display_power_put(dev_priv, - intel_ddi_main_link_aux_domain(dig_port), - fetch_and_zero(&dig_port->aux_wakeref)); + put_aux_power_for_main_link(dig_port, old_crtc_state); if (is_tc_port) intel_tc_port_put_link(dig_port); @@ -3065,12 +3088,7 @@ intel_ddi_pre_pll_enable(struct intel_atomic_state *state, if (is_tc_port) intel_tc_port_get_link(dig_port, crtc_state->lane_count); - if (intel_crtc_has_dp_encoder(crtc_state) || is_tc_port) { - drm_WARN_ON(&dev_priv->drm, dig_port->aux_wakeref); - dig_port->aux_wakeref = - intel_display_power_get(dev_priv, - intel_ddi_main_link_aux_domain(dig_port)); - } + get_aux_power_for_main_link(dig_port, crtc_state); if (is_tc_port && !intel_tc_port_in_tbt_alt_mode(dig_port)) /*
Factor out functions to get/put the AUX_IO power domain for the main link on DDI ports. While at it clarify the corresponding code comment. No functional change. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 84 ++++++++++++++---------- 1 file changed, 51 insertions(+), 33 deletions(-)