Re: [PATCH] drm/i915/bxt: Fix eDP panel power save/restore

[Jani Nikula <jani.nikula@linux.intel.com>]

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