On Tue, Nov 08, 2022 at 05:18:27PM +0200, Imre Deak wrote:
> Combo PHY ports require the AUX_IO power only for eDP/PSR, so don't
> enable it otherwise on these ports. So far the external DP and eDP case
> was handled the same way due to unclarity when AUX_IO for the main link
> is needed. However Bspec is clear in which cases it's required:
>
> - eDP/PSR on all ports and platforms (presumably due to HW/FW initiated
> PSR transactions that won't enable AUX_IO)
> Bspec: 4301, 49296
> - TypeC PHY ports on platforms before MTL in all TypeC modes (TBT,
> DP-alt, legacy) and for both HDMI and DP. The next patch will take
> into account the pre-MTL platform dependency.
> Bspec: 22243, 53339, 21750, 49190, 49191, 55424, 65448, 65750, 49294,
> 55480, 65380
>
> v2:
> - Rebased on checking intel_encoder_can_psr() instead of crtc->has_psr.
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 21f1a68a57598..cc4bc529a78a5 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -846,8 +846,7 @@ 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,
> - const struct intel_crtc_state *crtc_state)
> +intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port)
> {
> struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
> @@ -867,20 +866,18 @@ intel_ddi_main_link_aux_domain(struct intel_digital_port *dig_port,
> */
> if (intel_encoder_can_psr(&dig_port->base))
> return intel_display_power_aux_io_domain(i915, dig_port->aux_ch);
> - else if (intel_crtc_has_dp_encoder(crtc_state) ||
> - intel_phy_is_tc(i915, phy))
> + else if (intel_phy_is_tc(i915, phy))
> return intel_aux_power_domain(dig_port);
I was pondering this a bit more the other day and came to the conclusion
that getting rid of the full aux domain is desirable, if for no other
reason that perhaps testing the dmc interactions a little bit more when
psr isn't supported. I guess nothing much should actually happen on the
dmc firware side without psr, but at least we might end up poking
the DC_STATE_EN register occasionally?
> else
> return POWER_DOMAIN_INVALID;
> }
>
> static void
> -main_link_aux_power_domain_get(struct intel_digital_port *dig_port,
> - const struct intel_crtc_state *crtc_state)
> +main_link_aux_power_domain_get(struct intel_digital_port *dig_port)
> {
> 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);
> + intel_ddi_main_link_aux_domain(dig_port);
>
> drm_WARN_ON(&i915->drm, dig_port->aux_wakeref);
>
> @@ -891,13 +888,12 @@ main_link_aux_power_domain_get(struct intel_digital_port *dig_port,
> }
>
> static void
> -main_link_aux_power_domain_put(struct intel_digital_port *dig_port,
> - const struct intel_crtc_state *crtc_state)
> +main_link_aux_power_domain_put(struct intel_digital_port *dig_port)
> {
> 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);
> + intel_ddi_main_link_aux_domain(dig_port);
>
> if (!wf)
> return;
> @@ -928,7 +924,7 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
> dig_port->ddi_io_power_domain);
> }
>
> - main_link_aux_power_domain_get(dig_port, crtc_state);
> + main_link_aux_power_domain_get(dig_port);
> }
>
> void intel_ddi_enable_pipe_clock(struct intel_encoder *encoder,
> @@ -2767,7 +2763,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);
>
> - main_link_aux_power_domain_put(dig_port, old_crtc_state);
> + main_link_aux_power_domain_put(dig_port);
>
> if (is_tc_port)
> intel_tc_port_put_link(dig_port);
> @@ -3088,7 +3084,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);
>
> - main_link_aux_power_domain_get(dig_port, crtc_state);
> + main_link_aux_power_domain_get(dig_port);
>
> if (is_tc_port && !intel_tc_port_in_tbt_alt_mode(dig_port))
> /*
> --
> 2.37.1
--
Ville Syrjälä
Intel
