* [PATCH 0/4] Re-enable RC6 on Ironlake
@ 2011-03-18 23:12 Ben Widawsky
2011-03-18 23:12 ` [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking Ben Widawsky
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Ben Widawsky @ 2011-03-18 23:12 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking 2011-03-18 23:12 [PATCH 0/4] Re-enable RC6 on Ironlake Ben Widawsky @ 2011-03-18 23:12 ` Ben Widawsky 2011-03-19 7:44 ` Chris Wilson 2011-03-18 23:12 ` [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake Ben Widawsky ` (2 subsequent siblings) 3 siblings, 1 reply; 20+ messages in thread From: Ben Widawsky @ 2011-03-18 23:12 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking 2011-03-18 23:12 ` [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking Ben Widawsky @ 2011-03-19 7:44 ` Chris Wilson 2011-03-19 18:38 ` [PATCH v2 " Ben Widawsky ` (6 more replies) 0 siblings, 7 replies; 20+ messages in thread From: Chris Wilson @ 2011-03-19 7:44 UTC (permalink / raw) To: Ben Widawsky, intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/4] drm/i915: fix ilk rc6 teardown locking 2011-03-19 7:44 ` Chris Wilson @ 2011-03-19 18:38 ` Ben Widawsky 2011-03-19 18:46 ` [PATCH " Keith Packard ` (5 subsequent siblings) 6 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-19 18:38 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking 2011-03-19 7:44 ` Chris Wilson 2011-03-19 18:38 ` [PATCH v2 " Ben Widawsky @ 2011-03-19 18:46 ` Keith Packard 2011-03-20 1:14 ` [PATCH v2 1/5] " Ben Widawsky ` (4 subsequent siblings) 6 siblings, 0 replies; 20+ messages in thread From: Keith Packard @ 2011-03-19 18:46 UTC (permalink / raw) To: Chris Wilson, Ben Widawsky, intel-gfx [-- 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/5] drm/i915: fix ilk rc6 teardown locking 2011-03-19 7:44 ` Chris Wilson 2011-03-19 18:38 ` [PATCH v2 " Ben Widawsky 2011-03-19 18:46 ` [PATCH " Keith Packard @ 2011-03-20 1:14 ` Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 2/5] drm/1915: ringbuffer wait for idle function Ben Widawsky ` (3 subsequent siblings) 6 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-20 1:14 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/5] drm/1915: ringbuffer wait for idle function 2011-03-19 7:44 ` Chris Wilson ` (2 preceding siblings ...) 2011-03-20 1:14 ` [PATCH v2 1/5] " Ben Widawsky @ 2011-03-20 1:14 ` Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 3/5] drm/i915: fix rc6 initialization on Ironlake Ben Widawsky ` (2 subsequent siblings) 6 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-20 1:14 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 3/5] drm/i915: fix rc6 initialization on Ironlake 2011-03-19 7:44 ` Chris Wilson ` (3 preceding siblings ...) 2011-03-20 1:14 ` [PATCH v2 2/5] drm/1915: ringbuffer wait for idle function Ben Widawsky @ 2011-03-20 1:14 ` Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 4/5] drm/i915: debugfs for context information Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 5/5] drm/i915: re-enable rc6 for ironlake Ben Widawsky 6 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-20 1:14 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 4/5] drm/i915: debugfs for context information 2011-03-19 7:44 ` Chris Wilson ` (4 preceding siblings ...) 2011-03-20 1:14 ` [PATCH v2 3/5] drm/i915: fix rc6 initialization on Ironlake Ben Widawsky @ 2011-03-20 1:14 ` Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 5/5] drm/i915: re-enable rc6 for ironlake Ben Widawsky 6 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-20 1:14 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 5/5] drm/i915: re-enable rc6 for ironlake 2011-03-19 7:44 ` Chris Wilson ` (5 preceding siblings ...) 2011-03-20 1:14 ` [PATCH v2 4/5] drm/i915: debugfs for context information Ben Widawsky @ 2011-03-20 1:14 ` Ben Widawsky 6 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-20 1:14 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake 2011-03-18 23:12 [PATCH 0/4] Re-enable RC6 on Ironlake Ben Widawsky 2011-03-18 23:12 ` [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking Ben Widawsky @ 2011-03-18 23:12 ` Ben Widawsky 2011-03-19 7:55 ` Chris Wilson 2011-03-18 23:12 ` [PATCH 3/4] drm/i915: debugfs for context information Ben Widawsky 2011-03-18 23:12 ` [PATCH 4/4] drm/i915: re-enable rc6 for ironlake Ben Widawsky 3 siblings, 1 reply; 20+ messages in thread From: Ben Widawsky @ 2011-03-18 23:12 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake 2011-03-18 23:12 ` [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake Ben Widawsky @ 2011-03-19 7:55 ` Chris Wilson 2011-03-20 0:07 ` Ben Widawsky 0 siblings, 1 reply; 20+ messages in thread From: Chris Wilson @ 2011-03-19 7:55 UTC (permalink / raw) To: Ben Widawsky, intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake 2011-03-19 7:55 ` Chris Wilson @ 2011-03-20 0:07 ` Ben Widawsky 0 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-20 0:07 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] drm/i915: debugfs for context information 2011-03-18 23:12 [PATCH 0/4] Re-enable RC6 on Ironlake Ben Widawsky 2011-03-18 23:12 ` [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking Ben Widawsky 2011-03-18 23:12 ` [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake Ben Widawsky @ 2011-03-18 23:12 ` Ben Widawsky 2011-03-18 23:12 ` [PATCH 4/4] drm/i915: re-enable rc6 for ironlake Ben Widawsky 3 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-03-18 23:12 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/4] drm/i915: re-enable rc6 for ironlake 2011-03-18 23:12 [PATCH 0/4] Re-enable RC6 on Ironlake Ben Widawsky ` (2 preceding siblings ...) 2011-03-18 23:12 ` [PATCH 3/4] drm/i915: debugfs for context information Ben Widawsky @ 2011-03-18 23:12 ` Ben Widawsky 2011-04-27 7:49 ` Chris Wilson 3 siblings, 1 reply; 20+ messages in thread From: Ben Widawsky @ 2011-03-18 23:12 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake 2011-03-18 23:12 ` [PATCH 4/4] drm/i915: re-enable rc6 for ironlake Ben Widawsky @ 2011-04-27 7:49 ` Chris Wilson 2011-04-27 21:20 ` Jesse Barnes 0 siblings, 1 reply; 20+ messages in thread From: Chris Wilson @ 2011-04-27 7:49 UTC (permalink / raw) To: Ben Widawsky, intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake 2011-04-27 7:49 ` Chris Wilson @ 2011-04-27 21:20 ` Jesse Barnes 2011-04-27 22:03 ` Chris Wilson 0 siblings, 1 reply; 20+ messages in thread From: Jesse Barnes @ 2011-04-27 21:20 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake 2011-04-27 21:20 ` Jesse Barnes @ 2011-04-27 22:03 ` Chris Wilson 2011-04-27 22:49 ` Jesse Barnes 0 siblings, 1 reply; 20+ messages in thread From: Chris Wilson @ 2011-04-27 22:03 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake 2011-04-27 22:03 ` Chris Wilson @ 2011-04-27 22:49 ` Jesse Barnes 2011-04-28 17:10 ` Ben Widawsky 0 siblings, 1 reply; 20+ messages in thread From: Jesse Barnes @ 2011-04-27 22:49 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/4] drm/i915: re-enable rc6 for ironlake 2011-04-27 22:49 ` Jesse Barnes @ 2011-04-28 17:10 ` Ben Widawsky 0 siblings, 0 replies; 20+ messages in thread From: Ben Widawsky @ 2011-04-28 17:10 UTC (permalink / raw) To: Jesse Barnes; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2011-04-28 17:10 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-03-18 23:12 [PATCH 0/4] Re-enable RC6 on Ironlake Ben Widawsky 2011-03-18 23:12 ` [PATCH 1/4] drm/i915: fix ilk rc6 teardown locking Ben Widawsky 2011-03-19 7:44 ` Chris Wilson 2011-03-19 18:38 ` [PATCH v2 " Ben Widawsky 2011-03-19 18:46 ` [PATCH " Keith Packard 2011-03-20 1:14 ` [PATCH v2 1/5] " Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 2/5] drm/1915: ringbuffer wait for idle function Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 3/5] drm/i915: fix rc6 initialization on Ironlake Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 4/5] drm/i915: debugfs for context information Ben Widawsky 2011-03-20 1:14 ` [PATCH v2 5/5] drm/i915: re-enable rc6 for ironlake Ben Widawsky 2011-03-18 23:12 ` [PATCH 2/4] drm/i915: fix rc6 initialization on Ironlake Ben Widawsky 2011-03-19 7:55 ` Chris Wilson 2011-03-20 0:07 ` Ben Widawsky 2011-03-18 23:12 ` [PATCH 3/4] drm/i915: debugfs for context information Ben Widawsky 2011-03-18 23:12 ` [PATCH 4/4] drm/i915: re-enable rc6 for ironlake Ben Widawsky 2011-04-27 7:49 ` Chris Wilson 2011-04-27 21:20 ` Jesse Barnes 2011-04-27 22:03 ` Chris Wilson 2011-04-27 22:49 ` Jesse Barnes 2011-04-28 17:10 ` Ben Widawsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).