[PATCH 0/4] Re-enable RC6 on Ironlake

[PATCH 0/4] Re-enable RC6 on Ironlake

[Ben Widawsky]

There might be some further patches. I believe we may need to reinitialize rc6
after a GPU reset, but I don't know for certain yet.

Also it was recommended by Chris that we use i915_enable_rc6 for coarser
control over gen6 as well. That function was a little daunting, and so I'll
submit that as a separate patch when I have more time.

Ben

[PATCH 1/4] drm/i915: fix ilk rc6 teardown locking

[Ben Widawsky]

In the failure cases during rc6 initialization, both the power context
and render context may get !refcount without holding struct_mutex.

However, on rc6 disabling, the lock is held by the caller.

Added a simple parameter to control whether or not to acquire the lock.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_display.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..790af25 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6541,10 +6541,13 @@ void intel_enable_clock_gating(struct drm_device *dev)
 	}
 }
 
-static void ironlake_teardown_rc6(struct drm_device *dev)
+static void ironlake_teardown_rc6(struct drm_device *dev, bool need_lock)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (need_lock)
+		mutex_lock(&dev->struct_mutex);
+
 	if (dev_priv->renderctx) {
 		i915_gem_object_unpin(dev_priv->renderctx);
 		drm_gem_object_unreference(&dev_priv->renderctx->base);
@@ -6556,6 +6559,9 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
 		drm_gem_object_unreference(&dev_priv->pwrctx->base);
 		dev_priv->pwrctx = NULL;
 	}
+
+	if (need_lock)
+		mutex_unlock(&dev->struct_mutex);
 }
 
 static void ironlake_disable_rc6(struct drm_device *dev)
@@ -6575,7 +6581,7 @@ static void ironlake_disable_rc6(struct drm_device *dev)
 		POSTING_READ(RSTDBYCTL);
 	}
 
-	ironlake_teardown_rc6(dev);
+	ironlake_teardown_rc6(dev, false);
 }
 
 static int ironlake_setup_rc6(struct drm_device *dev)
@@ -6590,7 +6596,7 @@ static int ironlake_setup_rc6(struct drm_device *dev)
 	if (dev_priv->pwrctx == NULL)
 		dev_priv->pwrctx = intel_alloc_context_page(dev);
 	if (!dev_priv->pwrctx) {
-		ironlake_teardown_rc6(dev);
+		ironlake_teardown_rc6(dev, true);
 		return -ENOMEM;
 	}
 
@@ -6618,7 +6624,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	 */
 	ret = BEGIN_LP_RING(6);
 	if (ret) {
-		ironlake_teardown_rc6(dev);
+		ironlake_teardown_rc6(dev, true);
 		return;
 	}
 
-- 
1.7.3.4

[PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake

[Ben Widawsky]

There is a race condition between setting PWRCTXA and executing
MI_SET_CONTEXT. PWRCTXA must not be set until a valid context has been
written (or else the GPU could possible go into rc6, and return to an
invalid context).

Reported-and-Tested-by: Gu Rui <chaos.proton@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=28582
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_display.c |   32 +++++++++++++++++++++++++++++---
 1 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 790af25..c1e9c30 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6603,14 +6603,27 @@ static int ironlake_setup_rc6(struct drm_device *dev)
 	return 0;
 }
 
+static int ironlake_wait_set_context(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring = LP_RING(dev_priv);
+	int ret = -EAGAIN;
+
+	ret = wait_for((I915_READ_HEAD(ring) == I915_READ_TAIL(ring)), 1000);
+	if (ret && atomic_read(&dev_priv->mm.wedged)) {
+		ret = -EIO;
+	} else if (!ret) { /* head == tail */
+		ret = 0;
+	}
+
+	return ret;
+}
+
 void ironlake_enable_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	/* rc6 disabled by default due to repeated reports of hanging during
-	 * boot and resume.
-	 */
 	if (!i915_enable_rc6)
 		return;
 
@@ -6640,6 +6653,19 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	OUT_RING(MI_FLUSH);
 	ADVANCE_LP_RING();
 
+	/*
+	 * Wait for the command parser to advance past MI_SET_CONTEXT. The HW
+	 * does an implicit flush, combined with MI_FLUSH above, it should be
+	 * safe to assume that renderctx is valid
+	 */
+	ret = ironlake_wait_set_context(dev);
+	if (ret) {
+		if (ret == -EAGAIN)
+			DRM_ERROR("rc6 switch took too long, freeing the bo");
+		ironlake_teardown_rc6(dev, true);
+		return;
+	}
+
 	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
 }
-- 
1.7.3.4

