enabling forcewake from userspace

enabling forcewake from userspace

[Ben Widawsky]

Chris, do you already have something like this queued up?

I don't have a SNB with which to test this patch right now. I'd like to
know if anyone has any major issues with this, because I intend to use
this to enable userspace to properly read and write registers on SNB.

Also if you see any glaring issues before I get to testing it, might as
well point them out now :p

Ben

[PATCH] drm/i915: have a forcewake "refcount"

[Ben Widawsky]

On Gen6, forcewake must be used to ensure certain HW bits are powered to
process commands (such as register writes). Forcewake should be
persistent according to the spec, so it is unnecessary to set it more
than once.

Further this "refcount" allows a userspace interaction with this part of
the HW (for say, reading and writing registers).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   11 +++++++++++
 drivers/gpu/drm/i915/i915_dma.c     |    3 +++
 drivers/gpu/drm/i915/i915_drv.c     |   24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |    4 ++++
 4 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 87c8e29..91ff99c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1186,6 +1186,16 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_forcewake_count_info(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;
+	seq_printf(m, "forcewake_count: %d\n",
+		   dev_priv->gen6_gt_forcewake_count);
+	return 0;
+}
+
 static int
 i915_wedged_open(struct inode *inode,
 		 struct file *filp)
@@ -1324,6 +1334,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_forcewake_count", i915_forcewake_count_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..35e0eaf 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2026,6 +2026,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->irq_lock);
 	spin_lock_init(&dev_priv->error_lock);
 
+	if (IS_GEN6(dev))
+		spin_lock_init(&dev_priv->gen6_gt_forcewake_lock);
+
 	if (IS_MOBILE(dev) || !IS_GEN2(dev))
 		dev_priv->num_pipe = 2;
 	else
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c34a8dd..d8da39f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -266,6 +266,17 @@ void intel_detect_pch (struct drm_device *dev)
 void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
 	int count;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev_priv->gen6_gt_forcewake_lock, flags);
+	if (++dev_priv->gen6_gt_forcewake_count > 1) {
+		count = 0;
+		while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
+			udelay(10);
+		spin_unlock_irqrestore(&dev_priv->gen6_gt_forcewake_lock, flags);
+		return;
+	}
+	WARN_ON(dev_priv->gen6_gt_forcewake_count != 1);
 
 	count = 0;
 	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
@@ -277,12 +288,25 @@ void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 	count = 0;
 	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0)
 		udelay(10);
+
+	spin_unlock_irqrestore(&dev_priv->gen6_gt_forcewake_lock, flags);
 }
 
 void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev_priv->gen6_gt_forcewake_lock, flags);
+	if (--dev_priv->gen6_gt_forcewake_count > 0) {
+		spin_unlock_irqrestore(&dev_priv->gen6_gt_forcewake_lock, flags);
+		return;
+	}
+	WARN_ON(dev_priv->gen6_gt_forcewake_count != 0);
+
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
 	POSTING_READ(FORCEWAKE);
+
+	spin_unlock_irqrestore(&dev_priv->gen6_gt_forcewake_lock, flags);
 }
 
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..f37bacd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -703,6 +703,10 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+
+	/* gen6 forcewake state */
+	spinlock_t gen6_gt_forcewake_lock;
+	int gen6_gt_forcewake_count;
 } drm_i915_private_t;
 
 struct drm_i915_gem_object {
-- 
1.7.3.4

Re: enabling forcewake from userspace

[Ben Widawsky]

On Fri, Mar 25, 2011 at 07:07:48PM -0700, Ben Widawsky wrote:
> 
> Chris, do you already have something like this queued up?
> 
> I don't have a SNB with which to test this patch right now. I'd like to
> know if anyone has any major issues with this, because I intend to use
> this to enable userspace to properly read and write registers on SNB.

So right after hitting 'y' I realized how much I detest this. It leaves
buggy usespace the opportunity to allow the GPU to never sleep. However,
I'd really like to find a solution to allow intel_reg_read and
intel_reg_write to work properly on GEN6.

Chris' idea from IRC of using LOAD/STORE_REGISTER will not work unless
we use a secure batch buffer.

The only other option floating around is IOCTLs to read/write the
registers, which previously seemed like a bad idea, but is now looking
like the only option.

Ben

Re: enabling forcewake from userspace

[Chris Wilson]

On Fri, 25 Mar 2011 19:23:58 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> The only other option floating around is IOCTLs to read/write the
> registers, which previously seemed like a bad idea, but is now looking
> like the only option.

The lazy option is to cross your fingers and do the forced-wake from
userspace debug tools. I'm not exactly wild about the idea, but it does
cut down on the number of blessed interfaces for touching hardware.

The other option is to continue to extend the debugfs to print out groups
of registers of interest. Maybe this would be a good addition to the
i915_*_ringbuffer_info?

An ioctl does seem to be a better longterm solution. But maybe we can
ignore the problem for a few more years? ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: enabling forcewake from userspace

[Ben Widawsky]

On Sat, Mar 26, 2011 at 07:11:55AM +0000, Chris Wilson wrote:
> On Fri, 25 Mar 2011 19:23:58 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > The only other option floating around is IOCTLs to read/write the
> > registers, which previously seemed like a bad idea, but is now looking
> > like the only option.
> 
> The lazy option is to cross your fingers and do the forced-wake from
> userspace debug tools. I'm not exactly wild about the idea, but it does
> cut down on the number of blessed interfaces for touching hardware.
> 
> The other option is to continue to extend the debugfs to print out groups
> of registers of interest. Maybe this would be a good addition to the
> i915_*_ringbuffer_info?
This still leaves the reg write problem unsolved. But I think this is
better.

> 
> An ioctl does seem to be a better longterm solution. But maybe we can
> ignore the problem for a few more years? ;-)
I think I won't bother trying to upstream my solution. But I'm also
thinking we should make intel_reg_read/write print a warning/error if
it's running on GEN6? What do you think?


By the way, I do think the patch is beneficial just for kernel usage.
Right now it doesn't matter, but as more users of the interface pop up,
particularly in any time sensitive code... wouldn't it be awful if the
GPU could powerdown while we're servicing an interrupt?

(The patch probably doesn't need the poll when forceawake_ack is already
set, that was a copy/paste mistake.)

> -Chris

Ben

Re: enabling forcewake from userspace

[Chris Wilson]

On Sat, 26 Mar 2011 08:46:25 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> I think I won't bother trying to upstream my solution. But I'm also
> thinking we should make intel_reg_read/write print a warning/error if
> it's running on GEN6? What do you think?

Adding a code comment or an
if (IS_GEN6 && reg < 0x40000 && FORCE_WAKE == 0) printf("Muppet!\n");
is about as far as I would go. Certainly don't remind us on every
invocation.

> By the way, I do think the patch is beneficial just for kernel usage.
> Right now it doesn't matter, but as more users of the interface pop up,
> particularly in any time sensitive code... wouldn't it be awful if the
> GPU could powerdown while we're servicing an interrupt?

The GPU is definitely allowed to be powered down whilst the CPU is
servicing an interrupt. ;-) I take your point though that the current
method is not interrupt safe. However, we should not be doing such work
from an interrupt handler - if need be all the actual work is kicked off
from a workqueue.

Something we need to keep an eye on, but not a problem today. *touch wood*
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: enabling forcewake from userspace

[Ben Widawsky]

On Sat, Mar 26, 2011 at 05:18:57PM +0000, Chris Wilson wrote:
> 
> The GPU is definitely allowed to be powered down whilst the CPU is
> servicing an interrupt. ;-) I take your point though that the current
> method is not interrupt safe. However, we should not be doing such work
> from an interrupt handler - if need be all the actual work is kicked off
> from a workqueue.
> 
> Something we need to keep an eye on, but not a problem today. *touch wood*

Efficiency aside, workqueue suffers from the same hazard. Unless the
assumption is struct_mutex (or something similar) is protecting all
register access which are in the power wells that can get disabled. My
quick inspection of the code shows this is probably true, but in this
case I'd suggest a WARN_ON or something in force_wake_get(), because
this would be a really hard bug to track down.

By the way, I think we currently have potential problem with
kick_ring().  Although given when this occurs it is probably a DON'T
CARE.

> -Chris
> 
Ben

Re: enabling forcewake from userspace

[Chris Wilson]

On Sat, 26 Mar 2011 12:27:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Sat, Mar 26, 2011 at 05:18:57PM +0000, Chris Wilson wrote:
> > 
> > The GPU is definitely allowed to be powered down whilst the CPU is
> > servicing an interrupt. ;-) I take your point though that the current
> > method is not interrupt safe. However, we should not be doing such work
> > from an interrupt handler - if need be all the actual work is kicked off
> > from a workqueue.
> > 
> > Something we need to keep an eye on, but not a problem today. *touch wood*
> 
> Efficiency aside, workqueue suffers from the same hazard. Unless the
> assumption is struct_mutex (or something similar) is protecting all
> register access which are in the power wells that can get disabled.

Yes, currently we use struct mutex to serialise pretty much everything,
__gen6_gt_force_wake_* included.

> My
> quick inspection of the code shows this is probably true, but in this
> case I'd suggest a WARN_ON or something in force_wake_get(), because
> this would be a really hard bug to track down.

Similarly, a WARN_ON in i915_reg_write() if reg < 0x40000 && !force-wake.
 
> By the way, I think we currently have potential problem with
> kick_ring().  Although given when this occurs it is probably a DON'T
> CARE.

True. A task for a truly rainy day. And whilst you're there move the error
capture to a workqueue as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre