Re: [PATCH 8/8] drm/panfrost: Make sure the shrinker does not reclaim referenced BOs

[Steven Price <steven.price@arm.com>]

On 29/11/2019 13:59, Boris Brezillon wrote:
> Userspace might tag a BO purgeable while it's still referenced by GPU
> jobs. We need to make sure the shrinker does not purge such BOs until
> all jobs referencing it are finished.
> 
> Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

I'm happy with this, but I would also argue that it was previously user
space's job not to mark a BO purgeable while it's in use by the GPU.
This is in some ways an ABI change so we should be sure this is
something that we want to support "forever" more. But if Mesa has
'always' been assuming this behaviour we might as well match.

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c          | 1 +
>  drivers/gpu/drm/panfrost/panfrost_gem.h          | 6 ++++++
>  drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 ++
>  drivers/gpu/drm/panfrost/panfrost_job.c          | 7 ++++++-
>  4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index b406b5243b40..297c0e7304d2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -166,6 +166,7 @@ panfrost_lookup_bos(struct drm_device *dev,
>  			break;
>  		}
>  
> +		atomic_inc(&bo->gpu_usecount);
>  		job->mappings[i] = mapping;
>  	}
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index ca1bc9019600..b3517ff9630c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -30,6 +30,12 @@ struct panfrost_gem_object {
>  		struct mutex lock;
>  	} mappings;
>  
> +	/*
> +	 * Count the number of jobs referencing this BO so we don't let the
> +	 * shrinker reclaim this object prematurely.
> +	 */
> +	atomic_t gpu_usecount;
> +
>  	bool noexec		:1;
>  	bool is_heap		:1;
>  };
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index b36df326c860..288e46c40673 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -41,6 +41,8 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj)
>  	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>  	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
>  
> +	if (atomic_read(&bo->gpu_usecount))
> +		return false;
>  
>  	if (!mutex_trylock(&shmem->pages_lock))
>  		return false;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index c85d45be3b5e..2b12aa87ff32 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -270,8 +270,13 @@ static void panfrost_job_cleanup(struct kref *ref)
>  	dma_fence_put(job->render_done_fence);
>  
>  	if (job->mappings) {
> -		for (i = 0; i < job->bo_count; i++)
> +		for (i = 0; i < job->bo_count; i++) {
> +			if (!job->mappings[i])
> +				break;
> +
> +			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
>  			panfrost_gem_mapping_put(job->mappings[i]);
> +		}
>  		kvfree(job->mappings);
>  	}
>  
>