[PATCH] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[PATCH] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Without this, there could not be enough slots, which could trigger the
BUG_ON in reservation_object_add_shared_fence.

Cc: stable@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavrilov@gmail.com
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 590db78b8c72..75579200f4a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 		struct amdgpu_ring *ring;
 		struct dma_fence *fence;
 
+		r = reservation_object_reserve_shared(root->tbo.resv);
+		if (r)
+			return r;
+
 		ring = container_of(vm->entity.sched, struct amdgpu_ring,
 				    sched);
 
-- 
2.17.1

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

Re: [PATCH] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Zhang, Jerry (Junwei)]

On 06/23/2018 12:42 AM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> Without this, there could not be enough slots, which could trigger the
> BUG_ON in reservation_object_add_shared_fence.
>
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/106418
> Reported-by: mikhail.v.gavrilov@gmail.com
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 590db78b8c72..75579200f4a6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   		struct amdgpu_ring *ring;
>   		struct dma_fence *fence;
>
> +		r = reservation_object_reserve_shared(root->tbo.resv);
> +		if (r)
> +			return r;

Good catch.

But we may "goto error" here to do some cleanup.

Jerry

> +
>   		ring = container_of(vm->entity.sched, struct amdgpu_ring,
>   				    sched);
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Michel Dänzer]

On 2018-06-25 04:15 AM, Zhang, Jerry (Junwei) wrote:
> On 06/23/2018 12:42 AM, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> Without this, there could not be enough slots, which could trigger the
>> BUG_ON in reservation_object_add_shared_fence.
>>
>> Cc: stable@vger.kernel.org
>> Bugzilla: https://bugs.freedesktop.org/106418
>> Reported-by: mikhail.v.gavrilov@gmail.com
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 590db78b8c72..75579200f4a6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct
>> amdgpu_device *adev,
>>           struct amdgpu_ring *ring;
>>           struct dma_fence *fence;
>>
>> +        r = reservation_object_reserve_shared(root->tbo.resv);
>> +        if (r)
>> +            return r;
> 
> Good catch.
> 
> But we may "goto error" here to do some cleanup.

Indeed, thanks, fixed in v2.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Without this, there could not be enough slots, which could trigger the
BUG_ON in reservation_object_add_shared_fence.

v2:
* Jump to the error label instead of returning directly (Jerry Zhang)

Cc: stable@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/106418
Reported-by: mikhail.v.gavrilov@gmail.com
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 590db78b8c72..579bcc33d544 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 		struct amdgpu_ring *ring;
 		struct dma_fence *fence;
 
+		r = reservation_object_reserve_shared(root->tbo.resv);
+		if (r)
+			goto error;
+
 		ring = container_of(vm->entity.sched, struct amdgpu_ring,
 				    sched);
 
-- 
2.17.1

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

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Zhang, Jerry (Junwei)]

On 06/25/2018 05:07 PM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> Without this, there could not be enough slots, which could trigger the
> BUG_ON in reservation_object_add_shared_fence.
>
> v2:
> * Jump to the error label instead of returning directly (Jerry Zhang)
>
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/106418
> Reported-by: mikhail.v.gavrilov@gmail.com
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 590db78b8c72..579bcc33d544 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   		struct amdgpu_ring *ring;
>   		struct dma_fence *fence;
>
> +		r = reservation_object_reserve_shared(root->tbo.resv);
> +		if (r)
> +			goto error;
> +
>   		ring = container_of(vm->entity.sched, struct amdgpu_ring,
>   				    sched);
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Christian König]

Am 25.06.2018 um 11:07 schrieb Michel Dänzer:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> Without this, there could not be enough slots, which could trigger the
> BUG_ON in reservation_object_add_shared_fence.
>
> v2:
> * Jump to the error label instead of returning directly (Jerry Zhang)

Well good catch, but NAK that is not the right solution to the issue.

amdgpu_vm_update_directories() is only called by two places, the first 
one is amdgpu_bo_vm_update_pte() and the second amdgpu_gem_va_update_vm().

In both cases the directory is reserved using ttm_eu_reserve_buffers() 
with "entry->tv.shared = true;".

What most likely needs to be done instead is to call 
reservation_object_reserve_shared() after all the VM operations in 
amdgpu_bo_vm_update_pte() to make sure that we still have one additional 
slot for the command submission.

Regards,
Christian.

>
> Cc: stable@vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/106418
> Reported-by: mikhail.v.gavrilov@gmail.com
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 590db78b8c72..579bcc33d544 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>   		struct amdgpu_ring *ring;
>   		struct dma_fence *fence;
>   
> +		r = reservation_object_reserve_shared(root->tbo.resv);
> +		if (r)
> +			goto error;
> +
>   		ring = container_of(vm->entity.sched, struct amdgpu_ring,
>   				    sched);
>   

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

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Zhang, Jerry (Junwei)]

On 07/02/2018 05:23 PM, Christian König wrote:
> Am 25.06.2018 um 11:07 schrieb Michel Dänzer:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> Without this, there could not be enough slots, which could trigger the
>> BUG_ON in reservation_object_add_shared_fence.
>>
>> v2:
>> * Jump to the error label instead of returning directly (Jerry Zhang)
>
> Well good catch, but NAK that is not the right solution to the issue.
>
> amdgpu_vm_update_directories() is only called by two places, the first one is amdgpu_bo_vm_update_pte() and the second amdgpu_gem_va_update_vm().
>
> In both cases the directory is reserved using ttm_eu_reserve_buffers() with "entry->tv.shared = true;".
>
> What most likely needs to be done instead is to call reservation_object_reserve_shared() after all the VM operations in amdgpu_bo_vm_update_pte() to make sure that we still have one additional slot
> for the command submission.

In this case, it looks like amdgpu_vm_bo_update_mapping() handling, inside ttm_eu_reserve_buffers() as well.

Is there any specific reason to call it at the end of amdgpu_bo_vm_update_pte()?

AFAIW, that has to be called right before where to add shared slot.

Jerry

>
> Regards,
> Christian.
>
>>
>> Cc: stable@vger.kernel.org
>> Bugzilla: https://bugs.freedesktop.org/106418
>> Reported-by: mikhail.v.gavrilov@gmail.com
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 590db78b8c72..579bcc33d544 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>           struct amdgpu_ring *ring;
>>           struct dma_fence *fence;
>> +        r = reservation_object_reserve_shared(root->tbo.resv);
>> +        if (r)
>> +            goto error;
>> +
>>           ring = container_of(vm->entity.sched, struct amdgpu_ring,
>>                       sched);
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Christian König]

Am 03.07.2018 um 03:59 schrieb Zhang, Jerry (Junwei):
> On 07/02/2018 05:23 PM, Christian König wrote:
>> Am 25.06.2018 um 11:07 schrieb Michel Dänzer:
>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>> Without this, there could not be enough slots, which could trigger the
>>> BUG_ON in reservation_object_add_shared_fence.
>>>
>>> v2:
>>> * Jump to the error label instead of returning directly (Jerry Zhang)
>>
>> Well good catch, but NAK that is not the right solution to the issue.
>>
>> amdgpu_vm_update_directories() is only called by two places, the 
>> first one is amdgpu_bo_vm_update_pte() and the second 
>> amdgpu_gem_va_update_vm().
>>
>> In both cases the directory is reserved using 
>> ttm_eu_reserve_buffers() with "entry->tv.shared = true;".
>>
>> What most likely needs to be done instead is to call 
>> reservation_object_reserve_shared() after all the VM operations in 
>> amdgpu_bo_vm_update_pte() to make sure that we still have one 
>> additional slot
>> for the command submission.
>
> In this case, it looks like amdgpu_vm_bo_update_mapping() handling, 
> inside ttm_eu_reserve_buffers() as well.
>
> Is there any specific reason to call it at the end of 
> amdgpu_bo_vm_update_pte()?
>
> AFAIW, that has to be called right before where to add shared slot.

The reservation_object_reserve_shared() function needs to be called 
before you make submissions to the hardware to make sure that you have a 
free slot after you made the submission.

The problem is now that we always reserve one free slot during command 
submission, but for the page directory reservation object it can happen 
that this free slot is used by VM updates.

So the safest approach is to make sure that we have another free slot 
after the VM updates are done.

Michel's patch nearly did that, because we rarely have only one 
submission in amdgpu_vm_update_directories(). But it isn't 100% correct.

Christian.

