[4/5] drm: Push drm_global_mutex locking in drm_open
diff mbox series

Message ID 20200129082410.1691996-5-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • disable drm_global_mutex for most drivers
Related show

Commit Message

Daniel Vetter Jan. 29, 2020, 8:24 a.m. UTC
We want to only take the BKL on crap drivers, but to know whether
we have a crap driver we first need to look it up. Split this shuffle
out from the main BKL-disabling patch, for more clarity.

Since the minors are refcounted drm_minor_acquire is purely internal
and this does not have a driver visible effect.

v2: Push the locking even further into drm_open(), suggested by Chris.
This gives us more symmetry with drm_release(), and maybe a futuer
avenue where we make drm_globale_mutex locking (partially) opt-in like
with drm_release_noglobal().

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c  | 14 +++++---------
 drivers/gpu/drm/drm_file.c |  6 ++++++
 2 files changed, 11 insertions(+), 9 deletions(-)

Comments

Sam Ravnborg Jan. 29, 2020, 4:45 p.m. UTC | #1
Hi Daniel.

On Wed, Jan 29, 2020 at 09:24:09AM +0100, Daniel Vetter wrote:
> We want to only take the BKL on crap drivers, but to know whether
The BKL was killed long time ago..
In other words I was confused until I realized that
- BKL
- drm_global_mutex BKL
- drm_global_mutex

Was all the same. At least my OCD color me confused as is.

> we have a crap driver we first need to look it up. Split this shuffle
> out from the main BKL-disabling patch, for more clarity.
> 
> Since the minors are refcounted drm_minor_acquire is purely internal
> and this does not have a driver visible effect.
> 
> v2: Push the locking even further into drm_open(), suggested by Chris.
> This gives us more symmetry with drm_release(), and maybe a futuer
> avenue where we make drm_globale_mutex locking (partially) opt-in like
s/drm_globale_mutex/drm_global_mutex/

> with drm_release_noglobal().
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Above is IMO fix-while-committing stuff.

	Sam

> ---
>  drivers/gpu/drm/drm_drv.c  | 14 +++++---------
>  drivers/gpu/drm/drm_file.c |  6 ++++++
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8deff75b484c..05bdf0b9d2b3 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1085,17 +1085,14 @@ static int drm_stub_open(struct inode *inode, struct file *filp)
>  
>  	DRM_DEBUG("\n");
>  
> -	mutex_lock(&drm_global_mutex);
>  	minor = drm_minor_acquire(iminor(inode));
> -	if (IS_ERR(minor)) {
> -		err = PTR_ERR(minor);
> -		goto out_unlock;
> -	}
> +	if (IS_ERR(minor))
> +		return PTR_ERR(minor);
>  
>  	new_fops = fops_get(minor->dev->driver->fops);
>  	if (!new_fops) {
>  		err = -ENODEV;
> -		goto out_release;
> +		goto out;
>  	}
>  
>  	replace_fops(filp, new_fops);
> @@ -1104,10 +1101,9 @@ static int drm_stub_open(struct inode *inode, struct file *filp)
>  	else
>  		err = 0;
>  
> -out_release:
> +out:
>  	drm_minor_release(minor);
> -out_unlock:
> -	mutex_unlock(&drm_global_mutex);
> +
>  	return err;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 1075b3a8b5b1..d36cb74ebe0c 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -378,6 +378,8 @@ int drm_open(struct inode *inode, struct file *filp)
>  	if (IS_ERR(minor))
>  		return PTR_ERR(minor);
>  
> +	mutex_unlock(&drm_global_mutex);
> +
>  	dev = minor->dev;
>  	if (!atomic_fetch_inc(&dev->open_count))
>  		need_setup = 1;
> @@ -395,10 +397,14 @@ int drm_open(struct inode *inode, struct file *filp)
>  			goto err_undo;
>  		}
>  	}
> +
> +	mutex_unlock(&drm_global_mutex);
> +
>  	return 0;
>  
>  err_undo:
>  	atomic_dec(&dev->open_count);
> +	mutex_unlock(&drm_global_mutex);
>  	drm_minor_release(minor);
>  	return retcode;
>  }
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Jan. 29, 2020, 5:05 p.m. UTC | #2
On Wed, Jan 29, 2020 at 05:45:45PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
> 
> On Wed, Jan 29, 2020 at 09:24:09AM +0100, Daniel Vetter wrote:
> > We want to only take the BKL on crap drivers, but to know whether
> The BKL was killed long time ago..
> In other words I was confused until I realized that
> - BKL
> - drm_global_mutex BKL
> - drm_global_mutex
> 
> Was all the same. At least my OCD color me confused as is.

The Real BKL was converted into drm_global_mutex for drm when the BKL was
killed. Which is kinda relevant, because the BKL locking horrors (at least
in drm) have been inherited by drm_global_mutex (and the conversion broke
a few things that had to be papered over). Hence the motivation to finally
clean up the locking and figure out what exactly is still protected by
this lock. If you're bored, all this is at least in modern git history
afaics.
-Daniel

> 
> > we have a crap driver we first need to look it up. Split this shuffle
> > out from the main BKL-disabling patch, for more clarity.
> > 
> > Since the minors are refcounted drm_minor_acquire is purely internal
> > and this does not have a driver visible effect.
> > 
> > v2: Push the locking even further into drm_open(), suggested by Chris.
> > This gives us more symmetry with drm_release(), and maybe a futuer
> > avenue where we make drm_globale_mutex locking (partially) opt-in like
> s/drm_globale_mutex/drm_global_mutex/
> 
> > with drm_release_noglobal().
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Above is IMO fix-while-committing stuff.
> 
> 	Sam
> 
> > ---
> >  drivers/gpu/drm/drm_drv.c  | 14 +++++---------
> >  drivers/gpu/drm/drm_file.c |  6 ++++++
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 8deff75b484c..05bdf0b9d2b3 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -1085,17 +1085,14 @@ static int drm_stub_open(struct inode *inode, struct file *filp)
> >  
> >  	DRM_DEBUG("\n");
> >  
> > -	mutex_lock(&drm_global_mutex);
> >  	minor = drm_minor_acquire(iminor(inode));
> > -	if (IS_ERR(minor)) {
> > -		err = PTR_ERR(minor);
> > -		goto out_unlock;
> > -	}
> > +	if (IS_ERR(minor))
> > +		return PTR_ERR(minor);
> >  
> >  	new_fops = fops_get(minor->dev->driver->fops);
> >  	if (!new_fops) {
> >  		err = -ENODEV;
> > -		goto out_release;
> > +		goto out;
> >  	}
> >  
> >  	replace_fops(filp, new_fops);
> > @@ -1104,10 +1101,9 @@ static int drm_stub_open(struct inode *inode, struct file *filp)
> >  	else
> >  		err = 0;
> >  
> > -out_release:
> > +out:
> >  	drm_minor_release(minor);
> > -out_unlock:
> > -	mutex_unlock(&drm_global_mutex);
> > +
> >  	return err;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 1075b3a8b5b1..d36cb74ebe0c 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -378,6 +378,8 @@ int drm_open(struct inode *inode, struct file *filp)
> >  	if (IS_ERR(minor))
> >  		return PTR_ERR(minor);
> >  
> > +	mutex_unlock(&drm_global_mutex);
> > +
> >  	dev = minor->dev;
> >  	if (!atomic_fetch_inc(&dev->open_count))
> >  		need_setup = 1;
> > @@ -395,10 +397,14 @@ int drm_open(struct inode *inode, struct file *filp)
> >  			goto err_undo;
> >  		}
> >  	}
> > +
> > +	mutex_unlock(&drm_global_mutex);
> > +
> >  	return 0;
> >  
> >  err_undo:
> >  	atomic_dec(&dev->open_count);
> > +	mutex_unlock(&drm_global_mutex);
> >  	drm_minor_release(minor);
> >  	return retcode;
> >  }
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8deff75b484c..05bdf0b9d2b3 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -1085,17 +1085,14 @@  static int drm_stub_open(struct inode *inode, struct file *filp)
 
 	DRM_DEBUG("\n");
 
-	mutex_lock(&drm_global_mutex);
 	minor = drm_minor_acquire(iminor(inode));
-	if (IS_ERR(minor)) {
-		err = PTR_ERR(minor);
-		goto out_unlock;
-	}
+	if (IS_ERR(minor))
+		return PTR_ERR(minor);
 
 	new_fops = fops_get(minor->dev->driver->fops);
 	if (!new_fops) {
 		err = -ENODEV;
-		goto out_release;
+		goto out;
 	}
 
 	replace_fops(filp, new_fops);
@@ -1104,10 +1101,9 @@  static int drm_stub_open(struct inode *inode, struct file *filp)
 	else
 		err = 0;
 
-out_release:
+out:
 	drm_minor_release(minor);
-out_unlock:
-	mutex_unlock(&drm_global_mutex);
+
 	return err;
 }
 
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 1075b3a8b5b1..d36cb74ebe0c 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -378,6 +378,8 @@  int drm_open(struct inode *inode, struct file *filp)
 	if (IS_ERR(minor))
 		return PTR_ERR(minor);
 
+	mutex_unlock(&drm_global_mutex);
+
 	dev = minor->dev;
 	if (!atomic_fetch_inc(&dev->open_count))
 		need_setup = 1;
@@ -395,10 +397,14 @@  int drm_open(struct inode *inode, struct file *filp)
 			goto err_undo;
 		}
 	}
+
+	mutex_unlock(&drm_global_mutex);
+
 	return 0;
 
 err_undo:
 	atomic_dec(&dev->open_count);
+	mutex_unlock(&drm_global_mutex);
 	drm_minor_release(minor);
 	return retcode;
 }