On Thu, Jun 21, 2018 at 01:28:40PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-06-21 at 22:54 +0300, Imre Deak wrote:
> > On Thu, Jun 21, 2018 at 01:14:30PM -0700, Dhinakaran Pandiyan wrote:
> > >
> > > On Thu, 2018-06-21 at 21:44 +0300, Imre Deak wrote:
> > > >
> > > > So far we got an AUX power domain reference only for the duration
> > > > of
> > > > DP
> > > > AUX transfers. However, the following suggests that we also need
> > > > these
> > > > for main link functionality:
> > > > - The specification doesn't state whether it's needed or not for
> > > > main
> > > > link functionality, but suggests that these power wells need to
> > > > be
> > > > enabled already during display core initialization (Sequences
> > > > to
> > > > Initialize Display).
> > > Hmm. The sequence also says "If an Aux channel will not be used, it
> > > does not need to be powered up."
> > Well, I consider that hints at not using the port at all for DP,
> > since
> > for DP you will need AUX.
> >
>
> I also see an instruction to "set PORT_CL_DW12_<port letter> Lane
> Enable Aux to 1b" for aux channels on combo ports. A quick check shows
> the register is not defined in the driver. My concern is we might be
> missing a step for combo ports.
It is added in the ICL power well enabling patch (see internal branch),
and is something I tested the above scenario with.
>
>
> > >
> > >
> > > >
> > > > - For PSR we need to keep the AUX power well enabled.
> > > > - On ICL combo PHY ports (non-TC) the AUX power well is needed
> > > > for
> > > > link training too: while the port is enabled with a DP link
> > > > training
> > > I don't see this in the spec, is this something you observed?
> > Yes.
> >
> > >
> > >
> > > >
> > > > test pattern trying to toggle the AUX power well will time out.
> > > Can we enable AUX power well before link training starts and
> > > disable
> > > after we are done training? Is that enough?
> > That was my first idea too, but given the rest of the points I
> > consider
> > it a safer option to enable it for main link functionality.
> >
> > >
> > >
> > > >
> > > > - On ICL MG PHY ports (TC) the AUX power well is needed also for
> > > > main
> > > > link functionality (both in DP and HDMI modes).
> > > > - Windows enables these power wells both for main and AUX lane
> > > > functionality.
> > > >
> > > > Based on the above take an AUX power reference for main link
> > > > functionality too. This makes a difference only on GEN10+ (GLK+)
> > > > platforms, where we have separate port specific AUX power wells.
> > > >
> > > > For PSR we still need to distinguish between port A and the other
> > > > ports, since on port A DC states must stay enabled for main link
> > > > functionality, but DC states must be disabled for driver
> > > > initiated
> > > > AUX transfers. So re-use the corresponding helper from
> > > > intel_psr.c.
> > > >
> > > > Since we take now a reference for main link functionality on all
> > > > DP
> > > > ports we can forgo taking the separate power ref for PSR
> > > > functionality.
> > > >
> > > > v2:
> > > > - Make sure DC states stay enabled when taking the ref on port A.
> > > > (Ville)
> > > >
> > > > v3: (Ville)
> > > > - Fix comment about logic for encoders without a crtc state and
> > > > add FIXME note for a simplification to avoid calling
> > > > get_power_domains
> > > > in such cases.
> > > > - Use intel_crtc_has_dp_encoder() instead
> > > > !intel_crtc_has_type(HDMI).
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_ddi.c | 54
> > > > ++++++++++++++++++++++++++++++---
> > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++-
> > > > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > > > drivers/gpu/drm/i915/intel_psr.c | 41 -----------------
> > > > ----
> > > > ----
> > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 1 +
> > > > 5 files changed, 64 insertions(+), 47 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 044fe1fb9872..0319825b725b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -1983,15 +1983,55 @@ bool intel_ddi_get_hw_state(struct
> > > > intel_encoder *encoder,
> > > > return ret;
> > > > }
> > > >
> > > > -static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > > *encoder)
> > > > +static inline enum intel_display_power_domain
> > > > +intel_ddi_main_link_aux_domain(struct intel_dp *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
> > > > + * 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.
> > > > + */
> > > > + return intel_dp->aux_ch == AUX_CH_A ?
> > > > POWER_DOMAIN_AUX_IO_A
> > > > :
> > > > + intel_dp-
> > > > >
> > > > > aux_power_domain;
> > > > +}
> > > > +
> > > > +static u64 intel_ddi_get_power_domains(struct intel_encoder
> > > > *encoder,
> > > > + struct intel_crtc_state
> > > > *crtc_state)
> > > > {
> > > > struct intel_digital_port *dig_port =
> > > > enc_to_dig_port(&encoder->base);
> > > > enum pipe pipe;
> > > > + u64 domains;
> > > >
> > > > - if (intel_ddi_get_hw_state(encoder, &pipe))
> > > > - return BIT_ULL(dig_port->ddi_io_power_domain);
> > > > + if (!intel_ddi_get_hw_state(encoder, &pipe))
> > > > + return 0;
> > > >
> > > > - return 0;
> > > > + domains = BIT_ULL(dig_port->ddi_io_power_domain);
> > > > + if (!crtc_state)
> > > > + return domains;
> > > > +
> > > > + /*
> > > > + * TODO: Add support for MST encoders. Atm, the
> > > > following
> > > > should never
> > > > + * happen since this function will be called only for
> > > > the
> > > > primary
> > > > + * encoder which doesn't have its own pipe/crtc_state.
> > > > + */
> > > > + if (WARN_ON(intel_crtc_has_type(crtc_state,
> > > > INTEL_OUTPUT_DP_MST)))
> > > > + return domains;
> > > > +
> > > > + /* 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));
> > > > + }
> > > > +
> > > > + return domains;
> > > > }
> > > >
> > > > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state
> > > > *crtc_state)
> > > > @@ -2631,6 +2671,9 @@ 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(i
> > > > ntel
> > > > _dp));
> > > > +
> > > > intel_dp_set_link_params(intel_dp, crtc_state-
> > > > >port_clock,
> > > > crtc_state->lane_count,
> > > > is_mst);
> > > >
> > > > @@ -2775,6 +2818,9 @@ 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(i
> > > > ntel
> > > > _dp));
> > > > }
> > > >
> > > > static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > > > *encoder,
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 2c8fef3ede54..ce615f89b43a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct
> > > > drm_i915_private *dev_priv)
> > > > for_each_intel_encoder(&dev_priv->drm, encoder) {
> > > > u64 get_domains;
> > > > enum intel_display_power_domain domain;
> > > > + struct intel_crtc_state *crtc_state;
> > > >
> > > > if (!encoder->get_power_domains)
> > > > continue;
> > > >
> > > > - get_domains = encoder-
> > > > >get_power_domains(encoder);
> > > > + /*
> > > > + * For MST and inactive encoders we don't have a
> > > > crtc state.
> > > > + * FIXME: no need to call get_power_domains in
> > > > such
> > > > cases, it
> > > > + * will always return 0.
> > > > + */
> > > > + crtc_state = encoder->base.crtc ?
> > > > + to_intel_crtc_state(encoder-
> > > > >base.crtc-
> > > > >
> > > > > state) :
> > > > + NULL;
> > > > +
> > > > + get_domains = encoder-
> > > > >get_power_domains(encoder,
> > > > crtc_state);
> > > > for_each_power_domain(domain, get_domains)
> > > > intel_display_power_get(dev_priv,
> > > > domain);
> > > > }
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 0c3ac0eafde0..7174429d930a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -254,7 +254,8 @@ struct intel_encoder {
> > > > struct intel_crtc_state
> > > > *pipe_config);
> > > > /* Returns a mask of power domains that need to be
> > > > referenced as part
> > > > * of the hardware state readout code. */
> > > > - u64 (*get_power_domains)(struct intel_encoder *encoder);
> > > > + u64 (*get_power_domains)(struct intel_encoder *encoder,
> > > > + struct intel_crtc_state
> > > > *crtc_state);
> > > > /*
> > > > * Called during system suspend after all pending
> > > > requests
> > > > for the
> > > > * encoder are flushed (for example for DP AUX
> > > > transactions)
> > > > and
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index d4cd19fea148..eecdd8b5b5e0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -56,43 +56,6 @@
> > > > #include "intel_drv.h"
> > > > #include "i915_drv.h"
> > > >
> > > > -static inline enum intel_display_power_domain
> > > > -psr_aux_domain(struct intel_dp *intel_dp)
> > > > -{
> > > > - /* CNL HW requires corresponding AUX IOs to be powered
> > > > up
> > > > for PSR.
> > > > - * 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.
> > > > - */
> > > > - return intel_dp->aux_ch == AUX_CH_A ?
> > > > POWER_DOMAIN_AUX_IO_A
> > > > :
> > > > - intel_dp-
> > > > >
> > > > > aux_power_domain;
> > > > -}
> > > > -
> > > > -static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> > > > -{
> > > > - struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > - struct drm_i915_private *dev_priv =
> > > > to_i915(intel_dig_port-
> > > > >
> > > > > base.base.dev);
> > > > -
> > > > - if (INTEL_GEN(dev_priv) < 10)
> > > > - return;
> > > > -
> > > > - intel_display_power_get(dev_priv,
> > > > psr_aux_domain(intel_dp));
> > > > -}
> > > > -
> > > > -static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > > > -{
> > > > - struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > - struct drm_i915_private *dev_priv =
> > > > to_i915(intel_dig_port-
> > > > >
> > > > > base.base.dev);
> > > > -
> > > > - if (INTEL_GEN(dev_priv) < 10)
> > > > - return;
> > > > -
> > > > - intel_display_power_put(dev_priv,
> > > > psr_aux_domain(intel_dp));
> > > > -}
> > > > -
> > > > void intel_psr_irq_control(struct drm_i915_private *dev_priv,
> > > > bool
> > > > debug)
> > > > {
> > > > u32 debug_mask, mask;
> > > > @@ -595,8 +558,6 @@ static void hsw_psr_enable_source(struct
> > > > intel_dp
> > > > *intel_dp,
> > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > enum transcoder cpu_transcoder = crtc_state-
> > > > >cpu_transcoder;
> > > >
> > > > - psr_aux_io_power_get(intel_dp);
> > > > -
> > > > /* Only HSW and BDW have PSR AUX registers that need to
> > > > be
> > > > setup. SKL+
> > > > * use hardcoded values PSR AUX transactions
> > > > */
> > > > @@ -717,8 +678,6 @@ static void hsw_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > > else
> > > > WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > > EDP_PSR_ENABLE);
> > > > }
> > > > -
> > > > - psr_aux_io_power_put(intel_dp);
> > > > }
> > > >
> > > > /**
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index de3a81034f77..2969787201ef 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -1824,6 +1824,7 @@ void intel_display_power_put(struct
> > > > drm_i915_private *dev_priv,
> > > > BIT_ULL(POWER_DOMAIN_INIT))
> > > > #define GLK_DISPLAY_AUX_A_POWER_DOMAINS ( \
> > > > BIT_ULL(POWER_DOMAIN_AUX_A) | \
> > > > + BIT_ULL(POWER_DOMAIN_AUX_IO_A) | \
> > > > BIT_ULL(POWER_DOMAIN_INIT))
> > > > #define GLK_DISPLAY_AUX_B_POWER_DOMAINS ( \
> > > > BIT_ULL(POWER_DOMAIN_AUX_B) | \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
