[PATCH 1/5] staging/vboxvideo: Use gem_free_object_unlocked

[PATCH 1/5] staging/vboxvideo: Use gem_free_object_unlocked

[Daniel Vetter]

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,
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/rockchip: fixup comment for gem_free_object_unlocked

[Daniel Vetter]

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)
 {
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/rockchip: fixup comment for gem_free_object_unlocked

[Daniel Vetter]

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 at lists.infradead.org
Cc: linux-rockchip at 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

Re: [PATCH 2/5] drm/rockchip: fixup comment for gem_free_object_unlocked

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/rockchip: fixup comment for gem_free_object_unlocked

[Sean Paul]

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 at lists.infradead.org
> Cc: linux-rockchip at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 3/5] drm/udl: Get rid of dev->struct_mutex usage

[Daniel Vetter]

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");
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/udl: Get rid of dev->struct_mutex usage

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/omapdrm: Fix mm_list locking

[Daniel Vetter]

- 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;
 
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/omapdrm: Fix mm_list locking

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/omapdrm: Switch to gem_free_object_unlocked

[Daniel Vetter]

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,
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/omapdrm: Switch to gem_free_object_unlocked

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/omapdrm: Switch to gem_free_object_unlocked

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/omapdrm: Switch to gem_free_object_unlocked

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/omapdrm: Switch to gem_free_object_unlocked

[Daniel Vetter]

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);
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omapdrm: Switch to gem_free_object_unlocked

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omapdrm: Switch to gem_free_object_unlocked

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omapdrm: Switch to gem_free_object_unlocked

[Daniel Vetter]

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
> 
> 
> 

-- 
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

Re: [PATCH] drm/omapdrm: Switch to gem_free_object_unlocked

[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.

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] staging/vboxvideo: Use gem_free_object_unlocked

[Greg Kroah-Hartman]

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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] staging/vboxvideo: Use gem_free_object_unlocked

[Daniel Vetter]

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
-- 
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

Re: [PATCH 1/5] staging/vboxvideo: Use gem_free_object_unlocked

[Greg Kroah-Hartman]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] staging/vboxvideo: Use gem_free_object_unlocked

[Daniel Vetter]

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
-- 
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