On Wed, 02 Nov 2022, Imre Deak <imre.d...@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.d...@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))
> /*
--
Jani Nikula, Intel Open Source Graphics Center
