* [PATCH 1/2] drm: Release filp before global lock
@ 2020-01-24 12:56 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 12:56 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, intel-gfx, Thomas Hellström
The file is not part of the global drm resource and can be released
prior to take the global mutex to drop the open_count (and potentially
close) the drm device. As the global mutex is indeed global, not only
within the device but across devices, a slow file release mechanism can
bottleneck the entire system.
However, inside drm_close_helper() there are a number of dev->driver
callbacks that take the drm_device as the first parameter... Worryingly
some of those callbacks may be (implicitly) depending on the global
mutex.
v2: Drop the debug message for the open-count, it's included with the
drm_file_free() debug message -- and for good measure make that up as
reading outside of the mutex.
v3: Separate the calling of the filp cleanup outside of
drm_global_mutex into a new drm_release_noglobal() hook, so that we can
phase the transition. drm/savage relies on the global mutex, and there
may be more, so be cautious.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
Acked-by: Thomas Hellström (VMware) <thomas_os@shipmail.org>
---
drivers/gpu/drm/drm_file.c | 36 ++++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/i915_drv.c | 2 +-
include/drm/drm_file.h | 1 +
3 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 92d16724f949..e25306c49cc6 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file->minor->kdev->devt),
- dev->open_count);
+ READ_ONCE(dev->open_count));
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
dev->driver->preclose)
@@ -455,6 +455,40 @@ int drm_release(struct inode *inode, struct file *filp)
}
EXPORT_SYMBOL(drm_release);
+/**
+ * drm_release_noglobal - release method for DRM file
+ * @inode: device inode
+ * @filp: file pointer.
+ *
+ * This function may be used by drivers as their &file_operations.release
+ * method. It frees any resources associated with the open file prior to taking
+ * the drm_global_mutex, which then calls the &drm_driver.postclose driver
+ * callback. If this is the last open file for the DRM device also proceeds to
+ * call the &drm_driver.lastclose driver callback.
+ *
+ * RETURNS:
+ *
+ * Always succeeds and returns 0.
+ */
+int drm_release_noglobal(struct inode *inode, struct file *filp)
+{
+ struct drm_file *file_priv = filp->private_data;
+ struct drm_minor *minor = file_priv->minor;
+ struct drm_device *dev = minor->dev;
+
+ drm_close_helper(filp);
+
+ mutex_lock(&drm_global_mutex);
+ if (!--dev->open_count)
+ drm_lastclose(dev);
+ mutex_unlock(&drm_global_mutex);
+
+ drm_minor_release(minor);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_release_noglobal);
+
/**
* drm_read - read method for DRM file
* @filp: file pointer
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e9b42e962032..5a5846d892f4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2673,7 +2673,7 @@ const struct dev_pm_ops i915_pm_ops = {
static const struct file_operations i915_driver_fops = {
.owner = THIS_MODULE,
.open = drm_open,
- .release = drm_release,
+ .release = drm_release_noglobal,
.unlocked_ioctl = drm_ioctl,
.mmap = i915_gem_mmap,
.poll = drm_poll,
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 8b099b347817..19df8028a6c4 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -374,6 +374,7 @@ int drm_open(struct inode *inode, struct file *filp);
ssize_t drm_read(struct file *filp, char __user *buffer,
size_t count, loff_t *offset);
int drm_release(struct inode *inode, struct file *filp);
+int drm_release_noglobal(struct inode *inode, struct file *filp);
__poll_t drm_poll(struct file *filp, struct poll_table_struct *wait);
int drm_event_reserve_init_locked(struct drm_device *dev,
struct drm_file *file_priv,
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Intel-gfx] [PATCH 1/2] drm: Release filp before global lock
@ 2020-01-24 12:56 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 12:56 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, intel-gfx, Thomas Hellström
The file is not part of the global drm resource and can be released
prior to take the global mutex to drop the open_count (and potentially
close) the drm device. As the global mutex is indeed global, not only
within the device but across devices, a slow file release mechanism can
bottleneck the entire system.
However, inside drm_close_helper() there are a number of dev->driver
callbacks that take the drm_device as the first parameter... Worryingly
some of those callbacks may be (implicitly) depending on the global
mutex.
v2: Drop the debug message for the open-count, it's included with the
drm_file_free() debug message -- and for good measure make that up as
reading outside of the mutex.
v3: Separate the calling of the filp cleanup outside of
drm_global_mutex into a new drm_release_noglobal() hook, so that we can
phase the transition. drm/savage relies on the global mutex, and there
may be more, so be cautious.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
Acked-by: Thomas Hellström (VMware) <thomas_os@shipmail.org>
---
drivers/gpu/drm/drm_file.c | 36 ++++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/i915_drv.c | 2 +-
include/drm/drm_file.h | 1 +
3 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 92d16724f949..e25306c49cc6 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file->minor->kdev->devt),
- dev->open_count);
+ READ_ONCE(dev->open_count));
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
dev->driver->preclose)
@@ -455,6 +455,40 @@ int drm_release(struct inode *inode, struct file *filp)
}
EXPORT_SYMBOL(drm_release);
+/**
+ * drm_release_noglobal - release method for DRM file
+ * @inode: device inode
+ * @filp: file pointer.
+ *
+ * This function may be used by drivers as their &file_operations.release
+ * method. It frees any resources associated with the open file prior to taking
+ * the drm_global_mutex, which then calls the &drm_driver.postclose driver
+ * callback. If this is the last open file for the DRM device also proceeds to
+ * call the &drm_driver.lastclose driver callback.
+ *
+ * RETURNS:
+ *
+ * Always succeeds and returns 0.
+ */
+int drm_release_noglobal(struct inode *inode, struct file *filp)
+{
+ struct drm_file *file_priv = filp->private_data;
+ struct drm_minor *minor = file_priv->minor;
+ struct drm_device *dev = minor->dev;
+
+ drm_close_helper(filp);
+
+ mutex_lock(&drm_global_mutex);
+ if (!--dev->open_count)
+ drm_lastclose(dev);
+ mutex_unlock(&drm_global_mutex);
+
+ drm_minor_release(minor);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_release_noglobal);
+
/**
* drm_read - read method for DRM file
* @filp: file pointer
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e9b42e962032..5a5846d892f4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2673,7 +2673,7 @@ const struct dev_pm_ops i915_pm_ops = {
static const struct file_operations i915_driver_fops = {
.owner = THIS_MODULE,
.open = drm_open,
- .release = drm_release,
+ .release = drm_release_noglobal,
.unlocked_ioctl = drm_ioctl,
.mmap = i915_gem_mmap,
.poll = drm_poll,
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 8b099b347817..19df8028a6c4 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -374,6 +374,7 @@ int drm_open(struct inode *inode, struct file *filp);
ssize_t drm_read(struct file *filp, char __user *buffer,
size_t count, loff_t *offset);
int drm_release(struct inode *inode, struct file *filp);
+int drm_release_noglobal(struct inode *inode, struct file *filp);
__poll_t drm_poll(struct file *filp, struct poll_table_struct *wait);
int drm_event_reserve_init_locked(struct drm_device *dev,
struct drm_file *file_priv,
--
2.25.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
2020-01-24 12:56 ` [Intel-gfx] " Chris Wilson
@ 2020-01-24 12:56 ` Chris Wilson
-1 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 12:56 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, intel-gfx, Thomas Hellström
Since drm_global_mutex is a true global mutex across devices, we don't
want to acquire it unless absolutely necessary. For maintaining the
device local open_count, we can use atomic operations on the counter
itself, except when making the transition to/from 0. Here, we tackle the
easy portion of delaying acquiring the drm_global_mutex for the final
release by using atomic_dec_and_mutex_lock(), leaving the global
serialisation across the device opens.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
drivers/gpu/drm/drm_file.c | 14 ++++++--------
drivers/gpu/drm/i915/i915_switcheroo.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
include/drm/drm_device.h | 2 +-
6 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 53d882000101..c3c0356dfa61 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1136,7 +1136,7 @@ static bool amdgpu_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops amdgpu_switcheroo_ops = {
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e25306c49cc6..c4e9ef86c589 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file->minor->kdev->devt),
- READ_ONCE(dev->open_count));
+ atomic_read(&dev->open_count));
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
dev->driver->preclose)
@@ -379,7 +379,7 @@ int drm_open(struct inode *inode, struct file *filp)
return PTR_ERR(minor);
dev = minor->dev;
- if (!dev->open_count++)
+ if (!atomic_fetch_inc(&dev->open_count))
need_setup = 1;
/* share address_space across all char-devs of a single device */
@@ -398,7 +398,7 @@ int drm_open(struct inode *inode, struct file *filp)
return 0;
err_undo:
- dev->open_count--;
+ atomic_dec(&dev->open_count);
drm_minor_release(minor);
return retcode;
}
@@ -440,11 +440,11 @@ int drm_release(struct inode *inode, struct file *filp)
mutex_lock(&drm_global_mutex);
- DRM_DEBUG("open_count = %d\n", dev->open_count);
+ DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count));
drm_close_helper(filp);
- if (!--dev->open_count)
+ if (atomic_dec_and_test(&dev->open_count))
drm_lastclose(dev);
mutex_unlock(&drm_global_mutex);
@@ -478,10 +478,8 @@ int drm_release_noglobal(struct inode *inode, struct file *filp)
drm_close_helper(filp);
- mutex_lock(&drm_global_mutex);
- if (!--dev->open_count)
+ if (atomic_dec_and_mutex_lock(&dev->open_count, &drm_global_mutex))
drm_lastclose(dev);
- mutex_unlock(&drm_global_mutex);
drm_minor_release(minor);
diff --git a/drivers/gpu/drm/i915/i915_switcheroo.c b/drivers/gpu/drm/i915/i915_switcheroo.c
index 39c79e1c5b52..ed69b5d4a375 100644
--- a/drivers/gpu/drm/i915/i915_switcheroo.c
+++ b/drivers/gpu/drm/i915/i915_switcheroo.c
@@ -43,7 +43,7 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return i915 && i915->drm.open_count == 0;
+ return i915 && atomic_read(&i915->drm.open_count) == 0;
}
static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index d865d8aeac3c..c85dd8afa3c3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -72,7 +72,7 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index a522e092038b..266e3cbbd09b 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1263,7 +1263,7 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 1acfc3bbd3fb..bb60a949f416 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -144,7 +144,7 @@ struct drm_device {
* Usage counter for outstanding files open,
* protected by drm_global_mutex
*/
- int open_count;
+ atomic_t open_count;
/** @filelist_mutex: Protects @filelist. */
struct mutex filelist_mutex;
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Intel-gfx] [PATCH 2/2] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
@ 2020-01-24 12:56 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 12:56 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, intel-gfx, Thomas Hellström
Since drm_global_mutex is a true global mutex across devices, we don't
want to acquire it unless absolutely necessary. For maintaining the
device local open_count, we can use atomic operations on the counter
itself, except when making the transition to/from 0. Here, we tackle the
easy portion of delaying acquiring the drm_global_mutex for the final
release by using atomic_dec_and_mutex_lock(), leaving the global
serialisation across the device opens.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
drivers/gpu/drm/drm_file.c | 14 ++++++--------
drivers/gpu/drm/i915/i915_switcheroo.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
include/drm/drm_device.h | 2 +-
6 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 53d882000101..c3c0356dfa61 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1136,7 +1136,7 @@ static bool amdgpu_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops amdgpu_switcheroo_ops = {
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e25306c49cc6..c4e9ef86c589 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file->minor->kdev->devt),
- READ_ONCE(dev->open_count));
+ atomic_read(&dev->open_count));
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
dev->driver->preclose)
@@ -379,7 +379,7 @@ int drm_open(struct inode *inode, struct file *filp)
return PTR_ERR(minor);
dev = minor->dev;
- if (!dev->open_count++)
+ if (!atomic_fetch_inc(&dev->open_count))
need_setup = 1;
/* share address_space across all char-devs of a single device */
@@ -398,7 +398,7 @@ int drm_open(struct inode *inode, struct file *filp)
return 0;
err_undo:
- dev->open_count--;
+ atomic_dec(&dev->open_count);
drm_minor_release(minor);
return retcode;
}
@@ -440,11 +440,11 @@ int drm_release(struct inode *inode, struct file *filp)
mutex_lock(&drm_global_mutex);
- DRM_DEBUG("open_count = %d\n", dev->open_count);
+ DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count));
drm_close_helper(filp);
- if (!--dev->open_count)
+ if (atomic_dec_and_test(&dev->open_count))
drm_lastclose(dev);
mutex_unlock(&drm_global_mutex);
@@ -478,10 +478,8 @@ int drm_release_noglobal(struct inode *inode, struct file *filp)
drm_close_helper(filp);
- mutex_lock(&drm_global_mutex);
- if (!--dev->open_count)
+ if (atomic_dec_and_mutex_lock(&dev->open_count, &drm_global_mutex))
drm_lastclose(dev);
- mutex_unlock(&drm_global_mutex);
drm_minor_release(minor);
diff --git a/drivers/gpu/drm/i915/i915_switcheroo.c b/drivers/gpu/drm/i915/i915_switcheroo.c
index 39c79e1c5b52..ed69b5d4a375 100644
--- a/drivers/gpu/drm/i915/i915_switcheroo.c
+++ b/drivers/gpu/drm/i915/i915_switcheroo.c
@@ -43,7 +43,7 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return i915 && i915->drm.open_count == 0;
+ return i915 && atomic_read(&i915->drm.open_count) == 0;
}
static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index d865d8aeac3c..c85dd8afa3c3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -72,7 +72,7 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index a522e092038b..266e3cbbd09b 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1263,7 +1263,7 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 1acfc3bbd3fb..bb60a949f416 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -144,7 +144,7 @@ struct drm_device {
* Usage counter for outstanding files open,
* protected by drm_global_mutex
*/
- int open_count;
+ atomic_t open_count;
/** @filelist_mutex: Protects @filelist. */
struct mutex filelist_mutex;
--
2.25.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
2020-01-24 12:56 ` [Intel-gfx] " Chris Wilson
@ 2020-01-24 13:01 ` Chris Wilson
-1 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 13:01 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Thomas Hellström
Since drm_global_mutex is a true global mutex across devices, we don't
want to acquire it unless absolutely necessary. For maintaining the
device local open_count, we can use atomic operations on the counter
itself, except when making the transition to/from 0. Here, we tackle the
easy portion of delaying acquiring the drm_global_mutex for the final
release by using atomic_dec_and_mutex_lock(), leaving the global
serialisation across the device opens.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
---
atomic_dec_and_mutex_lock needs pairing with mutex_unlock (you fool)
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
drivers/gpu/drm/drm_file.c | 16 ++++++++--------
drivers/gpu/drm/i915/i915_switcheroo.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
include/drm/drm_device.h | 2 +-
6 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 53d882000101..c3c0356dfa61 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1136,7 +1136,7 @@ static bool amdgpu_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops amdgpu_switcheroo_ops = {
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e25306c49cc6..1075b3a8b5b1 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file->minor->kdev->devt),
- READ_ONCE(dev->open_count));
+ atomic_read(&dev->open_count));
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
dev->driver->preclose)
@@ -379,7 +379,7 @@ int drm_open(struct inode *inode, struct file *filp)
return PTR_ERR(minor);
dev = minor->dev;
- if (!dev->open_count++)
+ if (!atomic_fetch_inc(&dev->open_count))
need_setup = 1;
/* share address_space across all char-devs of a single device */
@@ -398,7 +398,7 @@ int drm_open(struct inode *inode, struct file *filp)
return 0;
err_undo:
- dev->open_count--;
+ atomic_dec(&dev->open_count);
drm_minor_release(minor);
return retcode;
}
@@ -440,11 +440,11 @@ int drm_release(struct inode *inode, struct file *filp)
mutex_lock(&drm_global_mutex);
- DRM_DEBUG("open_count = %d\n", dev->open_count);
+ DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count));
drm_close_helper(filp);
- if (!--dev->open_count)
+ if (atomic_dec_and_test(&dev->open_count))
drm_lastclose(dev);
mutex_unlock(&drm_global_mutex);
@@ -478,10 +478,10 @@ int drm_release_noglobal(struct inode *inode, struct file *filp)
drm_close_helper(filp);
- mutex_lock(&drm_global_mutex);
- if (!--dev->open_count)
+ if (atomic_dec_and_mutex_lock(&dev->open_count, &drm_global_mutex)) {
drm_lastclose(dev);
- mutex_unlock(&drm_global_mutex);
+ mutex_unlock(&drm_global_mutex);
+ }
drm_minor_release(minor);
diff --git a/drivers/gpu/drm/i915/i915_switcheroo.c b/drivers/gpu/drm/i915/i915_switcheroo.c
index 39c79e1c5b52..ed69b5d4a375 100644
--- a/drivers/gpu/drm/i915/i915_switcheroo.c
+++ b/drivers/gpu/drm/i915/i915_switcheroo.c
@@ -43,7 +43,7 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return i915 && i915->drm.open_count == 0;
+ return i915 && atomic_read(&i915->drm.open_count) == 0;
}
static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index d865d8aeac3c..c85dd8afa3c3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -72,7 +72,7 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index a522e092038b..266e3cbbd09b 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1263,7 +1263,7 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 1acfc3bbd3fb..bb60a949f416 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -144,7 +144,7 @@ struct drm_device {
* Usage counter for outstanding files open,
* protected by drm_global_mutex
*/
- int open_count;
+ atomic_t open_count;
/** @filelist_mutex: Protects @filelist. */
struct mutex filelist_mutex;
--
2.25.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Intel-gfx] [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
@ 2020-01-24 13:01 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 13:01 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx, Thomas Hellström
Since drm_global_mutex is a true global mutex across devices, we don't
want to acquire it unless absolutely necessary. For maintaining the
device local open_count, we can use atomic operations on the counter
itself, except when making the transition to/from 0. Here, we tackle the
easy portion of delaying acquiring the drm_global_mutex for the final
release by using atomic_dec_and_mutex_lock(), leaving the global
serialisation across the device opens.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
---
atomic_dec_and_mutex_lock needs pairing with mutex_unlock (you fool)
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
drivers/gpu/drm/drm_file.c | 16 ++++++++--------
drivers/gpu/drm/i915/i915_switcheroo.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
include/drm/drm_device.h | 2 +-
6 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 53d882000101..c3c0356dfa61 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1136,7 +1136,7 @@ static bool amdgpu_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops amdgpu_switcheroo_ops = {
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e25306c49cc6..1075b3a8b5b1 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
task_pid_nr(current),
(long)old_encode_dev(file->minor->kdev->devt),
- READ_ONCE(dev->open_count));
+ atomic_read(&dev->open_count));
if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
dev->driver->preclose)
@@ -379,7 +379,7 @@ int drm_open(struct inode *inode, struct file *filp)
return PTR_ERR(minor);
dev = minor->dev;
- if (!dev->open_count++)
+ if (!atomic_fetch_inc(&dev->open_count))
need_setup = 1;
/* share address_space across all char-devs of a single device */
@@ -398,7 +398,7 @@ int drm_open(struct inode *inode, struct file *filp)
return 0;
err_undo:
- dev->open_count--;
+ atomic_dec(&dev->open_count);
drm_minor_release(minor);
return retcode;
}
@@ -440,11 +440,11 @@ int drm_release(struct inode *inode, struct file *filp)
mutex_lock(&drm_global_mutex);
- DRM_DEBUG("open_count = %d\n", dev->open_count);
+ DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count));
drm_close_helper(filp);
- if (!--dev->open_count)
+ if (atomic_dec_and_test(&dev->open_count))
drm_lastclose(dev);
mutex_unlock(&drm_global_mutex);
@@ -478,10 +478,10 @@ int drm_release_noglobal(struct inode *inode, struct file *filp)
drm_close_helper(filp);
- mutex_lock(&drm_global_mutex);
- if (!--dev->open_count)
+ if (atomic_dec_and_mutex_lock(&dev->open_count, &drm_global_mutex)) {
drm_lastclose(dev);
- mutex_unlock(&drm_global_mutex);
+ mutex_unlock(&drm_global_mutex);
+ }
drm_minor_release(minor);
diff --git a/drivers/gpu/drm/i915/i915_switcheroo.c b/drivers/gpu/drm/i915/i915_switcheroo.c
index 39c79e1c5b52..ed69b5d4a375 100644
--- a/drivers/gpu/drm/i915/i915_switcheroo.c
+++ b/drivers/gpu/drm/i915/i915_switcheroo.c
@@ -43,7 +43,7 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return i915 && i915->drm.open_count == 0;
+ return i915 && atomic_read(&i915->drm.open_count) == 0;
}
static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index d865d8aeac3c..c85dd8afa3c3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -72,7 +72,7 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index a522e092038b..266e3cbbd09b 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1263,7 +1263,7 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
* locking inversion with the driver load path. And the access here is
* completely racy anyway. So don't bother with locking for now.
*/
- return dev->open_count == 0;
+ return atomic_read(&dev->open_count) == 0;
}
static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 1acfc3bbd3fb..bb60a949f416 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -144,7 +144,7 @@ struct drm_device {
* Usage counter for outstanding files open,
* protected by drm_global_mutex
*/
- int open_count;
+ atomic_t open_count;
/** @filelist_mutex: Protects @filelist. */
struct mutex filelist_mutex;
--
2.25.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
2020-01-24 13:01 ` [Intel-gfx] " Chris Wilson
@ 2020-01-24 13:37 ` Thomas Hellström (VMware)
-1 siblings, 0 replies; 20+ messages in thread
From: Thomas Hellström (VMware) @ 2020-01-24 13:37 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: intel-gfx
On 1/24/20 2:01 PM, Chris Wilson wrote:
> Since drm_global_mutex is a true global mutex across devices, we don't
> want to acquire it unless absolutely necessary. For maintaining the
> device local open_count, we can use atomic operations on the counter
> itself, except when making the transition to/from 0. Here, we tackle the
> easy portion of delaying acquiring the drm_global_mutex for the final
> release by using atomic_dec_and_mutex_lock(), leaving the global
> serialisation across the device opens.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
For the series:
Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
Now the only remaining (though pre-existing) problem I can see is that
there is no corresponding mutex lock in drm_open() so that firstopen
might race with lastclose.. Or I might be missing something..
/Thomas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
@ 2020-01-24 13:37 ` Thomas Hellström (VMware)
0 siblings, 0 replies; 20+ messages in thread
From: Thomas Hellström (VMware) @ 2020-01-24 13:37 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: intel-gfx
On 1/24/20 2:01 PM, Chris Wilson wrote:
> Since drm_global_mutex is a true global mutex across devices, we don't
> want to acquire it unless absolutely necessary. For maintaining the
> device local open_count, we can use atomic operations on the counter
> itself, except when making the transition to/from 0. Here, we tackle the
> easy portion of delaying acquiring the drm_global_mutex for the final
> release by using atomic_dec_and_mutex_lock(), leaving the global
> serialisation across the device opens.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
For the series:
Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
Now the only remaining (though pre-existing) problem I can see is that
there is no corresponding mutex lock in drm_open() so that firstopen
might race with lastclose.. Or I might be missing something..
/Thomas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
2020-01-24 13:37 ` [Intel-gfx] " Thomas Hellström (VMware)
@ 2020-01-24 13:40 ` Chris Wilson
-1 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 13:40 UTC (permalink / raw)
To: Thomas Hellström (VMware), dri-devel; +Cc: intel-gfx
Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
> On 1/24/20 2:01 PM, Chris Wilson wrote:
> > Since drm_global_mutex is a true global mutex across devices, we don't
> > want to acquire it unless absolutely necessary. For maintaining the
> > device local open_count, we can use atomic operations on the counter
> > itself, except when making the transition to/from 0. Here, we tackle the
> > easy portion of delaying acquiring the drm_global_mutex for the final
> > release by using atomic_dec_and_mutex_lock(), leaving the global
> > serialisation across the device opens.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
>
> For the series:
>
> Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
>
> Now the only remaining (though pre-existing) problem I can see is that
> there is no corresponding mutex lock in drm_open() so that firstopen
> might race with lastclose.. Or I might be missing something..
iirc, it's a complicated dance where it goes through drm_stub_open()
first which acquires the drm_global_mutex.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
@ 2020-01-24 13:40 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 13:40 UTC (permalink / raw)
To: Thomas Hellström (VMware), dri-devel; +Cc: intel-gfx
Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
> On 1/24/20 2:01 PM, Chris Wilson wrote:
> > Since drm_global_mutex is a true global mutex across devices, we don't
> > want to acquire it unless absolutely necessary. For maintaining the
> > device local open_count, we can use atomic operations on the counter
> > itself, except when making the transition to/from 0. Here, we tackle the
> > easy portion of delaying acquiring the drm_global_mutex for the final
> > release by using atomic_dec_and_mutex_lock(), leaving the global
> > serialisation across the device opens.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
>
> For the series:
>
> Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
>
> Now the only remaining (though pre-existing) problem I can see is that
> there is no corresponding mutex lock in drm_open() so that firstopen
> might race with lastclose.. Or I might be missing something..
iirc, it's a complicated dance where it goes through drm_stub_open()
first which acquires the drm_global_mutex.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm: Release filp before global lock (rev2)
2020-01-24 12:56 ` [Intel-gfx] " Chris Wilson
(?)
(?)
@ 2020-01-24 18:21 ` Patchwork
-1 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2020-01-24 18:21 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm: Release filp before global lock (rev2)
URL : https://patchwork.freedesktop.org/series/72530/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7809 -> Patchwork_16254
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/index.html
Known issues
------------
Here are the changes found in Patchwork_16254 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_module_load@reload-with-fault-injection:
- fi-skl-lmem: [PASS][1] -> [INCOMPLETE][2] ([i915#671])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-skl-lmem/igt@i915_module_load@reload-with-fault-injection.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-skl-lmem/igt@i915_module_load@reload-with-fault-injection.html
* igt@prime_self_import@basic-llseek-bad:
- fi-tgl-y: [PASS][3] -> [DMESG-WARN][4] ([CI#94] / [i915#402]) +1 similar issue
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-tgl-y/igt@prime_self_import@basic-llseek-bad.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-tgl-y/igt@prime_self_import@basic-llseek-bad.html
#### Possible fixes ####
* igt@i915_getparams_basic@basic-eu-total:
- fi-tgl-y: [DMESG-WARN][5] ([CI#94] / [i915#402]) -> [PASS][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-tgl-y/igt@i915_getparams_basic@basic-eu-total.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-tgl-y/igt@i915_getparams_basic@basic-eu-total.html
* igt@i915_module_load@reload-with-fault-injection:
- fi-kbl-7500u: [INCOMPLETE][7] ([i915#879]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-kbl-7500u/igt@i915_module_load@reload-with-fault-injection.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-kbl-7500u/igt@i915_module_load@reload-with-fault-injection.html
* igt@i915_pm_rpm@module-reload:
- fi-kbl-x1275: [DMESG-WARN][9] ([i915#889]) -> [PASS][10] +1 similar issue
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
* igt@i915_selftest@live_blt:
- fi-hsw-4770: [DMESG-FAIL][11] ([i915#553] / [i915#725]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-hsw-4770/igt@i915_selftest@live_blt.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-hsw-4770/igt@i915_selftest@live_blt.html
* igt@i915_selftest@live_sanitycheck:
- fi-kbl-x1275: [INCOMPLETE][13] -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-kbl-x1275/igt@i915_selftest@live_sanitycheck.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-kbl-x1275/igt@i915_selftest@live_sanitycheck.html
* igt@kms_chamelium@hdmi-crc-fast:
- fi-skl-6700k2: [FAIL][15] ([i915#410]) -> [PASS][16] +3 similar issues
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-skl-6700k2/igt@kms_chamelium@hdmi-crc-fast.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-skl-6700k2/igt@kms_chamelium@hdmi-crc-fast.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: [FAIL][17] ([fdo#111096] / [i915#323]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
#### Warnings ####
* igt@gem_exec_parallel@contexts:
- fi-byt-j1900: [TIMEOUT][19] ([fdo#112271]) -> [FAIL][20] ([i915#694])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-byt-j1900/igt@gem_exec_parallel@contexts.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-byt-j1900/igt@gem_exec_parallel@contexts.html
* igt@i915_selftest@live_blt:
- fi-hsw-4770r: [DMESG-FAIL][21] ([i915#725]) -> [DMESG-FAIL][22] ([i915#553] / [i915#725])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-hsw-4770r/igt@i915_selftest@live_blt.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-hsw-4770r/igt@i915_selftest@live_blt.html
* igt@kms_chamelium@dp-hpd-fast:
- fi-skl-6700k2: [FAIL][23] ([i915#410]) -> [SKIP][24] ([fdo#109271]) +4 similar issues
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-skl-6700k2/igt@kms_chamelium@dp-hpd-fast.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-skl-6700k2/igt@kms_chamelium@dp-hpd-fast.html
* igt@kms_chamelium@vga-edid-read:
- fi-tgl-y: [SKIP][25] ([CI#94] / [fdo#111827] / [i915#1017]) -> [SKIP][26] ([CI#94] / [fdo#111827]) +8 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-tgl-y/igt@kms_chamelium@vga-edid-read.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-tgl-y/igt@kms_chamelium@vga-edid-read.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-y: [SKIP][27] ([CI#94] / [fdo#109285] / [i915#1017]) -> [SKIP][28] ([CI#94] / [fdo#109285])
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/fi-tgl-y/igt@kms_force_connector_basic@force-load-detect.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/fi-tgl-y/igt@kms_force_connector_basic@force-load-detect.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[CI#94]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/94
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
[fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
[i915#1017]: https://gitlab.freedesktop.org/drm/intel/issues/1017
[i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
[i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
[i915#410]: https://gitlab.freedesktop.org/drm/intel/issues/410
[i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
[i915#668]: https://gitlab.freedesktop.org/drm/intel/issues/668
[i915#671]: https://gitlab.freedesktop.org/drm/intel/issues/671
[i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
[i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
[i915#879]: https://gitlab.freedesktop.org/drm/intel/issues/879
[i915#889]: https://gitlab.freedesktop.org/drm/intel/issues/889
Participating hosts (51 -> 40)
------------------------------
Missing (11): fi-bdw-samus fi-bdw-5557u fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 fi-blb-e6850 fi-byt-n2820 fi-byt-clapper fi-skl-6600u
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_7809 -> Patchwork_16254
CI-20190529: 20190529
CI_DRM_7809: 861f608ce6e3c1a1ad320a5d18055601cff36e45 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5382: 8dbe5ce61baa2d563d4dd7c56a018bb1e1077467 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_16254: f92078334c36120e838d994cd16bf7d1d9907d09 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
f92078334c36 drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
d491a21e0356 drm: Release filp before global lock
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
2020-01-24 13:37 ` [Intel-gfx] " Thomas Hellström (VMware)
@ 2020-01-24 18:39 ` Chris Wilson
-1 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 18:39 UTC (permalink / raw)
To: Thomas Hellström (VMware), dri-devel; +Cc: intel-gfx
Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
> On 1/24/20 2:01 PM, Chris Wilson wrote:
> > Since drm_global_mutex is a true global mutex across devices, we don't
> > want to acquire it unless absolutely necessary. For maintaining the
> > device local open_count, we can use atomic operations on the counter
> > itself, except when making the transition to/from 0. Here, we tackle the
> > easy portion of delaying acquiring the drm_global_mutex for the final
> > release by using atomic_dec_and_mutex_lock(), leaving the global
> > serialisation across the device opens.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
>
> For the series:
>
> Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
Now being opt-in, it is fairly limited in scope and will not randomly
break others (touch wood) and the close() racing in BAT didn't throw
anything up, so pushed to drm-misc-next. Thanks for the review and
suggestions,
Next task is to suggest others might like to use it as well.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
@ 2020-01-24 18:39 ` Chris Wilson
0 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2020-01-24 18:39 UTC (permalink / raw)
To: Thomas Hellström (VMware), dri-devel; +Cc: intel-gfx
Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
> On 1/24/20 2:01 PM, Chris Wilson wrote:
> > Since drm_global_mutex is a true global mutex across devices, we don't
> > want to acquire it unless absolutely necessary. For maintaining the
> > device local open_count, we can use atomic operations on the counter
> > itself, except when making the transition to/from 0. Here, we tackle the
> > easy portion of delaying acquiring the drm_global_mutex for the final
> > release by using atomic_dec_and_mutex_lock(), leaving the global
> > serialisation across the device opens.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
>
> For the series:
>
> Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
Now being opt-in, it is fairly limited in scope and will not randomly
break others (touch wood) and the close() racing in BAT didn't throw
anything up, so pushed to drm-misc-next. Thanks for the review and
suggestions,
Next task is to suggest others might like to use it as well.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
2020-01-24 18:39 ` [Intel-gfx] " Chris Wilson
@ 2020-01-24 20:32 ` Thomas Hellström (VMware)
-1 siblings, 0 replies; 20+ messages in thread
From: Thomas Hellström (VMware) @ 2020-01-24 20:32 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: intel-gfx
On 1/24/20 7:39 PM, Chris Wilson wrote:
> Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
>> On 1/24/20 2:01 PM, Chris Wilson wrote:
>>> Since drm_global_mutex is a true global mutex across devices, we don't
>>> want to acquire it unless absolutely necessary. For maintaining the
>>> device local open_count, we can use atomic operations on the counter
>>> itself, except when making the transition to/from 0. Here, we tackle the
>>> easy portion of delaying acquiring the drm_global_mutex for the final
>>> release by using atomic_dec_and_mutex_lock(), leaving the global
>>> serialisation across the device opens.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
>> For the series:
>>
>> Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
> Now being opt-in, it is fairly limited in scope and will not randomly
> break others (touch wood) and the close() racing in BAT didn't throw
> anything up, so pushed to drm-misc-next. Thanks for the review and
> suggestions,
>
> Next task is to suggest others might like to use it as well.
> -Chris
Thanks. I'll look at doing the same for those drivers I audited.
/Thomas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
@ 2020-01-24 20:32 ` Thomas Hellström (VMware)
0 siblings, 0 replies; 20+ messages in thread
From: Thomas Hellström (VMware) @ 2020-01-24 20:32 UTC (permalink / raw)
To: Chris Wilson, dri-devel; +Cc: intel-gfx
On 1/24/20 7:39 PM, Chris Wilson wrote:
> Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
>> On 1/24/20 2:01 PM, Chris Wilson wrote:
>>> Since drm_global_mutex is a true global mutex across devices, we don't
>>> want to acquire it unless absolutely necessary. For maintaining the
>>> device local open_count, we can use atomic operations on the counter
>>> itself, except when making the transition to/from 0. Here, we tackle the
>>> easy portion of delaying acquiring the drm_global_mutex for the final
>>> release by using atomic_dec_and_mutex_lock(), leaving the global
>>> serialisation across the device opens.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
>> For the series:
>>
>> Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
> Now being opt-in, it is fairly limited in scope and will not randomly
> break others (touch wood) and the close() racing in BAT didn't throw
> anything up, so pushed to drm-misc-next. Thanks for the review and
> suggestions,
>
> Next task is to suggest others might like to use it as well.
> -Chris
Thanks. I'll look at doing the same for those drivers I audited.
/Thomas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] drm: Release filp before global lock (rev2)
2020-01-24 12:56 ` [Intel-gfx] " Chris Wilson
` (2 preceding siblings ...)
(?)
@ 2020-01-26 23:33 ` Patchwork
-1 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2020-01-26 23:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm: Release filp before global lock (rev2)
URL : https://patchwork.freedesktop.org/series/72530/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7809_full -> Patchwork_16254_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_16254_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_isolation@vcs1-reset:
- shard-iclb: [PASS][1] -> [SKIP][2] ([fdo#109276] / [fdo#112080])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb1/igt@gem_ctx_isolation@vcs1-reset.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb8/igt@gem_ctx_isolation@vcs1-reset.html
* igt@gem_exec_schedule@out-order-bsd2:
- shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#109276]) +14 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb4/igt@gem_exec_schedule@out-order-bsd2.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb7/igt@gem_exec_schedule@out-order-bsd2.html
* igt@gem_exec_schedule@pi-distinct-iova-bsd:
- shard-iclb: [PASS][5] -> [SKIP][6] ([i915#677]) +1 similar issue
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb7/igt@gem_exec_schedule@pi-distinct-iova-bsd.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb2/igt@gem_exec_schedule@pi-distinct-iova-bsd.html
* igt@gem_exec_schedule@preempt-self-bsd:
- shard-iclb: [PASS][7] -> [SKIP][8] ([fdo#112146]) +4 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb6/igt@gem_exec_schedule@preempt-self-bsd.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb1/igt@gem_exec_schedule@preempt-self-bsd.html
* igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
- shard-hsw: [PASS][9] -> [FAIL][10] ([i915#520])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-hsw5/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-hsw6/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
* igt@gem_persistent_relocs@forked-interruptible-thrashing:
- shard-apl: [PASS][11] -> [FAIL][12] ([i915#520])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-apl2/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-apl3/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
* igt@gem_workarounds@suspend-resume-fd:
- shard-apl: [PASS][13] -> [DMESG-WARN][14] ([i915#180])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-apl4/igt@gem_workarounds@suspend-resume-fd.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-apl6/igt@gem_workarounds@suspend-resume-fd.html
* igt@i915_selftest@mock_requests:
- shard-glk: [PASS][15] -> [INCOMPLETE][16] ([i915#58] / [k.org#198133])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-glk8/igt@i915_selftest@mock_requests.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-glk2/igt@i915_selftest@mock_requests.html
- shard-apl: [PASS][17] -> [INCOMPLETE][18] ([fdo#103927])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-apl6/igt@i915_selftest@mock_requests.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-apl2/igt@i915_selftest@mock_requests.html
- shard-kbl: [PASS][19] -> [INCOMPLETE][20] ([fdo#103665])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-kbl3/igt@i915_selftest@mock_requests.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-kbl4/igt@i915_selftest@mock_requests.html
* igt@kms_cursor_legacy@cursor-vs-flip-toggle:
- shard-hsw: [PASS][21] -> [FAIL][22] ([i915#57])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-hsw1/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
* igt@kms_flip@flip-vs-expired-vblank:
- shard-skl: [PASS][23] -> [FAIL][24] ([i915#79])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl5/igt@kms_flip@flip-vs-expired-vblank.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl3/igt@kms_flip@flip-vs-expired-vblank.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-blt:
- shard-tglb: [PASS][25] -> [FAIL][26] ([i915#49]) +4 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-tglb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-blt.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-blt.html
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
- shard-skl: [PASS][27] -> [FAIL][28] ([fdo#108145]) +1 similar issue
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
* igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
- shard-skl: [PASS][29] -> [FAIL][30] ([fdo#108145] / [i915#265])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
* igt@kms_psr@no_drrs:
- shard-iclb: [PASS][31] -> [FAIL][32] ([i915#173])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb2/igt@kms_psr@no_drrs.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb1/igt@kms_psr@no_drrs.html
* igt@kms_psr@psr2_cursor_blt:
- shard-iclb: [PASS][33] -> [SKIP][34] ([fdo#109441]) +2 similar issues
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb3/igt@kms_psr@psr2_cursor_blt.html
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-kbl: [PASS][35] -> [DMESG-WARN][36] ([i915#180]) +5 similar issues
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-kbl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-kbl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
* igt@kms_vblank@pipe-c-ts-continuation-suspend:
- shard-skl: [PASS][37] -> [INCOMPLETE][38] ([i915#69])
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl10/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl6/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
* igt@perf_pmu@busy-vcs1:
- shard-iclb: [PASS][39] -> [SKIP][40] ([fdo#112080]) +6 similar issues
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb2/igt@perf_pmu@busy-vcs1.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb3/igt@perf_pmu@busy-vcs1.html
* igt@prime_mmap_coherency@ioctl-errors:
- shard-hsw: [PASS][41] -> [FAIL][42] ([i915#831])
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-hsw2/igt@prime_mmap_coherency@ioctl-errors.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-hsw2/igt@prime_mmap_coherency@ioctl-errors.html
#### Possible fixes ####
* igt@gem_ctx_isolation@bcs0-s3:
- shard-skl: [INCOMPLETE][43] ([i915#69]) -> [PASS][44]
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl6/igt@gem_ctx_isolation@bcs0-s3.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl2/igt@gem_ctx_isolation@bcs0-s3.html
* igt@gem_ctx_persistence@vcs1-queued:
- shard-iclb: [SKIP][45] ([fdo#109276] / [fdo#112080]) -> [PASS][46] +3 similar issues
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb8/igt@gem_ctx_persistence@vcs1-queued.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb1/igt@gem_ctx_persistence@vcs1-queued.html
* igt@gem_ctx_persistence@vecs0-mixed-process:
- shard-glk: [FAIL][47] ([i915#679]) -> [PASS][48]
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-glk4/igt@gem_ctx_persistence@vecs0-mixed-process.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-glk1/igt@gem_ctx_persistence@vecs0-mixed-process.html
- shard-skl: [FAIL][49] ([i915#679]) -> [PASS][50]
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl10/igt@gem_ctx_persistence@vecs0-mixed-process.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl4/igt@gem_ctx_persistence@vecs0-mixed-process.html
* igt@gem_ctx_shared@exec-single-timeline-bsd:
- shard-iclb: [SKIP][51] ([fdo#110841]) -> [PASS][52]
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb4/igt@gem_ctx_shared@exec-single-timeline-bsd.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb6/igt@gem_ctx_shared@exec-single-timeline-bsd.html
* igt@gem_exec_balancer@smoke:
- shard-iclb: [SKIP][53] ([fdo#110854]) -> [PASS][54]
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb7/igt@gem_exec_balancer@smoke.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb2/igt@gem_exec_balancer@smoke.html
* igt@gem_exec_flush@basic-batch-kernel-default-cmd:
- shard-glk: [INCOMPLETE][55] ([i915#58] / [k.org#198133]) -> [PASS][56]
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-glk9/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-glk2/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html
* igt@gem_exec_schedule@promotion-bsd:
- shard-iclb: [SKIP][57] ([fdo#112146]) -> [PASS][58] +3 similar issues
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb2/igt@gem_exec_schedule@promotion-bsd.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb3/igt@gem_exec_schedule@promotion-bsd.html
* igt@gem_exec_store@cachelines-vcs1:
- shard-iclb: [SKIP][59] ([fdo#112080]) -> [PASS][60] +3 similar issues
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb7/igt@gem_exec_store@cachelines-vcs1.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb2/igt@gem_exec_store@cachelines-vcs1.html
* igt@gem_ppgtt@flink-and-close-vma-leak:
- shard-glk: [FAIL][61] ([i915#644]) -> [PASS][62]
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-glk5/igt@gem_ppgtt@flink-and-close-vma-leak.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-glk3/igt@gem_ppgtt@flink-and-close-vma-leak.html
* igt@gen7_exec_parse@basic-offset:
- shard-hsw: [FAIL][63] ([i915#694]) -> [PASS][64]
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-hsw1/igt@gen7_exec_parse@basic-offset.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-hsw1/igt@gen7_exec_parse@basic-offset.html
* igt@i915_pm_rps@waitboost:
- shard-iclb: [FAIL][65] ([i915#413]) -> [PASS][66]
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb3/igt@i915_pm_rps@waitboost.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb7/igt@i915_pm_rps@waitboost.html
* igt@i915_selftest@mock_requests:
- shard-tglb: [INCOMPLETE][67] ([i915#472]) -> [PASS][68]
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-tglb7/igt@i915_selftest@mock_requests.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-tglb3/igt@i915_selftest@mock_requests.html
* igt@kms_color@pipe-b-ctm-0-5:
- shard-skl: [DMESG-WARN][69] ([i915#109]) -> [PASS][70]
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl8/igt@kms_color@pipe-b-ctm-0-5.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl7/igt@kms_color@pipe-b-ctm-0-5.html
* igt@kms_cursor_crc@pipe-a-cursor-suspend:
- shard-kbl: [DMESG-WARN][71] ([i915#180]) -> [PASS][72] +4 similar issues
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-kbl2/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-kbl2/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
* igt@kms_draw_crc@draw-method-xrgb8888-render-ytiled:
- shard-skl: [FAIL][73] ([i915#52] / [i915#54]) -> [PASS][74]
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-skl10/igt@kms_draw_crc@draw-method-xrgb8888-render-ytiled.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-skl5/igt@kms_draw_crc@draw-method-xrgb8888-render-ytiled.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-glk: [FAIL][75] ([i915#79]) -> [PASS][76]
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-glk2/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-glk9/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
* igt@kms_flip@flip-vs-suspend:
- shard-apl: [DMESG-WARN][77] ([i915#180]) -> [PASS][78]
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-apl8/igt@kms_flip@flip-vs-suspend.html
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-apl1/igt@kms_flip@flip-vs-suspend.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render:
- shard-tglb: [FAIL][79] ([i915#49]) -> [PASS][80] +4 similar issues
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render.html
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-tglb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-render.html
* igt@kms_psr@psr2_sprite_plane_move:
- shard-iclb: [SKIP][81] ([fdo#109441]) -> [PASS][82] +2 similar issues
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb7/igt@kms_psr@psr2_sprite_plane_move.html
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
* igt@perf@oa-exponents:
- shard-glk: [FAIL][83] ([i915#84]) -> [PASS][84]
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-glk8/igt@perf@oa-exponents.html
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-glk9/igt@perf@oa-exponents.html
* igt@prime_busy@hang-bsd2:
- shard-iclb: [SKIP][85] ([fdo#109276]) -> [PASS][86] +14 similar issues
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb7/igt@prime_busy@hang-bsd2.html
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb2/igt@prime_busy@hang-bsd2.html
#### Warnings ####
* igt@gem_ctx_isolation@vcs1-nonpriv-switch:
- shard-iclb: [FAIL][87] ([IGT#28]) -> [SKIP][88] ([fdo#109276] / [fdo#112080])
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb2/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb3/igt@gem_ctx_isolation@vcs1-nonpriv-switch.html
* igt@gem_tiled_blits@normal:
- shard-hsw: [FAIL][89] ([i915#694]) -> [FAIL][90] ([i915#818])
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-hsw7/igt@gem_tiled_blits@normal.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-hsw1/igt@gem_tiled_blits@normal.html
* igt@i915_selftest@live_blt:
- shard-hsw: [DMESG-FAIL][91] ([i915#563]) -> [DMESG-FAIL][92] ([i915#725])
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-hsw2/igt@i915_selftest@live_blt.html
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-hsw7/igt@i915_selftest@live_blt.html
* igt@kms_dp_dsc@basic-dsc-enable-edp:
- shard-iclb: [DMESG-WARN][93] ([fdo#107724]) -> [SKIP][94] ([fdo#109349])
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7809/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/shard-iclb5/igt@kms_dp_dsc@basic-dsc-enable-edp.html
[IGT#28]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/28
[fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
[fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
[fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
[fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146
[i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
[i915#173]: https://gitlab.freedesktop.org/drm/intel/issues/173
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
[i915#413]: https://gitlab.freedesktop.org/drm/intel/issues/413
[i915#472]: https://gitlab.freedesktop.org/drm/intel/issues/472
[i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49
[i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
[i915#520]: https://gitlab.freedesktop.org/drm/intel/issues/520
[i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
[i915#563]: https://gitlab.freedesktop.org/drm/intel/issues/563
[i915#57]: https://gitlab.freedesktop.org/drm/intel/issues/57
[i915#58]: https://gitlab.freedesktop.org/drm/intel/issues/58
[i915#644]: https://gitlab.freedesktop.org/drm/intel/issues/644
[i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677
[i915#679]: https://gitlab.freedesktop.org/drm/intel/issues/679
[i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
[i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
[i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
[i915#818]: https://gitlab.freedesktop.org/drm/intel/issues/818
[i915#831]: https://gitlab.freedesktop.org/drm/intel/issues/831
[i915#84]: https://gitlab.freedesktop.org/drm/intel/issues/84
[k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133
Participating hosts (10 -> 10)
------------------------------
No changes in participating hosts
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_7809 -> Patchwork_16254
CI-20190529: 20190529
CI_DRM_7809: 861f608ce6e3c1a1ad320a5d18055601cff36e45 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5382: 8dbe5ce61baa2d563d4dd7c56a018bb1e1077467 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_16254: f92078334c36120e838d994cd16bf7d1d9907d09 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16254/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
2020-01-24 18:39 ` [Intel-gfx] " Chris Wilson
@ 2020-01-27 9:19 ` Daniel Vetter
-1 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2020-01-27 9:19 UTC (permalink / raw)
To: Chris Wilson; +Cc: Thomas Hellström (VMware), intel-gfx, dri-devel
On Fri, Jan 24, 2020 at 06:39:26PM +0000, Chris Wilson wrote:
> Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
> > On 1/24/20 2:01 PM, Chris Wilson wrote:
> > > Since drm_global_mutex is a true global mutex across devices, we don't
> > > want to acquire it unless absolutely necessary. For maintaining the
> > > device local open_count, we can use atomic operations on the counter
> > > itself, except when making the transition to/from 0. Here, we tackle the
> > > easy portion of delaying acquiring the drm_global_mutex for the final
> > > release by using atomic_dec_and_mutex_lock(), leaving the global
> > > serialisation across the device opens.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
> >
> > For the series:
> >
> > Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
>
> Now being opt-in, it is fairly limited in scope and will not randomly
> break others (touch wood) and the close() racing in BAT didn't throw
> anything up, so pushed to drm-misc-next. Thanks for the review and
> suggestions,
Yeah this version looks reasonable compared to the previous few (I'm
catching up on dri-devel). I've looked at getting rid of the global_mutex,
and all I have is a simple patch with a pile of notes. It's real nasty.
This one here is a neat trick that I missed, and I'm semi-convinced it's safe :-)
> Next task is to suggest others might like to use it as well.
My idea for the opt-in was to look at whether ->load/->unload exists. And
ofc not bother with any of this for DRIVER_LEGACY. So maybe next step
would be to define a
drm_can_noglobal()
{
return !DRIVER_LEGACY && !->load && !->unload;
}
and inline the close helper again and see what breaks? At least from what
I've looked trying to duplicate paths and opt-in is going to be real tough
on the open side of things. Best I've done thus far is minor pushing of
the critical section.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count
@ 2020-01-27 9:19 ` Daniel Vetter
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2020-01-27 9:19 UTC (permalink / raw)
To: Chris Wilson; +Cc: Thomas Hellström (VMware), intel-gfx, dri-devel
On Fri, Jan 24, 2020 at 06:39:26PM +0000, Chris Wilson wrote:
> Quoting Thomas Hellström (VMware) (2020-01-24 13:37:47)
> > On 1/24/20 2:01 PM, Chris Wilson wrote:
> > > Since drm_global_mutex is a true global mutex across devices, we don't
> > > want to acquire it unless absolutely necessary. For maintaining the
> > > device local open_count, we can use atomic operations on the counter
> > > itself, except when making the transition to/from 0. Here, we tackle the
> > > easy portion of delaying acquiring the drm_global_mutex for the final
> > > release by using atomic_dec_and_mutex_lock(), leaving the global
> > > serialisation across the device opens.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
> >
> > For the series:
> >
> > Reviewed-by: Thomas Hellström <thellstrom@vmware.com>
>
> Now being opt-in, it is fairly limited in scope and will not randomly
> break others (touch wood) and the close() racing in BAT didn't throw
> anything up, so pushed to drm-misc-next. Thanks for the review and
> suggestions,
Yeah this version looks reasonable compared to the previous few (I'm
catching up on dri-devel). I've looked at getting rid of the global_mutex,
and all I have is a simple patch with a pile of notes. It's real nasty.
This one here is a neat trick that I missed, and I'm semi-convinced it's safe :-)
> Next task is to suggest others might like to use it as well.
My idea for the opt-in was to look at whether ->load/->unload exists. And
ofc not bother with any of this for DRIVER_LEGACY. So maybe next step
would be to define a
drm_can_noglobal()
{
return !DRIVER_LEGACY && !->load && !->unload;
}
and inline the close helper again and see what breaks? At least from what
I've looked trying to duplicate paths and opt-in is going to be real tough
on the open side of things. Best I've done thus far is minor pushing of
the critical section.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] drm: Release filp before global lock
2020-01-24 12:56 ` [Intel-gfx] " Chris Wilson
@ 2020-01-27 9:27 ` Daniel Vetter
-1 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2020-01-27 9:27 UTC (permalink / raw)
To: Chris Wilson; +Cc: daniel.vetter, intel-gfx, Thomas Hellström, dri-devel
On Fri, Jan 24, 2020 at 12:56:26PM +0000, Chris Wilson wrote:
> The file is not part of the global drm resource and can be released
> prior to take the global mutex to drop the open_count (and potentially
> close) the drm device. As the global mutex is indeed global, not only
> within the device but across devices, a slow file release mechanism can
> bottleneck the entire system.
>
> However, inside drm_close_helper() there are a number of dev->driver
> callbacks that take the drm_device as the first parameter... Worryingly
> some of those callbacks may be (implicitly) depending on the global
> mutex.
>
> v2: Drop the debug message for the open-count, it's included with the
> drm_file_free() debug message -- and for good measure make that up as
> reading outside of the mutex.
>
> v3: Separate the calling of the filp cleanup outside of
> drm_global_mutex into a new drm_release_noglobal() hook, so that we can
> phase the transition. drm/savage relies on the global mutex, and there
> may be more, so be cautious.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
> Acked-by: Thomas Hellström (VMware) <thomas_os@shipmail.org>
> ---
> drivers/gpu/drm/drm_file.c | 36 ++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> include/drm/drm_file.h | 1 +
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 92d16724f949..e25306c49cc6 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
> DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
> task_pid_nr(current),
> (long)old_encode_dev(file->minor->kdev->devt),
> - dev->open_count);
> + READ_ONCE(dev->open_count));
>
> if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
> dev->driver->preclose)
> @@ -455,6 +455,40 @@ int drm_release(struct inode *inode, struct file *filp)
> }
> EXPORT_SYMBOL(drm_release);
>
> +/**
> + * drm_release_noglobal - release method for DRM file
> + * @inode: device inode
> + * @filp: file pointer.
> + *
> + * This function may be used by drivers as their &file_operations.release
> + * method. It frees any resources associated with the open file prior to taking
> + * the drm_global_mutex, which then calls the &drm_driver.postclose driver
> + * callback. If this is the last open file for the DRM device also proceeds to
> + * call the &drm_driver.lastclose driver callback.
> + *
> + * RETURNS:
> + *
> + * Always succeeds and returns 0.
> + */
> +int drm_release_noglobal(struct inode *inode, struct file *filp)
> +{
> + struct drm_file *file_priv = filp->private_data;
> + struct drm_minor *minor = file_priv->minor;
> + struct drm_device *dev = minor->dev;
> +
> + drm_close_helper(filp);
> +
> + mutex_lock(&drm_global_mutex);
> + if (!--dev->open_count)
> + drm_lastclose(dev);
> + mutex_unlock(&drm_global_mutex);
btw my rough idea for lastclose is that we're just going to make it racy,
and then use the master lock and drm_client infrastructure to handle
fights between fbcon and a restarted compositor. That's already how that's
handled everywhere else. We might need to cut over drivers to the generic
fbcon stuff (or at least steal parts of the drm_client stuff I think), but
not sure.
-Daniel
> +
> + drm_minor_release(minor);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_release_noglobal);
> +
> /**
> * drm_read - read method for DRM file
> * @filp: file pointer
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e9b42e962032..5a5846d892f4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2673,7 +2673,7 @@ const struct dev_pm_ops i915_pm_ops = {
> static const struct file_operations i915_driver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> - .release = drm_release,
> + .release = drm_release_noglobal,
> .unlocked_ioctl = drm_ioctl,
> .mmap = i915_gem_mmap,
> .poll = drm_poll,
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 8b099b347817..19df8028a6c4 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -374,6 +374,7 @@ int drm_open(struct inode *inode, struct file *filp);
> ssize_t drm_read(struct file *filp, char __user *buffer,
> size_t count, loff_t *offset);
> int drm_release(struct inode *inode, struct file *filp);
> +int drm_release_noglobal(struct inode *inode, struct file *filp);
> __poll_t drm_poll(struct file *filp, struct poll_table_struct *wait);
> int drm_event_reserve_init_locked(struct drm_device *dev,
> struct drm_file *file_priv,
> --
> 2.25.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Intel-gfx] [PATCH 1/2] drm: Release filp before global lock
@ 2020-01-27 9:27 ` Daniel Vetter
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2020-01-27 9:27 UTC (permalink / raw)
To: Chris Wilson; +Cc: daniel.vetter, intel-gfx, Thomas Hellström, dri-devel
On Fri, Jan 24, 2020 at 12:56:26PM +0000, Chris Wilson wrote:
> The file is not part of the global drm resource and can be released
> prior to take the global mutex to drop the open_count (and potentially
> close) the drm device. As the global mutex is indeed global, not only
> within the device but across devices, a slow file release mechanism can
> bottleneck the entire system.
>
> However, inside drm_close_helper() there are a number of dev->driver
> callbacks that take the drm_device as the first parameter... Worryingly
> some of those callbacks may be (implicitly) depending on the global
> mutex.
>
> v2: Drop the debug message for the open-count, it's included with the
> drm_file_free() debug message -- and for good measure make that up as
> reading outside of the mutex.
>
> v3: Separate the calling of the filp cleanup outside of
> drm_global_mutex into a new drm_release_noglobal() hook, so that we can
> phase the transition. drm/savage relies on the global mutex, and there
> may be more, so be cautious.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Thomas Hellström (VMware) <thomas_os@shipmail.org>
> Acked-by: Thomas Hellström (VMware) <thomas_os@shipmail.org>
> ---
> drivers/gpu/drm/drm_file.c | 36 ++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> include/drm/drm_file.h | 1 +
> 3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 92d16724f949..e25306c49cc6 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -220,7 +220,7 @@ void drm_file_free(struct drm_file *file)
> DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n",
> task_pid_nr(current),
> (long)old_encode_dev(file->minor->kdev->devt),
> - dev->open_count);
> + READ_ONCE(dev->open_count));
>
> if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
> dev->driver->preclose)
> @@ -455,6 +455,40 @@ int drm_release(struct inode *inode, struct file *filp)
> }
> EXPORT_SYMBOL(drm_release);
>
> +/**
> + * drm_release_noglobal - release method for DRM file
> + * @inode: device inode
> + * @filp: file pointer.
> + *
> + * This function may be used by drivers as their &file_operations.release
> + * method. It frees any resources associated with the open file prior to taking
> + * the drm_global_mutex, which then calls the &drm_driver.postclose driver
> + * callback. If this is the last open file for the DRM device also proceeds to
> + * call the &drm_driver.lastclose driver callback.
> + *
> + * RETURNS:
> + *
> + * Always succeeds and returns 0.
> + */
> +int drm_release_noglobal(struct inode *inode, struct file *filp)
> +{
> + struct drm_file *file_priv = filp->private_data;
> + struct drm_minor *minor = file_priv->minor;
> + struct drm_device *dev = minor->dev;
> +
> + drm_close_helper(filp);
> +
> + mutex_lock(&drm_global_mutex);
> + if (!--dev->open_count)
> + drm_lastclose(dev);
> + mutex_unlock(&drm_global_mutex);
btw my rough idea for lastclose is that we're just going to make it racy,
and then use the master lock and drm_client infrastructure to handle
fights between fbcon and a restarted compositor. That's already how that's
handled everywhere else. We might need to cut over drivers to the generic
fbcon stuff (or at least steal parts of the drm_client stuff I think), but
not sure.
-Daniel
> +
> + drm_minor_release(minor);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_release_noglobal);
> +
> /**
> * drm_read - read method for DRM file
> * @filp: file pointer
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e9b42e962032..5a5846d892f4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2673,7 +2673,7 @@ const struct dev_pm_ops i915_pm_ops = {
> static const struct file_operations i915_driver_fops = {
> .owner = THIS_MODULE,
> .open = drm_open,
> - .release = drm_release,
> + .release = drm_release_noglobal,
> .unlocked_ioctl = drm_ioctl,
> .mmap = i915_gem_mmap,
> .poll = drm_poll,
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 8b099b347817..19df8028a6c4 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -374,6 +374,7 @@ int drm_open(struct inode *inode, struct file *filp);
> ssize_t drm_read(struct file *filp, char __user *buffer,
> size_t count, loff_t *offset);
> int drm_release(struct inode *inode, struct file *filp);
> +int drm_release_noglobal(struct inode *inode, struct file *filp);
> __poll_t drm_poll(struct file *filp, struct poll_table_struct *wait);
> int drm_event_reserve_init_locked(struct drm_device *dev,
> struct drm_file *file_priv,
> --
> 2.25.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-01-27 9:27 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 12:56 [PATCH 1/2] drm: Release filp before global lock Chris Wilson
2020-01-24 12:56 ` [Intel-gfx] " Chris Wilson
2020-01-24 12:56 ` [PATCH 2/2] drm: Avoid drm_global_mutex for simple inc/dec of dev->open_count Chris Wilson
2020-01-24 12:56 ` [Intel-gfx] " Chris Wilson
2020-01-24 13:01 ` [PATCH] " Chris Wilson
2020-01-24 13:01 ` [Intel-gfx] " Chris Wilson
2020-01-24 13:37 ` Thomas Hellström (VMware)
2020-01-24 13:37 ` [Intel-gfx] " Thomas Hellström (VMware)
2020-01-24 13:40 ` Chris Wilson
2020-01-24 13:40 ` [Intel-gfx] " Chris Wilson
2020-01-24 18:39 ` Chris Wilson
2020-01-24 18:39 ` [Intel-gfx] " Chris Wilson
2020-01-24 20:32 ` Thomas Hellström (VMware)
2020-01-24 20:32 ` [Intel-gfx] " Thomas Hellström (VMware)
2020-01-27 9:19 ` Daniel Vetter
2020-01-27 9:19 ` Daniel Vetter
2020-01-24 18:21 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm: Release filp before global lock (rev2) Patchwork
2020-01-26 23:33 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-01-27 9:27 ` [PATCH 1/2] drm: Release filp before global lock Daniel Vetter
2020-01-27 9:27 ` [Intel-gfx] " Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.