i915 fixes

i915 fixes

[Chris Wilson]

At the moment, only the first patch ("drm/i915: Initialise g4x watermarks for
disabled pipe") looks ready. Keith, I think this reflects the consensus
we reached for that particular patch; I have the second stage of
unifying the identical ironlake/g4x routines pending for -next.

The second two patches do fix a critical issue we have with generation
of PGTBL_ER upon initialisation and Daniel reported it improving the
stability of resume on his 855GM. However, the initialisation code is
quite hairy and these need lots of review and widespread testing.

Thanks,
-Chris

[PATCH 1/3] drm/i915: Initialise g4x watermarks for disabled pipes

[Chris Wilson]

We were using uninitialised watermarks values for disabled pipes which
were combined into a single WM register and so corrupting the values for
the enabled pipe and upsetting the display hardware.

Reported-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=32612
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4fc21e0..e522c70 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3771,8 +3771,11 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	int entries, tlb_miss;
 
 	crtc = intel_get_crtc_for_plane(dev, plane);
-	if (crtc->fb == NULL || !crtc->enabled)
+	if (crtc->fb == NULL || !crtc->enabled) {
+		*cursor_wm = cursor->guard_size;
+		*plane_wm = display->guard_size;
 		return false;
+	}
 
 	htotal = crtc->mode.htotal;
 	hdisplay = crtc->mode.hdisplay;
-- 
1.7.4.1

[PATCH 2/3] drm/i915: Move the irq wait queue initialisation into the ring init

[Chris Wilson]

Required so that we don't obliterate the queue if initialising the
rings after the global IRQ handler is installed.

[Jesse, you recently looked at refactoring the IRQ installation
routines, does moving the initialisation of ring buffer data structures away
from that routine make sense in your grand scheme?]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_irq.c         |    6 ------
 drivers/gpu/drm/i915/intel_ringbuffer.c |    1 +
 2 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 188b497..46ccfc8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1688,12 +1688,6 @@ int i915_driver_irq_postinstall(struct drm_device *dev)
 	u32 enable_mask = I915_INTERRUPT_ENABLE_FIX | I915_INTERRUPT_ENABLE_VAR;
 	u32 error_mask;
 
-	DRM_INIT_WAITQUEUE(&dev_priv->ring[RCS].irq_queue);
-	if (HAS_BSD(dev))
-		DRM_INIT_WAITQUEUE(&dev_priv->ring[VCS].irq_queue);
-	if (HAS_BLT(dev))
-		DRM_INIT_WAITQUEUE(&dev_priv->ring[BCS].irq_queue);
-
 	dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
 
 	if (HAS_PCH_SPLIT(dev))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e9e6f71..884556d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -800,6 +800,7 @@ int intel_init_ring_buffer(struct drm_device *dev,
 	INIT_LIST_HEAD(&ring->request_list);
 	INIT_LIST_HEAD(&ring->gpu_write_list);
 
+	init_waitqueue_head(&ring->irq_queue);
 	spin_lock_init(&ring->irq_lock);
 	ring->irq_mask = ~0;
 
-- 
1.7.4.1

[PATCH 3/3] drm/i915: Disable all outputs early, before KMS takeover

[Chris Wilson]

If the outputs are active and continuing to access the GATT when we
teardown the PTEs, then there is a potential for us to hang the GPU.
The hang tends to be a PGTBL_ER with either an invalid host access or
an invalid display plane fetch.

v2: Reorder IRQ initialisation to defer until after GEM is setup.

Reported-by: Pekka Enberg <penberg@kernel.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Daniel Vetter <daniel.vetter@ffwll.ch> (855GM)
Tested-by: Pekka Enberg <penberg@kernel.org>
           # note that this doesn't fix the underlying problem of the
             PGTBL_ER and pipe underruns being reported immediately upon
             init on his 965GM MacBook
---
 drivers/gpu/drm/i915/i915_dma.c      |   31 ++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   17 +++++++++++------
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..b28e023 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1176,11 +1176,11 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 	return can_switch;
 }
 
-static int i915_load_modeset_init(struct drm_device *dev)
+static int i915_load_gem_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long prealloc_size, gtt_size, mappable_size;
-	int ret = 0;
+	int ret;
 
 	prealloc_size = dev_priv->mm.gtt->stolen_size;
 	gtt_size = dev_priv->mm.gtt->gtt_total_entries << PAGE_SHIFT;
@@ -1204,7 +1204,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	ret = i915_gem_init_ringbuffer(dev);
 	mutex_unlock(&dev->struct_mutex);
 	if (ret)
-		goto out;
+		return ret;
 
 	/* Try to set up FBC with a reasonable compressed buffer size */
 	if (I915_HAS_FBC(dev) && i915_powersave) {
@@ -1222,6 +1222,13 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	/* Allow hardware batchbuffers unless told otherwise. */
 	dev_priv->allow_batchbuffer = 1;
+	return 0;
+}
+
+static int i915_load_modeset_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
 
 	ret = intel_parse_bios(dev);
 	if (ret)
@@ -1236,7 +1243,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 */
 	ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode);
 	if (ret && ret != -ENODEV)
-		goto cleanup_ringbuffer;
+		goto out;
 
 	intel_register_dsm_handler();
 
@@ -1253,10 +1260,16 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_modeset_init(dev);
 
-	ret = drm_irq_install(dev);
+	ret = i915_load_gem_init(dev);
 	if (ret)
 		goto cleanup_vga_switcheroo;
 
+	intel_modeset_gem_init(dev);
+
+	ret = drm_irq_install(dev);
+	if (ret)
+		goto cleanup_gem;
+
 	/* Always safe in the mode setting case. */
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
@@ -1274,14 +1287,14 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 cleanup_irq:
 	drm_irq_uninstall(dev);
+cleanup_gem:
+	mutex_lock(&dev->struct_mutex);
+	i915_gem_cleanup_ringbuffer(dev);
+	mutex_unlock(&dev->struct_mutex);
 cleanup_vga_switcheroo:
 	vga_switcheroo_unregister_client(dev->pdev);
 cleanup_vga_client:
 	vga_client_register(dev->pdev, NULL, NULL, NULL);
-cleanup_ringbuffer:
-	mutex_lock(&dev->struct_mutex);
-	i915_gem_cleanup_ringbuffer(dev);
-	mutex_unlock(&dev->struct_mutex);
 out:
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..8865ec1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1265,6 +1265,7 @@ static inline void intel_unregister_dsm_handler(void) { return; }
 
 /* modesetting */
 extern void intel_modeset_init(struct drm_device *dev);
+extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
 extern void i8xx_disable_fbc(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e522c70..2183c4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6504,6 +6504,9 @@ static void intel_setup_outputs(struct drm_device *dev)
 	}
 
 	intel_panel_setup_backlight(dev);
+
+	/* disable all the possible outputs/crtcs before entering KMS mode */
+	drm_helper_disable_unused_functions(dev);
 }
 
 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
@@ -7439,13 +7442,12 @@ void intel_modeset_init(struct drm_device *dev)
 		intel_crtc_init(dev, i);
 	}
 
+	/* Just disable it once at startup */
+	i915_disable_vga(dev);
 	intel_setup_outputs(dev);
 
 	intel_enable_clock_gating(dev);
 
-	/* Just disable it once at startup */
-	i915_disable_vga(dev);
-
 	if (IS_IRONLAKE_M(dev)) {
 		ironlake_enable_drps(dev);
 		intel_init_emon(dev);
@@ -7454,12 +7456,15 @@ void intel_modeset_init(struct drm_device *dev)
 	if (IS_GEN6(dev))
 		gen6_enable_rps(dev_priv);
 
-	if (IS_IRONLAKE_M(dev))
-		ironlake_enable_rc6(dev);
-
 	INIT_WORK(&dev_priv->idle_work, intel_idle_update);
 	setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
 		    (unsigned long)dev);
+}
+
+void intel_modeset_gem_init(struct drm_device *dev)
+{
+	if (IS_IRONLAKE_M(dev))
+		ironlake_enable_rc6(dev);
 
 	intel_setup_overlay(dev);
 }
-- 
1.7.4.1

Re: i915 fixes

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 522 bytes --]

