[PATCH 1/3] drm/i915: remove i915_disable_vga_mem declaration

[PATCH 1/3] drm/i915: remove i915_disable_vga_mem declaration

[Paulo Zanoni]

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

It was supposed to have been killed on the same commit that killed the
function, e1264ebe9ff48e1b3e1dd11805eec9f5b143ab7c, but I guess the
intel_drv.h reorganization accidentally brought it back.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f074c8..e4f01ca 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -703,7 +703,6 @@ void
 ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
 				int dotclock);
 bool intel_crtc_active(struct drm_crtc *crtc);
-void i915_disable_vga_mem(struct drm_device *dev);
 void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
 void intel_display_set_init_power(struct drm_device *dev, bool enable);
-- 
1.8.3.1

[PATCH 2/3] drm/i915: extract hsw_power_well_post_{enable, disable}

[Paulo Zanoni]

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

I want to add more code to the post_enable function.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 75 ++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c01d08d..ce2a188 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5682,12 +5682,53 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	return is_enabled;
 }
 
+static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	unsigned long irqflags;
+
+	if (IS_BROADWELL(dev)) {
+		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+		I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
+			   dev_priv->de_irq_mask[PIPE_B]);
+		I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
+			   ~dev_priv->de_irq_mask[PIPE_B] |
+			   GEN8_PIPE_VBLANK);
+		I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
+			   dev_priv->de_irq_mask[PIPE_C]);
+		I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
+			   ~dev_priv->de_irq_mask[PIPE_C] |
+			   GEN8_PIPE_VBLANK);
+		POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
+		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+	}
+}
+
+static void hsw_power_well_post_disable(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	enum pipe p;
+	unsigned long irqflags;
+
+	/*
+	 * After this, the registers on the pipes that are part of the power
+	 * well will become zero, so we have to adjust our counters according to
+	 * that.
+	 *
+	 * FIXME: Should we do this in general in drm_vblank_post_modeset?
+	 */
+	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	for_each_pipe(p)
+		if (p != PIPE_A)
+			dev->vblank[p].last = 0;
+	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+}
+
 static void hsw_set_power_well(struct drm_device *dev,
 			       struct i915_power_well *power_well, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool is_enabled, enable_requested;
-	unsigned long irqflags;
 	uint32_t tmp;
 
 	WARN_ON(dev_priv->pc8.enabled);
@@ -5708,42 +5749,14 @@ static void hsw_set_power_well(struct drm_device *dev,
 				DRM_ERROR("Timeout enabling power well\n");
 		}
 
-		if (IS_BROADWELL(dev)) {
-			spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-			I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
-				   dev_priv->de_irq_mask[PIPE_B]);
-			I915_WRITE(GEN8_DE_PIPE_IER(PIPE_B),
-				   ~dev_priv->de_irq_mask[PIPE_B] |
-				   GEN8_PIPE_VBLANK);
-			I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_C),
-				   dev_priv->de_irq_mask[PIPE_C]);
-			I915_WRITE(GEN8_DE_PIPE_IER(PIPE_C),
-				   ~dev_priv->de_irq_mask[PIPE_C] |
-				   GEN8_PIPE_VBLANK);
-			POSTING_READ(GEN8_DE_PIPE_IER(PIPE_C));
-			spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
-		}
+		hsw_power_well_post_enable(dev_priv);
 	} else {
 		if (enable_requested) {
-			enum pipe p;
-
 			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Requesting to disable the power well\n");
 
-			/*
-			 * After this, the registers on the pipes that are part
-			 * of the power well will become zero, so we have to
-			 * adjust our counters according to that.
-			 *
-			 * FIXME: Should we do this in general in
-			 * drm_vblank_post_modeset?
-			 */
-			spin_lock_irqsave(&dev->vbl_lock, irqflags);
-			for_each_pipe(p)
-				if (p != PIPE_A)
-					dev->vblank[p].last = 0;
-			spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+			hsw_power_well_post_disable(dev_priv);
 		}
 	}
 }
-- 
1.8.3.1

