Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Christian König <christian.koenig@xxxxxxx>]

Am 13.03.20 um 10:29 schrieb Pan, Xinhui:
> > 2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@xxxxxxx> 写道：
> >
> > Am 13.03.20 um 08:43 schrieb xinhui pan:
> > > The fence generated in ->commit is a shared one, so add it to resv.
> > > And we need do that with eviction lock hold.
> > >
> > > Currently we only sync last_direct/last_delayed before ->prepare. But we
> > > fail to sync the last fence generated by ->commit. That cuases problems
> > > if eviction happenes later, but it does not sync the last fence.
> > NAK, that won't work.
> >
> > We can only add fences when the dma_resv object is locked and that is only the case when validating.
> well, tha tis true.
> but considering this is a PT BO, and only eviction has race on it AFAIK.
> as for the individualized resv in bo release, we unref PT BO just after that.
> I am still thinking of other races in the real world.

We should probably just add all pipelined/delayed submissions directly 
to the reservation object in amdgpu_vm_sdma_commit().

Only the direct and invalidating submissions can't be added because we 
can't grab the reservation object in the MMU notifier.

Can you prepare a patch for this?

Regards,
Christian.

> thanks
> xinhui
>
> > I'm considering to just partially revert the patch originally stopping to add fences and instead only not add them when invalidating in a direct submit.
> >
> > Christian.
> >
> > > Cc: Christian König <christian.koenig@xxxxxxx>
> > > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > > Cc: Felix Kuehling <Felix.Kuehling@xxxxxxx>
> > > Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > index 73398831196f..f424b5969930 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> > >   	struct amdgpu_vm_update_params params;
> > >   	enum amdgpu_sync_mode sync_mode;
> > >   	int r;
> > > +	struct amdgpu_bo *root = vm->root.base.bo;
> > >     	memset(&params, 0, sizeof(params));
> > >   	params.adev = adev;
> > > @@ -1604,8 +1605,6 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> > >   	}
> > >     	if (flags & AMDGPU_PTE_VALID) {
> > > -		struct amdgpu_bo *root = vm->root.base.bo;
> > > -
> > >   		if (!dma_fence_is_signaled(vm->last_direct))
> > >   			amdgpu_bo_fence(root, vm->last_direct, true);
> > >   @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> > >     	r = vm->update_funcs->commit(&params, fence);
> > >   +	if (!dma_fence_is_signaled(vm->last_direct))
> > > +		amdgpu_bo_fence(root, vm->last_direct, true);
> > > +
> > > +	if (!dma_fence_is_signaled(vm->last_delayed))
> > > +		amdgpu_bo_fence(root, vm->last_delayed, true);
> > > +
> > >   error_unlock:
> > >   	amdgpu_vm_eviction_unlock(vm);
> > >   	return r;

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx