| Message ID | 20200129082410.1691996-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | disable drm_global_mutex for most drivers | expand |
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
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
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; }