[PATCH 3/3] drm/i915: touch VGA MSR after we enable the power well

[Paulo Zanoni]

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

Fixes regression introduced by:
    commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
    Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
    Date:   Wed Jul 3 17:12:13 2013 -0300
        drm/i915: switch disable_power_well default value to 1

The bug I'm seeing can be reproduced with:
  - Have vgacon configured/enabled
  - Make sure the power well gets disabled, then enabled. You can
    check this by seeing the messages print by hsw_set_power_well
  - Stop your display manager
  - echo 0 > /sys/class/vtconsole/vtcon1/bind

I can easily reproduce this by blacklising snd_hda_intel and booting
with eDP+HDMI.

If you do this and then look at dmesg, you'll see we're printing
infinite "Unclaimed register" messages. This is happening because
we're stuck on an infinite loop inside console_unlock(), which is
calling many functions from vgacon.c. And the code that's triggering
the error messages is from vgacon_set_cursor_size().

After we re-enable the power well, every time we read/write the VGA
address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
print error messages. If we write anything to the VGA MSR register (it
doesn't really matter which value you write to bit 0), any
reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
anymore (even if MSR bit 0 is zero). So what happens with the current
code is that when we unbind i915 and bind vgacon, we call
console_unlock(). Function console_unlock() is responsible for
printing any messages that were supposed to be print when the console
was locked, so it calls the TTY layer, which calls the console layer,
which calls vgacon to print the messages. At this point, vgacon
eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
triggers unclaimed register interrupts. The problem is that when we
get these interrupts, we print the error messages, so we add more work
to console_unlock(), which will try to print it again, and then call
vgacon again, trigger a new interrupt, which will put more stuff to
the buffer, and then we'll be stuck at console_unlock() forever.

If you patch intel_uncore.c to not print anything when we detect
unclaimed registers, we won't get into the console_unlock() infinite
loop and the driver unbind will work just fine. We will still be
getting interrupts every time vgacon touches those registers, but we
will survive. This is a valid experiment, but IMHO it's not the real
fix: if we don't print any error messages we will still keep getting
the interrupts, and if we disable ERR_INT we won't get the interrupt
anymore, but we will also stop getting all the other error interrupts.

I talked about this problem with the HW engineer and his
recommendation is "So don't do any VGA I/O or memory access while the
power well is disabled, and make to re-program MSR after enabling the
power well and before using VGA I/O or memory accesses.".

Notice that this is just a partial fix to fd.o #67813. This fixes the
case where the power well is already enabled when we unbind, not when
it's disabled when we unbind.

V2: - Rebase (first version was sent in September).
V3: - Complete rewrite of the same fix: smaller implementation,
      improved commit message.

Testcase: igt/drv_module_reload
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ce2a188..6695c1d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -30,6 +30,7 @@
 #include "intel_drv.h"
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
+#include <linux/vgaarb.h>
 #include <drm/i915_powerwell.h>
 #include <linux/pm_runtime.h>
 
@@ -5687,6 +5688,20 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	unsigned long irqflags;
 
+	/*
+	 * After we re-enable the power well, if we touch VGA register 0x3d5
+	 * we'll get unclaimed register interrupts. This stops after we write
+	 * anything to the VGA MSR register. The vgacon module uses this
+	 * register all the time, so if we unbind our driver and, as a
+	 * consequence, bind vgacon, we'll get stuck in an infinite loop at
+	 * console_unlock(). So make here we touch the VGA MSR register, making
+	 * sure vgacon can keep working normally without triggering interrupts
+	 * and error messages.
+	 */
+	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
+	outb(inb(VGA_MSR_READ), VGA_MSR_WRITE);
+	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+
 	if (IS_BROADWELL(dev)) {
 		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 		I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
-- 
1.8.3.1

Re: [PATCH 1/3] drm/i915: remove i915_disable_vga_mem declaration

[Damien Lespiau]

On Wed, Dec 11, 2013 at 06:50:08PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> It was supposed to have been killed on the same commit that killed the
> function, e1264ebe9ff48e1b3e1dd11805eec9f5b143ab7c, but I guess the
> intel_drv.h reorganization accidentally brought it back.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_drv.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3f074c8..e4f01ca 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -703,7 +703,6 @@ void
>  ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
>  				int dotclock);
>  bool intel_crtc_active(struct drm_crtc *crtc);
> -void i915_disable_vga_mem(struct drm_device *dev);
>  void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
>  void intel_display_set_init_power(struct drm_device *dev, bool enable);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: extract hsw_power_well_post_{enable, disable}

[Damien Lespiau]

On Wed, Dec 11, 2013 at 06:50:09PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I want to add more code to the post_enable function.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 3/3] drm/i915: touch VGA MSR after we enable the power well

[Damien Lespiau]

On Wed, Dec 11, 2013 at 06:50:10PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Fixes regression introduced by:
>     commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
>     Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
>     Date:   Wed Jul 3 17:12:13 2013 -0300
>         drm/i915: switch disable_power_well default value to 1
> 
> The bug I'm seeing can be reproduced with:
>   - Have vgacon configured/enabled
>   - Make sure the power well gets disabled, then enabled. You can
>     check this by seeing the messages print by hsw_set_power_well
>   - Stop your display manager
>   - echo 0 > /sys/class/vtconsole/vtcon1/bind
> 
> I can easily reproduce this by blacklising snd_hda_intel and booting
> with eDP+HDMI.
> 
> If you do this and then look at dmesg, you'll see we're printing
> infinite "Unclaimed register" messages. This is happening because
> we're stuck on an infinite loop inside console_unlock(), which is
> calling many functions from vgacon.c. And the code that's triggering
> the error messages is from vgacon_set_cursor_size().
> 
> After we re-enable the power well, every time we read/write the VGA
> address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
> print error messages. If we write anything to the VGA MSR register (it
> doesn't really matter which value you write to bit 0), any
> reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
> anymore (even if MSR bit 0 is zero). So what happens with the current
> code is that when we unbind i915 and bind vgacon, we call
> console_unlock(). Function console_unlock() is responsible for
> printing any messages that were supposed to be print when the console
> was locked, so it calls the TTY layer, which calls the console layer,
> which calls vgacon to print the messages. At this point, vgacon
> eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
> triggers unclaimed register interrupts. The problem is that when we
> get these interrupts, we print the error messages, so we add more work
> to console_unlock(), which will try to print it again, and then call
> vgacon again, trigger a new interrupt, which will put more stuff to
> the buffer, and then we'll be stuck at console_unlock() forever.
> 
> If you patch intel_uncore.c to not print anything when we detect
> unclaimed registers, we won't get into the console_unlock() infinite
> loop and the driver unbind will work just fine. We will still be
> getting interrupts every time vgacon touches those registers, but we
> will survive. This is a valid experiment, but IMHO it's not the real
> fix: if we don't print any error messages we will still keep getting
> the interrupts, and if we disable ERR_INT we won't get the interrupt
> anymore, but we will also stop getting all the other error interrupts.
> 
> I talked about this problem with the HW engineer and his
> recommendation is "So don't do any VGA I/O or memory access while the
> power well is disabled, and make to re-program MSR after enabling the
> power well and before using VGA I/O or memory accesses.".
> 
> Notice that this is just a partial fix to fd.o #67813. This fixes the
> case where the power well is already enabled when we unbind, not when
> it's disabled when we unbind.
> 
> V2: - Rebase (first version was sent in September).
> V3: - Complete rewrite of the same fix: smaller implementation,
>       improved commit message.
> 
> Testcase: igt/drv_module_reload
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

The commit message is convincing!

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ce2a188..6695c1d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -30,6 +30,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
> +#include <linux/vgaarb.h>
>  #include <drm/i915_powerwell.h>
>  #include <linux/pm_runtime.h>
>  
> @@ -5687,6 +5688,20 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	unsigned long irqflags;
>  
> +	/*
> +	 * After we re-enable the power well, if we touch VGA register 0x3d5
> +	 * we'll get unclaimed register interrupts. This stops after we write
> +	 * anything to the VGA MSR register. The vgacon module uses this
> +	 * register all the time, so if we unbind our driver and, as a
> +	 * consequence, bind vgacon, we'll get stuck in an infinite loop at
> +	 * console_unlock(). So make here we touch the VGA MSR register, making
> +	 * sure vgacon can keep working normally without triggering interrupts
> +	 * and error messages.
> +	 */
> +	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> +	outb(inb(VGA_MSR_READ), VGA_MSR_WRITE);
> +	vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> +
>  	if (IS_BROADWELL(dev)) {
>  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  		I915_WRITE(GEN8_DE_PIPE_IMR(PIPE_B),
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: touch VGA MSR after we enable the power well

[Daniel Vetter]

On Thu, Dec 12, 2013 at 11:36:09AM +0000, Damien Lespiau wrote:
> On Wed, Dec 11, 2013 at 06:50:10PM -0200, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Fixes regression introduced by:
> >     commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
> >     Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >     Date:   Wed Jul 3 17:12:13 2013 -0300
> >         drm/i915: switch disable_power_well default value to 1
> > 
> > The bug I'm seeing can be reproduced with:
> >   - Have vgacon configured/enabled
> >   - Make sure the power well gets disabled, then enabled. You can
> >     check this by seeing the messages print by hsw_set_power_well
> >   - Stop your display manager
> >   - echo 0 > /sys/class/vtconsole/vtcon1/bind
> > 
> > I can easily reproduce this by blacklising snd_hda_intel and booting
> > with eDP+HDMI.
> > 
> > If you do this and then look at dmesg, you'll see we're printing
> > infinite "Unclaimed register" messages. This is happening because
> > we're stuck on an infinite loop inside console_unlock(), which is
> > calling many functions from vgacon.c. And the code that's triggering
> > the error messages is from vgacon_set_cursor_size().
> > 
> > After we re-enable the power well, every time we read/write the VGA
> > address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
> > print error messages. If we write anything to the VGA MSR register (it
> > doesn't really matter which value you write to bit 0), any
> > reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
> > anymore (even if MSR bit 0 is zero). So what happens with the current
> > code is that when we unbind i915 and bind vgacon, we call
> > console_unlock(). Function console_unlock() is responsible for
> > printing any messages that were supposed to be print when the console
> > was locked, so it calls the TTY layer, which calls the console layer,
> > which calls vgacon to print the messages. At this point, vgacon
> > eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
> > triggers unclaimed register interrupts. The problem is that when we
> > get these interrupts, we print the error messages, so we add more work
> > to console_unlock(), which will try to print it again, and then call
> > vgacon again, trigger a new interrupt, which will put more stuff to
> > the buffer, and then we'll be stuck at console_unlock() forever.
> > 
> > If you patch intel_uncore.c to not print anything when we detect
> > unclaimed registers, we won't get into the console_unlock() infinite
> > loop and the driver unbind will work just fine. We will still be
> > getting interrupts every time vgacon touches those registers, but we
> > will survive. This is a valid experiment, but IMHO it's not the real
> > fix: if we don't print any error messages we will still keep getting
> > the interrupts, and if we disable ERR_INT we won't get the interrupt
> > anymore, but we will also stop getting all the other error interrupts.
> > 
> > I talked about this problem with the HW engineer and his
> > recommendation is "So don't do any VGA I/O or memory access while the
> > power well is disabled, and make to re-program MSR after enabling the
> > power well and before using VGA I/O or memory accesses.".
> > 
> > Notice that this is just a partial fix to fd.o #67813. This fixes the
> > case where the power well is already enabled when we unbind, not when
> > it's disabled when we unbind.
> > 
> > V2: - Rebase (first version was sent in September).
> > V3: - Complete rewrite of the same fix: smaller implementation,
> >       improved commit message.
> > 
> > Testcase: igt/drv_module_reload
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The commit message is convincing!
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

All three merged, thanks for review&patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch