[PATCH 1/3] Revert "drm/i915: dynamic Haswell display power well support"

[PATCH 1/3] Revert "drm/i915: dynamic Haswell display power well support"

[Daniel Vetter]

This reverts commit d6dd9eb1d96d2b7345fe4664066c2b7ed86da898.

The current hsw power wells code seems to break audio - the current
audio driver presumes that register contents are preserved when
disablng an output, which is no longer true with the power wells code.

And Takashi Iwai also reported a black screen regression. He has a
machine with an eDP panel on port D (usual all-in-one config), which
intel_dp.c identifies correctly as an eDP output, but our the power
wells code does not differentiate between eDP on port A or D. And the
magic only works on port A.

References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ddi.c     |    8 +-------
 drivers/gpu/drm/i915/intel_display.c |   31 -------------------------------
 2 files changed, 1 insertion(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 8d0bac3..42c79bf 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -992,13 +992,7 @@ void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)
 	if (cpu_transcoder == TRANSCODER_EDP) {
 		switch (pipe) {
 		case PIPE_A:
-			/* Can only use the always-on power well for eDP when
-			 * not using the panel fitter, and when not using motion
-			  * blur mitigation (which we don't support). */
-			if (dev_priv->pch_pf_size)
-				temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
-			else
-				temp |= TRANS_DDI_EDP_INPUT_A_ON;
+			temp |= TRANS_DDI_EDP_INPUT_A_ONOFF;
 			break;
 		case PIPE_B:
 			temp |= TRANS_DDI_EDP_INPUT_B_ONOFF;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 287b42c..7114e68 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5710,35 +5710,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	return fdi_config_ok ? ret : -EINVAL;
 }
 
-static void haswell_modeset_global_resources(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	bool enable = false;
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
-		if (crtc->pipe != PIPE_A && crtc->base.enabled)
-			enable = true;
-		/* XXX: Should check for edp transcoder here, but thanks to init
-		 * sequence that's not yet available. Just in case desktop eDP
-		 * on PORT D is possible on haswell, too. */
-	}
-
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
-		if (encoder->type != INTEL_OUTPUT_EDP &&
-		    encoder->connectors_active)
-			enable = true;
-	}
-
-	/* Even the eDP panel fitter is outside the always-on well. */
-	if (dev_priv->pch_pf_size)
-		enable = true;
-
-	intel_set_power_well(dev, enable);
-}
-
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 				 struct drm_display_mode *mode,
 				 struct drm_display_mode *adjusted_mode,
@@ -8635,8 +8606,6 @@ static void intel_init_display(struct drm_device *dev)
 		} else if (IS_HASWELL(dev)) {
 			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
 			dev_priv->display.write_eld = haswell_write_eld;
-			dev_priv->display.modeset_global_resources =
-				haswell_modeset_global_resources;
 		}
 	} else if (IS_G4X(dev)) {
 		dev_priv->display.write_eld = g4x_write_eld;
-- 
1.7.10.4

[PATCH 2/3] Revert "drm/i915: set TRANSCODER_EDP even earlier"

[Daniel Vetter]

This reverts commit cc464b2a17c59adedbdc02cc54341d630354edc3.

The reason is that Takashi Iwai reported a regression bisected to this
commit:

http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html

His machine has eDP on port D (usual desktop all-in-on setup), which
intel_dp.c identifies as an eDP panel, but the hsw ddi code
mishandles.

But in retrospective I also don't like that this leaks highly platform
specific details into common code, and the reason is that the drm
vblank layer sucks. So instead I think we should:
- move the cpu_transcoder into the dynamic pipe_config tracking (once
  that's merged).
- fix up the drm vblank layer to finally deal with kms crtc objects
  instead of int pipes.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7114e68..8eec205 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5742,6 +5742,11 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
+	if (is_cpu_edp)
+		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
+	else
+		intel_crtc->cpu_transcoder = pipe;
+
 	/* We are not sure yet this won't happen. */
 	WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
 	     INTEL_PCH_TYPE(dev));
@@ -5808,11 +5813,6 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 	int pipe = intel_crtc->pipe;
 	int ret;
 
-	if (IS_HASWELL(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))
-		intel_crtc->cpu_transcoder = TRANSCODER_EDP;
-	else
-		intel_crtc->cpu_transcoder = pipe;
-
 	drm_vblank_pre_modeset(dev, pipe);
 
 	ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
-- 
1.7.10.4

Re: [PATCH 2/3] Revert "drm/i915: set TRANSCODER_EDP even earlier"

[Paulo Zanoni]

2013/3/22 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This reverts commit cc464b2a17c59adedbdc02cc54341d630354edc3.
>
> The reason is that Takashi Iwai reported a regression bisected to this
> commit:
>
> http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
>
> His machine has eDP on port D (usual desktop all-in-on setup), which
> intel_dp.c identifies as an eDP panel, but the hsw ddi code
> mishandles.
>
> But in retrospective I also don't like that this leaks highly platform
> specific details into common code, and the reason is that the drm
> vblank layer sucks. So instead I think we should:
> - move the cpu_transcoder into the dynamic pipe_config tracking (once
>   that's merged).
> - fix up the drm vblank layer to finally deal with kms crtc objects
>   instead of int pipes.

Looks like we'll never get rid of all the unclaimed register errors...

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7114e68..8eec205 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5742,6 +5742,11 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>                 num_connectors++;
>         }
>
> +       if (is_cpu_edp)
> +               intel_crtc->cpu_transcoder = TRANSCODER_EDP;
> +       else
> +               intel_crtc->cpu_transcoder = pipe;
> +
>         /* We are not sure yet this won't happen. */
>         WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
>              INTEL_PCH_TYPE(dev));
> @@ -5808,11 +5813,6 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>         int pipe = intel_crtc->pipe;
>         int ret;
>
> -       if (IS_HASWELL(dev) && intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))
> -               intel_crtc->cpu_transcoder = TRANSCODER_EDP;
> -       else
> -               intel_crtc->cpu_transcoder = pipe;
> -
>         drm_vblank_pre_modeset(dev, pipe);
>
>         ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/3] Revert "drm/i915: set TRANSCODER_EDP even earlier"

[Daniel Vetter]

On Fri, Mar 22, 2013 at 09:30:05AM -0300, Paulo Zanoni wrote:
> 2013/3/22 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > This reverts commit cc464b2a17c59adedbdc02cc54341d630354edc3.
> >
> > The reason is that Takashi Iwai reported a regression bisected to this
> > commit:
> >
> > http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> >
> > His machine has eDP on port D (usual desktop all-in-on setup), which
> > intel_dp.c identifies as an eDP panel, but the hsw ddi code
> > mishandles.
> >
> > But in retrospective I also don't like that this leaks highly platform
> > specific details into common code, and the reason is that the drm
> > vblank layer sucks. So instead I think we should:
> > - move the cpu_transcoder into the dynamic pipe_config tracking (once
> >   that's merged).
> > - fix up the drm vblank layer to finally deal with kms crtc objects
> >   instead of int pipes.
> 
> Looks like we'll never get rid of all the unclaimed register errors...
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Picked up for -fixes, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] Revert "drm/i915: set TRANSCODER_EDP even earlier"

[Daniel Vetter]

On Fri, Mar 22, 2013 at 10:53 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This reverts commit cc464b2a17c59adedbdc02cc54341d630354edc3.
>
> The reason is that Takashi Iwai reported a regression bisected to this
> commit:
>
> http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
>
> His machine has eDP on port D (usual desktop all-in-on setup), which
> intel_dp.c identifies as an eDP panel, but the hsw ddi code
> mishandles.

Closer inspection of the code reveals that haswell_crtc_mode_set also
checks intel_encoder_is_pch_edp when setting is_cpu_edp. On haswell
that doesn't make much sense (since there's no edp on the pch), but
what this function _really_ checks is whether that edp connector is on
port A or port D. It's just that on ilk-ivb port D was on the pch ...

So that explains why this seemingly innocent change killed eDP on port
D. Furthermore it looks like everything else accidentally works, since
we've never enabled eDP on port D support for hsw intentionally (e.g.
we still register the HDMI output for port D in that case).

I'll add that to the commit message when merging.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Daniel Vetter]

This reverts the following commits:

cf0a6584aa6d382f802 drm/i915: write backlight harder
770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid

We've come full-circle in this mess and now broke the originally fixed
machines again with the new trick. So remove it all and start over.

References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_panel.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a3730e0..f31064a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -336,7 +336,7 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 		 * we don't track the backlight dpms state, hence check whether
 		 * we have to do anything first. */
 		if (tmp & BLM_PWM_ENABLE)
-			goto set_level;
+			return;
 
 		if (dev_priv->num_pipe == 3)
 			tmp &= ~BLM_PIPE_SELECT_IVB;
@@ -357,14 +357,6 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 			I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
 		}
 	}
-
-set_level:
-	/* Check the current backlight level and try to set again if it's zero.
-	 * On some machines, BLC_PWM_CPU_CTL is cleared to zero automatically
-	 * when BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1 are written.
-	 */
-	if (!intel_panel_get_backlight(dev))
-		intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
 }
 
 static void intel_panel_init_backlight(struct drm_device *dev)
-- 
1.7.10.4

Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Takashi Iwai]

At Fri, 22 Mar 2013 10:53:41 +0100,
Daniel Vetter wrote:
> 
> This reverts the following commits:
> 
> cf0a6584aa6d382f802 drm/i915: write backlight harder
> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
> 
> We've come full-circle in this mess and now broke the originally fixed
> machines again with the new trick. So remove it all and start over.
> 
> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Well, although I fully agree that it's better to clear all voodoo once
and fix from scratch, my concern is that it'll cause mess for stable
kernel.  This path won't fix anything by itself, so it's not suitable
for stable alone.  If any, it should be together with the real fix
patch.

I guess you'll submit the proper fix patch(es) to stable later on?


thanks,

Takashi

> ---
>  drivers/gpu/drm/i915/intel_panel.c |   10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index a3730e0..f31064a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -336,7 +336,7 @@ void intel_panel_enable_backlight(struct drm_device *dev,
>  		 * we don't track the backlight dpms state, hence check whether
>  		 * we have to do anything first. */
>  		if (tmp & BLM_PWM_ENABLE)
> -			goto set_level;
> +			return;
>  
>  		if (dev_priv->num_pipe == 3)
>  			tmp &= ~BLM_PIPE_SELECT_IVB;
> @@ -357,14 +357,6 @@ void intel_panel_enable_backlight(struct drm_device *dev,
>  			I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
>  		}
>  	}
> -
> -set_level:
> -	/* Check the current backlight level and try to set again if it's zero.
> -	 * On some machines, BLC_PWM_CPU_CTL is cleared to zero automatically
> -	 * when BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1 are written.
> -	 */
> -	if (!intel_panel_get_backlight(dev))
> -		intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
>  }
>  
>  static void intel_panel_init_backlight(struct drm_device *dev)
> -- 
> 1.7.10.4
> 

Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Daniel Vetter]

On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Fri, 22 Mar 2013 10:53:41 +0100,
> Daniel Vetter wrote:
>>
>> This reverts the following commits:
>>
>> cf0a6584aa6d382f802 drm/i915: write backlight harder
>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
>>
>> We've come full-circle in this mess and now broke the originally fixed
>> machines again with the new trick. So remove it all and start over.
>>
>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Well, although I fully agree that it's better to clear all voodoo once
> and fix from scratch, my concern is that it'll cause mess for stable
> kernel.  This path won't fix anything by itself, so it's not suitable
> for stable alone.  If any, it should be together with the real fix
> patch.
>
> I guess you'll submit the proper fix patch(es) to stable later on?

Oops, I've missed that 770c12312ad617172 itself is a regression fix,
but lacks the relevant commit citations. I'll resend with more
aggressive reverting.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Daniel Vetter]

On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> At Fri, 22 Mar 2013 10:53:41 +0100,
>> Daniel Vetter wrote:
>>>
>>> This reverts the following commits:
>>>
>>> cf0a6584aa6d382f802 drm/i915: write backlight harder
>>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
>>>
>>> We've come full-circle in this mess and now broke the originally fixed
>>> machines again with the new trick. So remove it all and start over.
>>>
>>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Takashi Iwai <tiwai@suse.de>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Well, although I fully agree that it's better to clear all voodoo once
>> and fix from scratch, my concern is that it'll cause mess for stable
>> kernel.  This path won't fix anything by itself, so it's not suitable
>> for stable alone.  If any, it should be together with the real fix
>> patch.
>>
>> I guess you'll submit the proper fix patch(es) to stable later on?
>
> Oops, I've missed that 770c12312ad617172 itself is a regression fix,
> but lacks the relevant commit citations. I'll resend with more
> aggressive reverting.

Actually the only other patch I've found is 6db65cbb941f, but no one
reported a regression against that one yet. So I think we can leave
that in for now. So I still think that this is the patch I want for
3.9&stable, since oldest regression wins.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Takashi Iwai]

At Fri, 22 Mar 2013 11:22:57 +0100,
Daniel Vetter wrote:
> 
> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> At Fri, 22 Mar 2013 10:53:41 +0100,
> >> Daniel Vetter wrote:
> >>>
> >>> This reverts the following commits:
> >>>
> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder
> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
> >>>
> >>> We've come full-circle in this mess and now broke the originally fixed
> >>> machines again with the new trick. So remove it all and start over.
> >>>
> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> >>> Cc: Jani Nikula <jani.nikula@intel.com>
> >>> Cc: Takashi Iwai <tiwai@suse.de>
> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> Well, although I fully agree that it's better to clear all voodoo once
> >> and fix from scratch, my concern is that it'll cause mess for stable
> >> kernel.  This path won't fix anything by itself, so it's not suitable
> >> for stable alone.  If any, it should be together with the real fix
> >> patch.
> >>
> >> I guess you'll submit the proper fix patch(es) to stable later on?
> >
> > Oops, I've missed that 770c12312ad617172 itself is a regression fix,
> > but lacks the relevant commit citations. I'll resend with more
> > aggressive reverting.
> 
> Actually the only other patch I've found is 6db65cbb941f, but no one
> reported a regression against that one yet. So I think we can leave
> that in for now. So I still think that this is the patch I want for
> 3.9&stable, since oldest regression wins.

Yeah, the situation is messy.  Let me write down as far as I remember:

- 770c12312ad617172 itself fixed the backlight at resume originally
  with 3.6 kernel.  The backlight worked at boot time, IIRC.  It had
  no mention of regressing commit since it was a series of changes,
  IIRC.  It fixed the same problem for a few Lenovo and HP laptops.

  However, this turned out to break a backlight on a Dell machine.

- cf0a6584aa6d382f802 is a try to fix the Dell backlight, so it went
  in 3.9 and propagated to stable.  But it hadn't been tested on
  Lenovo nor HP machines.

  Now this broke HP machines (not sure about Lenovo) again.
  Interestingly, at this time, it broke the backlight at boot time.
  Maybe the recent changes of initialization sequence at boot made
  difference?

- 6db65cbb941f is slightly different fix from other commits.
  AFAIK, this was found only on SandyBridge eDP (HP Z-something
  model), and this happens only at resume, again.

  Unfortunately I have no longer access to this machine, so I can't
  check the current status.

Now, you revert both first two, that is, the backlight handling is
like 3.6-rc1.  A Dell laptop should still work, but HP/Lenovo are
likely broken yet.  That's why I wonder whether it's suitable for
stable.  It cleans up the code but doesn't fix actual bugs.

Apart from that, a big question is why the commit 770c12312ad617172
broke the Dell laptop.  It just writes BLC_PWM_CPU_CTL again.  If this
really breaks, won't further changing the backlight (or even setting
the same level) break again even with the "fix" patch?
Since intel_panel_set_backlight() just updates  BLC_PWM_CPU_CTL, the
effect should be identical...


Takashi

Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Daniel Vetter]

On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Fri, 22 Mar 2013 11:22:57 +0100,
> Daniel Vetter wrote:
>> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> At Fri, 22 Mar 2013 10:53:41 +0100,
>> >> Daniel Vetter wrote:
>> >>>
>> >>> This reverts the following commits:
>> >>>
>> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder
>> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
>> >>>
>> >>> We've come full-circle in this mess and now broke the originally fixed
>> >>> machines again with the new trick. So remove it all and start over.
>> >>>
>> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
>> >>> Cc: Jani Nikula <jani.nikula@intel.com>
>> >>> Cc: Takashi Iwai <tiwai@suse.de>
>> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>
>> >> Well, although I fully agree that it's better to clear all voodoo once
>> >> and fix from scratch, my concern is that it'll cause mess for stable
>> >> kernel.  This path won't fix anything by itself, so it's not suitable
>> >> for stable alone.  If any, it should be together with the real fix
>> >> patch.
>> >>
>> >> I guess you'll submit the proper fix patch(es) to stable later on?
>> >
>> > Oops, I've missed that 770c12312ad617172 itself is a regression fix,
>> > but lacks the relevant commit citations. I'll resend with more
>> > aggressive reverting.
>>
>> Actually the only other patch I've found is 6db65cbb941f, but no one
>> reported a regression against that one yet. So I think we can leave
>> that in for now. So I still think that this is the patch I want for
>> 3.9&stable, since oldest regression wins.
>
> Yeah, the situation is messy.  Let me write down as far as I remember:
>
> - 770c12312ad617172 itself fixed the backlight at resume originally
>   with 3.6 kernel.  The backlight worked at boot time, IIRC.  It had
>   no mention of regressing commit since it was a series of changes,
>   IIRC.  It fixed the same problem for a few Lenovo and HP laptops.

I think the important question is whether those Lenovo/HPs ever
worked? If 770c is a regression fix, too, we'll need to revert more I
think.

>   However, this turned out to break a backlight on a Dell machine.
>
> - cf0a6584aa6d382f802 is a try to fix the Dell backlight, so it went
>   in 3.9 and propagated to stable.  But it hadn't been tested on
>   Lenovo nor HP machines.
>
>   Now this broke HP machines (not sure about Lenovo) again.
>   Interestingly, at this time, it broke the backlight at boot time.
>   Maybe the recent changes of initialization sequence at boot made
>   difference?
>
> - 6db65cbb941f is slightly different fix from other commits.
>   AFAIK, this was found only on SandyBridge eDP (HP Z-something
>   model), and this happens only at resume, again.
>
>   Unfortunately I have no longer access to this machine, so I can't
>   check the current status.

Iirc we've used 6db6 as inspiration/justification for the above commits.

> Now, you revert both first two, that is, the backlight handling is
> like 3.6-rc1.  A Dell laptop should still work, but HP/Lenovo are
> likely broken yet.  That's why I wonder whether it's suitable for
> stable.  It cleans up the code but doesn't fix actual bugs.
>
> Apart from that, a big question is why the commit 770c12312ad617172
> broke the Dell laptop.  It just writes BLC_PWM_CPU_CTL again.  If this
> really breaks, won't further changing the backlight (or even setting
> the same level) break again even with the "fix" patch?
> Since intel_panel_set_backlight() just updates  BLC_PWM_CPU_CTL, the
> effect should be identical...

One of the reasons why I want to go back to the beginning and rip out
all the hacks is that I've noticed that we completely lack locking in
the backlight code. And we have at least 3 concurrent useres:
- modeset code
- sysfs backlight interface
- asle interrupt handler

Especially the last one is known to fire when touching some of the
blc/panel registers through some smm magic. So imo before we add more
hacks again, we should fix up the locking (and generally make the code
a bit less messy, Jani has started with some patches), and then check
which bugs are left.

Yeah, this will suck a bit for distros :(

Yours, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Takashi Iwai]

At Fri, 22 Mar 2013 12:22:42 +0100,
Daniel Vetter wrote:
> 
> On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Fri, 22 Mar 2013 11:22:57 +0100,
> > Daniel Vetter wrote:
> >> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >> At Fri, 22 Mar 2013 10:53:41 +0100,
> >> >> Daniel Vetter wrote:
> >> >>>
> >> >>> This reverts the following commits:
> >> >>>
> >> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder
> >> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
> >> >>>
> >> >>> We've come full-circle in this mess and now broke the originally fixed
> >> >>> machines again with the new trick. So remove it all and start over.
> >> >>>
> >> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> >> >>> Cc: Jani Nikula <jani.nikula@intel.com>
> >> >>> Cc: Takashi Iwai <tiwai@suse.de>
> >> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> >>
> >> >> Well, although I fully agree that it's better to clear all voodoo once
> >> >> and fix from scratch, my concern is that it'll cause mess for stable
> >> >> kernel.  This path won't fix anything by itself, so it's not suitable
> >> >> for stable alone.  If any, it should be together with the real fix
> >> >> patch.
> >> >>
> >> >> I guess you'll submit the proper fix patch(es) to stable later on?
> >> >
> >> > Oops, I've missed that 770c12312ad617172 itself is a regression fix,
> >> > but lacks the relevant commit citations. I'll resend with more
> >> > aggressive reverting.
> >>
> >> Actually the only other patch I've found is 6db65cbb941f, but no one
> >> reported a regression against that one yet. So I think we can leave
> >> that in for now. So I still think that this is the patch I want for
> >> 3.9&stable, since oldest regression wins.
> >
> > Yeah, the situation is messy.  Let me write down as far as I remember:
> >
> > - 770c12312ad617172 itself fixed the backlight at resume originally
> >   with 3.6 kernel.  The backlight worked at boot time, IIRC.  It had
> >   no mention of regressing commit since it was a series of changes,
> >   IIRC.  It fixed the same problem for a few Lenovo and HP laptops.
> 
> I think the important question is whether those Lenovo/HPs ever
> worked?

IIRC, the boot screen worked on 3.6-rc but the resume was broken -- at
least on HP machines.  On Lenovo, it might be different.  Sorry, I
don't remember.  Need to dig into LKML archive.

> If 770c is a regression fix, too, we'll need to revert more I
> think.

Yes, but a revert wasn't trivial at that time because of large changes
in i915 code in 3.6-rc1.  Maybe we can try it again...

> >   However, this turned out to break a backlight on a Dell machine.
> >
> > - cf0a6584aa6d382f802 is a try to fix the Dell backlight, so it went
> >   in 3.9 and propagated to stable.  But it hadn't been tested on
> >   Lenovo nor HP machines.
> >
> >   Now this broke HP machines (not sure about Lenovo) again.
> >   Interestingly, at this time, it broke the backlight at boot time.
> >   Maybe the recent changes of initialization sequence at boot made
> >   difference?
> >
> > - 6db65cbb941f is slightly different fix from other commits.
> >   AFAIK, this was found only on SandyBridge eDP (HP Z-something
> >   model), and this happens only at resume, again.
> >
> >   Unfortunately I have no longer access to this machine, so I can't
> >   check the current status.
> 
> Iirc we've used 6db6 as inspiration/justification for the above commits.

Well, not from the beginning; it was reported on different machines as
an individual issue.  But the end fix looks similar, so very likely
related.

Another point is that the fixes are targeted to different ranges:

The fix 770c was for a variety of machines and all were LVDS.  (But
now we see it's affecting to HSW and IVY eDP, too.)

The fix 6db6 is for SNB eDP only.  We didn't see the issue on LVDS at
that time.


> > Now, you revert both first two, that is, the backlight handling is
> > like 3.6-rc1.  A Dell laptop should still work, but HP/Lenovo are
> > likely broken yet.  That's why I wonder whether it's suitable for
> > stable.  It cleans up the code but doesn't fix actual bugs.
> >
> > Apart from that, a big question is why the commit 770c12312ad617172
> > broke the Dell laptop.  It just writes BLC_PWM_CPU_CTL again.  If this
> > really breaks, won't further changing the backlight (or even setting
> > the same level) break again even with the "fix" patch?
> > Since intel_panel_set_backlight() just updates  BLC_PWM_CPU_CTL, the
> > effect should be identical...
> 
> One of the reasons why I want to go back to the beginning and rip out
> all the hacks is that I've noticed that we completely lack locking in
> the backlight code. And we have at least 3 concurrent useres:
> - modeset code
> - sysfs backlight interface
> - asle interrupt handler

Yeah, that looks fragile.

> Especially the last one is known to fire when touching some of the
> blc/panel registers through some smm magic. So imo before we add more
> hacks again, we should fix up the locking (and generally make the code
> a bit less messy, Jani has started with some patches), and then check
> which bugs are left.
> 
> Yeah, this will suck a bit for distros :(

Hehe, that's hard life :p


Takashi

Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Daniel Vetter]

On Fri, Mar 22, 2013 at 12:34:41PM +0100, Takashi Iwai wrote:
> At Fri, 22 Mar 2013 12:22:42 +0100,
> Daniel Vetter wrote:
> > 
> > On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > > At Fri, 22 Mar 2013 11:22:57 +0100,
> > > Daniel Vetter wrote:
> > >> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > >> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > >> >> At Fri, 22 Mar 2013 10:53:41 +0100,
> > >> >> Daniel Vetter wrote:
> > >> >>>
> > >> >>> This reverts the following commits:
> > >> >>>
> > >> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder
> > >> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
> > >> >>>
> > >> >>> We've come full-circle in this mess and now broke the originally fixed
> > >> >>> machines again with the new trick. So remove it all and start over.
> > >> >>>
> > >> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> > >> >>> Cc: Jani Nikula <jani.nikula@intel.com>
> > >> >>> Cc: Takashi Iwai <tiwai@suse.de>
> > >> >>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >> >>
> > >> >> Well, although I fully agree that it's better to clear all voodoo once
> > >> >> and fix from scratch, my concern is that it'll cause mess for stable
> > >> >> kernel.  This path won't fix anything by itself, so it's not suitable
> > >> >> for stable alone.  If any, it should be together with the real fix
> > >> >> patch.
> > >> >>
> > >> >> I guess you'll submit the proper fix patch(es) to stable later on?
> > >> >
> > >> > Oops, I've missed that 770c12312ad617172 itself is a regression fix,
> > >> > but lacks the relevant commit citations. I'll resend with more
> > >> > aggressive reverting.
> > >>
> > >> Actually the only other patch I've found is 6db65cbb941f, but no one
> > >> reported a regression against that one yet. So I think we can leave
> > >> that in for now. So I still think that this is the patch I want for
> > >> 3.9&stable, since oldest regression wins.
> > >
> > > Yeah, the situation is messy.  Let me write down as far as I remember:
> > >
> > > - 770c12312ad617172 itself fixed the backlight at resume originally
> > >   with 3.6 kernel.  The backlight worked at boot time, IIRC.  It had
> > >   no mention of regressing commit since it was a series of changes,
> > >   IIRC.  It fixed the same problem for a few Lenovo and HP laptops.
> > 
> > I think the important question is whether those Lenovo/HPs ever
> > worked?
> 
> IIRC, the boot screen worked on 3.6-rc but the resume was broken -- at
> least on HP machines.  On Lenovo, it might be different.  Sorry, I
> don't remember.  Need to dig into LKML archive.

Hm, right. And iirc (my google fu is lacking atm) we never figured out
what's really broken, only noticed that your patch fixes it. So I think I
should only revert cf0a6584aa6d382f802 drm/i915: write backlight harder.

Which leaves us with another regression, but afaics there's no way to win
this game here right now. I really hope that cleaning up the code and
introducing proper locking helps a bit to win this game of whack-a-mole.

I'll send the new revert asap.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] Revert "drm/i915: write backlight harder"

[Daniel Vetter]

This reverts commit cf0a6584aa6d382f802f2c3cacac23ccbccde0cd.

Turns out that cargo-culting breaks systems. Note that we can't revert
further, since

commit 770c12312ad617172b1a65b911d3e6564fc5aca8
Author: Takashi Iwai <tiwai@suse.de>
Date:   Sat Aug 11 08:56:42 2012 +0200

    drm/i915: Fix blank panel at reopening lid

fixed a regression in 3.6-rc kernels for which we've never figured out
the exact root cause. But some further inspection of the backlight
code reveals that it's seriously lacking locking. And especially the
asle backlight update is know to get fired (through some smm magic)
when writing specific backlight control registers. So the possibility
of suffering from races is rather real.

Until those races are fixed I don't think it makes sense to try
further hacks. Which sucks a bit, but sometimes that's how it is :(

References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_panel.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a3730e0..bee8cb6 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -321,9 +321,6 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 	if (dev_priv->backlight_level == 0)
 		dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
 
-	dev_priv->backlight_enabled = true;
-	intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
-
 	if (INTEL_INFO(dev)->gen >= 4) {
 		uint32_t reg, tmp;
 
@@ -359,12 +356,12 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 	}
 
 set_level:
-	/* Check the current backlight level and try to set again if it's zero.
-	 * On some machines, BLC_PWM_CPU_CTL is cleared to zero automatically
-	 * when BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1 are written.
+	/* Call below after setting BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1.
+	 * BLC_PWM_CPU_CTL may be cleared to zero automatically when these
+	 * registers are set.
 	 */
-	if (!intel_panel_get_backlight(dev))
-		intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
+	dev_priv->backlight_enabled = true;
+	intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
 }
 
 static void intel_panel_init_backlight(struct drm_device *dev)
-- 
1.7.10.4

Re: [PATCH] Revert "drm/i915: write backlight harder"

[Takashi Iwai]

At Fri, 22 Mar 2013 15:44:46 +0100,
Daniel Vetter wrote:
> 
> This reverts commit cf0a6584aa6d382f802f2c3cacac23ccbccde0cd.
> 
> Turns out that cargo-culting breaks systems. Note that we can't revert
> further, since
> 
> commit 770c12312ad617172b1a65b911d3e6564fc5aca8
> Author: Takashi Iwai <tiwai@suse.de>
> Date:   Sat Aug 11 08:56:42 2012 +0200
> 
>     drm/i915: Fix blank panel at reopening lid
> 
> fixed a regression in 3.6-rc kernels for which we've never figured out
> the exact root cause. But some further inspection of the backlight
> code reveals that it's seriously lacking locking. And especially the
> asle backlight update is know to get fired (through some smm magic)
> when writing specific backlight control registers. So the possibility
> of suffering from races is rather real.
> 
> Until those races are fixed I don't think it makes sense to try
> further hacks. Which sucks a bit, but sometimes that's how it is :(
> 
> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Feel free to add my tested-by tags to all three.

  Tested-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH] Revert "drm/i915: write backlight harder"

[Daniel Vetter]

On Fri, Mar 22, 2013 at 04:06:14PM +0100, Takashi Iwai wrote:
> At Fri, 22 Mar 2013 15:44:46 +0100,
> Daniel Vetter wrote:
> > 
> > This reverts commit cf0a6584aa6d382f802f2c3cacac23ccbccde0cd.
> > 
> > Turns out that cargo-culting breaks systems. Note that we can't revert
> > further, since
> > 
> > commit 770c12312ad617172b1a65b911d3e6564fc5aca8
> > Author: Takashi Iwai <tiwai@suse.de>
> > Date:   Sat Aug 11 08:56:42 2012 +0200
> > 
> >     drm/i915: Fix blank panel at reopening lid
> > 
> > fixed a regression in 3.6-rc kernels for which we've never figured out
> > the exact root cause. But some further inspection of the backlight
> > code reveals that it's seriously lacking locking. And especially the
> > asle backlight update is know to get fired (through some smm magic)
> > when writing specific backlight control registers. So the possibility
> > of suffering from races is rather real.
> > 
> > Until those races are fixed I don't think it makes sense to try
> > further hacks. Which sucks a bit, but sometimes that's how it is :(
> > 
> > References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Feel free to add my tested-by tags to all three.
> 
>   Tested-by: Takashi Iwai <tiwai@suse.de>

Picked up for -fixes with the additional kernel bz link for #47941 added.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/7] drm/i915: don't disable the power well yet

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We're still not 100% ready to disable the power well, so don't disable
it for now.

Also, instead of just reverting the code, add a Kernel option to let
us disable it if we want. This will allow us to keep developing and
testing the feature while it's not enabled.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    5 +++++
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/intel_pm.c |    3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b13c..0614aff 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -123,6 +123,11 @@ module_param_named(preliminary_hw_support, i915_preliminary_hw_support, int, 060
 MODULE_PARM_DESC(preliminary_hw_support,
 		"Enable preliminary hardware support. (default: false)");
 
+int i915_disable_power_well __read_mostly = 0;
+module_param_named(disable_power_well, i915_disable_power_well, int, 0600);
+MODULE_PARM_DESC(disable_power_well,
+		 "Disable the power well when possible (default: false)");
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1657d873..625cccf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1398,6 +1398,7 @@ extern int i915_enable_fbc __read_mostly;
 extern bool i915_enable_hangcheck __read_mostly;
 extern int i915_enable_ppgtt __read_mostly;
 extern unsigned int i915_preliminary_hw_support __read_mostly;
+extern int i915_disable_power_well __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ce3db2c..2de6da6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4095,6 +4095,9 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 	if (!HAS_POWER_WELL(dev))
 		return;
 
+	if (!i915_disable_power_well && !enable)
+		return;
+
 	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
 	is_enabled = tmp & HSW_PWR_WELL_STATE;
 	enable_requested = tmp & HSW_PWR_WELL_ENABLE;
-- 
1.7.10.4

Re: [PATCH 1/7] drm/i915: don't disable the power well yet

[Daniel Vetter]

On Fri, Mar 22, 2013 at 3:45 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We're still not 100% ready to disable the power well, so don't disable
> it for now.
>
> Also, instead of just reverting the code, add a Kernel option to let
> us disable it if we want. This will allow us to keep developing and
> testing the feature while it's not enabled.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Can you please pimp the commit message with the usual links to the
regression report + sha1 of the commit which introduced the
regression? I've just stumbled over a commit in the backlight mess
which didn't link to the lkml discussion (nor mentioned it) and it was
a pain the fish out details from my memory ;-) Also please cc: Takashi
and our own Intel audio guys, plus please mention what exactly blew up
(e.g. audio driver doesn't expect that registers get reset to 0 and
Takashi's machine has eDP on port D, which we did not consider).

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/7] drm/i915: don't disable the power well yet

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We're still not 100% ready to disable the power well, so don't disable
it for now. When we disable it we break the audio driver (because some
of the audio registers are on the power well) and machines with eDP on
port D (because it doesn't use TRANSCODER_EDP).

Also, instead of just reverting the code, add a Kernel option to let
us disable it if we want. This will allow us to keep developing and
testing the feature while it's not enabled.

This fixes problems caused by the following commit:
  commit d6dd9eb1d96d2b7345fe4664066c2b7ed86da898
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
  Date:   Tue Jan 29 16:35:20 2013 -0200
       drm/i915: dynamic Haswell display power well support

References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Mengdong Lin <mengdong.lin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    5 +++++
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/intel_pm.c |    3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b13c..0614aff 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -123,6 +123,11 @@ module_param_named(preliminary_hw_support, i915_preliminary_hw_support, int, 060
 MODULE_PARM_DESC(preliminary_hw_support,
 		"Enable preliminary hardware support. (default: false)");
 
+int i915_disable_power_well __read_mostly = 0;
+module_param_named(disable_power_well, i915_disable_power_well, int, 0600);
+MODULE_PARM_DESC(disable_power_well,
+		 "Disable the power well when possible (default: false)");
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1657d873..625cccf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1398,6 +1398,7 @@ extern int i915_enable_fbc __read_mostly;
 extern bool i915_enable_hangcheck __read_mostly;
 extern int i915_enable_ppgtt __read_mostly;
 extern unsigned int i915_preliminary_hw_support __read_mostly;
+extern int i915_disable_power_well __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ce3db2c..2de6da6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4095,6 +4095,9 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 	if (!HAS_POWER_WELL(dev))
 		return;
 
+	if (!i915_disable_power_well && !enable)
+		return;
+
 	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
 	is_enabled = tmp & HSW_PWR_WELL_STATE;
 	enable_requested = tmp & HSW_PWR_WELL_ENABLE;
-- 
1.7.10.4

Re: [PATCH 1/7] drm/i915: don't disable the power well yet

[Daniel Vetter]

On Fri, Mar 22, 2013 at 02:07:23PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We're still not 100% ready to disable the power well, so don't disable
> it for now. When we disable it we break the audio driver (because some
> of the audio registers are on the power well) and machines with eDP on
> port D (because it doesn't use TRANSCODER_EDP).
> 
> Also, instead of just reverting the code, add a Kernel option to let
> us disable it if we want. This will allow us to keep developing and
> testing the feature while it's not enabled.
> 
> This fixes problems caused by the following commit:
>   commit d6dd9eb1d96d2b7345fe4664066c2b7ed86da898
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>   Date:   Tue Jan 29 16:35:20 2013 -0200
>        drm/i915: dynamic Haswell display power well support
> 
> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Mengdong Lin <mengdong.lin@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Picked up for -fixes, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch