[Intel-gfx] [PATCH] drm/i915/bxt: Fix eDP panel power save/restore
Matt Roper
matthew.d.roper at intel.com
Thu Dec 31 08:51:46 PST 2015
On Thu, Dec 31, 2015 at 08:31:45AM +0530, Kannan, Vandana wrote:
> When I submitted the PPS patch in April, I got an input from Jani to
> not make changes in i915_suspend.c as it was to become obsolete.
> Below mail for your reference.
>
> Jani,
> Does your initial comment still hold good?
I don't think not touching i915_suspend.c at all is an option since that
means that the current code will try to read/write registers that simply
don't exist on the platform. Based on Jani's comment, maybe we should
just be adding BXT to the list of platforms we don't do any PP
save/restore on?
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 61bec8e..69d5965 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -60,7 +60,7 @@ static void i915_save_display(struct drm_device *dev)
dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
- } else if (!IS_VALLEYVIEW(dev)) {
+ } else if (!IS_VALLEYVIEW(dev) && !IS_BROXTON(dev)) {
dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
I'm not really familiar with the panel power sequencing stuff, so I'll let you
and Jani make the call there. I'm just looking at S3 on Broxton (which has all
kinds of problems right now), and figured I'd try to clear up a couple of the
unclaimed register warnings to make it more clear where the real S3 problems
lie.
Thanks.
Matt
>
> On Thu, 30 Apr 2015, Vandana Kannan <vandana.kannan at intel.com> wrote:
> > Changes based on future platform readiness patches related to
> > HAS_PCH_SPLIT(). Use HAS_GMCH_DISPLAY() instead of HAS_PCH_SPLIT
> >
> > Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
> > Signed-off-by: A.Sunil Kamath <sunil.kamath at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
> > drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> > b/drivers/gpu/drm/i915/i915_suspend.c
> > index cf67f82..e91d637 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -44,7 +44,7 @@ static void i915_save_display(struct drm_device *dev)
> > dev_priv->regfile.saveLVDS = I915_READ(LVDS);
> >
> > /* Panel power sequencer */
> > - if (HAS_PCH_SPLIT(dev)) {
> > + if (!HAS_GMCH_DISPLAY(dev)) {
> > dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
> > dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
> > dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
> > @@ -79,7 +79,7 @@ static void i915_restore_display(struct
> drm_device *dev)
> > I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
> >
> > /* Panel power sequencer */
> > - if (HAS_PCH_SPLIT(dev)) {
> > + if (!HAS_GMCH_DISPLAY(dev)) {
> > I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
> > I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
> > I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>
> I don't think you should touch i915_suspend.c at all. We're trying
> to get rid of this blind register save/restore, and make everything
> work in the encoder/connector code. Do note that we already skip
> this for vlv/chv power sequencer registers.
>
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index 937ba31..68e10c1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -559,7 +559,7 @@ static u32 _pp_ctrl_reg(struct intel_dp *intel_dp)
> > {
> > struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >
> > - if (HAS_PCH_SPLIT(dev))
> > + if (!HAS_GMCH_DISPLAY(dev))
> > return PCH_PP_CONTROL;
> > else
> > return VLV_PIPE_PP_CONTROL(vlv_power_sequencer_pipe(intel_dp));
> > @@ -569,7 +569,7 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
> > {
> > struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >
> > - if (HAS_PCH_SPLIT(dev))
> > + if (!HAS_GMCH_DISPLAY(dev))
> > return PCH_PP_STATUS;
> > else
> > return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
> > @@ -4963,7 +4963,7 @@ intel_dp_init_panel_power_sequencer(struct
> drm_device *dev,
> > if (final->t11_t12 != 0)
> > return;
> >
> > - if (HAS_PCH_SPLIT(dev)) {
> > + if (!HAS_GMCH_DISPLAY(dev)) {
> > pp_ctrl_reg = PCH_PP_CONTROL;
> > pp_on_reg = PCH_PP_ON_DELAYS;
> > pp_off_reg = PCH_PP_OFF_DELAYS;
> > @@ -5063,7 +5063,7 @@
> > intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> >
> > lockdep_assert_held(&dev_priv->pps_mutex);
> >
> > - if (HAS_PCH_SPLIT(dev)) {
> > + if (!HAS_GMCH_DISPLAY(dev)) {
> > pp_on_reg = PCH_PP_ON_DELAYS;
> > pp_off_reg = PCH_PP_OFF_DELAYS;
> > pp_div_reg = PCH_PP_DIVISOR;
>
> For the rest, I'd like you to do what I suggested in my reply to
> patch 3, i.e. add a new if (IS_BROXTON(dev)) at the top of each of
> these, with
> BXT_PP_WHATEVER(0) as the register. Later on, we can add code to
> choose the power sequencer, a bit similar to what vlv/chv do now
> (except there it's based on pipe).
>
> BR,
> Jani.
>
> > --
> > 2.0.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
>
> - Vandana
>
>
> On 12/31/2015 4:26 AM, Matt Roper wrote:
> >Broxton-specific panel power sequencing was added in commit
> >
> > commit b0a08bec96318be54db97c3f0b9e37b52561f9ea
> > Author: Vandana Kannan <vandana.kannan at intel.com>
> > Date: Thu Jun 18 11:00:55 2015 +0530
> >
> > drm/i915/bxt: eDP Panel Power sequencing
> >
> >As noted in that commit, Broxton has two sets of registers (and we're
> >supposed to read which to use from the VBT, although we're punting on
> >that part at the moment) and also doesn't have a divisor register.
> >Although we take the BXT-specific details into account at initial system
> >init, we fail to save/restore the appropriate registers around system
> >suspend/resume, which leads to unclaimed register warnings at the
> >moment, and may also fail to save the proper registers once we start
> >actually paying attention to the VBT.
> >
> >Cc: Vandana Kannan <vandana.kannan at intel.com>
> >Cc: drm-intel-fixes at lists.freedesktop.org
> >Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> >---
> > drivers/gpu/drm/i915/i915_suspend.c | 24 ++++++++++++++++++++++--
> > 1 file changed, 22 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> >index bf582fe..61bec8e 100644
> >--- a/drivers/gpu/drm/i915/i915_suspend.c
> >+++ b/drivers/gpu/drm/i915/i915_suspend.c
> >@@ -44,7 +44,18 @@ static void i915_save_display(struct drm_device *dev)
> > dev_priv->regfile.saveLVDS = I915_READ(LVDS);
> >
> > /* Panel power sequencer */
> >- if (HAS_PCH_SPLIT(dev)) {
> >+ if (IS_BROXTON(dev)) {
> >+ /*
> >+ * TODO: BXT has 2 sets of PPS registers.
> >+ * Correct Register for Broxton need to be identified
> >+ * using VBT. hardcoding for now.
> >+ *
> >+ * There's also no divisor register on Broxton.
> >+ */
> >+ dev_priv->regfile.savePP_CONTROL = I915_READ(BXT_PP_CONTROL(0));
> >+ dev_priv->regfile.savePP_ON_DELAYS = I915_READ(BXT_PP_ON_DELAYS(0));
> >+ dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(BXT_PP_OFF_DELAYS(0));
> >+ } else if (HAS_PCH_SPLIT(dev)) {
> > dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
> > dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
> > dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
> >@@ -83,7 +94,16 @@ static void i915_restore_display(struct drm_device *dev)
> > I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
> >
> > /* Panel power sequencer */
> >- if (HAS_PCH_SPLIT(dev)) {
> >+ if (IS_BROXTON(dev)) {
> >+ /*
> >+ * TODO: BXT has 2 sets of PPS registers.
> >+ * Correct Register for Broxton need to be identified
> >+ * using VBT. hardcoding for now
> >+ */
> >+ I915_WRITE(BXT_PP_ON_DELAYS(0), dev_priv->regfile.savePP_ON_DELAYS);
> >+ I915_WRITE(BXT_PP_OFF_DELAYS(0), dev_priv->regfile.savePP_OFF_DELAYS);
> >+ I915_WRITE(BXT_PP_CONTROL(0), dev_priv->regfile.savePP_CONTROL);
> >+ } else if (HAS_PCH_SPLIT(dev)) {
> > I915_WRITE(PCH_PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
> > I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
> > I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
> >
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list