[PATCH 3/4] drm/i915: debugfs for context information

[Ben Widawsky]

Currently this is only useful for the rc6 stuff. But this would also be
useful when I finally get around to the logical context + ppgtt stuff.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4ff9b6c..1b563a0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1156,6 +1156,30 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_context_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->mode_config.mutex);
+	if (ret)
+		return ret;
+
+	seq_printf(m, "power context ");
+	describe_obj(m, dev_priv->pwrctx);
+	seq_printf(m, "\n");
+
+	seq_printf(m, "render context ");
+	describe_obj(m, dev_priv->renderctx);
+	seq_printf(m, "\n");
+
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return 0;
+}
+
 static int
 i915_wedged_open(struct inode *inode,
 		 struct file *filp)
@@ -1294,6 +1318,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
+	{"i915_context_status", i915_context_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.7.3.4

[PATCH 4/4] drm/i915: re-enable rc6 for ironlake

[Ben Widawsky]

The previous patches should fix enough of the known issues to try
re-enabling rc6 for general consumption

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 22ec066..e3c808d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -49,7 +49,7 @@ module_param_named(powersave, i915_powersave, int, 0600);
 unsigned int i915_semaphores = 0;
 module_param_named(semaphores, i915_semaphores, int, 0600);
 
-unsigned int i915_enable_rc6 = 0;
+unsigned int i915_enable_rc6 = 1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
 unsigned int i915_lvds_downclock = 0;
-- 
1.7.3.4

Re: [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking

[Chris Wilson]

On Fri, 18 Mar 2011 16:12:45 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> In the failure cases during rc6 initialization, both the power context
> and render context may get !refcount without holding struct_mutex.
> 
> However, on rc6 disabling, the lock is held by the caller.
> 
> Added a simple parameter to control whether or not to acquire the lock.

I have an aversion to passing parameters around to control locking.
Considering that we modify dev_private and adjust LP_RING during enable,
wouldn't it be prudent to move that under the struct mutex? Especially in
the future if we allow it be enabled at runtime.

Failing that: bool was_locked = mutex_is_locked(&dev->struct_mutex);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake

[Chris Wilson]

On Fri, 18 Mar 2011 16:12:46 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> There is a race condition between setting PWRCTXA and executing
> MI_SET_CONTEXT. PWRCTXA must not be set until a valid context has been
> written (or else the GPU could possible go into rc6, and return to an
> invalid context).
> 
> Reported-and-Tested-by: Gu Rui <chaos.proton@gmail.com>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=28582
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   32 +++++++++++++++++++++++++++++---
>  1 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 790af25..c1e9c30 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6603,14 +6603,27 @@ static int ironlake_setup_rc6(struct drm_device *dev)
>  	return 0;
>  }
>  
> +static int ironlake_wait_set_context(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring = LP_RING(dev_priv);
> +	int ret = -EAGAIN;
> +
> +	ret = wait_for((I915_READ_HEAD(ring) == I915_READ_TAIL(ring)), 1000);
> +	if (ret && atomic_read(&dev_priv->mm.wedged)) {
> +		ret = -EIO;
> +	} else if (!ret) { /* head == tail */
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}

Let's make this a more generic function because it does look useful.

static inline int intel_ring_wait_idle(struct intel_ring_buffer *ring)
{
	return intel_wait_ring_buffer(ring, ring->space - 8);
}

> +
>  void ironlake_enable_rc6(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> -	/* rc6 disabled by default due to repeated reports of hanging during
> -	 * boot and resume.
> -	 */
>  	if (!i915_enable_rc6)
>  		return;
>  
> @@ -6640,6 +6653,19 @@ void ironlake_enable_rc6(struct drm_device *dev)
>  	OUT_RING(MI_FLUSH);
>  	ADVANCE_LP_RING();
>  
> +	/*
> +	 * Wait for the command parser to advance past MI_SET_CONTEXT. The HW
> +	 * does an implicit flush, combined with MI_FLUSH above, it should be
> +	 * safe to assume that renderctx is valid
> +	 */
> +	ret = ironlake_wait_set_context(dev);
> +	if (ret) {
> +		if (ret == -EAGAIN)
> +			DRM_ERROR("rc6 switch took too long, freeing the bo");

Just report the failure in all cases and avoid jargon (if possible)!

if (intel_ring_wait_idle(LP_RING(dev_priv))) {
	DRM_ERROR("failed to enable automatic power saving\n");
	return;
}

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 1/4] drm/i915: fix ilk rc6 teardown locking

[Ben Widawsky]

In the failure cases during rc6 initialization, both the power context
and render context may get !refcount without holding struct_mutex.
However, on rc6 disabling, the lock is held by the caller.

Rearranged the locking so that it's safe in both cases.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_display.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..2e3c626 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6025,13 +6025,14 @@ intel_alloc_context_page(struct drm_device *dev)
 	struct drm_i915_gem_object *ctx;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	ctx = i915_gem_alloc_object(dev, 4096);
 	if (!ctx) {
 		DRM_DEBUG("failed to alloc power context, RC6 disabled\n");
 		return NULL;
 	}
 
-	mutex_lock(&dev->struct_mutex);
 	ret = i915_gem_object_pin(ctx, 4096, true);
 	if (ret) {
 		DRM_ERROR("failed to pin power context: %d\n", ret);
@@ -6043,7 +6044,6 @@ intel_alloc_context_page(struct drm_device *dev)
 		DRM_ERROR("failed to set-domain on power context: %d\n", ret);
 		goto err_unpin;
 	}
-	mutex_unlock(&dev->struct_mutex);
 
 	return ctx;
 
@@ -6608,9 +6608,12 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	if (!i915_enable_rc6)
 		return;
 
+	mutex_lock(&dev->struct_mutex);
 	ret = ironlake_setup_rc6(dev);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&dev->struct_mutex);
 		return;
+	}
 
 	/*
 	 * GPU can automatically power down the render unit if given a page
@@ -6619,6 +6622,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	ret = BEGIN_LP_RING(6);
 	if (ret) {
 		ironlake_teardown_rc6(dev);
+		mutex_unlock(&dev->struct_mutex);
 		return;
 	}
 
@@ -6636,6 +6640,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
 
 	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 
-- 
1.7.3.4

Re: [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking

[Keith Packard]

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

On Sat, 19 Mar 2011 07:44:24 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I have an aversion to passing parameters around to control locking.
> Considering that we modify dev_private and adjust LP_RING during enable,
> wouldn't it be prudent to move that under the struct mutex? Especially in
> the future if we allow it be enabled at runtime.

This seems like the right plan to me as well -- make access to the
renderctx always covered by the struct mutex.

> Failing that: bool was_locked = mutex_is_locked(&dev->struct_mutex);

Ick. Better to just stick the needed locks around the two calls to
ironlake_teardown_rc6. You're just masking the issue otherwise.

-- 
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: [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake

[Ben Widawsky]

On Sat, Mar 19, 2011 at 07:55:01AM +0000, Chris Wilson wrote:

> Let's make this a more generic function because it does look useful.
> 
> static inline int intel_ring_wait_idle(struct intel_ring_buffer *ring)
> {
> 	return intel_wait_ring_buffer(ring, ring->space - 8);
> }

Sounds good. I'm currently working this into a new patch in the series
and putting it in intel_ringbuffer.c, as well as replacing two callers.

> 
> Just report the failure in all cases and avoid jargon (if possible)!
> 
> if (intel_ring_wait_idle(LP_RING(dev_priv))) {
> 	DRM_ERROR("failed to enable automatic power saving\n");
> 	return;
> }
Why not do the teardown? This is was I was saying on IRC: in the GPU
hang case we can safely free the bos. In the non-hang case, we can
unsafely free them. As for the jargon, that's fine.

> -- 
> Chris Wilson, Intel Open Source Technology Centre
Ben

[PATCH v2 1/5] drm/i915: fix ilk rc6 teardown locking

[Ben Widawsky]

In the failure cases during rc6 initialization, both the power context
and render context may get !refcount without holding struct_mutex.
However, on rc6 disabling, the lock is held by the caller.

Rearranged the locking so that it's safe in both cases.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_display.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..2e3c626 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6025,13 +6025,14 @@ intel_alloc_context_page(struct drm_device *dev)
 	struct drm_i915_gem_object *ctx;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	ctx = i915_gem_alloc_object(dev, 4096);
 	if (!ctx) {
 		DRM_DEBUG("failed to alloc power context, RC6 disabled\n");
 		return NULL;
 	}
 
-	mutex_lock(&dev->struct_mutex);
 	ret = i915_gem_object_pin(ctx, 4096, true);
 	if (ret) {
 		DRM_ERROR("failed to pin power context: %d\n", ret);
@@ -6043,7 +6044,6 @@ intel_alloc_context_page(struct drm_device *dev)
 		DRM_ERROR("failed to set-domain on power context: %d\n", ret);
 		goto err_unpin;
 	}
-	mutex_unlock(&dev->struct_mutex);
 
 	return ctx;
 
@@ -6608,9 +6608,12 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	if (!i915_enable_rc6)
 		return;
 
+	mutex_lock(&dev->struct_mutex);
 	ret = ironlake_setup_rc6(dev);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&dev->struct_mutex);
 		return;
+	}
 
 	/*
 	 * GPU can automatically power down the render unit if given a page
@@ -6619,6 +6622,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	ret = BEGIN_LP_RING(6);
 	if (ret) {
 		ironlake_teardown_rc6(dev);
+		mutex_unlock(&dev->struct_mutex);
 		return;
 	}
 
@@ -6636,6 +6640,7 @@ void ironlake_enable_rc6(struct drm_device *dev)
 
 	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 
-- 
1.7.3.4

[PATCH v2 2/5] drm/1915: ringbuffer wait for idle function

[Ben Widawsky]

Added a new function which waits for the ringbuffer space to be equal to
(total - 8). This is the empty condition of the ringbuffer, and
equivalent to head==tail.

Also modified two users of this functionality elsewhere in the code.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c         |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h |    6 ++++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e33d9be..cdca465 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -563,7 +563,7 @@ static int i915_quiescent(struct drm_device *dev)
 	struct intel_ring_buffer *ring = LP_RING(dev->dev_private);
 
 	i915_kernel_lost_context(dev);
-	return intel_wait_ring_buffer(ring, ring->size - 8);
+	return intel_wait_ring_idle(ring);
 }
 
 static int i915_flush_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 445f27e..0b83059 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -893,7 +893,7 @@ void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring)
 
 	/* Disable the ring buffer. The ring must be idle at this point */
 	dev_priv = ring->dev->dev_private;
-	ret = intel_wait_ring_buffer(ring, ring->size - 8);
+	ret = intel_wait_ring_idle(ring);
 	I915_WRITE_CTL(ring, 0);
 
 	drm_core_ioremapfree(&ring->map, ring->dev);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 3430686..30d0437 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -143,7 +143,13 @@ intel_read_status_page(struct intel_ring_buffer *ring,
 }
 
 void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring);
+
 int __must_check intel_wait_ring_buffer(struct intel_ring_buffer *ring, int n);
+static inline int intel_wait_ring_idle(struct intel_ring_buffer *ring)
+{
+	return intel_wait_ring_buffer(ring, ring->space - 8);
+}
+
 int __must_check intel_ring_begin(struct intel_ring_buffer *ring, int n);
 
 static inline void intel_ring_emit(struct intel_ring_buffer *ring,
-- 
1.7.3.4

[PATCH v2 3/5] drm/i915: fix rc6 initialization on Ironlake

[Ben Widawsky]

There is a race condition between setting PWRCTXA and executing
MI_SET_CONTEXT. PWRCTXA must not be set until a valid context has been
written (or else the GPU could possible go into rc6, and return to an
invalid context).

Reported-and-Tested-by: Gu Rui <chaos.proton@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=28582
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2e3c626..6c92980 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6602,9 +6602,6 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	/* rc6 disabled by default due to repeated reports of hanging during
-	 * boot and resume.
-	 */
 	if (!i915_enable_rc6)
 		return;
 
@@ -6638,6 +6635,19 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	OUT_RING(MI_FLUSH);
 	ADVANCE_LP_RING();
 
+	/*
+	 * Wait for the command parser to advance past MI_SET_CONTEXT. The HW
+	 * does an implicit flush, combined with MI_FLUSH above, it should be
+	 * safe to assume that renderctx is valid
+	 */
+	ret = intel_wait_ring_idle(LP_RING(dev_priv));
+	if (ret) {
+		DRM_ERROR("failed to enable ironlake power power savings\n");
+		ironlake_teardown_rc6(dev);
+		mutex_unlock(&dev->struct_mutex);
+		return;
+	}
+
 	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.7.3.4

[PATCH v2 4/5] drm/i915: debugfs for context information

[Ben Widawsky]

Currently this is only useful for the rc6 stuff. But this would also be
useful when I finally get around to the logical context + ppgtt stuff.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4ff9b6c..1b563a0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1156,6 +1156,30 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_context_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->mode_config.mutex);
+	if (ret)
+		return ret;
+
+	seq_printf(m, "power context ");
+	describe_obj(m, dev_priv->pwrctx);
+	seq_printf(m, "\n");
+
+	seq_printf(m, "render context ");
+	describe_obj(m, dev_priv->renderctx);
+	seq_printf(m, "\n");
+
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return 0;
+}
+
 static int
 i915_wedged_open(struct inode *inode,
 		 struct file *filp)
@@ -1294,6 +1318,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
+	{"i915_context_status", i915_context_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.7.3.4

[PATCH v2 5/5] drm/i915: re-enable rc6 for ironlake

[Ben Widawsky]

The previous patches should fix enough of the known issues to try
re-enabling rc6 for general consumption

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 22ec066..e3c808d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -49,7 +49,7 @@ module_param_named(powersave, i915_powersave, int, 0600);
 unsigned int i915_semaphores = 0;
 module_param_named(semaphores, i915_semaphores, int, 0600);
 
-unsigned int i915_enable_rc6 = 0;
+unsigned int i915_enable_rc6 = 1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
 unsigned int i915_lvds_downclock = 0;
-- 
1.7.3.4

Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake

[Chris Wilson]

On Fri, 18 Mar 2011 16:12:48 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> The previous patches should fix enough of the known issues to try
> re-enabling rc6 for general consumption

Yay, and they bring back the old bugs! :)

So upon enabling fbc on a 2560x1440 external panel, it goes blank again.
The same symptoms as prompted disabling fbc last year.  Can anyone confirm
this bad behaviour?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake

[Jesse Barnes]

On Wed, 27 Apr 2011 08:49:28 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 18 Mar 2011 16:12:48 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > The previous patches should fix enough of the known issues to try
> > re-enabling rc6 for general consumption
> 
> Yay, and they bring back the old bugs! :)
> 
> So upon enabling fbc on a 2560x1440 external panel, it goes blank again.
> The same symptoms as prompted disabling fbc last year.  Can anyone confirm
> this bad behaviour?

Re-enabling rc6 causes problems when you enable fbc?

We should probably just enable fbc on the pipe connected to the
internal panel (if any) and keep it disabled otherwise.

Jesse

Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake

[Chris Wilson]

On Wed, 27 Apr 2011 14:20:09 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed, 27 Apr 2011 08:49:28 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Fri, 18 Mar 2011 16:12:48 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > The previous patches should fix enough of the known issues to try
> > > re-enabling rc6 for general consumption
> > 
> > Yay, and they bring back the old bugs! :)
> > 
> > So upon enabling fbc on a 2560x1440 external panel, it goes blank again.
> > The same symptoms as prompted disabling fbc last year.  Can anyone confirm
> > this bad behaviour?
> 
> Re-enabling rc6 causes problems when you enable fbc?

Yes, I'm pretty sure that's the causal link. Disabling the LVDS makes the
external DP go haywire, with fbc enabled for the external, but worked fine
until we tried to enable rc6 again. If I remember my bugs correctly, pure
fbc failures resulted in a stripy display, whereas this is a solid
corruption. And I'm pretty certain this is the same bug that convinced me
to disable rc6 the last time.
 
> We should probably just enable fbc on the pipe connected to the
> internal panel (if any) and keep it disabled otherwise.

On Arrandale, fbc is potentially a bigger win than rc6 - but is also a
potential loss - right?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake

[Jesse Barnes]

On Wed, 27 Apr 2011 23:03:14 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > We should probably just enable fbc on the pipe connected to the
> > internal panel (if any) and keep it disabled otherwise.
> 
> On Arrandale, fbc is potentially a bigger win than rc6 - but is also a
> potential loss - right?

No, I think rc6 is still a much bigger win even on Arrandale (iirc >>1W
for rc6, ~0.5W for fbc best case).  And yeah, there is the potential for
increased power consumption when fbc is enabled if a framebuffer that
doesn't compress very well continues to get re-compressed over a long
period of time.

Jesse

Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake

[Ben Widawsky]

On Wed, Apr 27, 2011 at 03:49:40PM -0700, Jesse Barnes wrote:
> On Wed, 27 Apr 2011 23:03:14 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > We should probably just enable fbc on the pipe connected to the
> > > internal panel (if any) and keep it disabled otherwise.
> > 
> > On Arrandale, fbc is potentially a bigger win than rc6 - but is also a
> > potential loss - right?
> 
> No, I think rc6 is still a much bigger win even on Arrandale (iirc >>1W
> for rc6, ~0.5W for fbc best case).  And yeah, there is the potential for
> increased power consumption when fbc is enabled if a framebuffer that
> doesn't compress very well continues to get re-compressed over a long
> period of time.
> 
> Jesse

The last tests that I ran it seemed we needed to be really careful about
when to turn on FBC on for Arrandale as noted in the bug that Chris
assigned to me below :). I did some rc6 tests prior to the FBC tests and
can't find my notes, but I actually recall it being closer to 2W.

https://bugs.freedesktop.org/show_bug.cgi?id=31742

Ben