On Thu, Nov 10, 2022 at 12:37:30PM +0200, Ville Syrjälä wrote:
> 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?
Yes, that's how things work already now on port A. I think the DMC
context save handler is either not started when an external output is
enabled on the port (like HDMI) or the handler will check if PSR is
enabled and if not it won't do anything. So enabling/disabling DC state
via the DC_STATE_EN register shouldn't cause actual DC state entry/exit
transitions.
> > 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