On Wed, 13 Apr 2011 09:28:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> At the moment, only the first patch ("drm/i915: Initialise g4x watermarks for
> disabled pipe") looks ready. Keith, I think this reflects the consensus
> we reached for that particular patch; I have the second stage of
> unifying the identical ironlake/g4x routines pending for -next.

Yup. I've merged that to my drm-intel-fixes branch.

Do we have any other regression fixes pending for .39?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 fixes

[Chris Wilson]

On Wed, 13 Apr 2011 09:30:01 -0700, Keith Packard <keithp@keithp.com> wrote:
> Do we have any other regression fixes pending for .39?

That's every stability related patch I have for the time being, and
certainly all the known regressions. Bugzilla still hates me though.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm/i915: Move the irq wait queue initialisation into the ring init

[Ben Widawsky]

On Wed, Apr 13, 2011 at 09:28:24AM +0100, Chris Wilson wrote:
> Required so that we don't obliterate the queue if initialising the
> rings after the global IRQ handler is installed.
> 
> [Jesse, you recently looked at refactoring the IRQ installation
> routines, does moving the initialisation of ring buffer data structures away
> from that routine make sense in your grand scheme?]
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_irq.c         |    6 ------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    1 +
>  2 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 188b497..46ccfc8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1688,12 +1688,6 @@ int i915_driver_irq_postinstall(struct drm_device *dev)
>  	u32 enable_mask = I915_INTERRUPT_ENABLE_FIX | I915_INTERRUPT_ENABLE_VAR;
>  	u32 error_mask;
>  
> -	DRM_INIT_WAITQUEUE(&dev_priv->ring[RCS].irq_queue);
> -	if (HAS_BSD(dev))
> -		DRM_INIT_WAITQUEUE(&dev_priv->ring[VCS].irq_queue);
> -	if (HAS_BLT(dev))
> -		DRM_INIT_WAITQUEUE(&dev_priv->ring[BCS].irq_queue);
> -
>  	dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
>  
>  	if (HAS_PCH_SPLIT(dev))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e9e6f71..884556d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -800,6 +800,7 @@ int intel_init_ring_buffer(struct drm_device *dev,
>  	INIT_LIST_HEAD(&ring->request_list);
>  	INIT_LIST_HEAD(&ring->gpu_write_list);
>  
> +	init_waitqueue_head(&ring->irq_queue);
>  	spin_lock_init(&ring->irq_lock);
>  	ring->irq_mask = ~0;
>  

I don't really understand why we went from DRM_INIT_WAITQUEUE to
init_waitqueue_head, but I'm okay with it.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH 3/3] drm/i915: Disable all outputs early, before KMS takeover

[Ben Widawsky]

On Wed, Apr 13, 2011 at 09:28:25AM +0100, Chris Wilson wrote:
> If the outputs are active and continuing to access the GATT when we
> teardown the PTEs, then there is a potential for us to hang the GPU.
> The hang tends to be a PGTBL_ER with either an invalid host access or
> an invalid display plane fetch.
> 
> v2: Reorder IRQ initialisation to defer until after GEM is setup.
> 
> Reported-by: Pekka Enberg <penberg@kernel.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Daniel Vetter <daniel.vetter@ffwll.ch> (855GM)
> Tested-by: Pekka Enberg <penberg@kernel.org>
>            # note that this doesn't fix the underlying problem of the
>              PGTBL_ER and pipe underruns being reported immediately upon
>              init on his 965GM MacBook
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |   31 ++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   17 +++++++++++------
>  3 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..b28e023 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1176,11 +1176,11 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
>  	return can_switch;
>  }
>  
> -static int i915_load_modeset_init(struct drm_device *dev)
> +static int i915_load_gem_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long prealloc_size, gtt_size, mappable_size;
> -	int ret = 0;
> +	int ret;
>  
>  	prealloc_size = dev_priv->mm.gtt->stolen_size;
>  	gtt_size = dev_priv->mm.gtt->gtt_total_entries << PAGE_SHIFT;
> @@ -1204,7 +1204,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	ret = i915_gem_init_ringbuffer(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  	if (ret)
> -		goto out;
> +		return ret;
>  
>  	/* Try to set up FBC with a reasonable compressed buffer size */
>  	if (I915_HAS_FBC(dev) && i915_powersave) {
> @@ -1222,6 +1222,13 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	/* Allow hardware batchbuffers unless told otherwise. */
>  	dev_priv->allow_batchbuffer = 1;
> +	return 0;
> +}
> +
> +static int i915_load_modeset_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
>  
>  	ret = intel_parse_bios(dev);
>  	if (ret)
> @@ -1236,7 +1243,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	 */
>  	ret = vga_client_register(dev->pdev, dev, NULL, i915_vga_set_decode);
>  	if (ret && ret != -ENODEV)
> -		goto cleanup_ringbuffer;
> +		goto out;
>  
>  	intel_register_dsm_handler();
>  
> @@ -1253,10 +1260,16 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	intel_modeset_init(dev);
>  
> -	ret = drm_irq_install(dev);
> +	ret = i915_load_gem_init(dev);
>  	if (ret)
>  		goto cleanup_vga_switcheroo;
>  
> +	intel_modeset_gem_init(dev);
> +
> +	ret = drm_irq_install(dev);
> +	if (ret)
> +		goto cleanup_gem;
> +
>  	/* Always safe in the mode setting case. */
>  	/* FIXME: do pre/post-mode set stuff in core KMS code */
>  	dev->vblank_disable_allowed = 1;
> @@ -1274,14 +1287,14 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  cleanup_irq:
>  	drm_irq_uninstall(dev);
> +cleanup_gem:
> +	mutex_lock(&dev->struct_mutex);
> +	i915_gem_cleanup_ringbuffer(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  cleanup_vga_switcheroo:
>  	vga_switcheroo_unregister_client(dev->pdev);
>  cleanup_vga_client:
>  	vga_client_register(dev->pdev, NULL, NULL, NULL);
> -cleanup_ringbuffer:
> -	mutex_lock(&dev->struct_mutex);
> -	i915_gem_cleanup_ringbuffer(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  out:
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5004724..8865ec1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1265,6 +1265,7 @@ static inline void intel_unregister_dsm_handler(void) { return; }
>  
>  /* modesetting */
>  extern void intel_modeset_init(struct drm_device *dev);
> +extern void intel_modeset_gem_init(struct drm_device *dev);
>  extern void intel_modeset_cleanup(struct drm_device *dev);
>  extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
>  extern void i8xx_disable_fbc(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e522c70..2183c4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6504,6 +6504,9 @@ static void intel_setup_outputs(struct drm_device *dev)
>  	}
>  
>  	intel_panel_setup_backlight(dev);
> +
> +	/* disable all the possible outputs/crtcs before entering KMS mode */
> +	drm_helper_disable_unused_functions(dev);
>  }
>  
>  static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> @@ -7439,13 +7442,12 @@ void intel_modeset_init(struct drm_device *dev)
>  		intel_crtc_init(dev, i);
>  	}
>  
> +	/* Just disable it once at startup */
> +	i915_disable_vga(dev);
>  	intel_setup_outputs(dev);
>  
>  	intel_enable_clock_gating(dev);
>  
> -	/* Just disable it once at startup */
> -	i915_disable_vga(dev);
> -
>  	if (IS_IRONLAKE_M(dev)) {
>  		ironlake_enable_drps(dev);
>  		intel_init_emon(dev);
> @@ -7454,12 +7456,15 @@ void intel_modeset_init(struct drm_device *dev)
>  	if (IS_GEN6(dev))
>  		gen6_enable_rps(dev_priv);
>  
> -	if (IS_IRONLAKE_M(dev))
> -		ironlake_enable_rc6(dev);
> -
>  	INIT_WORK(&dev_priv->idle_work, intel_idle_update);
>  	setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
>  		    (unsigned long)dev);
> +}
> +
> +void intel_modeset_gem_init(struct drm_device *dev)
> +{
> +	if (IS_IRONLAKE_M(dev))
> +		ironlake_enable_rc6(dev);
>  
>  	intel_setup_overlay(dev);
>  }

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH 2/3] drm/i915: Move the irq wait queue initialisation into the ring init

[Chris Wilson]

On Wed, 13 Apr 2011 23:20:19 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> I don't really understand why we went from DRM_INIT_WAITQUEUE to
> init_waitqueue_head, but I'm okay with it.

Because I share upstream's opinion that we really should not have an OS
agnostic middlelayer (that is nothing more than SHOUTING at times like
this), and gradually wean ourselves off it whenever I get the chance.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre