Re: [PATCH 2/3] drm/amdgpu: explicitely sync to VM updates

[Felix Kuehling <felix.kuehling@xxxxxxx>]

On 2019-12-04 10:38 a.m., Christian König wrote:
> Allows us to reduce the overhead while syncing to fences a bit.

This allows some further simplification. See two comments inline.


> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   | 18 +++++++-----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 22 +++++++++++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h |  2 ++
>   3 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 9e0c99760367..f21475352b88 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -799,29 +799,25 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   	if (r)
>   		return r;
>   
> -	r = amdgpu_sync_fence(adev, &p->job->sync,
> -			      fpriv->prt_va->last_pt_update, false);
> +	r = amdgpu_sync_vm_fence(adev, &p->job->sync,
> +				 fpriv->prt_va->last_pt_update);
>   	if (r)
>   		return r;
>   
>   	if (amdgpu_mcbp || amdgpu_sriov_vf(adev)) {
> -		struct dma_fence *f;
> -
>   		bo_va = fpriv->csa_va;
>   		BUG_ON(!bo_va);
>   		r = amdgpu_vm_bo_update(adev, bo_va, false);
>   		if (r)
>   			return r;
>   
> -		f = bo_va->last_pt_update;
> -		r = amdgpu_sync_fence(adev, &p->job->sync, f, false);
> +		r = amdgpu_sync_vm_fence(adev, &p->job->sync,
> +					 bo_va->last_pt_update);
>   		if (r)
>   			return r;
>   	}
>   
>   	amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> -		struct dma_fence *f;
> -
>   		/* ignore duplicates */
>   		bo = ttm_to_amdgpu_bo(e->tv.bo);
>   		if (!bo)
> @@ -835,8 +831,8 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   		if (r)
>   			return r;
>   
> -		f = bo_va->last_pt_update;
> -		r = amdgpu_sync_fence(adev, &p->job->sync, f, false);
> +		r = amdgpu_sync_vm_fence(adev, &p->job->sync,
> +					 bo_va->last_pt_update);
>   		if (r)
>   			return r;
>   	}
> @@ -849,7 +845,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   	if (r)
>   		return r;
>   
> -	r = amdgpu_sync_fence(adev, &p->job->sync, vm->last_update, false);
> +	r = amdgpu_sync_vm_fence(adev, &p->job->sync, vm->last_update);
>   	if (r)
>   		return r;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index 95e5e93edd18..9b28c1eb5f49 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -161,9 +161,6 @@ int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
>   
>   	if (!f)
>   		return 0;
> -	if (amdgpu_sync_same_dev(adev, f) &&
> -	    amdgpu_sync_get_owner(f) == AMDGPU_FENCE_OWNER_VM)
> -		amdgpu_sync_keep_later(&sync->last_vm_update, f);

If you remove this, you can remove the adev parameter from this function.


>   
>   	if (amdgpu_sync_add_later(sync, f, explicit))
>   		return 0;
> @@ -179,6 +176,25 @@ int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
>   	return 0;
>   }
>   
> +/**
> + * amdgpu_sync_vm_fence - remember to sync to this VM fence
> + *
> + * @adev: amdgpu device
> + * @sync: sync object to add fence to
> + * @fence: the VM fence to add
> + *
> + * Add the fence to the sync object and remember it as VM update.
> + */
> +int amdgpu_sync_vm_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
> +			 struct dma_fence *fence)
> +{
> +	if (!fence)
> +		return 0;
> +
> +	amdgpu_sync_keep_later(&sync->last_vm_update, fence);
> +	return amdgpu_sync_fence(adev, sync, fence, false);

Looks like you don't need adev here either, because you don't have the 
amdgpu_sync_same_dev condition any more.

Regards,
  Felix

> +}
> +
>   /**
>    * amdgpu_sync_resv - sync to a reservation object
>    *
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> index b5f1778a2319..ac210dd34371 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> @@ -42,6 +42,8 @@ struct amdgpu_sync {
>   void amdgpu_sync_create(struct amdgpu_sync *sync);
>   int amdgpu_sync_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
>   		      struct dma_fence *f, bool explicit);
> +int amdgpu_sync_vm_fence(struct amdgpu_device *adev, struct amdgpu_sync *sync,
> +			 struct dma_fence *fence);
>   int amdgpu_sync_resv(struct amdgpu_device *adev,
>   		     struct amdgpu_sync *sync,
>   		     struct dma_resv *resv,
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx