Re: [Intel-gfx] [PATCH] Revert "drm/shmem-helper: Switch to reservation lock"

2023-02-28 Thread Saarinen, Jani


> -Original Message-
> From: Intel-gfx  On Behalf Of Thomas
> Zimmermann
> Sent: tiistai 28. helmikuuta 2023 17.46
> To: Dmitry Osipenko ;
> maarten.lankho...@linux.intel.com; airl...@gmail.com; dan...@ffwll.ch; Nikula,
> Jani 
> Cc: intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] Revert "drm/shmem-helper: Switch to
> reservation lock"
> 
> Hi
> 
> Am 28.02.23 um 16:28 schrieb Dmitry Osipenko:
> > On 2/28/23 18:26, Thomas Zimmermann wrote:
> >> This reverts commit 67b7836d4458790f1261e31fe0ce3250989784f0.
> >>
> >> The locking appears incomplete. A caller of SHMEM helper's pin
> >> function never acquires the dma-buf reservation lock. So we get
> >>
> >>WARNING: CPU: 3 PID: 967 at
> >> drivers/gpu/drm/drm_gem_shmem_helper.c:243
> >> drm_gem_shmem_pin+0x42/0x90 [drm_shmem_helper]
> >>
> >> Signed-off-by: Thomas Zimmermann 
> >> ---
> >
> > Thanks Thomas,
> >
> > Acked-by: Dmitry Osipenko 
> >
> 
> Thanks, merged now. I hope this fixes the immediate issues.
According to pre-merge results: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114478v1/index.html? 
So should fix. 

> 
> Best regards
> Thomas
> 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev


Re: [Intel-gfx] [PATCH] Revert "drm/shmem-helper: Switch to reservation lock"

2023-02-28 Thread Thomas Zimmermann

Hi

Am 28.02.23 um 16:28 schrieb Dmitry Osipenko:

On 2/28/23 18:26, Thomas Zimmermann wrote:

This reverts commit 67b7836d4458790f1261e31fe0ce3250989784f0.

The locking appears incomplete. A caller of SHMEM helper's pin
function never acquires the dma-buf reservation lock. So we get

   WARNING: CPU: 3 PID: 967 at drivers/gpu/drm/drm_gem_shmem_helper.c:243 
drm_gem_shmem_pin+0x42/0x90 [drm_shmem_helper]

Signed-off-by: Thomas Zimmermann 
---


Thanks Thomas,

Acked-by: Dmitry Osipenko 



Thanks, merged now. I hope this fixes the immediate issues.

Best regards
Thomas

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev


OpenPGP_signature
Description: OpenPGP digital signature


Re: [Intel-gfx] [PATCH] Revert "drm/shmem-helper: Switch to reservation lock"

2023-02-28 Thread Dmitry Osipenko
On 2/28/23 18:26, Thomas Zimmermann wrote:
> This reverts commit 67b7836d4458790f1261e31fe0ce3250989784f0.
> 
> The locking appears incomplete. A caller of SHMEM helper's pin
> function never acquires the dma-buf reservation lock. So we get
> 
>   WARNING: CPU: 3 PID: 967 at drivers/gpu/drm/drm_gem_shmem_helper.c:243 
> drm_gem_shmem_pin+0x42/0x90 [drm_shmem_helper]
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Thanks Thomas,

Acked-by: Dmitry Osipenko 

-- 
Best regards,
Dmitry



[Intel-gfx] [PATCH] Revert "drm/shmem-helper: Switch to reservation lock"

2023-02-28 Thread Thomas Zimmermann
This reverts commit 67b7836d4458790f1261e31fe0ce3250989784f0.

The locking appears incomplete. A caller of SHMEM helper's pin
function never acquires the dma-buf reservation lock. So we get

  WARNING: CPU: 3 PID: 967 at drivers/gpu/drm/drm_gem_shmem_helper.c:243 
drm_gem_shmem_pin+0x42/0x90 [drm_shmem_helper]

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/drm_gem_shmem_helper.c| 185 +++---
 drivers/gpu/drm/lima/lima_gem.c   |   8 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c   |   7 +-
 .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |   6 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c   |  19 +-
 include/drm/drm_gem_shmem_helper.h|  14 +-
 6 files changed, 148 insertions(+), 91 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c 
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 3d43e5961573..f75e50273d7a 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -88,6 +88,8 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, 
bool private)
if (ret)
goto err_release;
 
+   mutex_init(>pages_lock);
+   mutex_init(>vmap_lock);
INIT_LIST_HEAD(>madv_list);
 
if (!private) {
@@ -139,13 +141,11 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object 
*shmem)
 {
struct drm_gem_object *obj = >base;
 
+   drm_WARN_ON(obj->dev, shmem->vmap_use_count);
+
if (obj->import_attach) {
drm_prime_gem_destroy(obj, shmem->sgt);
} else {
-   dma_resv_lock(shmem->base.resv, NULL);
-
-   drm_WARN_ON(obj->dev, shmem->vmap_use_count);
-
if (shmem->sgt) {
dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
  DMA_BIDIRECTIONAL, 0);
@@ -154,18 +154,18 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object 
*shmem)
}
if (shmem->pages)
drm_gem_shmem_put_pages(shmem);
-
-   drm_WARN_ON(obj->dev, shmem->pages_use_count);
-
-   dma_resv_unlock(shmem->base.resv);
}
 
+   drm_WARN_ON(obj->dev, shmem->pages_use_count);
+
drm_gem_object_release(obj);
+   mutex_destroy(>pages_lock);
+   mutex_destroy(>vmap_lock);
kfree(shmem);
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
 
-static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
+static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
 {
struct drm_gem_object *obj = >base;
struct page **pages;
@@ -197,16 +197,35 @@ static int drm_gem_shmem_get_pages(struct 
drm_gem_shmem_object *shmem)
 }
 
 /*
- * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a 
shmem GEM object
+ * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object
  * @shmem: shmem GEM object
  *
- * This function decreases the use count and puts the backing pages when use 
drops to zero.
+ * This function makes sure that backing pages exists for the shmem GEM object
+ * and increases the use count.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
  */
-void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
+int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
 {
struct drm_gem_object *obj = >base;
+   int ret;
 
-   dma_resv_assert_held(shmem->base.resv);
+   drm_WARN_ON(obj->dev, obj->import_attach);
+
+   ret = mutex_lock_interruptible(>pages_lock);
+   if (ret)
+   return ret;
+   ret = drm_gem_shmem_get_pages_locked(shmem);
+   mutex_unlock(>pages_lock);
+
+   return ret;
+}
+EXPORT_SYMBOL(drm_gem_shmem_get_pages);
+
+static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
+{
+   struct drm_gem_object *obj = >base;
 
if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_use_count))
return;
@@ -224,6 +243,19 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object 
*shmem)
  shmem->pages_mark_accessed_on_put);
shmem->pages = NULL;
 }
+
+/*
+ * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a 
shmem GEM object
+ * @shmem: shmem GEM object
+ *
+ * This function decreases the use count and puts the backing pages when use 
drops to zero.
+ */
+void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
+{
+   mutex_lock(>pages_lock);
+   drm_gem_shmem_put_pages_locked(shmem);
+   mutex_unlock(>pages_lock);
+}
 EXPORT_SYMBOL(drm_gem_shmem_put_pages);
 
 /**
@@ -240,8 +272,6 @@ int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem)
 {
struct drm_gem_object *obj = >base;
 
-   dma_resv_assert_held(shmem->base.resv);
-
drm_WARN_ON(obj->dev, obj->import_attach);
 
return drm_gem_shmem_get_pages(shmem);
@@ -259,31 +289,14 @@ void drm_gem_shmem_unpin(struct drm_gem_shmem_object 
*shmem)
 {