[Intel-gfx] [PATCH v3] drm/i915/psr: New power domain for AUX IO.
Imre Deak
imre.deak at intel.com
Thu Feb 22 23:06:59 UTC 2018
On Fri, Feb 23, 2018 at 12:58:44AM +0200, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-23 at 00:42 +0200, Imre Deak wrote:
> > On Fri, Feb 23, 2018 at 12:30:14AM +0200, Pandiyan, Dhinakaran wrote:
> > > On Fri, 2018-02-23 at 00:05 +0200, Imre Deak wrote:
> > > > On Thu, Feb 22, 2018 at 11:53:30PM +0200, Imre Deak wrote:
> > > > > On Thu, Feb 22, 2018 at 11:33:10PM +0200, Pandiyan, Dhinakaran wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Thu, 2018-02-22 at 12:44 -0800, Rodrigo Vivi wrote:
> > > > > > > On Thu, Feb 22, 2018 at 12:28:50PM -0800, Dhinakaran Pandiyan wrote:
> > > > > > > > PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> > > > > > > > for AUX-A enables DC_OFF well too. This is not required, so add a new
> > > > > > > > AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> > > > > > > > channels re-use the existing AUX domains as they do need power well 2.
> > > > > > > >
> > > > > > > > v3: Extract aux domain selection into a function (Ville)
> > > > > > > > v2: Add AUX IO domain only for AUX-A
> > > > > > > > Rebased on top of Ville's AUX series.
> > > > > > > >
> > > > > > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > > Suggested-by: Imre Deak <imre.deak at intel.com>
> > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/i915/intel_display.h | 1 +
> > > > > > > > drivers/gpu/drm/i915/intel_dp.c | 2 +-
> > > > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > > > > > drivers/gpu/drm/i915/intel_psr.c | 38 +++++++++++++++++++++++++++++++++
> > > > > > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++
> > > > > > > > 5 files changed, 44 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > index f5733a2576e7..4e7418b345bc 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > > > > > @@ -186,6 +186,7 @@ enum intel_display_power_domain {
> > > > > > > > POWER_DOMAIN_AUX_C,
> > > > > > > > POWER_DOMAIN_AUX_D,
> > > > > > > > POWER_DOMAIN_AUX_F,
> > > > > > > > + POWER_DOMAIN_AUX_IO_A,
> > > > > > > > POWER_DOMAIN_GMBUS,
> > > > > > > > POWER_DOMAIN_MODESET,
> > > > > > > > POWER_DOMAIN_GT_IRQ,
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > index 005735a7fc29..b78b9972ebae 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > @@ -1331,7 +1331,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > > > > > return ret;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp)
> > > > > > > > {
> > > > > > > > struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > > > > > > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > index 5132f6a58c6d..bcd6dc9fb13d 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > > > @@ -1685,6 +1685,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
> > > > > > > > int intel_dp_link_required(int pixel_clock, int bpp);
> > > > > > > > int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> > > > > > > > bool intel_digital_port_connected(struct intel_encoder *encoder);
> > > > > > > > +enum aux_ch intel_aux_ch(struct intel_dp *intel_dp);
> > > > > > > >
> > > > > > > > /* intel_dp_aux_backlight.c */
> > > > > > > > int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > index 2ef374f936b9..03814f7bc2d3 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > > > @@ -56,6 +56,40 @@
> > > > > > > > #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, AUX-A
> > > > > > > > + * does not require the driver to disable DC states, but the rest do.
> > > > > > >
> > > > > > > This phrase is strange. Specially "the rest do" part.
> > > > > > > It seems that we need to disable DC states on other ports than A,
> > > > > > > what it is not true.
> > > > > >
> > > > > > Okay, let's back up a little bit.
> > > > > >
> > > > > > AUX domain for port A enables DC_OFF and AUX-IO. AUX domains for non-A
> > > > > > ports enable DC_OFF, power well 2 and AUX-IO, in fact all domains that
> > > > > > require power well 2 enable DC_OFF too. I can't see this is in bspec but
> > > > > > that's what the code does currently. So, this is for AUX transfers.
> > > > >
> > > > > To enable DC5/6 power well 2 needs to be disabled; I see this is still
> > > > > missing from the BSpec DC5/6 enable sequence, though I asked already for
> > > > > an update there before. Will ask again.
> > > >
> > > > Actually it's at "Mode Set[SKL+]/Sequences for Display C5 and C6":
> > > >
> > > > """
> > > > ...
> > > > 2. Configure display engine to have power wells above power well 1
> > > > disabled, following the appropriate mode set disable sequences for any
> > > > ports using power wells above power well 1. This can be done earlier if
> > > > desired.
> > > > ...
> > > > 4. Set DC_STATE_EN Dynamic DC State Enable = "Enable up to DC5" for DC5
> > > > or "Enable up to DC6" for DC6.
> > > >
> > > > """
> > > >
> > > Thanks, finding this information would have been much easier if it was
> > > all in one place. We still have to assume the converse from the enable
> > > step. I wonder if enabling power well 2 is sufficient to bring out the
> > > hardware from DC6 without having to explicitly disable it.
> >
> > Yep, that's not that clear, but I guess in any caes we can do that always
> > manually. Should file an update request for this.
> >
> > > > > Non-A AUX channels are contained in power well 2, so we have to disable
> > > > > DC states and enable power well 2 for non-A AUX channels.
> > > > >
> > > > > >
> > > > > > which means, this comment for the previous iteration of the patch
> > > > > > "I mean, on CNL
> > > > > >
> > > > > > POWER_DOMAIN_AUX_{B,C,D,F} are exactly the same as
> > > > > > POWER_DOMAIN_AUX__IO_{B,C,D,F}"
> > > > > >
> > > > > > was not accurate. _AUX_IO_{B,C,D,F} domains enable only AUX_IO well and
> > > > > > _AUX_{B,C,D,F} domains enable power well 2, DC_OFF and AUX_IO.
> > > > > >
> > > > > >
> > > > > > For PSR, we want only the AUX_IO to be enabled for any port because the
> > > > > > spec says -
> > > > > > "PSR spontaneously sends Aux transactions. If PSR is enabled on a port,
> > > > > > then the associated Aux IO must be kept powered up."
> > > > > >
> > > > > > which means, this comment
> > > > > > "Hm, so in general (for AUX transfers) to enable AUX-A we first need to
> > > > > > disable DC states _except_ if we enable AUX-A for PSR where we want
> > > > > > DC5/6 to stay enabled." applies to all ports.
> > > > >
> > > > > It doesn't make sense and we can't enable DC5/6 for non-A AUX channels,
> > > > > since those are contained in power well 2, which won't be powered on/off
> > > > > dynamically by DMC.
> > >
> > > Is the AUX-IO contained in power well 2? The AUX_CTL registers are.
> >
> > Not sure if the two things are divided but I would assume the HW/FW
> > needs the same registers to perform an AUX transfer as what the driver
> > programs. (Hence the need for the HW mutex.)
> >
> That's a fair guess but kinda defeats the point of having a separate
> power well for AUX IO, doesn't it?
No, if you have also other functionality backed by PW#1/2. Then by
powering off the AUX-IO power well when AUX transfers are not needed
(like when you have only the main link on) saves power even if PW#1/2 is
enabled.
> We should perhaps check this with Art.
Sure, I agree it's not completely clear.
> > > If the hardware is sending AUX transactions on AUX_A when PSR is
> > > enabled, does it mean it is also powering up power well 1 or just the
> > > AUX-IO?
> >
> > Again, I'd say it needs the same power as what's needed in case the
> > driver does AUX transfers. So yes, power well 1 would be powered on.
> >
> > --Imre
> >
> > >
> > >
> > > > >
> > > > > --Imre
> > > > >
> > > > > >
> > > > > > To just enable the AUX-IO well, the correct version is ->
> > > > > > https://patchwork.freedesktop.org/patch/205328/
> > > > > >
> > > > > >
> > > > > >
> > > > > > -DK
> > > > > > >
> > > > > > > > + * Although PSR is enabled only on Port A currently, let's do this
> > > > > > > > + * correctly 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_power_get(struct intel_dp *intel_dp)
> > > > > > >
> > > > > > > nip: psr_aux_io_power_get ?!
> > > > > > >
> > > > > > > > +{
> > > > > > > > + 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_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));
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
> > > > > > > > {
> > > > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > @@ -459,6 +493,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > > > > > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > > > > > > u32 chicken;
> > > > > > > >
> > > > > > > > + psr_power_get(intel_dp);
> > > > > > > > +
> > > > > > > > if (dev_priv->psr.psr2_support) {
> > > > > > > > chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > > > > > > if (dev_priv->psr.y_cord_support)
> > > > > > > > @@ -617,6 +653,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > > > > > > > else
> > > > > > > > WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > > > > > }
> > > > > > > > +
> > > > > > > > + psr_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 b7924feb9f27..53ea564f971e 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > > @@ -130,6 +130,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> > > > > > > > return "AUX_D";
> > > > > > > > case POWER_DOMAIN_AUX_F:
> > > > > > > > return "AUX_F";
> > > > > > > > + case POWER_DOMAIN_AUX_IO_A:
> > > > > > > > + return "AUX_IO_A";
> > > > > > > > case POWER_DOMAIN_GMBUS:
> > > > > > > > return "GMBUS";
> > > > > > > > case POWER_DOMAIN_INIT:
> > > > > > > > @@ -1853,6 +1855,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > > > > > > > BIT_ULL(POWER_DOMAIN_INIT))
> > > > > > > > #define CNL_DISPLAY_AUX_A_POWER_DOMAINS ( \
> > > > > > > > BIT_ULL(POWER_DOMAIN_AUX_A) | \
> > > > > > > > + BIT_ULL(POWER_DOMAIN_AUX_IO_A) | \
> > > > > > > > BIT_ULL(POWER_DOMAIN_INIT))
> > > > > > > > #define CNL_DISPLAY_AUX_B_POWER_DOMAINS ( \
> > > > > > > > BIT_ULL(POWER_DOMAIN_AUX_B) | \
> > > > > > > > --
> > > > > > > > 2.14.1
> > > > > > > >
> > > > > > > _______________________________________________
> > > > > > > Intel-gfx mailing list
> > > > > > > Intel-gfx at lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list