>
> Jerry
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Cc: stable@vger.kernel.org
>>> Bugzilla: https://bugs.freedesktop.org/106418
>>> Reported-by: mikhail.v.gavrilov@gmail.com
>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 590db78b8c72..579bcc33d544 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct 
>>> amdgpu_device *adev,
>>>           struct amdgpu_ring *ring;
>>>           struct dma_fence *fence;
>>> +        r = reservation_object_reserve_shared(root->tbo.resv);
>>> +        if (r)
>>> +            goto error;
>>> +
>>>           ring = container_of(vm->entity.sched, struct amdgpu_ring,
>>>                       sched);
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Zhang, Jerry (Junwei)]

On 07/03/2018 02:58 PM, Christian König wrote:
> Am 03.07.2018 um 03:59 schrieb Zhang, Jerry (Junwei):
>> On 07/02/2018 05:23 PM, Christian König wrote:
>>> Am 25.06.2018 um 11:07 schrieb Michel Dänzer:
>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>
>>>> Without this, there could not be enough slots, which could trigger the
>>>> BUG_ON in reservation_object_add_shared_fence.
>>>>
>>>> v2:
>>>> * Jump to the error label instead of returning directly (Jerry Zhang)
>>>
>>> Well good catch, but NAK that is not the right solution to the issue.
>>>
>>> amdgpu_vm_update_directories() is only called by two places, the first one is amdgpu_bo_vm_update_pte() and the second amdgpu_gem_va_update_vm().
>>>
>>> In both cases the directory is reserved using ttm_eu_reserve_buffers() with "entry->tv.shared = true;".
>>>
>>> What most likely needs to be done instead is to call reservation_object_reserve_shared() after all the VM operations in amdgpu_bo_vm_update_pte() to make sure that we still have one additional slot
>>> for the command submission.
>>
>> In this case, it looks like amdgpu_vm_bo_update_mapping() handling, inside ttm_eu_reserve_buffers() as well.
>>
>> Is there any specific reason to call it at the end of amdgpu_bo_vm_update_pte()?
>>
>> AFAIW, that has to be called right before where to add shared slot.
>
> The reservation_object_reserve_shared() function needs to be called before you make submissions to the hardware to make sure that you have a free slot after you made the submission.
>
> The problem is now that we always reserve one free slot during command submission, but for the page directory reservation object it can happen that this free slot is used by VM updates.
>
> So the safest approach is to make sure that we have another free slot after the VM updates are done.
>
> Michel's patch nearly did that, because we rarely have only one submission in amdgpu_vm_update_directories(). But it isn't 100% correct.

Thanks to explain that, I think got your meaning that we should reserve slot for command submission after VM update.

if so, shall we add reservation_object_reserve_shared() for both of them, since any of place may run out of slot.

While the gap from me is I fail to find reserving slot during command submission,
the root bo reservation seems to be used by VM update only(if not correct me).
command submission is likely to use bo's reservation from bo list rather than root bo reservation.

BTW, reservation_object_reserve_shared() will extend the max num as 2 times if it's not enough.
(default value is 4)

Jerry

>
> Christian.
>
>>
>> Jerry
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Bugzilla: https://bugs.freedesktop.org/106418
>>>> Reported-by: mikhail.v.gavrilov@gmail.com
>>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 590db78b8c72..579bcc33d544 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1111,6 +1111,10 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
>>>>           struct amdgpu_ring *ring;
>>>>           struct dma_fence *fence;
>>>> +        r = reservation_object_reserve_shared(root->tbo.resv);
>>>> +        if (r)
>>>> +            goto error;
>>>> +
>>>>           ring = container_of(vm->entity.sched, struct amdgpu_ring,
>>>>                       sched);
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Michel Dänzer]

On 2018-07-03 10:12 AM, Zhang, Jerry (Junwei) wrote:
> 
> BTW, reservation_object_reserve_shared() will extend the max num as 2
> times if it's not enough.
> (default value is 4)

That's an implementation detail and depends on the circumstances. The
only guarantee is that there's space for at least one fence after
calling reservation_object_reserve_shared, until
reservation_object_add_shared_fence is called.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Zhang, Jerry (Junwei)]

On 07/03/2018 04:28 PM, Michel Dänzer wrote:
> On 2018-07-03 10:12 AM, Zhang, Jerry (Junwei) wrote:
>>
>> BTW, reservation_object_reserve_shared() will extend the max num as 2
>> times if it's not enough.
>> (default value is 4)
>
> That's an implementation detail and depends on the circumstances. The
> only guarantee is that there's space for at least one fence after
> calling reservation_object_reserve_shared, until
> reservation_object_add_shared_fence is called.

yes, it's a bit more detail discussion about that.
Anyway, it should reserve at least one for shared reservation.
Let's back to the upper level discussion.

Jerry

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

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Christian König]

Am 03.07.2018 um 10:12 schrieb Zhang, Jerry (Junwei):
> On 07/03/2018 02:58 PM, Christian König wrote:
>> Am 03.07.2018 um 03:59 schrieb Zhang, Jerry (Junwei):
>>> On 07/02/2018 05:23 PM, Christian König wrote:
>>>> [SNIP]
>> The reservation_object_reserve_shared() function needs to be called 
>> before you make submissions to the hardware to make sure that you 
>> have a free slot after you made the submission.
>>
>> The problem is now that we always reserve one free slot during 
>> command submission, but for the page directory reservation object it 
>> can happen that this free slot is used by VM updates.
>>
>> So the safest approach is to make sure that we have another free slot 
>> after the VM updates are done.
>>
>> Michel's patch nearly did that, because we rarely have only one 
>> submission in amdgpu_vm_update_directories(). But it isn't 100% correct.
>
> Thanks to explain that, I think got your meaning that we should 
> reserve slot for command submission after VM update.
>
> if so, shall we add reservation_object_reserve_shared() for both of 
> them, since any of place may run out of slot.
>
> While the gap from me is I fail to find reserving slot during command 
> submission,

That's done by ttm_eu_reserve_buffers().

> the root bo reservation seems to be used by VM update only(if not 
> correct me).

No, that is not correct. The root BO is also part of the list of BOs for 
the command submission used by ttm_eu_reserve_buffers() and 
ttm_eu_fence_buffer_objects().

All VM updates will use the same slot, so we just need one extra 
reservation_object_reserve_shared() after all the VM updates are done.

Regards,
Christian.

> command submission is likely to use bo's reservation from bo list 
> rather than root bo reservation.
>
> BTW, reservation_object_reserve_shared() will extend the max num as 2 
> times if it's not enough.
> (default value is 4)
>
> Jerry

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

Re: [PATCH v2] drm/amdgpu: Reserve shared fence slot in amdgpu_vm_update_directories

[Zhang, Jerry (Junwei)]

On 07/03/2018 04:42 PM, Christian König wrote:
> Am 03.07.2018 um 10:12 schrieb Zhang, Jerry (Junwei):
>> On 07/03/2018 02:58 PM, Christian König wrote:
>>> Am 03.07.2018 um 03:59 schrieb Zhang, Jerry (Junwei):
>>>> On 07/02/2018 05:23 PM, Christian König wrote:
>>>>> [SNIP]
>>> The reservation_object_reserve_shared() function needs to be called before you make submissions to the hardware to make sure that you have a free slot after you made the submission.
>>>
>>> The problem is now that we always reserve one free slot during command submission, but for the page directory reservation object it can happen that this free slot is used by VM updates.
>>>
>>> So the safest approach is to make sure that we have another free slot after the VM updates are done.
>>>
>>> Michel's patch nearly did that, because we rarely have only one submission in amdgpu_vm_update_directories(). But it isn't 100% correct.
>>
>> Thanks to explain that, I think got your meaning that we should reserve slot for command submission after VM update.
>>
>> if so, shall we add reservation_object_reserve_shared() for both of them, since any of place may run out of slot.
>>
>> While the gap from me is I fail to find reserving slot during command submission,
>
> That's done by ttm_eu_reserve_buffers().
>
>> the root bo reservation seems to be used by VM update only(if not correct me).
>
> No, that is not correct. The root BO is also part of the list of BOs for the command submission used by ttm_eu_reserve_buffers() and ttm_eu_fence_buffer_objects().
>
> All VM updates will use the same slot, so we just need one extra reservation_object_reserve_shared() after all the VM updates are done.

Yes, thanks to point it out.
the root bo indeed needs at least one extra shared slot for command submission.

Jerry

>
> Regards,
> Christian.
>
>> command submission is likely to use bo's reservation from bo list rather than root bo reservation.
>>
>> BTW, reservation_object_reserve_shared() will extend the max num as 2 times if it's not enough.
>> (default value is 4)
>>
>> Jerry
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx