Hi all
New version without the deadlock in pan_display that Noralf spotted (hopefully that appeases CI a bit, v1 was bad), plus now being co-tested with the right igt series (hopefully).
Review (especially but not only on the igt side) very much appreciated.
Test-With: 20200128112549.172135-1-daniel.vetter@ffwll.ch
igt series link https://patchwork.freedesktop.org/series/72654/
Cheers, Daniel
Daniel Vetter (5): drm: Complain if drivers still use the ->load callback drm/fbdev-helper: don't force restores drm/client: Rename _force to _locked drm: Push drm_global_mutex locking in drm_open drm: Nerv drm_global_mutex BKL for good drivers
drivers/gpu/drm/drm_client_modeset.c | 12 ++++---- drivers/gpu/drm/drm_drv.c | 26 +++++++++------- drivers/gpu/drm/drm_fb_helper.c | 16 ++-------- drivers/gpu/drm/drm_file.c | 44 +++++++++++++++++++++++++++- drivers/gpu/drm/drm_internal.h | 1 + include/drm/drm_client.h | 7 ++++- include/drm/drm_drv.h | 3 ++ 7 files changed, 78 insertions(+), 31 deletions(-)
Kinda time to get this sorted. The locking around this really is not nice.
Thomas mentioned in his review that the only drivers left unconverted are radeon and amdgpu.
Cc: Harry Wentland harry.wentland@amd.com Cc: Alex Deucher alexander.deucher@amd.com Reviewed-by: Chris Wilson chris@chris-wilson.co.uk Reviewed-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_drv.c | 6 ++++++ include/drm/drm_drv.h | 3 +++ 2 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 7c18a980cd4b..8deff75b484c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -948,6 +948,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
mutex_lock(&drm_global_mutex);
+ if (dev->driver->load) { + if (!drm_core_check_feature(dev, DRIVER_LEGACY)) + DRM_INFO("drm driver %s is using deprecated ->load callback\n", + dev->driver->name); + } + ret = drm_minor_register(dev, DRM_MINOR_RENDER); if (ret) goto err_minors; diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 77685ed7aa65..77bc63de0a91 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -173,6 +173,9 @@ struct drm_driver { * * This is deprecated, do not use! * + * FIXME: A few non-DRIVER_LEGACY drivers still use this, and should be + * converted. + * * Returns: * * Zero on success, non-zero value on failure.
Instead check for master status, in case we've raced.
This is the last exception to the general rule that we restore fbcon only when there's no master active. Compositors are supposed to drop their master status before they switch to a different console back to text mode (or just switch to text mode directly, without a vt switch).
This is known to break some subtests of kms_fbcon_fbt in igt, but they're just wrong - it does a graphics/text mode switch for the vt without updating the master status.
Also add a comment to the drm_client->restore hook that this is expected going forward from all clients (there's currently just one).
v2: Also drop the force in pan_display
v3: Restore the _force to pan_display, this actually means _locked in that path. Spotted by Noralf.
Cc: Noralf Trønnes noralf@tronnes.org Reviewed-by: Noralf Trønnes noralf@tronnes.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_fb_helper.c | 12 +----------- include/drm/drm_client.h | 5 +++++ 2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 4c7cbce7bae7..672934e0eeed 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -250,17 +250,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) return 0;
mutex_lock(&fb_helper->lock); - /* - * TODO: - * We should bail out here if there is a master by dropping _force. - * Currently these igt tests fail if we do that: - * - kms_fbcon_fbt@psr - * - kms_fbcon_fbt@psr-suspend - * - * So first these tests need to be fixed so they drop master or don't - * have an fd open. - */ - ret = drm_client_modeset_commit_force(&fb_helper->client); + ret = drm_client_modeset_commit(&fb_helper->client);
do_delayed = fb_helper->delayed_hotplug; if (do_delayed) diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h index 5cf2c5dd8b1e..d01d311023ac 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -44,6 +44,11 @@ struct drm_client_funcs { * returns zero gets the privilege to restore and no more clients are * called. This callback is not called after @unregister has been called. * + * Note that the core does not guarantee exclusion against concurrent + * drm_open(). Clients need to ensure this themselves, for example by + * using drm_master_internal_acquire() and + * drm_master_internal_release(). + * * This callback is optional. */ int (*restore)(struct drm_client_dev *client);
Plus extend the kerneldoc a bit to explain how this should be used. With the previous patch to drop the force restore the main user of this function is not emphasis on the "I hold the internal master lock already" aspect, so rename the function to match.
Suggested by Noralf.
Cc: Noralf Trønnes noralf@tronnes.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_client_modeset.c | 12 +++++++----- drivers/gpu/drm/drm_fb_helper.c | 4 ++-- include/drm/drm_client.h | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 6d4a29e99ae2..841794e19eb6 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -1094,15 +1094,17 @@ static int drm_client_modeset_commit_legacy(struct drm_client_dev *client) }
/** - * drm_client_modeset_commit_force() - Force commit CRTC configuration + * drm_client_modeset_commit_locked() - Force commit CRTC configuration * @client: DRM client * - * Commit modeset configuration to crtcs without checking if there is a DRM master. + * Commit modeset configuration to crtcs without checking if there is a DRM + * master. The assumption is that the caller already holds an internal DRM + * master reference acquired with drm_master_internal_acquire(). * * Returns: * Zero on success or negative error code on failure. */ -int drm_client_modeset_commit_force(struct drm_client_dev *client) +int drm_client_modeset_commit_locked(struct drm_client_dev *client) { struct drm_device *dev = client->dev; int ret; @@ -1116,7 +1118,7 @@ int drm_client_modeset_commit_force(struct drm_client_dev *client)
return ret; } -EXPORT_SYMBOL(drm_client_modeset_commit_force); +EXPORT_SYMBOL(drm_client_modeset_commit_locked);
/** * drm_client_modeset_commit() - Commit CRTC configuration @@ -1135,7 +1137,7 @@ int drm_client_modeset_commit(struct drm_client_dev *client) if (!drm_master_internal_acquire(dev)) return -EBUSY;
- ret = drm_client_modeset_commit_force(client); + ret = drm_client_modeset_commit_locked(client);
drm_master_internal_release(dev);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 672934e0eeed..490a99de6ec1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -284,7 +284,7 @@ static bool drm_fb_helper_force_kernel_mode(void) continue;
mutex_lock(&helper->lock); - ret = drm_client_modeset_commit_force(&helper->client); + ret = drm_client_modeset_commit_locked(&helper->client); if (ret) error = true; mutex_unlock(&helper->lock); @@ -1347,7 +1347,7 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,
pan_set(fb_helper, var->xoffset, var->yoffset);
- ret = drm_client_modeset_commit_force(&fb_helper->client); + ret = drm_client_modeset_commit_locked(&fb_helper->client); if (!ret) { info->var.xoffset = var->xoffset; info->var.yoffset = var->yoffset; diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h index d01d311023ac..3ed5dee899fd 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -161,7 +161,7 @@ int drm_client_modeset_create(struct drm_client_dev *client); void drm_client_modeset_free(struct drm_client_dev *client); int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width, unsigned int height); bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation); -int drm_client_modeset_commit_force(struct drm_client_dev *client); +int drm_client_modeset_commit_locked(struct drm_client_dev *client); int drm_client_modeset_commit(struct drm_client_dev *client); int drm_client_modeset_dpms(struct drm_client_dev *client, int mode);
Den 29.01.2020 09.24, skrev Daniel Vetter:
The _not_ confuses me, the emphasis is now that "I hold the lock" right?
Reviewed-by: Noralf Trønnes noralf@tronnes.org
On Wed, Jan 29, 2020 at 2:16 PM Noralf Trønnes noralf@tronnes.org wrote:
Oops, I confused myself and mixed things up between old and new meaning and what's now important and what not. s/not/now/, I'll fix when applying. -Daniel
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(-)
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; }
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.
s/drm_globale_mutex/drm_global_mutex/
Above is IMO fix-while-committing stuff.
Sam
On Wed, Jan 29, 2020 at 05:45:45PM +0100, Sam Ravnborg wrote:
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 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. Historical aside: When the kernel-wide BKL was removed, it was replaced by drm_global_mutex within the scope of the drm subsystem hence why these two things are (almost) interchangeable as concepts here.
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_global_mutex locking (partially) opt-in like with drm_release_noglobal().
v3: - Actually push this stuff correctly, don't unlock twice (Chris) - Fix typo on commit message, plus explain why BKL = drm_global_mutex (Sam)
Cc: Sam Ravnborg sam@ravnborg.org 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(-)
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..80d556402ab4 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_lock(&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; }
This catches the majority of drivers (unfortunately not if we take users into account, because all the big drivers have at least a lastclose hook).
With the prep patches out of the way all drm state is fully protected and either prevents or can deal with the races from dropping the BKL around open/close. The only thing left to audit are the various driver hooks - by keeping the BKL around if any of them are set we have a very simple cop-out!
Note that one of the biggest prep pieces to get here was making dev->open_count atomic, which was done in
commit 7e13ad896484a0165a68197a2e64091ea28c9602 Author: Chris Wilson chris@chris-wilson.co.uk Date: Fri Jan 24 13:01:07 2020 +0000
drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
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 | 6 +++-- drivers/gpu/drm/drm_file.c | 46 ++++++++++++++++++++++++++++++---- drivers/gpu/drm/drm_internal.h | 1 + 3 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 05bdf0b9d2b3..9fcd6ab3c154 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -946,7 +946,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) struct drm_driver *driver = dev->driver; int ret;
- mutex_lock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_lock(&drm_global_mutex);
if (dev->driver->load) { if (!drm_core_check_feature(dev, DRIVER_LEGACY)) @@ -992,7 +993,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); out_unlock: - mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex); return ret; } EXPORT_SYMBOL(drm_dev_register); diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index d36cb74ebe0c..efd6fe0b6b4f 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -51,6 +51,37 @@ /* from BKL pushdown */ DEFINE_MUTEX(drm_global_mutex);
+bool drm_dev_needs_global_mutex(struct drm_device *dev) +{ + /* + * Legacy drivers rely on all kinds of BKL locking semantics, don't + * bother. They also still need BKL locking for their ioctls, so better + * safe than sorry. + */ + if (drm_core_check_feature(dev, DRIVER_LEGACY)) + return true; + + /* + * The deprecated ->load callback must be called after the driver is + * already registered. This means such drivers rely on the BKL to make + * sure an open can't proceed until the driver is actually fully set up. + * Similar hilarity holds for the unload callback. + */ + if (dev->driver->load || dev->driver->unload) + return true; + + /* + * Drivers with the lastclose callback assume that it's synchronized + * against concurrent opens, which again needs the BKL. The proper fix + * is to use the drm_client infrastructure with proper locking for each + * client. + */ + if (dev->driver->lastclose) + return true; + + return false; +} + /** * DOC: file operations * @@ -378,9 +409,10 @@ 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 (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex); + if (!atomic_fetch_inc(&dev->open_count)) need_setup = 1;
@@ -398,13 +430,15 @@ int drm_open(struct inode *inode, struct file *filp) } }
- mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex);
return 0;
err_undo: atomic_dec(&dev->open_count); - mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex); drm_minor_release(minor); return retcode; } @@ -444,6 +478,7 @@ int drm_release(struct inode *inode, struct file *filp) struct drm_minor *minor = file_priv->minor; struct drm_device *dev = minor->dev;
+ if (drm_dev_needs_global_mutex(dev)) mutex_lock(&drm_global_mutex);
DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count)); @@ -453,7 +488,8 @@ int drm_release(struct inode *inode, struct file *filp) if (atomic_dec_and_test(&dev->open_count)) drm_lastclose(dev);
- mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex);
drm_minor_release(minor);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 6937bf923f05..aeec2e68d772 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -41,6 +41,7 @@ struct drm_printer;
/* drm_file.c */ extern struct mutex drm_global_mutex; +bool drm_dev_needs_global_mutex(struct drm_device *dev); struct drm_file *drm_file_alloc(struct drm_minor *minor); void drm_file_free(struct drm_file *file); void drm_lastclose(struct drm_device *dev);
Hi Daniel.
In the nit-pick department today - sorry.
Subject: [PATCH 5/5] drm: Nerv drm_global_mutex BKL for good drivers I did not understand this subject... - what is "Nerv"?
Sam
On Wed, Jan 29, 2020 at 09:24:10AM +0100, Daniel Vetter wrote:
On Wed, Jan 29, 2020 at 05:47:33PM +0100, Sam Ravnborg wrote:
It's a typo, supposed to be nerf:
https://www.urbandictionary.com/define.php?term=nerf
Cheers, Daniel
This catches the majority of drivers (unfortunately not if we take users into account, because all the big drivers have at least a lastclose hook).
With the prep patches out of the way all drm state is fully protected and either prevents or can deal with the races from dropping the BKL around open/close. The only thing left to audit are the various driver hooks - by keeping the BKL around if any of them are set we have a very simple cop-out!
Note that one of the biggest prep pieces to get here was making dev->open_count atomic, which was done in
commit 7e13ad896484a0165a68197a2e64091ea28c9602 Author: Chris Wilson chris@chris-wilson.co.uk Date: Fri Jan 24 13:01:07 2020 +0000
drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
v2: - Rebase and fix locking in drm_open() (Chris) - Indentation fix in drm_release - Typo fix in the commit message (Sam)
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Sam Ravnborg sam@ravnborg.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_drv.c | 6 +++-- drivers/gpu/drm/drm_file.c | 48 +++++++++++++++++++++++++++++----- drivers/gpu/drm/drm_internal.h | 1 + 3 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 05bdf0b9d2b3..9fcd6ab3c154 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -946,7 +946,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) struct drm_driver *driver = dev->driver; int ret;
- mutex_lock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_lock(&drm_global_mutex);
if (dev->driver->load) { if (!drm_core_check_feature(dev, DRIVER_LEGACY)) @@ -992,7 +993,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); out_unlock: - mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex); return ret; } EXPORT_SYMBOL(drm_dev_register); diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 80d556402ab4..c4c704e01961 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -51,6 +51,37 @@ /* from BKL pushdown */ DEFINE_MUTEX(drm_global_mutex);
+bool drm_dev_needs_global_mutex(struct drm_device *dev) +{ + /* + * Legacy drivers rely on all kinds of BKL locking semantics, don't + * bother. They also still need BKL locking for their ioctls, so better + * safe than sorry. + */ + if (drm_core_check_feature(dev, DRIVER_LEGACY)) + return true; + + /* + * The deprecated ->load callback must be called after the driver is + * already registered. This means such drivers rely on the BKL to make + * sure an open can't proceed until the driver is actually fully set up. + * Similar hilarity holds for the unload callback. + */ + if (dev->driver->load || dev->driver->unload) + return true; + + /* + * Drivers with the lastclose callback assume that it's synchronized + * against concurrent opens, which again needs the BKL. The proper fix + * is to use the drm_client infrastructure with proper locking for each + * client. + */ + if (dev->driver->lastclose) + return true; + + return false; +} + /** * DOC: file operations * @@ -378,9 +409,10 @@ int drm_open(struct inode *inode, struct file *filp) if (IS_ERR(minor)) return PTR_ERR(minor);
- mutex_lock(&drm_global_mutex); - dev = minor->dev; + if (drm_dev_needs_global_mutex(dev)) + mutex_lock(&drm_global_mutex); + if (!atomic_fetch_inc(&dev->open_count)) need_setup = 1;
@@ -398,13 +430,15 @@ int drm_open(struct inode *inode, struct file *filp) } }
- mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex);
return 0;
err_undo: atomic_dec(&dev->open_count); - mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex); drm_minor_release(minor); return retcode; } @@ -444,7 +478,8 @@ int drm_release(struct inode *inode, struct file *filp) struct drm_minor *minor = file_priv->minor; struct drm_device *dev = minor->dev;
- mutex_lock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_lock(&drm_global_mutex);
DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count));
@@ -453,7 +488,8 @@ int drm_release(struct inode *inode, struct file *filp) if (atomic_dec_and_test(&dev->open_count)) drm_lastclose(dev);
- mutex_unlock(&drm_global_mutex); + if (drm_dev_needs_global_mutex(dev)) + mutex_unlock(&drm_global_mutex);
drm_minor_release(minor);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 6937bf923f05..aeec2e68d772 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -41,6 +41,7 @@ struct drm_printer;
/* drm_file.c */ extern struct mutex drm_global_mutex; +bool drm_dev_needs_global_mutex(struct drm_device *dev); struct drm_file *drm_file_alloc(struct drm_minor *minor); void drm_file_free(struct drm_file *file); void drm_lastclose(struct drm_device *dev);
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Noralf Trønnes -
Sam Ravnborg