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

[ssusheel at codeaurora.org]

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