| Message ID | 20180222202850.30221-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Suggested-by: Imre Deak <imre.deak@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. > + * 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 >
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@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Suggested-by: Imre Deak <imre.deak@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. 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. 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Feb 22, 2018 at 09:33:10PM +0000, 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@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. > > 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. > > To just enable the AUX-IO well, the correct version is -> > https://patchwork.freedesktop.org/patch/205328/ While that is true I don't think it actually matters in the end. As long as transcoder A-C is enabled DC will be off and PG2 will be enabled. So I suppose we can (ab)use that fact to save a few power domain bits and only define AUX_IO_A. The comment should probably be reworded to say as much. > > > > -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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. 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. --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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. """ > > 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. > > --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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-02-22 at 23:47 +0200, Ville Syrjälä wrote: > On Thu, Feb 22, 2018 at 09:33:10PM +0000, 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@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. > > > > 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. > > > > To just enable the AUX-IO well, the correct version is -> > > https://patchwork.freedesktop.org/patch/205328/ > > While that is true I don't think it actually matters in the end. As > long as transcoder A-C is enabled DC will be off and PG2 will be > enabled. So I suppose we can (ab)use that fact to save a few power > domain bits and only define AUX_IO_A. The comment should probably > be reworded to say as much. > "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." Sound good? > > > > > > > > -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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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@intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. > > > > 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. 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? > > > > --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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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.) > 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@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Feb 22, 2018 at 10:10:24PM +0000, Pandiyan, Dhinakaran wrote: > > > On Thu, 2018-02-22 at 23:47 +0200, Ville Syrjälä wrote: > > On Thu, Feb 22, 2018 at 09:33:10PM +0000, 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@intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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. > > > > > > 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. > > > > > > To just enable the AUX-IO well, the correct version is -> > > > https://patchwork.freedesktop.org/patch/205328/ > > > > While that is true I don't think it actually matters in the end. As > > long as transcoder A-C is enabled DC will be off and PG2 will be > > enabled. So I suppose we can (ab)use that fact to save a few power > > domain bits and only define AUX_IO_A. The comment should probably > > be reworded to say as much. > > > > "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." > > Sound good? Oh ok... thanks for explaining it to me in person as well... I was really missing that AUX B to F were already part for PG2 hence part of DC_OFF... Maybe the first version was less confusing actually... but we can go with this optmization. If you want to go with this version: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> and ville can merge when merging the aux_ch one. > > > > > > > > > > > > > -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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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? We should perhaps check this with Art. > > 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@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Suggested-by: Imre Deak <imre.deak@intel.com> > > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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@lists.freedesktop.org > > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. + * 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) +{ + 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) | \
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@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Suggested-by: Imre Deak <imre.deak@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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(-)