[PATCH 1/5] drm: add optional per device rwsem for all ioctls

[PATCH 1/5] drm: add optional per device rwsem for all ioctls

[Marcin Slusarz]

Nouveau, in normal circumstances, does not need device lock for every ioctl,
but incoming "gpu reset" code needs exclusive access to the device.
This commit adds drm_driver flag which turns on read lock ioctl encapsulation.

Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
---
 drivers/gpu/drm/drm_drv.c  |    6 ++++++
 drivers/gpu/drm/drm_stub.c |    2 ++
 include/drm/drmP.h         |    4 ++++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 6116e3b..c54e9f8 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -464,6 +464,9 @@ long drm_ioctl(struct file *filp,
 		} else
 			memset(kdata, 0, usize);
 
+		if (dev->driver->ioctls_need_rwsem)
+			down_read(&dev->ioctls_rwsem);
+
 		if (ioctl->flags & DRM_UNLOCKED)
 			retcode = func(dev, kdata, file_priv);
 		else {
@@ -472,6 +475,9 @@ long drm_ioctl(struct file *filp,
 			mutex_unlock(&drm_global_mutex);
 		}
 
+		if (dev->driver->ioctls_need_rwsem)
+			up_read(&dev->ioctls_rwsem);
+
 		if (cmd & IOC_OUT) {
 			if (copy_to_user((void __user *)arg, kdata,
 					 usize) != 0)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index aa454f8..4af3227 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -275,6 +275,8 @@ int drm_fill_in_dev(struct drm_device *dev,
 	mutex_init(&dev->struct_mutex);
 	mutex_init(&dev->ctxlist_mutex);
 
+	init_rwsem(&dev->ioctls_rwsem);
+
 	if (drm_ht_create(&dev->map_hash, 12)) {
 		return -ENOMEM;
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index dd73104..527dca5 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -954,6 +954,8 @@ struct drm_driver {
 	int dev_priv_size;
 	struct drm_ioctl_desc *ioctls;
 	int num_ioctls;
+	bool ioctls_need_rwsem;
+
 	const struct file_operations *fops;
 	union {
 		struct pci_driver *pci;
@@ -1070,6 +1072,8 @@ struct drm_device {
 	/*@{ */
 	spinlock_t count_lock;		/**< For inuse, drm_device::open_count, drm_device::buf_use */
 	struct mutex struct_mutex;	/**< For others */
+
+	struct rw_semaphore ioctls_rwsem;
 	/*@} */
 
 	/** \name Usage Counters */
-- 
1.7.8.5

Re: [PATCH 1/5] drm: add optional per device rwsem for all ioctls

[Daniel Vetter]

On Mon, Apr 23, 2012 at 12:18:28AM +0200, Marcin Slusarz wrote:
> Nouveau, in normal circumstances, does not need device lock for every ioctl,
> but incoming "gpu reset" code needs exclusive access to the device.
> This commit adds drm_driver flag which turns on read lock ioctl encapsulation.
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Why can't we just move this down to nouveau driver code?

I really hate adding random stuff to drm core that other drivers can't opt
out of - all the dri1 nightmares are made of these essentially. And radeon
and i915 seem to be able to reset without adding anything to drm core.

So why can't nouveau do the same? Also, if this is indeed necessary and we
add this as some mandatory core infrastructure, it's not good enough: In
i915 we go to great lengths to ensure that all processes waiting for gpu
reset are still interruptible, in case the gpu reset dies unexpectedly
(which happens), so this would need to be improved.
-Daniel

> ---
>  drivers/gpu/drm/drm_drv.c  |    6 ++++++
>  drivers/gpu/drm/drm_stub.c |    2 ++
>  include/drm/drmP.h         |    4 ++++
>  3 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 6116e3b..c54e9f8 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -464,6 +464,9 @@ long drm_ioctl(struct file *filp,
>  		} else
>  			memset(kdata, 0, usize);
>  
> +		if (dev->driver->ioctls_need_rwsem)
> +			down_read(&dev->ioctls_rwsem);
> +
>  		if (ioctl->flags & DRM_UNLOCKED)
>  			retcode = func(dev, kdata, file_priv);
>  		else {
> @@ -472,6 +475,9 @@ long drm_ioctl(struct file *filp,
>  			mutex_unlock(&drm_global_mutex);
>  		}
>  
> +		if (dev->driver->ioctls_need_rwsem)
> +			up_read(&dev->ioctls_rwsem);
> +
>  		if (cmd & IOC_OUT) {
>  			if (copy_to_user((void __user *)arg, kdata,
>  					 usize) != 0)
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index aa454f8..4af3227 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -275,6 +275,8 @@ int drm_fill_in_dev(struct drm_device *dev,
>  	mutex_init(&dev->struct_mutex);
>  	mutex_init(&dev->ctxlist_mutex);
>  
> +	init_rwsem(&dev->ioctls_rwsem);
> +
>  	if (drm_ht_create(&dev->map_hash, 12)) {
>  		return -ENOMEM;
>  	}
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index dd73104..527dca5 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -954,6 +954,8 @@ struct drm_driver {
>  	int dev_priv_size;
>  	struct drm_ioctl_desc *ioctls;
>  	int num_ioctls;
> +	bool ioctls_need_rwsem;
> +
>  	const struct file_operations *fops;
>  	union {
>  		struct pci_driver *pci;
> @@ -1070,6 +1072,8 @@ struct drm_device {
>  	/*@{ */
>  	spinlock_t count_lock;		/**< For inuse, drm_device::open_count, drm_device::buf_use */
>  	struct mutex struct_mutex;	/**< For others */
> +
> +	struct rw_semaphore ioctls_rwsem;
>  	/*@} */
>  
>  	/** \name Usage Counters */
> -- 
> 1.7.8.5
> 

-- 
Daniel Vetter
Mail: daniel-/w4YWyX8dFk@public.gmane.org
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/5] drm: add optional per device rwsem for all ioctls

[Marcin Slusarz]

On Mon, Apr 23, 2012 at 09:51:48AM +0200, Daniel Vetter wrote:
> On Mon, Apr 23, 2012 at 12:18:28AM +0200, Marcin Slusarz wrote:
> > Nouveau, in normal circumstances, does not need device lock for every ioctl,
> > but incoming "gpu reset" code needs exclusive access to the device.
> > This commit adds drm_driver flag which turns on read lock ioctl encapsulation.
> > 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Why can't we just move this down to nouveau driver code?

I can't wrap drm core ioctls this way and adding locks at driver entry points
would be at best unmaintainable, at worst - impossible (some of them are
probably called in atomic context).

> I really hate adding random stuff to drm core that other drivers can't opt
> out of - all the dri1 nightmares are made of these essentially.

It's enabled only when driver requested it. I'm not sure what dri1 nightmares
you are talking about - I wasn't involved in dri1. Can you elaborate?

> And radeon
> and i915 seem to be able to reset without adding anything to drm core.

Both radeon and i915 take per-device locks (radeon_device->cs_mutex for radeon,
drm_device->struct_mutex for i915), which nouveau does not need. They might be
bottlenecks some day.

> So why can't nouveau do the same? Also, if this is indeed necessary and we
> add this as some mandatory core infrastructure, it's not good enough: In
> i915 we go to great lengths to ensure that all processes waiting for gpu
> reset are still interruptible, in case the gpu reset dies unexpectedly
> (which happens), so this would need to be improved.

Well, I don't like non-interruptability of rwsems too, but I don't see any
other locking primitive which is both interruptible and have read-write
semantics. I was hoping someone would point me at one.

How gpu reset can "die unexpectedly"?

Marcin

Re: [PATCH 1/5] drm: add optional per device rwsem for all ioctls

[Daniel Vetter]

On Mon, Apr 23, 2012 at 07:20:35PM +0200, Marcin Slusarz wrote:
> On Mon, Apr 23, 2012 at 09:51:48AM +0200, Daniel Vetter wrote:
> > On Mon, Apr 23, 2012 at 12:18:28AM +0200, Marcin Slusarz wrote:
> > > Nouveau, in normal circumstances, does not need device lock for every ioctl,
> > > but incoming "gpu reset" code needs exclusive access to the device.
> > > This commit adds drm_driver flag which turns on read lock ioctl encapsulation.
> > > 
> > > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> > 
> > Why can't we just move this down to nouveau driver code?
> 
> I can't wrap drm core ioctls this way and adding locks at driver entry points
> would be at best unmaintainable, at worst - impossible (some of them are
> probably called in atomic context).

Well, not necessarily at ioctl entry, but before you touch stuff that the
gpu reset can change. That has the problem that you get stuck in a bad
place sometimes, but i915 solves that by bailing out and restarting the
ioctl.

> > I really hate adding random stuff to drm core that other drivers can't opt
> > out of - all the dri1 nightmares are made of these essentially.
> 
> It's enabled only when driver requested it. I'm not sure what dri1 nightmares
> you are talking about - I wasn't involved in dri1. Can you elaborate?

Generally sprinkling all these little bits and pieces in drm core that one
driver uses, and only one. If you ever try to refactor things, these bits
and pieces tend to get horribly in the way. E.g. I'm trying to fix up
teardown around lastclose and driver unload, but tons of dri1 features get
in the way (most of them used only by 1 or 2 drivers).

Now your lock is only enabled for nouveau, but the code is still there for
everyone. Which essentially reduces everyone else's abitility to changes
things, because they have to ensure that they don't break a strange
invariant that nouveau (and only nouveau) relies upon. I've tried to
remove some old code in the lastclose paths from drm core and move it into
driver lastclose callbacks which looked pretty innoncent.

It all blew up in actual testing.

So given that i915 and radeon can both deal with this in the driver
specific code I'd really prefer if nouveau could, too.

> > And radeon
> > and i915 seem to be able to reset without adding anything to drm core.
> 
> Both radeon and i915 take per-device locks (radeon_device->cs_mutex for radeon,
> drm_device->struct_mutex for i915), which nouveau does not need. They might be
> bottlenecks some day.

Yeah, we're working on that ;-)

> > So why can't nouveau do the same? Also, if this is indeed necessary and we
> > add this as some mandatory core infrastructure, it's not good enough: In
> > i915 we go to great lengths to ensure that all processes waiting for gpu
> > reset are still interruptible, in case the gpu reset dies unexpectedly
> > (which happens), so this would need to be improved.
> 
> Well, I don't like non-interruptability of rwsems too, but I don't see any
> other locking primitive which is both interruptible and have read-write
> semantics. I was hoping someone would point me at one.

One thing to look into if you really want low overhead is sleepable rcu.
But as I've said above, my main gripe is not with the overhead at runtime,
but just with adding stuff to core for random driver corner-cases.

> How gpu reset can "die unexpectedly"?

Bugs in the not-so-well-tested code and plainly totally screwed-up hw ;-)

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/5] drm: add optional per device rwsem for all ioctls

[Marcin Slusarz]

On Mon, Apr 23, 2012 at 09:51:48AM +0200, Daniel Vetter wrote:
> On Mon, Apr 23, 2012 at 12:18:28AM +0200, Marcin Slusarz wrote:
> > Nouveau, in normal circumstances, does not need device lock for every ioctl,
> > but incoming "gpu reset" code needs exclusive access to the device.
> > This commit adds drm_driver flag which turns on read lock ioctl encapsulation.
> > 
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> 
> Why can't we just move this down to nouveau driver code?

Ok, I think it's easily possible by wrapping drm_ioctl.

> So why can't nouveau do the same? Also, if this is indeed necessary and we
> add this as some mandatory core infrastructure, it's not good enough: In
> i915 we go to great lengths to ensure that all processes waiting for gpu
> reset are still interruptible, in case the gpu reset dies unexpectedly
> (which happens), so this would need to be improved.

I came up with a scheme where down_read can be made interruptible.

I'll post both changes in a few days.

(Unfortunately adding proper down_read_interruptible / down_write_interruptible
require a bit of unfunny work - 8 architectures have their own code for rwsems)

Marcin