vboxvideo doesn't use dev->struct_mutex and therefore has no need to use gem_free_object.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Hans de Goede hdegoede@redhat.com Cc: Michael Thayer michael.thayer@oracle.com Cc: Colin Ian King colin.king@canonical.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Stephen Rothwell sfr@canb.auug.org.au --- drivers/staging/vboxvideo/vbox_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c index e18642e5027e..f6d26beffa54 100644 --- a/drivers/staging/vboxvideo/vbox_drv.c +++ b/drivers/staging/vboxvideo/vbox_drv.c @@ -242,7 +242,7 @@ static struct drm_driver driver = { .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL,
- .gem_free_object = vbox_gem_free_object, + .gem_free_object_unlocked = vbox_gem_free_object, .dumb_create = vbox_dumb_create, .dumb_map_offset = vbox_dumb_mmap_offset, .dumb_destroy = drm_gem_dumb_destroy,
Just want to clean out all grep hits. gem_free_object is deprecated.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sandy Huang hjc@rock-chips.com Cc: "Heiko Stübner" heiko@sntech.de Cc: linux-arm-kernel@lists.infradead.org Cc: linux-rockchip@lists.infradead.org --- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c index 074db7a92809..a8db758d523e 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c @@ -357,8 +357,8 @@ rockchip_gem_create_object(struct drm_device *drm, unsigned int size, }
/* - * rockchip_gem_free_object - (struct drm_driver)->gem_free_object callback - * function + * rockchip_gem_free_object - (struct drm_driver)->gem_free_object_unlocked + * callback function */ void rockchip_gem_free_object(struct drm_gem_object *obj) {
On Tue, Mar 27, 2018 at 10:23:53AM +0200, Daniel Vetter wrote:
Just want to clean out all grep hits. gem_free_object is deprecated.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Sean Paul seanpaul@chromium.org
Cc: Sandy Huang hjc@rock-chips.com Cc: "Heiko Stübner" heiko@sntech.de Cc: linux-arm-kernel@lists.infradead.org Cc: linux-rockchip@lists.infradead.org
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c index 074db7a92809..a8db758d523e 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c @@ -357,8 +357,8 @@ rockchip_gem_create_object(struct drm_device *drm, unsigned int size, }
/*
- rockchip_gem_free_object - (struct drm_driver)->gem_free_object callback
- function
- rockchip_gem_free_object - (struct drm_driver)->gem_free_object_unlocked
*/
- callback function
void rockchip_gem_free_object(struct drm_gem_object *obj) { -- 2.16.2
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
It's only used to protect our page list, and only when we know we have a full reference. This means none of these code paths can ever race with the final unref, and hence we do not need dev->struct_mutex serialization and can simply switch to our own locking.
For more context the only magic the locked gem_free_object provides is that it prevents concurrent final unref (and destruction) of gem objects while anyone is holding dev->struct_mutex. This was used by i915 (and other drivers) to implement eviction handling with less headaches.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/udl/udl_dmabuf.c | 5 +++-- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 2 ++ drivers/gpu/drm/udl/udl_gem.c | 5 +++-- drivers/gpu/drm/udl/udl_main.c | 2 ++ 5 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 2867ed155ff6..0a20695eb120 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -76,6 +76,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach, struct udl_drm_dmabuf_attachment *udl_attach = attach->priv; struct udl_gem_object *obj = to_udl_bo(attach->dmabuf->priv); struct drm_device *dev = obj->base.dev; + struct udl_device *udl = dev->dev_private; struct scatterlist *rd, *wr; struct sg_table *sgt = NULL; unsigned int i; @@ -112,7 +113,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach, return ERR_PTR(-ENOMEM); }
- mutex_lock(&dev->struct_mutex); + mutex_lock(&udl->gem_lock);
rd = obj->sg->sgl; wr = sgt->sgl; @@ -137,7 +138,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach, attach->priv = udl_attach;
err_unlock: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&udl->gem_lock); return sgt; }
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 3c45a3064726..9ef515df724b 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -53,7 +53,7 @@ static struct drm_driver driver = { .unload = udl_driver_unload,
/* gem hooks */ - .gem_free_object = udl_gem_free_object, + .gem_free_object_unlocked = udl_gem_free_object, .gem_vm_ops = &udl_gem_vm_ops,
.dumb_create = udl_dumb_create, diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 2a75ab80527a..55c0cc309198 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -54,6 +54,8 @@ struct udl_device { struct usb_device *udev; struct drm_crtc *crtc;
+ struct mutex gem_lock; + int sku_pixel_limit;
struct urb_list urbs; diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index dee6bd9a3dd1..9a15cce22cce 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -214,9 +214,10 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, { struct udl_gem_object *gobj; struct drm_gem_object *obj; + struct udl_device *udl = dev->dev_private; int ret = 0;
- mutex_lock(&dev->struct_mutex); + mutex_lock(&udl->gem_lock); obj = drm_gem_object_lookup(file, handle); if (obj == NULL) { ret = -ENOENT; @@ -236,6 +237,6 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, out: drm_gem_object_put(&gobj->base); unlock: - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&udl->gem_lock); return ret; } diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index f1ec4528a73e..d518de8f496b 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -324,6 +324,8 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags) udl->ddev = dev; dev->dev_private = udl;
+ mutex_init(&udl->gem_lock); + if (!udl_parse_vendor_descriptor(dev, udl->udev)) { ret = -ENODEV; DRM_ERROR("firmware not recognized. Assume incompatible device\n");
On Tue, Mar 27, 2018 at 10:23:54AM +0200, Daniel Vetter wrote:
It's only used to protect our page list, and only when we know we have a full reference. This means none of these code paths can ever race with the final unref, and hence we do not need dev->struct_mutex serialization and can simply switch to our own locking.
For more context the only magic the locked gem_free_object provides is that it prevents concurrent final unref (and destruction) of gem objects while anyone is holding dev->struct_mutex. This was used by i915 (and other drivers) to implement eviction handling with less headaches.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Reviewed-by: Sean Paul seanpaul@chromium.org
Cc: Dave Airlie airlied@redhat.com
drivers/gpu/drm/udl/udl_dmabuf.c | 5 +++-- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 2 ++ drivers/gpu/drm/udl/udl_gem.c | 5 +++-- drivers/gpu/drm/udl/udl_main.c | 2 ++ 5 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 2867ed155ff6..0a20695eb120 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -76,6 +76,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach, struct udl_drm_dmabuf_attachment *udl_attach = attach->priv; struct udl_gem_object *obj = to_udl_bo(attach->dmabuf->priv); struct drm_device *dev = obj->base.dev;
- struct udl_device *udl = dev->dev_private; struct scatterlist *rd, *wr; struct sg_table *sgt = NULL; unsigned int i;
@@ -112,7 +113,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach, return ERR_PTR(-ENOMEM); }
- mutex_lock(&dev->struct_mutex);
mutex_lock(&udl->gem_lock);
rd = obj->sg->sgl; wr = sgt->sgl;
@@ -137,7 +138,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach, attach->priv = udl_attach;
err_unlock:
- mutex_unlock(&dev->struct_mutex);
- mutex_unlock(&udl->gem_lock); return sgt;
}
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 3c45a3064726..9ef515df724b 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -53,7 +53,7 @@ static struct drm_driver driver = { .unload = udl_driver_unload,
/* gem hooks */
- .gem_free_object = udl_gem_free_object,
.gem_free_object_unlocked = udl_gem_free_object, .gem_vm_ops = &udl_gem_vm_ops,
.dumb_create = udl_dumb_create,
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 2a75ab80527a..55c0cc309198 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -54,6 +54,8 @@ struct udl_device { struct usb_device *udev; struct drm_crtc *crtc;
struct mutex gem_lock;
int sku_pixel_limit;
struct urb_list urbs;
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index dee6bd9a3dd1..9a15cce22cce 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -214,9 +214,10 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, { struct udl_gem_object *gobj; struct drm_gem_object *obj;
- struct udl_device *udl = dev->dev_private; int ret = 0;
- mutex_lock(&dev->struct_mutex);
- mutex_lock(&udl->gem_lock); obj = drm_gem_object_lookup(file, handle); if (obj == NULL) { ret = -ENOENT;
@@ -236,6 +237,6 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, out: drm_gem_object_put(&gobj->base); unlock:
- mutex_unlock(&dev->struct_mutex);
- mutex_unlock(&udl->gem_lock); return ret;
} diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index f1ec4528a73e..d518de8f496b 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -324,6 +324,8 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags) udl->ddev = dev; dev->dev_private = udl;
- mutex_init(&udl->gem_lock);
- if (!udl_parse_vendor_descriptor(dev, udl->udev)) { ret = -ENODEV; DRM_ERROR("firmware not recognized. Assume incompatible device\n");
-- 2.16.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
- None of the list walkings where protected.
- Switch to a mutex since the list walking could potentially take a very long time.
Only thing we need to be careful with here is that while we walk the list we cant unreference any gem objects, since the final unref would result in a recursive deadlock. But the only functions that walk the list is the device resume and debugfs dumping, so all safe.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/omapdrm/omap_debugfs.c | 2 ++ drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 11 +++++++---- 4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c b/drivers/gpu/drm/omapdrm/omap_debugfs.c index b42e286616b0..84da7a5b84f3 100644 --- a/drivers/gpu/drm/omapdrm/omap_debugfs.c +++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c @@ -37,7 +37,9 @@ static int gem_show(struct seq_file *m, void *arg) return ret;
seq_printf(m, "All Objects:\n"); + mutex_lock(&priv->list_lock); omap_gem_describe_objects(&priv->obj_list, m); + mutex_unlock(&priv->list_lock);
mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 3632854c2b91..3f40f7af3285 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -537,7 +537,7 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev) priv->omaprev = soc ? (unsigned int)soc->data : 0; priv->wq = alloc_ordered_workqueue("omapdrm", 0);
- spin_lock_init(&priv->list_lock); + mutex_init(&priv->list_lock); INIT_LIST_HEAD(&priv->obj_list);
/* Allocate and initialize the DRM device. */ diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 6eaee4df4559..f27c8e216adf 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -71,7 +71,7 @@ struct omap_drm_private { struct workqueue_struct *wq;
/* lock for obj_list below */ - spinlock_t list_lock; + struct mutex list_lock;
/* list of GEM objects: */ struct list_head obj_list; diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 0faf042b82e1..183447572e90 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1001,6 +1001,7 @@ int omap_gem_resume(struct drm_device *dev) struct omap_gem_object *omap_obj; int ret = 0;
+ mutex_lock(&priv->list_lock); list_for_each_entry(omap_obj, &priv->obj_list, mm_list) { if (omap_obj->block) { struct drm_gem_object *obj = &omap_obj->base; @@ -1012,10 +1013,12 @@ int omap_gem_resume(struct drm_device *dev) omap_obj->roll, true); if (ret) { dev_err(dev->dev, "could not repin: %d\n", ret); + mutex_unlock(&priv->list_lock); return ret; } } } + mutex_unlock(&priv->list_lock);
return 0; } @@ -1085,9 +1088,9 @@ void omap_gem_free_object(struct drm_gem_object *obj)
WARN_ON(!mutex_is_locked(&dev->struct_mutex));
- spin_lock(&priv->list_lock); + mutex_lock(&priv->list_lock); list_del(&omap_obj->mm_list); - spin_unlock(&priv->list_lock); + mutex_unlock(&priv->list_lock);
/* this means the object is still pinned.. which really should * not happen. I think.. @@ -1206,9 +1209,9 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, goto err_release; }
- spin_lock(&priv->list_lock); + mutex_lock(&priv->list_lock); list_add(&omap_obj->mm_list, &priv->obj_list); - spin_unlock(&priv->list_lock); + mutex_unlock(&priv->list_lock);
return obj;
On 27/03/18 11:23, Daniel Vetter wrote:
None of the list walkings where protected.
Switch to a mutex since the list walking could potentially take a very long time.
Only thing we need to be careful with here is that while we walk the list we cant unreference any gem objects, since the final unref would result in a recursive deadlock. But the only functions that walk the list is the device resume and debugfs dumping, so all safe.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com
drivers/gpu/drm/omapdrm/omap_debugfs.c | 2 ++ drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 11 +++++++---- 4 files changed, 11 insertions(+), 6 deletions(-)
Thanks, I'll pick this to omapdrm branch.
Tomi
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 3f40f7af3285..6d52877ed25a 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -490,7 +490,7 @@ static struct drm_driver omap_drm_driver = { .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = omap_gem_prime_export, .gem_prime_import = omap_gem_prime_import, - .gem_free_object = omap_gem_free_object, + .gem_free_object_unlocked = omap_gem_free_object, .gem_vm_ops = &omap_gem_vm_ops, .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset,
Hi,
On 27/03/18 11:23, Daniel Vetter wrote:
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com
drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 3f40f7af3285..6d52877ed25a 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -490,7 +490,7 @@ static struct drm_driver omap_drm_driver = { .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = omap_gem_prime_export, .gem_prime_import = omap_gem_prime_import,
- .gem_free_object = omap_gem_free_object,
- .gem_free_object_unlocked = omap_gem_free_object, .gem_vm_ops = &omap_gem_vm_ops, .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset,
This triggers WARN_ON(!mutex_is_locked(&dev->struct_mutex)), we have a few of those. I need to reverse engineer the omap_gem locking a bit to understand what's needed. And I need to figure out what exactly does struct_mutext protect =).
Tomi
On Wed, Mar 28, 2018 at 11:52 AM, Tomi Valkeinen tomi.valkeinen@ti.com wrote:
Hi,
On 27/03/18 11:23, Daniel Vetter wrote:
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com
drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 3f40f7af3285..6d52877ed25a 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -490,7 +490,7 @@ static struct drm_driver omap_drm_driver = { .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = omap_gem_prime_export, .gem_prime_import = omap_gem_prime_import,
.gem_free_object = omap_gem_free_object,
.gem_free_object_unlocked = omap_gem_free_object, .gem_vm_ops = &omap_gem_vm_ops, .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset,This triggers WARN_ON(!mutex_is_locked(&dev->struct_mutex)), we have a few of those. I need to reverse engineer the omap_gem locking a bit to understand what's needed. And I need to figure out what exactly does struct_mutext protect =).
Oops, I thought I read the code carefully and audited for that. Can you pls attach some example backtraces?
Thanks, Daniel
On 28/03/18 13:12, Daniel Vetter wrote:
This triggers WARN_ON(!mutex_is_locked(&dev->struct_mutex)), we have a few of those. I need to reverse engineer the omap_gem locking a bit to understand what's needed. And I need to figure out what exactly does struct_mutext protect =).
Oops, I thought I read the code carefully and audited for that. Can you pls attach some example backtraces?
Here's one I get easily.
[ 22.204433] ------------[ cut here ]------------ [ 22.209135] WARNING: CPU: 1 PID: 368 at drivers/gpu/drm/omapdrm/omap_gem.c:1089 omap_gem_free_object+0x2a4/0x2f8 [omapdrm] [ 22.220611] Modules linked in: omapdrm drm_kms_helper drm drm_panel_orientation_quirks cfbfillrect cfbimgblt cfbcopyarea connector_dvi panel_dsi_cm panel_dpi connector_ana log_tv connector_hdmi encoder_tpd12s015 encoder_tfp410 omapdss omapdss_base cec snd_soc_omap_hdmi_audio [ 22.245266] CPU: 1 PID: 368 Comm: kmstest Not tainted 4.16.0-rc7-01687-gcbc98ccce117 #10 [ 22.253406] Hardware name: Generic DRA74X (Flattened Device Tree) [ 22.259539] Backtrace: [ 22.262029] [<c010ec28>] (dump_backtrace) from [<c010eef8>] (show_stack+0x18/0x1c) [ 22.269646] r7:00000000 r6:600d0013 r5:00000000 r4:c0fbf8f0 [ 22.275349] [<c010eee0>] (show_stack) from [<c09b9960>] (dump_stack+0xac/0xe0) [ 22.282620] [<c09b98b4>] (dump_stack) from [<c013dff8>] (__warn+0xd4/0x100) [ 22.289625] r7:00000009 r6:bf193728 r5:00000000 r4:00000000 [ 22.295324] [<c013df24>] (__warn) from [<c013e140>] (warn_slowpath_null+0x44/0x50) [ 22.302941] r9:ed435e34 r8:ed36241c r7:ed3b7300 r6:bf18d4a4 r5:00000441 r4:bf193728 [ 22.310759] [<c013e0fc>] (warn_slowpath_null) from [<bf18d4a4>] (omap_gem_free_object+0x2a4/0x2f8 [omapdrm]) [ 22.320645] r6:ed14a000 r5:ed259200 r4:ed3b7300 [ 22.325450] [<bf18d200>] (omap_gem_free_object [omapdrm]) from [<bf0c9324>] (drm_gem_object_free+0x28/0x6c [drm]) [ 22.335774] r10:000000b4 r9:ed435e34 r8:ed36241c r7:ed3b7300 r6:ed3b7300 r5:ed3b7300 [ 22.343649] r4:ed14a000 [ 22.346427] [<bf0c92fc>] (drm_gem_object_free [drm]) from [<bf0c9934>] (drm_gem_object_put_unlocked+0xb4/0xc4 [drm]) [ 22.357008] r5:ed14a5b8 r4:bf18d200 [ 22.360833] [<bf0c9880>] (drm_gem_object_put_unlocked [drm]) from [<bf0c99bc>] (drm_gem_object_handle_put_unlocked+0x78/0xc0 [drm]) [ 22.372724] r7:ed3b7300 r6:00000000 r5:ed14a5b8 r4:ed3b7300 [ 22.378642] [<bf0c9944>] (drm_gem_object_handle_put_unlocked [drm]) from [<bf0c9a5c>] (drm_gem_object_release_handle+0x58/0x90 [drm]) [ 22.390707] r7:ed3b7300 r6:ed362558 r5:ed362400 r4:ed3b7300 [ 22.396625] [<bf0c9a04>] (drm_gem_object_release_handle [drm]) from [<bf0c9af4>] (drm_gem_handle_delete+0x60/0x8c [drm]) [ 22.407555] r7:ed3b7300 r6:00000001 r5:ed362400 r4:ed36242c [ 22.413472] [<bf0c9a94>] (drm_gem_handle_delete [drm]) from [<bf0c9b34>] (drm_gem_dumb_destroy+0x14/0x18 [drm]) [ 22.423620] r9:ed435e34 r8:bf0e7970 r7:00000018 r6:00000000 r5:ed14a000 r4:ed362400 [ 22.431630] [<bf0c9b20>] (drm_gem_dumb_destroy [drm]) from [<bf0e79b4>] (drm_mode_destroy_dumb_ioctl+0x44/0x50 [drm]) [ 22.442518] [<bf0e7970>] (drm_mode_destroy_dumb_ioctl [drm]) from [<bf0ca8c0>] (drm_ioctl_kernel+0x6c/0xb8 [drm]) [ 22.453059] [<bf0ca854>] (drm_ioctl_kernel [drm]) from [<bf0cad78>] (drm_ioctl+0x2b4/0x3e4 [drm]) [ 22.461985] r9:ed435e34 r8:c00464b4 r7:ed362400 r6:00000004 r5:00000004 r4:bf0f2f40 [ 22.469889] [<bf0caac4>] (drm_ioctl [drm]) from [<c030b324>] (do_vfs_ioctl+0xa8/0xa24) [ 22.477856] r10:00000000 r9:c030bd0c r8:ed420068 r7:00000003 r6:ed2f1e40 r5:bebb487c [ 22.485732] r4:c0f08948 [ 22.488291] [<c030b27c>] (do_vfs_ioctl) from [<c030bd0c>] (SyS_ioctl+0x6c/0x7c) [ 22.495648] r10:00000000 r9:00000003 r8:bebb487c r7:c00464b4 r6:00000000 r5:ed2f1e40 [ 22.503523] r4:ed2f1e40 [ 22.506084] [<c030bca0>] (SyS_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x28) [ 22.513698] Exception stack(0xed435fa8 to 0xed435ff0) [ 22.518788] 5fa0: 00061780 bebb487c 00000003 c00464b4 bebb487c 00000001 [ 22.527018] 5fc0: 00061780 bebb487c c00464b4 00000036 bebb487c 00062e58 0005dfdc 00000001 [ 22.535245] 5fe0: b6f52090 bebb485c b6f3a500 b6cb7c6c [ 22.540334] r9:ed434000 r8:c01011c4 r7:00000036 r6:c00464b4 r5:bebb487c r4:00061780 [ 22.548236] ---[ end trace 653a10f34c5fdc64 ]---
Tomi
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
v2: Just auditing the code isn't enough, I actually have to remove the now wrong locking check in omap_gem_free_object ...
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 5fcf9eaf3eaf..5005ecc284d2 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -493,7 +493,7 @@ static struct drm_driver omap_drm_driver = { .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = omap_gem_prime_export, .gem_prime_import = omap_gem_prime_import, - .gem_free_object = omap_gem_free_object, + .gem_free_object_unlocked = omap_gem_free_object, .gem_vm_ops = &omap_gem_vm_ops, .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset, diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 183447572e90..ac4175c13ecb 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1086,8 +1086,6 @@ void omap_gem_free_object(struct drm_gem_object *obj)
evict(obj);
- WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - mutex_lock(&priv->list_lock); list_del(&omap_obj->mm_list); mutex_unlock(&priv->list_lock);
On 28/03/18 14:41, Daniel Vetter wrote:
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
v2: Just auditing the code isn't enough, I actually have to remove the now wrong locking check in omap_gem_free_object ...
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com
This version works fine. I'll pick this to omapdrm branch. Thanks!
Tomi
Hello,
On Thursday, 29 March 2018 12:41:33 EEST Tomi Valkeinen wrote:
On 28/03/18 14:41, Daniel Vetter wrote:
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
v2: Just auditing the code isn't enough, I actually have to remove the now wrong locking check in omap_gem_free_object ...
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com
This version works fine. I'll pick this to omapdrm branch. Thanks!
Unless I'm mistaken (this is only based on code analysis), a WARN_ON could also be triggered through the following call stack.
gem_free_object_unlocked() omap_gem_free_object() evict() evict_entry() mmap_offset() WARN_ON(!mutex_is_locked(&dev->struct_mutex))
There could be other such call stacks.
I don't think we should switch to gem_free_object_unlocked() until all usage of struct_mutex is removed from the omapdrm driver.
On Mon, Apr 02, 2018 at 06:05:22PM +0300, Laurent Pinchart wrote:
Hello,
On Thursday, 29 March 2018 12:41:33 EEST Tomi Valkeinen wrote:
On 28/03/18 14:41, Daniel Vetter wrote:
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
v2: Just auditing the code isn't enough, I actually have to remove the now wrong locking check in omap_gem_free_object ...
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com
This version works fine. I'll pick this to omapdrm branch. Thanks!
Unless I'm mistaken (this is only based on code analysis), a WARN_ON could also be triggered through the following call stack.
gem_free_object_unlocked() omap_gem_free_object() evict() evict_entry() mmap_offset() WARN_ON(!mutex_is_locked(&dev->struct_mutex))
There could be other such call stacks.
I don't think we should switch to gem_free_object_unlocked() until all usage of struct_mutex is removed from the omapdrm driver.
Hm, indeed I missed that one. I'm not entirely sure how the tiler locking will pan out though, and whether there's not some stuff that won't be protected anymore. Specifically omapdrm_priv->usergart looks like it actually requires struct_mutex :-/ -Daniel
-- Regards,
Laurent Pinchart
Hi Daniel,
On Tuesday, 3 April 2018 12:07:31 EEST Daniel Vetter wrote:
On Mon, Apr 02, 2018 at 06:05:22PM +0300, Laurent Pinchart wrote:
On Thursday, 29 March 2018 12:41:33 EEST Tomi Valkeinen wrote:
On 28/03/18 14:41, Daniel Vetter wrote:
The only thing that omap_gem_free_object does that might need the magic protection of struct_mutex (of keeping all objects alive if that lock is held, even if the last reference is gone) is the mm_list manipulation.
But that is already protected by the separate omapdrm->list_lock, which means struct_mutex is an entirely internal lock for omapdrm. Everything else is just releasing resources, which is all protected already by the various subsystems and allocators.
To make this even more obvious we could do an s/dev->struct_mutex/omapdrm->gem_lock/ like I've done for udl. But since omapdrm is a lot bigger and a lot more active I'll refrain from that - this is better done by omapdrm developers at some suitable time in the future.
v2: Just auditing the code isn't enough, I actually have to remove the now wrong locking check in omap_gem_free_object ...
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com
This version works fine. I'll pick this to omapdrm branch. Thanks!
Unless I'm mistaken (this is only based on code analysis), a WARN_ON could also be triggered through the following call stack.
gem_free_object_unlocked() omap_gem_free_object() evict() evict_entry() mmap_offset() WARN_ON(!mutex_is_locked(&dev->struct_mutex))
There could be other such call stacks.
I don't think we should switch to gem_free_object_unlocked() until all usage of struct_mutex is removed from the omapdrm driver.
Hm, indeed I missed that one. I'm not entirely sure how the tiler locking will pan out though, and whether there's not some stuff that won't be protected anymore.
I reviewed locking through the driver when writing "drm/omap: gem: Replace struct_mutex usage with omap_obj private lock", but I'm also concerned that I might have missed something and introduced a race condition :-/
Specifically omapdrm_priv->usergart looks like it actually requires struct_mutex :-/
I think you're right. Or at rather not struct_mutex, but an omapdrm-specific gart mutex. I'm not too familiar with that code, I suppose I'll have to dive in.
On Tue, Mar 27, 2018 at 10:23:52AM +0200, Daniel Vetter wrote:
vboxvideo doesn't use dev->struct_mutex and therefore has no need to use gem_free_object.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Hans de Goede hdegoede@redhat.com Cc: Michael Thayer michael.thayer@oracle.com Cc: Colin Ian King colin.king@canonical.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Stephen Rothwell sfr@canb.auug.org.au
drivers/staging/vboxvideo/vbox_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
On Tue, Mar 27, 2018 at 10:34:07AM +0200, Greg Kroah-Hartman wrote:
On Tue, Mar 27, 2018 at 10:23:52AM +0200, Daniel Vetter wrote:
vboxvideo doesn't use dev->struct_mutex and therefore has no need to use gem_free_object.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Hans de Goede hdegoede@redhat.com Cc: Michael Thayer michael.thayer@oracle.com Cc: Colin Ian King colin.king@canonical.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Stephen Rothwell sfr@canb.auug.org.au
drivers/staging/vboxvideo/vbox_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
You'll pick this up or ack for stuffing into drm-misc (but only for 4.18)? There's no other patches that need this (we still have some drivers using the gem_free_object hook which won't be easy to convert to the _unlocked variant), so doesn't matter which tree. -Daniel
On Tue, Mar 27, 2018 at 11:18:03AM +0200, Daniel Vetter wrote:
On Tue, Mar 27, 2018 at 10:34:07AM +0200, Greg Kroah-Hartman wrote:
On Tue, Mar 27, 2018 at 10:23:52AM +0200, Daniel Vetter wrote:
vboxvideo doesn't use dev->struct_mutex and therefore has no need to use gem_free_object.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Hans de Goede hdegoede@redhat.com Cc: Michael Thayer michael.thayer@oracle.com Cc: Colin Ian King colin.king@canonical.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Stephen Rothwell sfr@canb.auug.org.au
drivers/staging/vboxvideo/vbox_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
You'll pick this up or ack for stuffing into drm-misc (but only for 4.18)?
Feel free to take it through your tree if you want.
thanks,
greg k-h
On Tue, Mar 27, 2018 at 05:25:21PM +0200, Greg Kroah-Hartman wrote:
On Tue, Mar 27, 2018 at 11:18:03AM +0200, Daniel Vetter wrote:
On Tue, Mar 27, 2018 at 10:34:07AM +0200, Greg Kroah-Hartman wrote:
On Tue, Mar 27, 2018 at 10:23:52AM +0200, Daniel Vetter wrote:
vboxvideo doesn't use dev->struct_mutex and therefore has no need to use gem_free_object.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Hans de Goede hdegoede@redhat.com Cc: Michael Thayer michael.thayer@oracle.com Cc: Colin Ian King colin.king@canonical.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Stephen Rothwell sfr@canb.auug.org.au
drivers/staging/vboxvideo/vbox_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
You'll pick this up or ack for stuffing into drm-misc (but only for 4.18)?
Feel free to take it through your tree if you want.
Pushed the first 3 from this series to drm-misc-next for 4.18. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Greg Kroah-Hartman -
Laurent Pinchart -
Sean Paul -
Tomi Valkeinen