Re: [Freedreno] [PATCH] fixup! drm/msm: Separate locking of buffer resources from struct_mutex

[ssusheel]

Hi Rob,

This looks good to me!
Just one nit: msm_gem_vunmap becomes very shrinker specific as it holds 
the msm_obj->lock with the shrinker class. Should we have the caller 
i.e. msm_gem_shrinker_vmap hold it instead? Or change it's name to 
msm_gem_vunmap_shrinker or something like that...?

It does make sense to make the madv/priv->inactive_list locking cleanup 
separately.

Thanks,
Sushmita


On 2017-06-16 08:22, Rob Clark wrote:
---
Ok, 2nd fixup to handle vmap shrinking.

 drivers/gpu/drm/msm/msm_gem.c | 44 
+++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c 
b/drivers/gpu/drm/msm/msm_gem.c
index f5d1f84..3190e05 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -42,6 +42,9 @@ enum {
        OBJ_LOCK_SHRINKER,
 };

+static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
+
+
 static dma_addr_t physaddr(struct drm_gem_object *obj)
 {
        struct msm_gem_object *msm_obj = to_msm_bo(obj);
@@ -484,6 +487,7 @@ int msm_gem_dumb_map_offset(struct drm_file *file,
struct drm_device *dev,
 void *msm_gem_get_vaddr(struct drm_gem_object *obj)
 {
        struct msm_gem_object *msm_obj = to_msm_bo(obj);
+       int ret = 0;

        mutex_lock(&msm_obj->lock);

@@ -492,22 +496,35 @@ void *msm_gem_get_vaddr(struct drm_gem_object 
*obj)
                return ERR_PTR(-EBUSY);
        }

+       /* increment vmap_count *before* vmap() call, so shrinker can
+        * check vmap_count (is_vunmapable()) outside of msm_obj->lock.
+        * This guarantees that we won't try to msm_gem_vunmap() this
+        * same object from within the vmap() call (while we already
+        * hold msm_obj->lock)
+        */
+       msm_obj->vmap_count++;
+
        if (!msm_obj->vaddr) {
                struct page **pages = get_pages(obj);
                if (IS_ERR(pages)) {
-                       mutex_unlock(&msm_obj->lock);
-                       return ERR_CAST(pages);
+                       ret = PTR_ERR(pages);
+                       goto fail;
                }
                msm_obj->vaddr = vmap(pages, obj->size >> PAGE_SHIFT,
                                VM_MAP, pgprot_writecombine(PAGE_KERNEL));
                if (msm_obj->vaddr == NULL) {
-                       mutex_unlock(&msm_obj->lock);
-                       return ERR_PTR(-ENOMEM);
+                       ret = -ENOMEM;
+                       goto fail;
                }
        }
-       msm_obj->vmap_count++;
+
        mutex_unlock(&msm_obj->lock);
        return msm_obj->vaddr;
+
+fail:
+       msm_obj->vmap_count--;
+       mutex_unlock(&msm_obj->lock);
+       return ERR_PTR(ret);
 }

 void msm_gem_put_vaddr(struct drm_gem_object *obj)
@@ -554,7 +571,7 @@ void msm_gem_purge(struct drm_gem_object *obj)

        put_iova(obj);

-       msm_gem_vunmap(obj);
+       msm_gem_vunmap_locked(obj);

        put_pages(obj);

@@ -576,10 +593,12 @@ void msm_gem_purge(struct drm_gem_object *obj)
        mutex_unlock(&msm_obj->lock);
 }

-void msm_gem_vunmap(struct drm_gem_object *obj)
+static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
 {
        struct msm_gem_object *msm_obj = to_msm_bo(obj);

+       WARN_ON(!mutex_is_locked(&msm_obj->lock));
+
        if (!msm_obj->vaddr || WARN_ON(!is_vunmapable(msm_obj)))
                return;

@@ -587,6 +606,15 @@ void msm_gem_vunmap(struct drm_gem_object *obj)
        msm_obj->vaddr = NULL;
 }

+void msm_gem_vunmap(struct drm_gem_object *obj)
+{
+       struct msm_gem_object *msm_obj = to_msm_bo(obj);
+
+       mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
+       msm_gem_vunmap_locked(obj);
+       mutex_unlock(&msm_obj->lock);
+}
+
 /* must be called before _move_to_active().. */
 int msm_gem_sync_object(struct drm_gem_object *obj,
                struct msm_fence_context *fctx, bool exclusive)
@@ -799,7 +827,7 @@ void msm_gem_free_object(struct drm_gem_object 
*obj)

                drm_prime_gem_destroy(obj, msm_obj->sgt);
        } else {
-               msm_gem_vunmap(obj);
+               msm_gem_vunmap_locked(obj);
                put_pages(obj);
        }
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno