* [PATCH] drm/ttm: fix pipelined gutting for evictions
@ 2020-07-23 9:00 Christian König
2020-07-23 14:39 ` Felix Kuehling
2020-07-23 23:02 ` Felix Kuehling
0 siblings, 2 replies; 6+ messages in thread
From: Christian König @ 2020-07-23 9:00 UTC (permalink / raw
To: dri-devel; +Cc: Alex.Sierra, Philip.Yang, Felix.Kuehling
We can't pipeline that during eviction because the memory needs
to be available immediately.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index bc2230ecb7e3..122040056a07 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -651,8 +651,16 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
placement.num_busy_placement = 0;
bdev->driver->evict_flags(bo, &placement);
- if (!placement.num_placement && !placement.num_busy_placement)
- return ttm_bo_pipeline_gutting(bo);
+ if (!placement.num_placement && !placement.num_busy_placement) {
+ ttm_bo_wait(bo, false, false);
+
+ ttm_tt_destroy(bo->ttm);
+
+ memset(&bo->mem, 0, sizeof(bo->mem));
+ bo->mem.mem_type = TTM_PL_SYSTEM;
+ bo->ttm = NULL;
+ return 0;
+ }
evict_mem = bo->mem;
evict_mem.mm_node = NULL;
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/ttm: fix pipelined gutting for evictions
2020-07-23 9:00 [PATCH] drm/ttm: fix pipelined gutting for evictions Christian König
@ 2020-07-23 14:39 ` Felix Kuehling
2020-07-23 23:02 ` Felix Kuehling
1 sibling, 0 replies; 6+ messages in thread
From: Felix Kuehling @ 2020-07-23 14:39 UTC (permalink / raw
To: Christian König, dri-devel; +Cc: Alex.Sierra, Philip.Yang
Am 2020-07-23 um 5:00 a.m. schrieb Christian König:
> We can't pipeline that during eviction because the memory needs
> to be available immediately.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Looks good to me.
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
Alex, in this case the synchronous ttm_bo_wait would be triggering the
eviction fence rather than a delayed delete.
Scheduling an eviction worker, like we currently do, would only add
unnecessary latency here. The best place to do the HMM migration to
system memory synchronously and minimize the wait time here may be in
amdgpu_eviction_flags. That way all the SDMA copies to system memory
pages would already be in the pipe by the time we get to the ttm_bo_wait.
Regards,
Felix
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index bc2230ecb7e3..122040056a07 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -651,8 +651,16 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> placement.num_busy_placement = 0;
> bdev->driver->evict_flags(bo, &placement);
>
> - if (!placement.num_placement && !placement.num_busy_placement)
> - return ttm_bo_pipeline_gutting(bo);
> + if (!placement.num_placement && !placement.num_busy_placement) {
> + ttm_bo_wait(bo, false, false);
> +
> + ttm_tt_destroy(bo->ttm);
> +
> + memset(&bo->mem, 0, sizeof(bo->mem));
> + bo->mem.mem_type = TTM_PL_SYSTEM;
> + bo->ttm = NULL;
> + return 0;
> + }
>
> evict_mem = bo->mem;
> evict_mem.mm_node = NULL;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/ttm: fix pipelined gutting for evictions
2020-07-23 9:00 [PATCH] drm/ttm: fix pipelined gutting for evictions Christian König
2020-07-23 14:39 ` Felix Kuehling
@ 2020-07-23 23:02 ` Felix Kuehling
2020-07-24 1:58 ` philip yang
1 sibling, 1 reply; 6+ messages in thread
From: Felix Kuehling @ 2020-07-23 23:02 UTC (permalink / raw
To: Christian König, dri-devel; +Cc: Alex.Sierra, Philip.Yang
Am 2020-07-23 um 5:00 a.m. schrieb Christian König:
> We can't pipeline that during eviction because the memory needs
> to be available immediately.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index bc2230ecb7e3..122040056a07 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -651,8 +651,16 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
> placement.num_busy_placement = 0;
> bdev->driver->evict_flags(bo, &placement);
>
> - if (!placement.num_placement && !placement.num_busy_placement)
> - return ttm_bo_pipeline_gutting(bo);
> + if (!placement.num_placement && !placement.num_busy_placement) {
> + ttm_bo_wait(bo, false, false);
> +
> + ttm_tt_destroy(bo->ttm);
> +
> + memset(&bo->mem, 0, sizeof(bo->mem));
Where does the memory in the bo->mem (ttm_mem_reg) get destroyed? It
doesn't get attached to a ghost BO in this case, so someone will have to
call ttm_bo_mem_put explicitly before you wipe out bo->mem.
Regards,
Felix
> + bo->mem.mem_type = TTM_PL_SYSTEM;
> + bo->ttm = NULL;
> + return 0;
> + }
>
> evict_mem = bo->mem;
> evict_mem.mm_node = NULL;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/ttm: fix pipelined gutting for evictions
2020-07-23 23:02 ` Felix Kuehling
@ 2020-07-24 1:58 ` philip yang
2020-07-24 2:56 ` Felix Kuehling
0 siblings, 1 reply; 6+ messages in thread
From: philip yang @ 2020-07-24 1:58 UTC (permalink / raw
To: Felix Kuehling, Christian König, dri-devel; +Cc: Alex.Sierra, Philip.Yang
On 2020-07-23 7:02 p.m., Felix Kuehling wrote:
> Am 2020-07-23 um 5:00 a.m. schrieb Christian König:
>> We can't pipeline that during eviction because the memory needs
>> to be available immediately.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index bc2230ecb7e3..122040056a07 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -651,8 +651,16 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
>> placement.num_busy_placement = 0;
>> bdev->driver->evict_flags(bo, &placement);
>>
>> - if (!placement.num_placement && !placement.num_busy_placement)
>> - return ttm_bo_pipeline_gutting(bo);
>> + if (!placement.num_placement && !placement.num_busy_placement) {
>> + ttm_bo_wait(bo, false, false);
>> +
>> + ttm_tt_destroy(bo->ttm);
>> +
>> + memset(&bo->mem, 0, sizeof(bo->mem));
> Where does the memory in the bo->mem (ttm_mem_reg) get destroyed? It
> doesn't get attached to a ghost BO in this case, so someone will have to
> call ttm_bo_mem_put explicitly before you wipe out bo->mem.
After migrating to ram,
svm_range_bo_unref-->amdgpu_unref_bo->ttm_bo_put->ttm_bo_release calls
ttm_bo_mem_put.
vram is already freed before we signal fence, right?
Regards,
Philip
> Regards,
> Felix
>
>
>> + bo->mem.mem_type = TTM_PL_SYSTEM;
>> + bo->ttm = NULL;
>> + return 0;
>> + }
>>
>> evict_mem = bo->mem;
>> evict_mem.mm_node = NULL;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/ttm: fix pipelined gutting for evictions
2020-07-24 1:58 ` philip yang
@ 2020-07-24 2:56 ` Felix Kuehling
2020-07-24 7:21 ` Christian König
0 siblings, 1 reply; 6+ messages in thread
From: Felix Kuehling @ 2020-07-24 2:56 UTC (permalink / raw
To: philip yang, Christian König, dri-devel; +Cc: Alex.Sierra, Philip.Yang
Am 2020-07-23 um 9:58 p.m. schrieb philip yang:
>
> On 2020-07-23 7:02 p.m., Felix Kuehling wrote:
>> Am 2020-07-23 um 5:00 a.m. schrieb Christian König:
>>> We can't pipeline that during eviction because the memory needs
>>> to be available immediately.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>> index bc2230ecb7e3..122040056a07 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>> @@ -651,8 +651,16 @@ static int ttm_bo_evict(struct
>>> ttm_buffer_object *bo,
>>> placement.num_busy_placement = 0;
>>> bdev->driver->evict_flags(bo, &placement);
>>> - if (!placement.num_placement && !placement.num_busy_placement)
>>> - return ttm_bo_pipeline_gutting(bo);
>>> + if (!placement.num_placement && !placement.num_busy_placement) {
>>> + ttm_bo_wait(bo, false, false);
>>> +
>>> + ttm_tt_destroy(bo->ttm);
>>> +
>>> + memset(&bo->mem, 0, sizeof(bo->mem));
>> Where does the memory in the bo->mem (ttm_mem_reg) get destroyed? It
>> doesn't get attached to a ghost BO in this case, so someone will have to
>> call ttm_bo_mem_put explicitly before you wipe out bo->mem.
>
> After migrating to ram,
> svm_range_bo_unref-->amdgpu_unref_bo->ttm_bo_put->ttm_bo_release calls
> ttm_bo_mem_put.
amdgpu_bo_unref won't free anything if the reference count doesn't go to
0. And TTM is still holding a reference here. The BO won't be freed
until ttm_mem_evict_first calls ttm_bo_put.
The memset above overwrites the ttm_mem_reg structure, which means it
will forget about the VRAM nodes held by the BO. They need to be
released first. That's what frees the space that ttm_bo_evict was trying
to make available in the first place.
Regards,
Felix
> vram is already freed before we signal fence, right?
>
> Regards,
>
> Philip
>
>> Regards,
>> Felix
>>
>>
>>> + bo->mem.mem_type = TTM_PL_SYSTEM;
>>> + bo->ttm = NULL;
>>> + return 0;
>>> + }
>>> evict_mem = bo->mem;
>>> evict_mem.mm_node = NULL;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/ttm: fix pipelined gutting for evictions
2020-07-24 2:56 ` Felix Kuehling
@ 2020-07-24 7:21 ` Christian König
0 siblings, 0 replies; 6+ messages in thread
From: Christian König @ 2020-07-24 7:21 UTC (permalink / raw
To: Felix Kuehling, philip yang, dri-devel; +Cc: Alex.Sierra, Philip.Yang
Am 24.07.20 um 04:56 schrieb Felix Kuehling:
> Am 2020-07-23 um 9:58 p.m. schrieb philip yang:
>> On 2020-07-23 7:02 p.m., Felix Kuehling wrote:
>>> Am 2020-07-23 um 5:00 a.m. schrieb Christian König:
>>>> We can't pipeline that during eviction because the memory needs
>>>> to be available immediately.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>> drivers/gpu/drm/ttm/ttm_bo.c | 12 ++++++++++--
>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> index bc2230ecb7e3..122040056a07 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> @@ -651,8 +651,16 @@ static int ttm_bo_evict(struct
>>>> ttm_buffer_object *bo,
>>>> placement.num_busy_placement = 0;
>>>> bdev->driver->evict_flags(bo, &placement);
>>>> - if (!placement.num_placement && !placement.num_busy_placement)
>>>> - return ttm_bo_pipeline_gutting(bo);
>>>> + if (!placement.num_placement && !placement.num_busy_placement) {
>>>> + ttm_bo_wait(bo, false, false);
>>>> +
>>>> + ttm_tt_destroy(bo->ttm);
>>>> +
>>>> + memset(&bo->mem, 0, sizeof(bo->mem));
>>> Where does the memory in the bo->mem (ttm_mem_reg) get destroyed? It
>>> doesn't get attached to a ghost BO in this case, so someone will have to
>>> call ttm_bo_mem_put explicitly before you wipe out bo->mem.
>> After migrating to ram,
>> svm_range_bo_unref-->amdgpu_unref_bo->ttm_bo_put->ttm_bo_release calls
>> ttm_bo_mem_put.
> amdgpu_bo_unref won't free anything if the reference count doesn't go to
> 0. And TTM is still holding a reference here. The BO won't be freed
> until ttm_mem_evict_first calls ttm_bo_put.
>
> The memset above overwrites the ttm_mem_reg structure, which means it
> will forget about the VRAM nodes held by the BO. They need to be
> released first. That's what frees the space that ttm_bo_evict was trying
> to make available in the first place.
Yeah, Felix is right that this is a bug.
Going to send a v2 in a minute.
Thanks,
Christian.
>
> Regards,
> Felix
>
>
>> vram is already freed before we signal fence, right?
>>
>> Regards,
>>
>> Philip
>>
>>> Regards,
>>> Felix
>>>
>>>
>>>> + bo->mem.mem_type = TTM_PL_SYSTEM;
>>>> + bo->ttm = NULL;
>>>> + return 0;
>>>> + }
>>>> evict_mem = bo->mem;
>>>> evict_mem.mm_node = NULL;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-24 7:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-23 9:00 [PATCH] drm/ttm: fix pipelined gutting for evictions Christian König
2020-07-23 14:39 ` Felix Kuehling
2020-07-23 23:02 ` Felix Kuehling
2020-07-24 1:58 ` philip yang
2020-07-24 2:56 ` Felix Kuehling
2020-07-24 7:21 ` Christian König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.