[PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Christian König]

From: Christian König <christian.koenig@amd.com>

We still need to unbind explicitely during a move.

This partial reverts commit ff20caa0bcbfef9f7686f8d1868a3b990921afd6.

v2: remove unnecessary check and unused variable

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c |  1 +
 drivers/gpu/drm/ttm/ttm_tt.c      | 19 ++++++++++++-------
 include/drm/ttm/ttm_bo_driver.h   |  9 +++++++++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 0c389a5..6c412c3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -53,6 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
 	int ret;
 
 	if (old_mem->mem_type != TTM_PL_SYSTEM) {
+		ttm_tt_unbind(ttm);
 		ttm_bo_free_old_node(bo);
 		ttm_flag_masked(&old_mem->placement, TTM_PL_FLAG_SYSTEM,
 				TTM_PL_MASK_MEM);
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
index d28d4333..526e5a7 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -166,16 +166,10 @@ EXPORT_SYMBOL(ttm_tt_set_placement_caching);
 
 void ttm_tt_destroy(struct ttm_tt *ttm)
 {
-	int ret;
-
 	if (ttm == NULL)
 		return;
 
-	if (ttm->state == tt_bound) {
-		ret = ttm->func->unbind(ttm);
-		BUG_ON(ret);
-		ttm->state = tt_unbound;
-	}
+	ttm_tt_unbind(ttm);
 
 	if (ttm->state == tt_unbound)
 		ttm_tt_unpopulate(ttm);
@@ -255,6 +249,17 @@ void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma)
 }
 EXPORT_SYMBOL(ttm_dma_tt_fini);
 
+void ttm_tt_unbind(struct ttm_tt *ttm)
+{
+	int ret;
+
+	if (ttm->state == tt_bound) {
+		ret = ttm->func->unbind(ttm);
+		BUG_ON(ret);
+		ttm->state = tt_unbound;
+	}
+}
+
 int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem)
 {
 	int ret = 0;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 141cefb..e7d81a8 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -628,6 +628,15 @@ extern int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);
 extern void ttm_tt_destroy(struct ttm_tt *ttm);
 
 /**
+ * ttm_ttm_unbind:
+ *
+ * @ttm: The struct ttm_tt.
+ *
+ * Unbind a struct ttm_tt.
+ */
+extern void ttm_tt_unbind(struct ttm_tt *ttm);
+
+/**
  * ttm_tt_swapin:
  *
  * @ttm: The struct ttm_tt.
-- 
2.5.0

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

Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Sean Paul]

On Fri, Jul 22, 2016 at 9:10 AM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> We still need to unbind explicitely during a move.

Sorry for the drive-by nit, but:

s/explicitely/explicitly/


>
> This partial reverts commit ff20caa0bcbfef9f7686f8d1868a3b990921afd6.
>
> v2: remove unnecessary check and unused variable
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo_util.c |  1 +
>  drivers/gpu/drm/ttm/ttm_tt.c      | 19 ++++++++++++-------
>  include/drm/ttm/ttm_bo_driver.h   |  9 +++++++++
>  3 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 0c389a5..6c412c3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -53,6 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
>         int ret;
>
>         if (old_mem->mem_type != TTM_PL_SYSTEM) {
> +               ttm_tt_unbind(ttm);
>                 ttm_bo_free_old_node(bo);
>                 ttm_flag_masked(&old_mem->placement, TTM_PL_FLAG_SYSTEM,
>                                 TTM_PL_MASK_MEM);
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index d28d4333..526e5a7 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -166,16 +166,10 @@ EXPORT_SYMBOL(ttm_tt_set_placement_caching);
>
>  void ttm_tt_destroy(struct ttm_tt *ttm)
>  {
> -       int ret;
> -
>         if (ttm == NULL)
>                 return;
>
> -       if (ttm->state == tt_bound) {
> -               ret = ttm->func->unbind(ttm);
> -               BUG_ON(ret);
> -               ttm->state = tt_unbound;
> -       }
> +       ttm_tt_unbind(ttm);
>
>         if (ttm->state == tt_unbound)
>                 ttm_tt_unpopulate(ttm);
> @@ -255,6 +249,17 @@ void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma)
>  }
>  EXPORT_SYMBOL(ttm_dma_tt_fini);
>
> +void ttm_tt_unbind(struct ttm_tt *ttm)
> +{
> +       int ret;
> +
> +       if (ttm->state == tt_bound) {
> +               ret = ttm->func->unbind(ttm);
> +               BUG_ON(ret);
> +               ttm->state = tt_unbound;
> +       }
> +}
> +
>  int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem)
>  {
>         int ret = 0;
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 141cefb..e7d81a8 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -628,6 +628,15 @@ extern int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);
>  extern void ttm_tt_destroy(struct ttm_tt *ttm);
>
>  /**
> + * ttm_ttm_unbind:
> + *
> + * @ttm: The struct ttm_tt.
> + *
> + * Unbind a struct ttm_tt.
> + */
> +extern void ttm_tt_unbind(struct ttm_tt *ttm);
> +
> +/**
>   * ttm_tt_swapin:
>   *
>   * @ttm: The struct ttm_tt.
> --
> 2.5.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Edward O'Callaghan]

[-- Attachment #1.1.1: Type: text/plain, Size: 2814 bytes --]

Reviewed-by: Edward O'Callaghan <funfunctor-dczkZgxz+BNUPWh3PAxdjQ@public.gmane.org>

On 07/22/2016 11:10 PM, Christian König wrote:
> From: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
> 
> We still need to unbind explicitely during a move.
> 
> This partial reverts commit ff20caa0bcbfef9f7686f8d1868a3b990921afd6.
> 
> v2: remove unnecessary check and unused variable
> 
> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/gpu/drm/ttm/ttm_bo_util.c |  1 +
>  drivers/gpu/drm/ttm/ttm_tt.c      | 19 ++++++++++++-------
>  include/drm/ttm/ttm_bo_driver.h   |  9 +++++++++
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 0c389a5..6c412c3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -53,6 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
>  	int ret;
>  
>  	if (old_mem->mem_type != TTM_PL_SYSTEM) {
> +		ttm_tt_unbind(ttm);
>  		ttm_bo_free_old_node(bo);
>  		ttm_flag_masked(&old_mem->placement, TTM_PL_FLAG_SYSTEM,
>  				TTM_PL_MASK_MEM);
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index d28d4333..526e5a7 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -166,16 +166,10 @@ EXPORT_SYMBOL(ttm_tt_set_placement_caching);
>  
>  void ttm_tt_destroy(struct ttm_tt *ttm)
>  {
> -	int ret;
> -
>  	if (ttm == NULL)
>  		return;
>  
> -	if (ttm->state == tt_bound) {
> -		ret = ttm->func->unbind(ttm);
> -		BUG_ON(ret);
> -		ttm->state = tt_unbound;
> -	}
> +	ttm_tt_unbind(ttm);
>  
>  	if (ttm->state == tt_unbound)
>  		ttm_tt_unpopulate(ttm);
> @@ -255,6 +249,17 @@ void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma)
>  }
>  EXPORT_SYMBOL(ttm_dma_tt_fini);
>  
> +void ttm_tt_unbind(struct ttm_tt *ttm)
> +{
> +	int ret;
> +
> +	if (ttm->state == tt_bound) {
> +		ret = ttm->func->unbind(ttm);
> +		BUG_ON(ret);
> +		ttm->state = tt_unbound;
> +	}
> +}
> +
>  int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem)
>  {
>  	int ret = 0;
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 141cefb..e7d81a8 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -628,6 +628,15 @@ extern int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);
>  extern void ttm_tt_destroy(struct ttm_tt *ttm);
>  
>  /**
> + * ttm_ttm_unbind:
> + *
> + * @ttm: The struct ttm_tt.
> + *
> + * Unbind a struct ttm_tt.
> + */
> +extern void ttm_tt_unbind(struct ttm_tt *ttm);
> +
> +/**
>   * ttm_tt_swapin:
>   *
>   * @ttm: The struct ttm_tt.
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Michel Dänzer]

On 22.07.2016 22:10, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
> 
> We still need to unbind explicitely during a move.

This change fixed a hang for me when running the piglit test
max-texture-size with the radeon driver on Kaveri.

However, there's still a similar hang left when letting the piglit test
tex3d-maxsize run concurrently with other tests (running tex3d-maxsize
alone doesn't hang, but fails due to running out of GPU memory; that's a
recent radeonsi regression). There are

 [TTM] Buffer eviction failed

messages in dmesg shortly before the hang.

I haven't seen such hangs with older kernels. Any ideas offhand what the
problem could be? If not, I'll try bisecting.


-- 
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] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Felix Kuehling]

We're also looking into a hang with a KFD unit test that allocates lots
of memory and fragments it deliberately, without mapping it all at once.
It's a new problem for us as we're rebasing on amd-staging-4.6.
Something weird seems to be happening with evictions, but I haven't been
able to figure it out.

I was able to see that SDMA page table updates stop working at some
point, though SDMA fences are still signaling. If I let the test run
longer, SDMA and CP hang. I dumped the SDMA IBs and didn't see anything
suspicious. My guess was that maybe the SDMA IBs or the ring are getting
corrupted, or maybe the GART table entries for the IBs or ring are
corrupted. But I haven't been able to prove that or track it down to a
root cause. We're now trying to reimplement the test using libdrm-amdgpu
APIs so we can bisect on the amd-staging-4.6 branch without KFD.

Regards,
  Felix

On 16-07-26 10:26 PM, Michel Dänzer wrote:
> On 22.07.2016 22:10, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> We still need to unbind explicitely during a move.
> This change fixed a hang for me when running the piglit test
> max-texture-size with the radeon driver on Kaveri.
>
> However, there's still a similar hang left when letting the piglit test
> tex3d-maxsize run concurrently with other tests (running tex3d-maxsize
> alone doesn't hang, but fails due to running out of GPU memory; that's a
> recent radeonsi regression). There are
>
>  [TTM] Buffer eviction failed
>
> messages in dmesg shortly before the hang.
>
> I haven't seen such hangs with older kernels. Any ideas offhand what the
> problem could be? If not, I'll try bisecting.
>
>

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

Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Felix Kuehling]

For the record, Michel's patch "drm/ttm: Wait for a BO to become idle
before unbinding it from GTT" fixes our KFD problem as well.

Thanks,
  Felix

On 16-07-27 05:27 PM, Felix Kuehling wrote:
> We're also looking into a hang with a KFD unit test that allocates lots
> of memory and fragments it deliberately, without mapping it all at once.
> It's a new problem for us as we're rebasing on amd-staging-4.6.
> Something weird seems to be happening with evictions, but I haven't been
> able to figure it out.
>
> I was able to see that SDMA page table updates stop working at some
> point, though SDMA fences are still signaling. If I let the test run
> longer, SDMA and CP hang. I dumped the SDMA IBs and didn't see anything
> suspicious. My guess was that maybe the SDMA IBs or the ring are getting
> corrupted, or maybe the GART table entries for the IBs or ring are
> corrupted. But I haven't been able to prove that or track it down to a
> root cause. We're now trying to reimplement the test using libdrm-amdgpu
> APIs so we can bisect on the amd-staging-4.6 branch without KFD.
>
> Regards,
>   Felix
>
> On 16-07-26 10:26 PM, Michel Dänzer wrote:
>> On 22.07.2016 22:10, Christian König wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> We still need to unbind explicitely during a move.
>> This change fixed a hang for me when running the piglit test
>> max-texture-size with the radeon driver on Kaveri.
>>
>> However, there's still a similar hang left when letting the piglit test
>> tex3d-maxsize run concurrently with other tests (running tex3d-maxsize
>> alone doesn't hang, but fails due to running out of GPU memory; that's a
>> recent radeonsi regression). There are
>>
>>  [TTM] Buffer eviction failed
>>
>> messages in dmesg shortly before the hang.
>>
>> I haven't seen such hangs with older kernels. Any ideas offhand what the
>> problem could be? If not, I'll try bisecting.
>>
>>

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

Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Felix Kuehling]

We had to revert another change on the KFD branch to fix a buffer move
problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu: group BOs
by log2 of the size on the LRU v2

We haven't looked into this change in detail yet, to understand the
cause. Kent found it by bisecting on amd-staging-4.6 and applying KFD
changes on top.

Regards,
  Felix

On 16-08-05 11:06 AM, Felix Kuehling wrote:
> For the record, Michel's patch "drm/ttm: Wait for a BO to become idle
> before unbinding it from GTT" fixes our KFD problem as well.
>
> Thanks,
>   Felix
>
> On 16-07-27 05:27 PM, Felix Kuehling wrote:
>> We're also looking into a hang with a KFD unit test that allocates lots
>> of memory and fragments it deliberately, without mapping it all at once.
>> It's a new problem for us as we're rebasing on amd-staging-4.6.
>> Something weird seems to be happening with evictions, but I haven't been
>> able to figure it out.
>>
>> I was able to see that SDMA page table updates stop working at some
>> point, though SDMA fences are still signaling. If I let the test run
>> longer, SDMA and CP hang. I dumped the SDMA IBs and didn't see anything
>> suspicious. My guess was that maybe the SDMA IBs or the ring are getting
>> corrupted, or maybe the GART table entries for the IBs or ring are
>> corrupted. But I haven't been able to prove that or track it down to a
>> root cause. We're now trying to reimplement the test using libdrm-amdgpu
>> APIs so we can bisect on the amd-staging-4.6 branch without KFD.
>>
>> Regards,
>>   Felix
>>
>> On 16-07-26 10:26 PM, Michel Dänzer wrote:
>>> On 22.07.2016 22:10, Christian König wrote:
>>>> From: Christian König <christian.koenig@amd.com>
>>>>
>>>> We still need to unbind explicitely during a move.
>>> This change fixed a hang for me when running the piglit test
>>> max-texture-size with the radeon driver on Kaveri.
>>>
>>> However, there's still a similar hang left when letting the piglit test
>>> tex3d-maxsize run concurrently with other tests (running tex3d-maxsize
>>> alone doesn't hang, but fails due to running out of GPU memory; that's a
>>> recent radeonsi regression). There are
>>>
>>>  [TTM] Buffer eviction failed
>>>
>>> messages in dmesg shortly before the hang.
>>>
>>> I haven't seen such hangs with older kernels. Any ideas offhand what the
>>> problem could be? If not, I'll try bisecting.
>>>
>>>

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

RE: Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Felix Kuehling
> Sent: Thursday, August 11, 2016 3:52 PM
> To: Michel Dänzer; Christian König
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Reverted another change to fix buffer move hangs (was Re:
> [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)
> 
> We had to revert another change on the KFD branch to fix a buffer move
> problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu:
> group BOs
> by log2 of the size on the LRU v2

That makes sense.  I think you may want a different LRU scheme for KFD or at least special handling for KFD buffers.

Alex

> 
> We haven't looked into this change in detail yet, to understand the
> cause. Kent found it by bisecting on amd-staging-4.6 and applying KFD
> changes on top.
> 
> Regards,
>   Felix
> 
> On 16-08-05 11:06 AM, Felix Kuehling wrote:
> > For the record, Michel's patch "drm/ttm: Wait for a BO to become idle
> > before unbinding it from GTT" fixes our KFD problem as well.
> >
> > Thanks,
> >   Felix
> >
> > On 16-07-27 05:27 PM, Felix Kuehling wrote:
> >> We're also looking into a hang with a KFD unit test that allocates lots
> >> of memory and fragments it deliberately, without mapping it all at once.
> >> It's a new problem for us as we're rebasing on amd-staging-4.6.
> >> Something weird seems to be happening with evictions, but I haven't
> been
> >> able to figure it out.
> >>
> >> I was able to see that SDMA page table updates stop working at some
> >> point, though SDMA fences are still signaling. If I let the test run
> >> longer, SDMA and CP hang. I dumped the SDMA IBs and didn't see
> anything
> >> suspicious. My guess was that maybe the SDMA IBs or the ring are getting
> >> corrupted, or maybe the GART table entries for the IBs or ring are
> >> corrupted. But I haven't been able to prove that or track it down to a
> >> root cause. We're now trying to reimplement the test using libdrm-
> amdgpu
> >> APIs so we can bisect on the amd-staging-4.6 branch without KFD.
> >>
> >> Regards,
> >>   Felix
> >>
> >> On 16-07-26 10:26 PM, Michel Dänzer wrote:
> >>> On 22.07.2016 22:10, Christian König wrote:
> >>>> From: Christian König <christian.koenig@amd.com>
> >>>>
> >>>> We still need to unbind explicitely during a move.
> >>> This change fixed a hang for me when running the piglit test
> >>> max-texture-size with the radeon driver on Kaveri.
> >>>
> >>> However, there's still a similar hang left when letting the piglit test
> >>> tex3d-maxsize run concurrently with other tests (running tex3d-maxsize
> >>> alone doesn't hang, but fails due to running out of GPU memory; that's a
> >>> recent radeonsi regression). There are
> >>>
> >>>  [TTM] Buffer eviction failed
> >>>
> >>> messages in dmesg shortly before the hang.
> >>>
> >>> I haven't seen such hangs with older kernels. Any ideas offhand what
> the
> >>> problem could be? If not, I'll try bisecting.
> >>>
> >>>
> 
> _______________________________________________
> 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: Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Felix Kuehling]

[CC Kent FYI]

On 16-08-11 04:31 PM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Felix Kuehling
>> Sent: Thursday, August 11, 2016 3:52 PM
>> To: Michel Dänzer; Christian König
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Reverted another change to fix buffer move hangs (was Re:
>> [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)
>>
>> We had to revert another change on the KFD branch to fix a buffer move
>> problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu:
>> group BOs
>> by log2 of the size on the LRU v2
> That makes sense.  I think you may want a different LRU scheme for KFD or at least special handling for KFD buffers.

[FK] But I think the patch shouldn't cause hangs, regardless.

I eventually found what the problem was. The "group BOs by log2 of the
size on the LRU v2" patch exposed a latent bug related to the GART size.
On our KFD branch, we calculate the GART size differently, and it can
easily go above 4GB. I think on amd-staging-4.6 the GART size can also
go above 4GB on cards with lots of VRAM.

However, the offset parameter in amdgpu_gart_bind and unbind is only
32-bit. With the patch our test ended up using GART offsets beyond 4GB
for the first time. Changing the offset parameter to uint64_t fixes the
problem.

Our test also demonstrates a potential flaw in the log2 grouping patch:
When a buffer of a previously unused size is added to the LRU, it gets
added to the front of the list, rather than the tail. So an application
that allocates a very large buffer after a bunch of smaller buffers, is
very likely to have that buffer evicted over and over again before any
smaller buffers are considered for eviction. I believe, this can result
in thrashing of large buffers.

Some other observations: When the last BO of a given size is removed
from the LRU list, the LRU tail for that size is left "floating" in the
middle of the LRU list. So the next BO of that size that is added, will
be added at an arbitrary position in the list. It may even end up in the
middle of a block of pages of a different size. So a log2 grouping may
end up being split.

Regards,
  Felix

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

Re: Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Christian König]

Am 13.08.2016 um 01:22 schrieb Felix Kuehling:
> [CC Kent FYI]
>
> On 16-08-11 04:31 PM, Deucher, Alexander wrote:
>>> -----Original Message-----
>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>>> Of Felix Kuehling
>>> Sent: Thursday, August 11, 2016 3:52 PM
>>> To: Michel Dänzer; Christian König
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Subject: Reverted another change to fix buffer move hangs (was Re:
>>> [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)
>>>
>>> We had to revert another change on the KFD branch to fix a buffer move
>>> problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu:
>>> group BOs
>>> by log2 of the size on the LRU v2
>> That makes sense.  I think you may want a different LRU scheme for KFD or at least special handling for KFD buffers.
> [FK] But I think the patch shouldn't cause hangs, regardless.
>
> I eventually found what the problem was. The "group BOs by log2 of the
> size on the LRU v2" patch exposed a latent bug related to the GART size.
> On our KFD branch, we calculate the GART size differently, and it can
> easily go above 4GB. I think on amd-staging-4.6 the GART size can also
> go above 4GB on cards with lots of VRAM.
>
> However, the offset parameter in amdgpu_gart_bind and unbind is only
> 32-bit. With the patch our test ended up using GART offsets beyond 4GB
> for the first time. Changing the offset parameter to uint64_t fixes the
> problem.

Nice catch, please provide a patch to fix this.

> Our test also demonstrates a potential flaw in the log2 grouping patch:
> When a buffer of a previously unused size is added to the LRU, it gets
> added to the front of the list, rather than the tail. So an application
> that allocates a very large buffer after a bunch of smaller buffers, is
> very likely to have that buffer evicted over and over again before any
> smaller buffers are considered for eviction. I believe, this can result
> in thrashing of large buffers.
>
> Some other observations: When the last BO of a given size is removed
> from the LRU list, the LRU tail for that size is left "floating" in the
> middle of the LRU list. So the next BO of that size that is added, will
> be added at an arbitrary position in the list. It may even end up in the
> middle of a block of pages of a different size. So a log2 grouping may
> end up being split.

Yeah, those are more or less known issues.

Keep in mind that we only added the grouping by log2 of the size to have 
a justification to push the TTM changes upstream for the coming KFD fences.

E.g. so that we are able to have this upstream before we try to push on 
the fence code.

I will take a look at fixing those issues when I have time, shouldn't be 
to complicated to set the entries to zero when they aren't used or 
adjust other entries as well when some are added.

Regards,
Christian.

>
> Regards,
>    Felix
>
> _______________________________________________
> 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: Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Felix Kuehling]

[-- Attachment #1: Type: text/plain, Size: 3357 bytes --]

Patch against current amd-staging-4.6 is attached.

Regards,
  Felix


On 16-08-13 05:25 AM, Christian König wrote:
> Am 13.08.2016 um 01:22 schrieb Felix Kuehling:
>> [CC Kent FYI]
>>
>> On 16-08-11 04:31 PM, Deucher, Alexander wrote:
>>>> -----Original Message-----
>>>> From: amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On Behalf
>>>> Of Felix Kuehling
>>>> Sent: Thursday, August 11, 2016 3:52 PM
>>>> To: Michel Dänzer; Christian König
>>>> Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>> Subject: Reverted another change to fix buffer move hangs (was Re:
>>>> [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)
>>>>
>>>> We had to revert another change on the KFD branch to fix a buffer move
>>>> problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu:
>>>> group BOs
>>>> by log2 of the size on the LRU v2
>>> That makes sense.  I think you may want a different LRU scheme for
>>> KFD or at least special handling for KFD buffers.
>> [FK] But I think the patch shouldn't cause hangs, regardless.
>>
>> I eventually found what the problem was. The "group BOs by log2 of the
>> size on the LRU v2" patch exposed a latent bug related to the GART size.
>> On our KFD branch, we calculate the GART size differently, and it can
>> easily go above 4GB. I think on amd-staging-4.6 the GART size can also
>> go above 4GB on cards with lots of VRAM.
>>
>> However, the offset parameter in amdgpu_gart_bind and unbind is only
>> 32-bit. With the patch our test ended up using GART offsets beyond 4GB
>> for the first time. Changing the offset parameter to uint64_t fixes the
>> problem.
>
> Nice catch, please provide a patch to fix this.
>
>> Our test also demonstrates a potential flaw in the log2 grouping patch:
>> When a buffer of a previously unused size is added to the LRU, it gets
>> added to the front of the list, rather than the tail. So an application
>> that allocates a very large buffer after a bunch of smaller buffers, is
>> very likely to have that buffer evicted over and over again before any
>> smaller buffers are considered for eviction. I believe, this can result
>> in thrashing of large buffers.
>>
>> Some other observations: When the last BO of a given size is removed
>> from the LRU list, the LRU tail for that size is left "floating" in the
>> middle of the LRU list. So the next BO of that size that is added, will
>> be added at an arbitrary position in the list. It may even end up in the
>> middle of a block of pages of a different size. So a log2 grouping may
>> end up being split.
>
> Yeah, those are more or less known issues.
>
> Keep in mind that we only added the grouping by log2 of the size to
> have a justification to push the TTM changes upstream for the coming
> KFD fences.
>
> E.g. so that we are able to have this upstream before we try to push
> on the fence code.
>
> I will take a look at fixing those issues when I have time, shouldn't
> be to complicated to set the entries to zero when they aren't used or
> adjust other entries as well when some are added.
>
> Regards,
> Christian.
>
>>
>> Regards,
>>    Felix
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>


[-- Attachment #2: 0001-drm-amdgpu-Change-GART-offset-to-64-bit.patch --]
[-- Type: text/x-patch, Size: 2419 bytes --]

>From 34b9f9e33fc83e375c291e8d3480f8faff150183 Mon Sep 17 00:00:00 2001
From: Felix Kuehling <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
Date: Fri, 12 Aug 2016 19:25:21 -0400
Subject: [PATCH] drm/amdgpu: Change GART offset to 64-bit

The GART aperture size can be bigger than 4GB. Therefore the offset
used in amdgpu_gart_bind and amdgpu_gart_unbind must be 64-bit.

Signed-off-by: Felix Kuehling <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h      | 4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index d4ab35a..a55eb6f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -662,9 +662,9 @@ int amdgpu_gart_table_vram_pin(struct amdgpu_device *adev);
 void amdgpu_gart_table_vram_unpin(struct amdgpu_device *adev);
 int amdgpu_gart_init(struct amdgpu_device *adev);
 void amdgpu_gart_fini(struct amdgpu_device *adev);
-void amdgpu_gart_unbind(struct amdgpu_device *adev, unsigned offset,
+void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
 			int pages);
-int amdgpu_gart_bind(struct amdgpu_device *adev, unsigned offset,
+int amdgpu_gart_bind(struct amdgpu_device *adev, uint64_t offset,
 		     int pages, struct page **pagelist,
 		     dma_addr_t *dma_addr, uint32_t flags);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
index 921bce2..0feea34 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
@@ -221,7 +221,7 @@ void amdgpu_gart_table_vram_free(struct amdgpu_device *adev)
  * Unbinds the requested pages from the gart page table and
  * replaces them with the dummy page (all asics).
  */
-void amdgpu_gart_unbind(struct amdgpu_device *adev, unsigned offset,
+void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
 			int pages)
 {
 	unsigned t;
@@ -268,7 +268,7 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, unsigned offset,
  * (all asics).
  * Returns 0 for success, -EINVAL for failure.
  */
-int amdgpu_gart_bind(struct amdgpu_device *adev, unsigned offset,
+int amdgpu_gart_bind(struct amdgpu_device *adev, uint64_t offset,
 		     int pages, struct page **pagelist, dma_addr_t *dma_addr,
 		     uint32_t flags)
 {
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

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

Re: Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Alex Deucher]

On Mon, Aug 15, 2016 at 3:06 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
> Patch against current amd-staging-4.6 is attached.

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Regards,
>   Felix
>
>
> On 16-08-13 05:25 AM, Christian König wrote:
>> Am 13.08.2016 um 01:22 schrieb Felix Kuehling:
>>> [CC Kent FYI]
>>>
>>> On 16-08-11 04:31 PM, Deucher, Alexander wrote:
>>>>> -----Original Message-----
>>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>>>>> Of Felix Kuehling
>>>>> Sent: Thursday, August 11, 2016 3:52 PM
>>>>> To: Michel Dänzer; Christian König
>>>>> Cc: amd-gfx@lists.freedesktop.org
>>>>> Subject: Reverted another change to fix buffer move hangs (was Re:
>>>>> [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)
>>>>>
>>>>> We had to revert another change on the KFD branch to fix a buffer move
>>>>> problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu:
>>>>> group BOs
>>>>> by log2 of the size on the LRU v2
>>>> That makes sense.  I think you may want a different LRU scheme for
>>>> KFD or at least special handling for KFD buffers.
>>> [FK] But I think the patch shouldn't cause hangs, regardless.
>>>
>>> I eventually found what the problem was. The "group BOs by log2 of the
>>> size on the LRU v2" patch exposed a latent bug related to the GART size.
>>> On our KFD branch, we calculate the GART size differently, and it can
>>> easily go above 4GB. I think on amd-staging-4.6 the GART size can also
>>> go above 4GB on cards with lots of VRAM.
>>>
>>> However, the offset parameter in amdgpu_gart_bind and unbind is only
>>> 32-bit. With the patch our test ended up using GART offsets beyond 4GB
>>> for the first time. Changing the offset parameter to uint64_t fixes the
>>> problem.
>>
>> Nice catch, please provide a patch to fix this.
>>
>>> Our test also demonstrates a potential flaw in the log2 grouping patch:
>>> When a buffer of a previously unused size is added to the LRU, it gets
>>> added to the front of the list, rather than the tail. So an application
>>> that allocates a very large buffer after a bunch of smaller buffers, is
>>> very likely to have that buffer evicted over and over again before any
>>> smaller buffers are considered for eviction. I believe, this can result
>>> in thrashing of large buffers.
>>>
>>> Some other observations: When the last BO of a given size is removed
>>> from the LRU list, the LRU tail for that size is left "floating" in the
>>> middle of the LRU list. So the next BO of that size that is added, will
>>> be added at an arbitrary position in the list. It may even end up in the
>>> middle of a block of pages of a different size. So a log2 grouping may
>>> end up being split.
>>
>> Yeah, those are more or less known issues.
>>
>> Keep in mind that we only added the grouping by log2 of the size to
>> have a justification to push the TTM changes upstream for the coming
>> KFD fences.
>>
>> E.g. so that we are able to have this upstream before we try to push
>> on the fence code.
>>
>> I will take a look at fixing those issues when I have time, shouldn't
>> be to complicated to set the entries to zero when they aren't used or
>> adjust other entries as well when some are added.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>    Felix
>>>
>>> _______________________________________________
>>> 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
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Christian König]

Am 15.08.2016 um 23:03 schrieb Alex Deucher:
> On Mon, Aug 15, 2016 at 3:06 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
>> Patch against current amd-staging-4.6 is attached.
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>.

>
>> Regards,
>>    Felix
>>
>>
>> On 16-08-13 05:25 AM, Christian König wrote:
>>> Am 13.08.2016 um 01:22 schrieb Felix Kuehling:
>>>> [CC Kent FYI]
>>>>
>>>> On 16-08-11 04:31 PM, Deucher, Alexander wrote:
>>>>>> -----Original Message-----
>>>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>>>>>> Of Felix Kuehling
>>>>>> Sent: Thursday, August 11, 2016 3:52 PM
>>>>>> To: Michel Dänzer; Christian König
>>>>>> Cc: amd-gfx@lists.freedesktop.org
>>>>>> Subject: Reverted another change to fix buffer move hangs (was Re:
>>>>>> [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)
>>>>>>
>>>>>> We had to revert another change on the KFD branch to fix a buffer move
>>>>>> problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu:
>>>>>> group BOs
>>>>>> by log2 of the size on the LRU v2
>>>>> That makes sense.  I think you may want a different LRU scheme for
>>>>> KFD or at least special handling for KFD buffers.
>>>> [FK] But I think the patch shouldn't cause hangs, regardless.
>>>>
>>>> I eventually found what the problem was. The "group BOs by log2 of the
>>>> size on the LRU v2" patch exposed a latent bug related to the GART size.
>>>> On our KFD branch, we calculate the GART size differently, and it can
>>>> easily go above 4GB. I think on amd-staging-4.6 the GART size can also
>>>> go above 4GB on cards with lots of VRAM.
>>>>
>>>> However, the offset parameter in amdgpu_gart_bind and unbind is only
>>>> 32-bit. With the patch our test ended up using GART offsets beyond 4GB
>>>> for the first time. Changing the offset parameter to uint64_t fixes the
>>>> problem.
>>> Nice catch, please provide a patch to fix this.
>>>
>>>> Our test also demonstrates a potential flaw in the log2 grouping patch:
>>>> When a buffer of a previously unused size is added to the LRU, it gets
>>>> added to the front of the list, rather than the tail. So an application
>>>> that allocates a very large buffer after a bunch of smaller buffers, is
>>>> very likely to have that buffer evicted over and over again before any
>>>> smaller buffers are considered for eviction. I believe, this can result
>>>> in thrashing of large buffers.
>>>>
>>>> Some other observations: When the last BO of a given size is removed
>>>> from the LRU list, the LRU tail for that size is left "floating" in the
>>>> middle of the LRU list. So the next BO of that size that is added, will
>>>> be added at an arbitrary position in the list. It may even end up in the
>>>> middle of a block of pages of a different size. So a log2 grouping may
>>>> end up being split.
>>> Yeah, those are more or less known issues.
>>>
>>> Keep in mind that we only added the grouping by log2 of the size to
>>> have a justification to push the TTM changes upstream for the coming
>>> KFD fences.
>>>
>>> E.g. so that we are able to have this upstream before we try to push
>>> on the fence code.
>>>
>>> I will take a look at fixing those issues when I have time, shouldn't
>>> be to complicated to set the entries to zero when they aren't used or
>>> adjust other entries as well when some are added.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> Regards,
>>>>     Felix
>>>>
>>>> _______________________________________________
>>>> 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
>>

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

Re: Reverted another change to fix buffer move hangs (was Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2)

[Felix Kuehling]

Thank you. Sorry, I already pushed it with Alex's R-B, without yours.


On 16-08-16 03:53 AM, Christian König wrote:
> Am 15.08.2016 um 23:03 schrieb Alex Deucher:
>> On Mon, Aug 15, 2016 at 3:06 PM, Felix Kuehling
>> <felix.kuehling@amd.com> wrote:
>>> Patch against current amd-staging-4.6 is attached.
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>.
>
>>
>>> Regards,
>>>    Felix
>>>
>>>
>>> On 16-08-13 05:25 AM, Christian König wrote:
>>>> Am 13.08.2016 um 01:22 schrieb Felix Kuehling:
>>>>> [CC Kent FYI]
>>>>>
>>>>> On 16-08-11 04:31 PM, Deucher, Alexander wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
>>>>>>> Behalf
>>>>>>> Of Felix Kuehling
>>>>>>> Sent: Thursday, August 11, 2016 3:52 PM
>>>>>>> To: Michel Dänzer; Christian König
>>>>>>> Cc: amd-gfx@lists.freedesktop.org
>>>>>>> Subject: Reverted another change to fix buffer move hangs (was Re:
>>>>>>> [PATCH] drm/ttm: partial revert "cleanup
>>>>>>> ttm_tt_(unbind|destroy)" v2)
>>>>>>>
>>>>>>> We had to revert another change on the KFD branch to fix a
>>>>>>> buffer move
>>>>>>> problem: 8b6b79f43801f00ddcdc10a4d5719eba4b2e32aa (drm/amdgpu:
>>>>>>> group BOs
>>>>>>> by log2 of the size on the LRU v2
>>>>>> That makes sense.  I think you may want a different LRU scheme for
>>>>>> KFD or at least special handling for KFD buffers.
>>>>> [FK] But I think the patch shouldn't cause hangs, regardless.
>>>>>
>>>>> I eventually found what the problem was. The "group BOs by log2 of
>>>>> the
>>>>> size on the LRU v2" patch exposed a latent bug related to the GART
>>>>> size.
>>>>> On our KFD branch, we calculate the GART size differently, and it can
>>>>> easily go above 4GB. I think on amd-staging-4.6 the GART size can
>>>>> also
>>>>> go above 4GB on cards with lots of VRAM.
>>>>>
>>>>> However, the offset parameter in amdgpu_gart_bind and unbind is only
>>>>> 32-bit. With the patch our test ended up using GART offsets beyond
>>>>> 4GB
>>>>> for the first time. Changing the offset parameter to uint64_t
>>>>> fixes the
>>>>> problem.
>>>> Nice catch, please provide a patch to fix this.
>>>>
>>>>> Our test also demonstrates a potential flaw in the log2 grouping
>>>>> patch:
>>>>> When a buffer of a previously unused size is added to the LRU, it
>>>>> gets
>>>>> added to the front of the list, rather than the tail. So an
>>>>> application
>>>>> that allocates a very large buffer after a bunch of smaller
>>>>> buffers, is
>>>>> very likely to have that buffer evicted over and over again before
>>>>> any
>>>>> smaller buffers are considered for eviction. I believe, this can
>>>>> result
>>>>> in thrashing of large buffers.
>>>>>
>>>>> Some other observations: When the last BO of a given size is removed
>>>>> from the LRU list, the LRU tail for that size is left "floating"
>>>>> in the
>>>>> middle of the LRU list. So the next BO of that size that is added,
>>>>> will
>>>>> be added at an arbitrary position in the list. It may even end up
>>>>> in the
>>>>> middle of a block of pages of a different size. So a log2 grouping
>>>>> may
>>>>> end up being split.
>>>> Yeah, those are more or less known issues.
>>>>
>>>> Keep in mind that we only added the grouping by log2 of the size to
>>>> have a justification to push the TTM changes upstream for the coming
>>>> KFD fences.
>>>>
>>>> E.g. so that we are able to have this upstream before we try to push
>>>> on the fence code.
>>>>
>>>> I will take a look at fixing those issues when I have time, shouldn't
>>>> be to complicated to set the entries to zero when they aren't used or
>>>> adjust other entries as well when some are added.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>>     Felix
>>>>>
>>>>> _______________________________________________
>>>>> 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
>>>
>

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

Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Christian König]

Am 27.07.2016 um 04:26 schrieb Michel Dänzer:
> On 22.07.2016 22:10, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> We still need to unbind explicitely during a move.
> This change fixed a hang for me when running the piglit test
> max-texture-size with the radeon driver on Kaveri.
>
> However, there's still a similar hang left when letting the piglit test
> tex3d-maxsize run concurrently with other tests (running tex3d-maxsize
> alone doesn't hang, but fails due to running out of GPU memory; that's a
> recent radeonsi regression). There are
>
>   [TTM] Buffer eviction failed
>
> messages in dmesg shortly before the hang.
>
> I haven't seen such hangs with older kernels. Any ideas offhand what the
> problem could be? If not, I'll try bisecting.
Of hand I would certainly try to disable pipelined buffer moves.

You could also try to revert "6b5f77799274c151f6cc5467c5abe6ad366a965e 
drm/amdgpu: remove pre move wait".

Regards,
Christian.


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

Re: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Michel Dänzer]

On 04.08.2016 19:18, Christian König wrote:
> Am 27.07.2016 um 04:26 schrieb Michel Dänzer:
>> On 22.07.2016 22:10, Christian König wrote:
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> We still need to unbind explicitely during a move.
>> This change fixed a hang for me when running the piglit test
>> max-texture-size with the radeon driver on Kaveri.
>>
>> However, there's still a similar hang left when letting the piglit test
>> tex3d-maxsize run concurrently with other tests (running tex3d-maxsize
>> alone doesn't hang, but fails due to running out of GPU memory; that's a
>> recent radeonsi regression). There are
>>
>>   [TTM] Buffer eviction failed
>>
>> messages in dmesg shortly before the hang.
>>
>> I haven't seen such hangs with older kernels. Any ideas offhand what the
>> problem could be? If not, I'll try bisecting.
> Of hand I would certainly try to disable pipelined buffer moves.
> 
> You could also try to revert "6b5f77799274c151f6cc5467c5abe6ad366a965e
> drm/amdgpu: remove pre move wait".

This is using the radeon driver.


-- 
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] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)" v2

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Christian König
> Sent: Friday, July 22, 2016 9:10 AM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Lin, Amber; Kuehling, Felix; funfunctor@folklore1984.net
> Subject: [PATCH] drm/ttm: partial revert "cleanup ttm_tt_(unbind|destroy)"
> v2
> 
> From: Christian König <christian.koenig@amd.com>
> 
> We still need to unbind explicitely during a move.
> 
> This partial reverts commit ff20caa0bcbfef9f7686f8d1868a3b990921afd6.
> 
> v2: remove unnecessary check and unused variable
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/ttm/ttm_bo_util.c |  1 +
>  drivers/gpu/drm/ttm/ttm_tt.c      | 19 ++++++++++++-------
>  include/drm/ttm/ttm_bo_driver.h   |  9 +++++++++
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
> b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 0c389a5..6c412c3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -53,6 +53,7 @@ int ttm_bo_move_ttm(struct ttm_buffer_object *bo,
>  	int ret;
> 
>  	if (old_mem->mem_type != TTM_PL_SYSTEM) {
> +		ttm_tt_unbind(ttm);
>  		ttm_bo_free_old_node(bo);
>  		ttm_flag_masked(&old_mem->placement,
> TTM_PL_FLAG_SYSTEM,
>  				TTM_PL_MASK_MEM);
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index d28d4333..526e5a7 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -166,16 +166,10 @@ EXPORT_SYMBOL(ttm_tt_set_placement_caching);
> 
>  void ttm_tt_destroy(struct ttm_tt *ttm)
>  {
> -	int ret;
> -
>  	if (ttm == NULL)
>  		return;
> 
> -	if (ttm->state == tt_bound) {
> -		ret = ttm->func->unbind(ttm);
> -		BUG_ON(ret);
> -		ttm->state = tt_unbound;
> -	}
> +	ttm_tt_unbind(ttm);
> 
>  	if (ttm->state == tt_unbound)
>  		ttm_tt_unpopulate(ttm);
> @@ -255,6 +249,17 @@ void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma)
>  }
>  EXPORT_SYMBOL(ttm_dma_tt_fini);
> 
> +void ttm_tt_unbind(struct ttm_tt *ttm)
> +{
> +	int ret;
> +
> +	if (ttm->state == tt_bound) {
> +		ret = ttm->func->unbind(ttm);
> +		BUG_ON(ret);
> +		ttm->state = tt_unbound;
> +	}
> +}
> +
>  int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem)
>  {
>  	int ret = 0;
> diff --git a/include/drm/ttm/ttm_bo_driver.h
> b/include/drm/ttm/ttm_bo_driver.h
> index 141cefb..e7d81a8 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -628,6 +628,15 @@ extern int ttm_tt_bind(struct ttm_tt *ttm, struct
> ttm_mem_reg *bo_mem);
>  extern void ttm_tt_destroy(struct ttm_tt *ttm);
> 
>  /**
> + * ttm_ttm_unbind:
> + *
> + * @ttm: The struct ttm_tt.
> + *
> + * Unbind a struct ttm_tt.
> + */
> +extern void ttm_tt_unbind(struct ttm_tt *ttm);
> +
> +/**
>   * ttm_tt_swapin:
>   *
>   * @ttm: The struct ttm_tt.
> --
> 2.5.0
> 
> _______________________________________________
> 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