Re: [PATCH 02/10] drm/amdgpu: Protect the amdgpu_bo_list list with a mutex v2

[Luben Tuikov <luben.tuikov@xxxxxxx>]

Tested-by: Luben Tuikov <luben.tuikov@xxxxxxx>

This passes all IGT amd_cs_nop tests.

Regards,
Luben

On 2022-07-14 06:38, Christian König wrote:
> From: Luben Tuikov <luben.tuikov@xxxxxxx>
> 
> Protect the struct amdgpu_bo_list with a mutex. This is used during command
> submission in order to avoid buffer object corruption as recorded in
> the link below.
> 
> v2 (chk): Keep the mutex looked for the whole CS to avoid using the
> 	  list from multiple CS threads at the same time.
> 
> Suggested-by: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <Alexander.Deucher@xxxxxxx>
> Cc: Andrey Grodzovsky <Andrey.Grodzovsky@xxxxxxx>
> Cc: Vitaly Prosyak <Vitaly.Prosyak@xxxxxxx>
> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F2048&amp;data=05%7C01%7Cluben.tuikov%40amd.com%7C13c905354c8b49ca582c08da658514fe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637933919507590618%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=CIO%2B%2BfexpAQFjyyy%2B6ZeTXQEEyrR9RIei3wEFq4OIaI%3D&amp;reserved=0
> Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c |  3 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h |  4 ++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 16 +++++++++++++---
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index 714178f1b6c6..2168163aad2d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -40,7 +40,7 @@ static void amdgpu_bo_list_free_rcu(struct rcu_head *rcu)
>  {
>  	struct amdgpu_bo_list *list = container_of(rcu, struct amdgpu_bo_list,
>  						   rhead);
> -
> +	mutex_destroy(&list->bo_list_mutex);
>  	kvfree(list);
>  }
>  
> @@ -136,6 +136,7 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>  
>  	trace_amdgpu_cs_bo_status(list->num_entries, total_size);
>  
> +	mutex_init(&list->bo_list_mutex);
>  	*result = list;
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> index 529d52a204cf..9caea1688fc3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
> @@ -47,6 +47,10 @@ struct amdgpu_bo_list {
>  	struct amdgpu_bo *oa_obj;
>  	unsigned first_userptr;
>  	unsigned num_entries;
> +
> +	/* Protect access during command submission.
> +	 */
> +	struct mutex bo_list_mutex;
>  };
>  
>  int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, int id,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index b28af04b0c3e..d8f1335bc68f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -519,6 +519,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  			return r;
>  	}
>  
> +	mutex_lock(&p->bo_list->bo_list_mutex);
> +
>  	/* One for TTM and one for the CS job */
>  	amdgpu_bo_list_for_each_entry(e, p->bo_list)
>  		e->tv.num_shared = 2;
> @@ -651,6 +653,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>  			kvfree(e->user_pages);
>  			e->user_pages = NULL;
>  		}
> +		mutex_unlock(&p->bo_list->bo_list_mutex);
>  	}
>  	return r;
>  }
> @@ -690,9 +693,11 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
>  {
>  	unsigned i;
>  
> -	if (error && backoff)
> +	if (error && backoff) {
>  		ttm_eu_backoff_reservation(&parser->ticket,
>  					   &parser->validated);
> +		mutex_unlock(&parser->bo_list->bo_list_mutex);
> +	}
>  
>  	for (i = 0; i < parser->num_post_deps; i++) {
>  		drm_syncobj_put(parser->post_deps[i].syncobj);
> @@ -832,12 +837,16 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>  			continue;
>  
>  		r = amdgpu_vm_bo_update(adev, bo_va, false);
> -		if (r)
> +		if (r) {
> +			mutex_unlock(&p->bo_list->bo_list_mutex);
>  			return r;
> +		}
>  
>  		r = amdgpu_sync_fence(&p->job->sync, bo_va->last_pt_update);
> -		if (r)
> +		if (r) {
> +			mutex_unlock(&p->bo_list->bo_list_mutex);
>  			return r;
> +		}
>  	}
>  
>  	r = amdgpu_vm_handle_moved(adev, vm);
> @@ -1278,6 +1287,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  
>  	ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
>  	mutex_unlock(&p->adev->notifier_lock);
> +	mutex_unlock(&p->bo_list->bo_list_mutex);
>  
>  	return 0;
>  

Regards,
-- 
Luben