[PATCH v2 1/2] drm/msm: Fix premature purging of BO

[PATCH v2 1/2] drm/msm: Fix premature purging of BO

[Akhil P Oommen]

In the case where we have a back-to-back submission that shares the same
BO, this BO will be prematurely moved to inactive_list while retiring the
first submit. But it will be still part of the second submit which is
being processed by the GPU. Now, if the shrinker happens to be triggered at
this point, it will result in a premature purging of this BO.

To fix this, we need to refcount BO while doing submit and retire. Then,
it should be moved to inactive list when this refcount becomes 0.

Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
---
Changes in v2:
	1. Keep Active List around
	2. Put back the deleted WARN_ON

 drivers/gpu/drm/msm/msm_drv.h |  5 ++---
 drivers/gpu/drm/msm/msm_gem.c | 32 ++++++++++++++++----------------
 drivers/gpu/drm/msm/msm_gem.h |  4 +++-
 drivers/gpu/drm/msm/msm_gpu.c | 11 +++++++----
 4 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 3193274..28e3c8d 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -309,9 +309,8 @@ void msm_gem_put_vaddr(struct drm_gem_object *obj);
 int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
 int msm_gem_sync_object(struct drm_gem_object *obj,
 		struct msm_fence_context *fctx, bool exclusive);
-void msm_gem_move_to_active(struct drm_gem_object *obj,
-		struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence);
-void msm_gem_move_to_inactive(struct drm_gem_object *obj);
+void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu);
+void msm_gem_active_put(struct drm_gem_object *obj);
 int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout);
 int msm_gem_cpu_fini(struct drm_gem_object *obj);
 void msm_gem_free_object(struct drm_gem_object *obj);
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 76a6c52..14e14ca 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -743,31 +743,31 @@ int msm_gem_sync_object(struct drm_gem_object *obj,
 	return 0;
 }
 
-void msm_gem_move_to_active(struct drm_gem_object *obj,
-		struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence)
+void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
 {
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
+	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
 	WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
-	msm_obj->gpu = gpu;
-	if (exclusive)
-		dma_resv_add_excl_fence(obj->resv, fence);
-	else
-		dma_resv_add_shared_fence(obj->resv, fence);
-	list_del_init(&msm_obj->mm_list);
-	list_add_tail(&msm_obj->mm_list, &gpu->active_list);
+
+	if (!atomic_fetch_inc(&msm_obj->active_count)) {
+		msm_obj->gpu = gpu;
+		list_del_init(&msm_obj->mm_list);
+		list_add_tail(&msm_obj->mm_list, &gpu->active_list);
+	}
 }
 
-void msm_gem_move_to_inactive(struct drm_gem_object *obj)
+void msm_gem_active_put(struct drm_gem_object *obj)
 {
-	struct drm_device *dev = obj->dev;
-	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
+	struct msm_drm_private *priv = obj->dev->dev_private;
 
-	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
 
-	msm_obj->gpu = NULL;
-	list_del_init(&msm_obj->mm_list);
-	list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
+	if (!atomic_dec_return(&msm_obj->active_count)) {
+		msm_obj->gpu = NULL;
+		list_del_init(&msm_obj->mm_list);
+		list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
+	}
 }
 
 int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout)
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 7b1c7a5..a1bf741 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -88,12 +88,14 @@ struct msm_gem_object {
 	struct mutex lock; /* Protects resources associated with bo */
 
 	char name[32]; /* Identifier to print for the debugfs files */
+
+	atomic_t active_count;
 };
 #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
 
 static inline bool is_active(struct msm_gem_object *msm_obj)
 {
-	return msm_obj->gpu != NULL;
+	return atomic_read(&msm_obj->active_count);
 }
 
 static inline bool is_purgeable(struct msm_gem_object *msm_obj)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 29c8d73c..55d1648 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -698,8 +698,8 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
 
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct msm_gem_object *msm_obj = submit->bos[i].obj;
-		/* move to inactive: */
-		msm_gem_move_to_inactive(&msm_obj->base);
+
+		msm_gem_active_put(&msm_obj->base);
 		msm_gem_unpin_iova(&msm_obj->base, submit->aspace);
 		drm_gem_object_put_locked(&msm_obj->base);
 	}
@@ -774,6 +774,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
 
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct msm_gem_object *msm_obj = submit->bos[i].obj;
+		struct drm_gem_object *drm_obj = &msm_obj->base;
 		uint64_t iova;
 
 		/* can't happen yet.. but when we add 2d support we'll have
@@ -786,9 +787,11 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
 		msm_gem_get_and_pin_iova(&msm_obj->base, submit->aspace, &iova);
 
 		if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
-			msm_gem_move_to_active(&msm_obj->base, gpu, true, submit->fence);
+			dma_resv_add_excl_fence(drm_obj->resv, submit->fence);
 		else if (submit->bos[i].flags & MSM_SUBMIT_BO_READ)
-			msm_gem_move_to_active(&msm_obj->base, gpu, false, submit->fence);
+			dma_resv_add_shared_fence(drm_obj->resv, submit->fence);
+
+		msm_gem_active_get(drm_obj, gpu);
 	}
 
 	gpu->funcs->submit(gpu, submit);
-- 
2.7.4

[PATCH v2 2/2] drm/msm: Leave inuse count intact on map failure

[Akhil P Oommen]

Leave the inuse count intact on map failure to keep the accounting
accurate.

Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
---
 drivers/gpu/drm/msm/msm_gem_vma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 80a8a26..f914ddb 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -88,8 +88,10 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
 		ret = aspace->mmu->funcs->map(aspace->mmu, vma->iova, sgt,
 				size, prot);
 
-	if (ret)
+	if (ret) {
 		vma->mapped = false;
+		vma->inuse--;
+	}
 
 	return ret;
 }
-- 
2.7.4

Re: [PATCH v2 1/2] drm/msm: Fix premature purging of BO

[Jordan Crouse]

On Tue, Sep 22, 2020 at 08:25:26PM +0530, Akhil P Oommen wrote:
> In the case where we have a back-to-back submission that shares the same
> BO, this BO will be prematurely moved to inactive_list while retiring the
> first submit. But it will be still part of the second submit which is
> being processed by the GPU. Now, if the shrinker happens to be triggered at
> this point, it will result in a premature purging of this BO.
> 
> To fix this, we need to refcount BO while doing submit and retire. Then,
> it should be moved to inactive list when this refcount becomes 0.
> 
> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
> ---
> Changes in v2:
> 	1. Keep Active List around
> 	2. Put back the deleted WARN_ON
> 
>  drivers/gpu/drm/msm/msm_drv.h |  5 ++---
>  drivers/gpu/drm/msm/msm_gem.c | 32 ++++++++++++++++----------------
>  drivers/gpu/drm/msm/msm_gem.h |  4 +++-
>  drivers/gpu/drm/msm/msm_gpu.c | 11 +++++++----
>  4 files changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 3193274..28e3c8d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -309,9 +309,8 @@ void msm_gem_put_vaddr(struct drm_gem_object *obj);
>  int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
>  int msm_gem_sync_object(struct drm_gem_object *obj,
>  		struct msm_fence_context *fctx, bool exclusive);
> -void msm_gem_move_to_active(struct drm_gem_object *obj,
> -		struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence);
> -void msm_gem_move_to_inactive(struct drm_gem_object *obj);
> +void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu);
> +void msm_gem_active_put(struct drm_gem_object *obj);
>  int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout);
>  int msm_gem_cpu_fini(struct drm_gem_object *obj);
>  void msm_gem_free_object(struct drm_gem_object *obj);
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 76a6c52..14e14ca 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -743,31 +743,31 @@ int msm_gem_sync_object(struct drm_gem_object *obj,
>  	return 0;
>  }
>  
> -void msm_gem_move_to_active(struct drm_gem_object *obj,
> -		struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence)
> +void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
>  {
>  	struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>  	WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
> -	msm_obj->gpu = gpu;
> -	if (exclusive)
> -		dma_resv_add_excl_fence(obj->resv, fence);
> -	else
> -		dma_resv_add_shared_fence(obj->resv, fence);
> -	list_del_init(&msm_obj->mm_list);
> -	list_add_tail(&msm_obj->mm_list, &gpu->active_list);
> +
> +	if (!atomic_fetch_inc(&msm_obj->active_count)) {
> +		msm_obj->gpu = gpu;
> +		list_del_init(&msm_obj->mm_list);
> +		list_add_tail(&msm_obj->mm_list, &gpu->active_list);
> +	}

I'm not sure if all the renaming and reorganization are really needed here -
this is the meat of the change and it would have fit in reasonably well with the
existing function design.

>  }
>  
> -void msm_gem_move_to_inactive(struct drm_gem_object *obj)
> +void msm_gem_active_put(struct drm_gem_object *obj)
>  {
> -	struct drm_device *dev = obj->dev;
> -	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +	struct msm_drm_private *priv = obj->dev->dev_private;
>  
> -	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>  
> -	msm_obj->gpu = NULL;
> -	list_del_init(&msm_obj->mm_list);
> -	list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> +	if (!atomic_dec_return(&msm_obj->active_count)) {
> +		msm_obj->gpu = NULL;
> +		list_del_init(&msm_obj->mm_list);
> +		list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> +	}

Same.

Jordan
>  }
>  
>  int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout)
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 7b1c7a5..a1bf741 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -88,12 +88,14 @@ struct msm_gem_object {
>  	struct mutex lock; /* Protects resources associated with bo */
>  
>  	char name[32]; /* Identifier to print for the debugfs files */
> +
> +	atomic_t active_count;
>  };
>  #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
>  
>  static inline bool is_active(struct msm_gem_object *msm_obj)
>  {
> -	return msm_obj->gpu != NULL;
> +	return atomic_read(&msm_obj->active_count);
>  }
>  
>  static inline bool is_purgeable(struct msm_gem_object *msm_obj)
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 29c8d73c..55d1648 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -698,8 +698,8 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
>  
>  	for (i = 0; i < submit->nr_bos; i++) {
>  		struct msm_gem_object *msm_obj = submit->bos[i].obj;
> -		/* move to inactive: */
> -		msm_gem_move_to_inactive(&msm_obj->base);
> +
> +		msm_gem_active_put(&msm_obj->base);
>  		msm_gem_unpin_iova(&msm_obj->base, submit->aspace);
>  		drm_gem_object_put_locked(&msm_obj->base);
>  	}
> @@ -774,6 +774,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>  
>  	for (i = 0; i < submit->nr_bos; i++) {
>  		struct msm_gem_object *msm_obj = submit->bos[i].obj;
> +		struct drm_gem_object *drm_obj = &msm_obj->base;
>  		uint64_t iova;
>  
>  		/* can't happen yet.. but when we add 2d support we'll have
> @@ -786,9 +787,11 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>  		msm_gem_get_and_pin_iova(&msm_obj->base, submit->aspace, &iova);
>  
>  		if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
> -			msm_gem_move_to_active(&msm_obj->base, gpu, true, submit->fence);
> +			dma_resv_add_excl_fence(drm_obj->resv, submit->fence);
>  		else if (submit->bos[i].flags & MSM_SUBMIT_BO_READ)
> -			msm_gem_move_to_active(&msm_obj->base, gpu, false, submit->fence);
> +			dma_resv_add_shared_fence(drm_obj->resv, submit->fence);
> +
> +		msm_gem_active_get(drm_obj, gpu);
>  	}
>  
>  	gpu->funcs->submit(gpu, submit);
> -- 
> 2.7.4
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] drm/msm: Leave inuse count intact on map failure

[Jordan Crouse]

On Tue, Sep 22, 2020 at 08:25:27PM +0530, Akhil P Oommen wrote:
> Leave the inuse count intact on map failure to keep the accounting
> accurate.
> 
> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>

Reviewed-by: Jordan Crouse <jcrouse@codeaurora.org>

> ---
>  drivers/gpu/drm/msm/msm_gem_vma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 80a8a26..f914ddb 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -88,8 +88,10 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
>  		ret = aspace->mmu->funcs->map(aspace->mmu, vma->iova, sgt,
>  				size, prot);
>  
> -	if (ret)
> +	if (ret) {
>  		vma->mapped = false;
> +		vma->inuse--;
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.7.4
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 1/2] drm/msm: Fix premature purging of BO

[Akhil P Oommen]

On 9/23/2020 8:20 PM, Jordan Crouse wrote:
> On Tue, Sep 22, 2020 at 08:25:26PM +0530, Akhil P Oommen wrote:
>> In the case where we have a back-to-back submission that shares the same
>> BO, this BO will be prematurely moved to inactive_list while retiring the
>> first submit. But it will be still part of the second submit which is
>> being processed by the GPU. Now, if the shrinker happens to be triggered at
>> this point, it will result in a premature purging of this BO.
>>
>> To fix this, we need to refcount BO while doing submit and retire. Then,
>> it should be moved to inactive list when this refcount becomes 0.
>>
>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
>> ---
>> Changes in v2:
>> 	1. Keep Active List around
>> 	2. Put back the deleted WARN_ON
>>
>>   drivers/gpu/drm/msm/msm_drv.h |  5 ++---
>>   drivers/gpu/drm/msm/msm_gem.c | 32 ++++++++++++++++----------------
>>   drivers/gpu/drm/msm/msm_gem.h |  4 +++-
>>   drivers/gpu/drm/msm/msm_gpu.c | 11 +++++++----
>>   4 files changed, 28 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 3193274..28e3c8d 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -309,9 +309,8 @@ void msm_gem_put_vaddr(struct drm_gem_object *obj);
>>   int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
>>   int msm_gem_sync_object(struct drm_gem_object *obj,
>>   		struct msm_fence_context *fctx, bool exclusive);
>> -void msm_gem_move_to_active(struct drm_gem_object *obj,
>> -		struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence);
>> -void msm_gem_move_to_inactive(struct drm_gem_object *obj);
>> +void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu);
>> +void msm_gem_active_put(struct drm_gem_object *obj);
>>   int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout);
>>   int msm_gem_cpu_fini(struct drm_gem_object *obj);
>>   void msm_gem_free_object(struct drm_gem_object *obj);
>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> index 76a6c52..14e14ca 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -743,31 +743,31 @@ int msm_gem_sync_object(struct drm_gem_object *obj,
>>   	return 0;
>>   }
>>   
>> -void msm_gem_move_to_active(struct drm_gem_object *obj,
>> -		struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence)
>> +void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
>>   {
>>   	struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> +	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>>   	WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
>> -	msm_obj->gpu = gpu;
>> -	if (exclusive)
>> -		dma_resv_add_excl_fence(obj->resv, fence);
>> -	else
>> -		dma_resv_add_shared_fence(obj->resv, fence);
>> -	list_del_init(&msm_obj->mm_list);
>> -	list_add_tail(&msm_obj->mm_list, &gpu->active_list);
>> +
>> +	if (!atomic_fetch_inc(&msm_obj->active_count)) {
>> +		msm_obj->gpu = gpu;
>> +		list_del_init(&msm_obj->mm_list);
>> +		list_add_tail(&msm_obj->mm_list, &gpu->active_list);
>> +	}
> 
> I'm not sure if all the renaming and reorganization are really needed here -
> this is the meat of the change and it would have fit in reasonably well with the
> existing function design.
This happened due to the way I implemented the v1 patch. In the 
hindsight, I think you are right.

Akhil.
> 
>>   }
>>   
>> -void msm_gem_move_to_inactive(struct drm_gem_object *obj)
>> +void msm_gem_active_put(struct drm_gem_object *obj)
>>   {
>> -	struct drm_device *dev = obj->dev;
>> -	struct msm_drm_private *priv = dev->dev_private;
>>   	struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> +	struct msm_drm_private *priv = obj->dev->dev_private;
>>   
>> -	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> +	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>>   
>> -	msm_obj->gpu = NULL;
>> -	list_del_init(&msm_obj->mm_list);
>> -	list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
>> +	if (!atomic_dec_return(&msm_obj->active_count)) {
>> +		msm_obj->gpu = NULL;
>> +		list_del_init(&msm_obj->mm_list);
>> +		list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
>> +	}
> 
> Same.
> 
> Jordan
>>   }
>>   
>>   int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout)
>> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
>> index 7b1c7a5..a1bf741 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.h
>> +++ b/drivers/gpu/drm/msm/msm_gem.h
>> @@ -88,12 +88,14 @@ struct msm_gem_object {
>>   	struct mutex lock; /* Protects resources associated with bo */
>>   
>>   	char name[32]; /* Identifier to print for the debugfs files */
>> +
>> +	atomic_t active_count;
>>   };
>>   #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
>>   
>>   static inline bool is_active(struct msm_gem_object *msm_obj)
>>   {
>> -	return msm_obj->gpu != NULL;
>> +	return atomic_read(&msm_obj->active_count);
>>   }
>>   
>>   static inline bool is_purgeable(struct msm_gem_object *msm_obj)
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>> index 29c8d73c..55d1648 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>> @@ -698,8 +698,8 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
>>   
>>   	for (i = 0; i < submit->nr_bos; i++) {
>>   		struct msm_gem_object *msm_obj = submit->bos[i].obj;
>> -		/* move to inactive: */
>> -		msm_gem_move_to_inactive(&msm_obj->base);
>> +
>> +		msm_gem_active_put(&msm_obj->base);
>>   		msm_gem_unpin_iova(&msm_obj->base, submit->aspace);
>>   		drm_gem_object_put_locked(&msm_obj->base);
>>   	}
>> @@ -774,6 +774,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>   
>>   	for (i = 0; i < submit->nr_bos; i++) {
>>   		struct msm_gem_object *msm_obj = submit->bos[i].obj;
>> +		struct drm_gem_object *drm_obj = &msm_obj->base;
>>   		uint64_t iova;
>>   
>>   		/* can't happen yet.. but when we add 2d support we'll have
>> @@ -786,9 +787,11 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>   		msm_gem_get_and_pin_iova(&msm_obj->base, submit->aspace, &iova);
>>   
>>   		if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
>> -			msm_gem_move_to_active(&msm_obj->base, gpu, true, submit->fence);
>> +			dma_resv_add_excl_fence(drm_obj->resv, submit->fence);
>>   		else if (submit->bos[i].flags & MSM_SUBMIT_BO_READ)
>> -			msm_gem_move_to_active(&msm_obj->base, gpu, false, submit->fence);
>> +			dma_resv_add_shared_fence(drm_obj->resv, submit->fence);
>> +
>> +		msm_gem_active_get(drm_obj, gpu);
>>   	}
>>   
>>   	gpu->funcs->submit(gpu, submit);
>> -- 
>> 2.7.4
>>
> 

Re: [PATCH v2 1/2] drm/msm: Fix premature purging of BO

[Rob Clark]

On Wed, Sep 23, 2020 at 11:28 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>
> On 9/23/2020 8:20 PM, Jordan Crouse wrote:
> > On Tue, Sep 22, 2020 at 08:25:26PM +0530, Akhil P Oommen wrote:
> >> In the case where we have a back-to-back submission that shares the same
> >> BO, this BO will be prematurely moved to inactive_list while retiring the
> >> first submit. But it will be still part of the second submit which is
> >> being processed by the GPU. Now, if the shrinker happens to be triggered at
> >> this point, it will result in a premature purging of this BO.
> >>
> >> To fix this, we need to refcount BO while doing submit and retire. Then,
> >> it should be moved to inactive list when this refcount becomes 0.
> >>
> >> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
> >> ---
> >> Changes in v2:
> >>      1. Keep Active List around
> >>      2. Put back the deleted WARN_ON
> >>
> >>   drivers/gpu/drm/msm/msm_drv.h |  5 ++---
> >>   drivers/gpu/drm/msm/msm_gem.c | 32 ++++++++++++++++----------------
> >>   drivers/gpu/drm/msm/msm_gem.h |  4 +++-
> >>   drivers/gpu/drm/msm/msm_gpu.c | 11 +++++++----
> >>   4 files changed, 28 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> >> index 3193274..28e3c8d 100644
> >> --- a/drivers/gpu/drm/msm/msm_drv.h
> >> +++ b/drivers/gpu/drm/msm/msm_drv.h
> >> @@ -309,9 +309,8 @@ void msm_gem_put_vaddr(struct drm_gem_object *obj);
> >>   int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
> >>   int msm_gem_sync_object(struct drm_gem_object *obj,
> >>              struct msm_fence_context *fctx, bool exclusive);
> >> -void msm_gem_move_to_active(struct drm_gem_object *obj,
> >> -            struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence);
> >> -void msm_gem_move_to_inactive(struct drm_gem_object *obj);
> >> +void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu);
> >> +void msm_gem_active_put(struct drm_gem_object *obj);
> >>   int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout);
> >>   int msm_gem_cpu_fini(struct drm_gem_object *obj);
> >>   void msm_gem_free_object(struct drm_gem_object *obj);
> >> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> >> index 76a6c52..14e14ca 100644
> >> --- a/drivers/gpu/drm/msm/msm_gem.c
> >> +++ b/drivers/gpu/drm/msm/msm_gem.c
> >> @@ -743,31 +743,31 @@ int msm_gem_sync_object(struct drm_gem_object *obj,
> >>      return 0;
> >>   }
> >>
> >> -void msm_gem_move_to_active(struct drm_gem_object *obj,
> >> -            struct msm_gpu *gpu, bool exclusive, struct dma_fence *fence)
> >> +void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
> >>   {
> >>      struct msm_gem_object *msm_obj = to_msm_bo(obj);
> >> +    WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> >>      WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
> >> -    msm_obj->gpu = gpu;
> >> -    if (exclusive)
> >> -            dma_resv_add_excl_fence(obj->resv, fence);
> >> -    else
> >> -            dma_resv_add_shared_fence(obj->resv, fence);
> >> -    list_del_init(&msm_obj->mm_list);
> >> -    list_add_tail(&msm_obj->mm_list, &gpu->active_list);
> >> +
> >> +    if (!atomic_fetch_inc(&msm_obj->active_count)) {
> >> +            msm_obj->gpu = gpu;
> >> +            list_del_init(&msm_obj->mm_list);
> >> +            list_add_tail(&msm_obj->mm_list, &gpu->active_list);
> >> +    }
> >
> > I'm not sure if all the renaming and reorganization are really needed here -
> > this is the meat of the change and it would have fit in reasonably well with the
> > existing function design.
> This happened due to the way I implemented the v1 patch. In the
> hindsight, I think you are right.
>

fwiw, I'd already pushed v2 to msm-next.. I guess it could have been a
smaller diff-stat, but I don't really object to the other changes
(which were replacing things that were already a result of how things
had previously evolved)

BR,
-R

> Akhil.
> >
> >>   }
> >>
> >> -void msm_gem_move_to_inactive(struct drm_gem_object *obj)
> >> +void msm_gem_active_put(struct drm_gem_object *obj)
> >>   {
> >> -    struct drm_device *dev = obj->dev;
> >> -    struct msm_drm_private *priv = dev->dev_private;
> >>      struct msm_gem_object *msm_obj = to_msm_bo(obj);
> >> +    struct msm_drm_private *priv = obj->dev->dev_private;
> >>
> >> -    WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >> +    WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> >>
> >> -    msm_obj->gpu = NULL;
> >> -    list_del_init(&msm_obj->mm_list);
> >> -    list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> >> +    if (!atomic_dec_return(&msm_obj->active_count)) {
> >> +            msm_obj->gpu = NULL;
> >> +            list_del_init(&msm_obj->mm_list);
> >> +            list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> >> +    }
> >
> > Same.
> >
> > Jordan
> >>   }
> >>
> >>   int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout)
> >> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> >> index 7b1c7a5..a1bf741 100644
> >> --- a/drivers/gpu/drm/msm/msm_gem.h
> >> +++ b/drivers/gpu/drm/msm/msm_gem.h
> >> @@ -88,12 +88,14 @@ struct msm_gem_object {
> >>      struct mutex lock; /* Protects resources associated with bo */
> >>
> >>      char name[32]; /* Identifier to print for the debugfs files */
> >> +
> >> +    atomic_t active_count;
> >>   };
> >>   #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
> >>
> >>   static inline bool is_active(struct msm_gem_object *msm_obj)
> >>   {
> >> -    return msm_obj->gpu != NULL;
> >> +    return atomic_read(&msm_obj->active_count);
> >>   }
> >>
> >>   static inline bool is_purgeable(struct msm_gem_object *msm_obj)
> >> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> >> index 29c8d73c..55d1648 100644
> >> --- a/drivers/gpu/drm/msm/msm_gpu.c
> >> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> >> @@ -698,8 +698,8 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
> >>
> >>      for (i = 0; i < submit->nr_bos; i++) {
> >>              struct msm_gem_object *msm_obj = submit->bos[i].obj;
> >> -            /* move to inactive: */
> >> -            msm_gem_move_to_inactive(&msm_obj->base);
> >> +
> >> +            msm_gem_active_put(&msm_obj->base);
> >>              msm_gem_unpin_iova(&msm_obj->base, submit->aspace);
> >>              drm_gem_object_put_locked(&msm_obj->base);
> >>      }
> >> @@ -774,6 +774,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >>
> >>      for (i = 0; i < submit->nr_bos; i++) {
> >>              struct msm_gem_object *msm_obj = submit->bos[i].obj;
> >> +            struct drm_gem_object *drm_obj = &msm_obj->base;
> >>              uint64_t iova;
> >>
> >>              /* can't happen yet.. but when we add 2d support we'll have
> >> @@ -786,9 +787,11 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
> >>              msm_gem_get_and_pin_iova(&msm_obj->base, submit->aspace, &iova);
> >>
> >>              if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
> >> -                    msm_gem_move_to_active(&msm_obj->base, gpu, true, submit->fence);
> >> +                    dma_resv_add_excl_fence(drm_obj->resv, submit->fence);
> >>              else if (submit->bos[i].flags & MSM_SUBMIT_BO_READ)
> >> -                    msm_gem_move_to_active(&msm_obj->base, gpu, false, submit->fence);
> >> +                    dma_resv_add_shared_fence(drm_obj->resv, submit->fence);
> >> +
> >> +            msm_gem_active_get(drm_obj, gpu);
> >>      }
> >>
> >>      gpu->funcs->submit(gpu, submit);
> >> --
> >> 2.7.4
> >>
> >
>