* [PATCH] drm/i915/bxt: Fix eDP panel power save/restore
@ 2015-12-30 22:56 Matt Roper
2015-12-31 3:01 ` Kannan, Vandana
2015-12-31 7:43 ` ✗ failure: Fi.CI.BAT Patchwork
0 siblings, 2 replies; 7+ messages in thread
From: Matt Roper @ 2015-12-30 22:56 UTC (permalink / raw)
To: intel-gfx; +Cc: drm-intel-fixes
Broxton-specific panel power sequencing was added in commit
commit b0a08bec96318be54db97c3f0b9e37b52561f9ea
Author: Vandana Kannan <vandana.kannan@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@intel.com>
Cc: drm-intel-fixes@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@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);
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915/bxt: Fix eDP panel power save/restore 2015-12-30 22:56 [PATCH] drm/i915/bxt: Fix eDP panel power save/restore Matt Roper @ 2015-12-31 3:01 ` Kannan, Vandana 2015-12-31 16:51 ` Matt Roper 2015-12-31 7:43 ` ✗ failure: Fi.CI.BAT Patchwork 1 sibling, 1 reply; 7+ messages in thread From: Kannan, Vandana @ 2015-12-31 3:01 UTC (permalink / raw) To: Matt Roper, intel-gfx, Jani Nikula; +Cc: drm-intel-fixes 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? On Thu, 30 Apr 2015, Vandana Kannan <vandana.kannan@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@intel.com> > Signed-off-by: A.Sunil Kamath <sunil.kamath@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@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@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@intel.com> > Cc: drm-intel-fixes@lists.freedesktop.org > Signed-off-by: Matt Roper <matthew.d.roper@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); > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/bxt: Fix eDP panel power save/restore 2015-12-31 3:01 ` Kannan, Vandana @ 2015-12-31 16:51 ` Matt Roper 2016-01-02 13:41 ` Jani Nikula 0 siblings, 1 reply; 7+ messages in thread From: Matt Roper @ 2015-12-31 16:51 UTC (permalink / raw) To: Kannan, Vandana; +Cc: intel-gfx, drm-intel-fixes 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@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@intel.com> > > Signed-off-by: A.Sunil Kamath <sunil.kamath@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@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@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@intel.com> > >Cc: drm-intel-fixes@lists.freedesktop.org > >Signed-off-by: Matt Roper <matthew.d.roper@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/bxt: Fix eDP panel power save/restore 2015-12-31 16:51 ` Matt Roper @ 2016-01-02 13:41 ` Jani Nikula 2016-01-06 8:12 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Jani Nikula @ 2016-01-02 13:41 UTC (permalink / raw) To: Matt Roper, Kannan, Vandana; +Cc: intel-gfx, drm-intel-fixes On Thu, 31 Dec 2015, Matt Roper <matthew.d.roper@intel.com> wrote: > 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? Not touching the registers in i915_suspend.c for BXT was what I meant, but probably failed at expressing it. The encoder/connector hooks should cover this, and if not, should be updated to cover this. The big hammer suspend/resume in i915_suspend.c is oblivious to most modeset state and is unable to do anything clever. BR, Jani. > > 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@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@intel.com> >> > Signed-off-by: A.Sunil Kamath <sunil.kamath@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@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@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@intel.com> >> >Cc: drm-intel-fixes@lists.freedesktop.org >> >Signed-off-by: Matt Roper <matthew.d.roper@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); >> > -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/bxt: Fix eDP panel power save/restore 2016-01-02 13:41 ` Jani Nikula @ 2016-01-06 8:12 ` Daniel Vetter 2016-01-07 8:37 ` Jani Nikula 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2016-01-06 8:12 UTC (permalink / raw) To: Jani Nikula; +Cc: drm-intel-fixes, intel-gfx On Sat, Jan 02, 2016 at 03:41:29PM +0200, Jani Nikula wrote: > On Thu, 31 Dec 2015, Matt Roper <matthew.d.roper@intel.com> wrote: > > 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? > > Not touching the registers in i915_suspend.c for BXT was what I meant, > but probably failed at expressing it. The encoder/connector hooks should > cover this, and if not, should be updated to cover this. The big hammer > suspend/resume in i915_suspend.c is oblivious to most modeset state and > is unable to do anything clever. Hm, I thought it died already? What's keeping the pp code in i915_suspend.c on life-support? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/bxt: Fix eDP panel power save/restore 2016-01-06 8:12 ` Daniel Vetter @ 2016-01-07 8:37 ` Jani Nikula 0 siblings, 0 replies; 7+ messages in thread From: Jani Nikula @ 2016-01-07 8:37 UTC (permalink / raw) To: Daniel Vetter; +Cc: drm-intel-fixes, intel-gfx On Wed, 06 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > On Sat, Jan 02, 2016 at 03:41:29PM +0200, Jani Nikula wrote: >> On Thu, 31 Dec 2015, Matt Roper <matthew.d.roper@intel.com> wrote: >> > 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? >> >> Not touching the registers in i915_suspend.c for BXT was what I meant, >> but probably failed at expressing it. The encoder/connector hooks should >> cover this, and if not, should be updated to cover this. The big hammer >> suspend/resume in i915_suspend.c is oblivious to most modeset state and >> is unable to do anything clever. > > Hm, I thought it died already? What's keeping the pp code in > i915_suspend.c on life-support? We just haven't added stuff for new platforms, but the old cruft is still there for old platforms. Haven't been bold enought to nuke it. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* ✗ failure: Fi.CI.BAT 2015-12-30 22:56 [PATCH] drm/i915/bxt: Fix eDP panel power save/restore Matt Roper 2015-12-31 3:01 ` Kannan, Vandana @ 2015-12-31 7:43 ` Patchwork 1 sibling, 0 replies; 7+ messages in thread From: Patchwork @ 2015-12-31 7:43 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx == Summary == Built on 79686f613b3955a4ed09cee936e7f70ec4e61b67 drm-intel-nightly: 2015y-12m-30d-11h-59m-54s UTC integration manifest Test gem_ctx_param_basic: Subgroup basic: pass -> DMESG-WARN (skl-i7k-2) Subgroup invalid-param-set: pass -> DMESG-WARN (skl-i7k-2) Test gem_mmap_gtt: Subgroup basic-write: pass -> DMESG-WARN (skl-i7k-2) Test gem_storedw_loop: Subgroup basic-render: dmesg-warn -> PASS (skl-i5k-2) Test kms_addfb_basic: Subgroup too-wide: pass -> DMESG-WARN (skl-i7k-2) Test kms_flip: Subgroup basic-flip-vs-dpms: dmesg-warn -> PASS (ilk-hp8440p) Subgroup basic-flip-vs-modeset: dmesg-warn -> PASS (bsw-nuc-2) dmesg-warn -> PASS (hsw-xps12) dmesg-warn -> PASS (hsw-brixbox) dmesg-warn -> PASS (bdw-nuci7) dmesg-warn -> PASS (ilk-hp8440p) Test kms_pipe_crc_basic: Subgroup read-crc-pipe-a: dmesg-warn -> PASS (snb-x220t) dmesg-warn -> PASS (byt-nuc) Subgroup read-crc-pipe-b: pass -> DMESG-WARN (snb-dellxps) Subgroup read-crc-pipe-b-frame-sequence: pass -> DMESG-FAIL (skl-i7k-2) Test kms_setmode: Subgroup basic-clone-single-crtc: dmesg-warn -> PASS (snb-dellxps) Test pm_rpm: Subgroup basic-rte: dmesg-warn -> PASS (byt-nuc) bdw-nuci7 total:132 pass:122 dwarn:1 dfail:0 fail:0 skip:9 bdw-ultra total:132 pass:124 dwarn:2 dfail:0 fail:0 skip:6 bsw-nuc-2 total:135 pass:114 dwarn:1 dfail:0 fail:0 skip:20 byt-nuc total:135 pass:121 dwarn:1 dfail:0 fail:0 skip:13 hsw-brixbox total:135 pass:127 dwarn:1 dfail:0 fail:0 skip:7 hsw-gt2 total:135 pass:130 dwarn:1 dfail:0 fail:0 skip:4 hsw-xps12 total:132 pass:126 dwarn:2 dfail:0 fail:0 skip:4 ilk-hp8440p total:135 pass:100 dwarn:0 dfail:0 fail:0 skip:35 ivb-t430s total:135 pass:127 dwarn:2 dfail:0 fail:0 skip:6 skl-i5k-2 total:135 pass:124 dwarn:3 dfail:0 fail:0 skip:8 skl-i7k-2 total:135 pass:119 dwarn:7 dfail:1 fail:0 skip:8 snb-dellxps total:135 pass:121 dwarn:2 dfail:0 fail:0 skip:12 snb-x220t total:135 pass:122 dwarn:1 dfail:0 fail:1 skip:11 Results at /archive/results/CI_IGT_test/Patchwork_970/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-07 8:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-12-30 22:56 [PATCH] drm/i915/bxt: Fix eDP panel power save/restore Matt Roper 2015-12-31 3:01 ` Kannan, Vandana 2015-12-31 16:51 ` Matt Roper 2016-01-02 13:41 ` Jani Nikula 2016-01-06 8:12 ` Daniel Vetter 2016-01-07 8:37 ` Jani Nikula 2015-12-31 7:43 ` ✗ failure: Fi.CI.BAT Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).