Re: [PATCH v2 2/3] drm/amdgpu: Allow dma_map_sg() coalescing

[Robin Murphy]

On 27/04/18 20:42, Sinan Kaya wrote:

On 4/27/2018 11:54 AM, Robin Murphy wrote:



ubuntu@ubuntu:~/amdgpu$_./vectoradd_hip.exe
[  834.002206] create_process:620
[  837.413021] Unable to handle kernel NULL pointer dereference at virtual 
address 0018


£5 says that's sg_dma_len(NULL), which implies either that something's gone 
horribly wrong with the scatterlist DMA mapping such that the lengths don't 
match, or much more likely that ttm.dma_address is NULL and I've missed the 
tiny subtlety below. Does that fix matters?


Turned out to be a null pointer problem after sg_next(). The following helped.


Ugh, right, the whole thing's in the wrong place such that when addrs is 
valid we can dereference junk on the way out of the loop (entirely 
needlessly)... v3 coming up.


Robin.



+   if (addrs && (dma_len == 0)) {
 dma_sg = sg_next(dma_sg);
-   dma_len = sg_dma_len(dma_sg);
-   addr = sg_dma_address(dma_sg);
+   if (dma_sg) {
+   dma_len = sg_dma_len(dma_sg);
+   addr = sg_dma_address(dma_sg);
+   }
 }
  


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

Re: [PATCH v2 2/3] drm/amdgpu: Allow dma_map_sg() coalescing

[Robin Murphy]

On 25/04/18 21:44, Sinan Kaya wrote:

On 4/17/2018 5:13 PM, Sinan Kaya wrote:

Tested-by: Sinan Kaya 

using QDF2400 and XFX Vega64 GPU for the first two patches.

./builddir/tests/amdgpu/amdgpu_test -s 1

Suite: Basic Tests
   Test: Userptr Test ...passed

Userptr Test fails without this patch.



I'm taking this back. I observed a crash with the HSA applications:


FWIW, was this working before, or is this somewhere new that we're only 
reaching now that pin_userpages can succeed?



ubuntu@ubuntu:~/amdgpu$_./vectoradd_hip.exe
[  834.002206] create_process:620
[  837.413021] Unable to handle kernel NULL pointer dereference at virtual 
address 0018


£5 says that's sg_dma_len(NULL), which implies either that something's 
gone horribly wrong with the scatterlist DMA mapping such that the 
lengths don't match, or much more likely that ttm.dma_address is NULL 
and I've missed the tiny subtlety below. Does that fix matters?


Robin.


->8-
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index b8ca06ea7d80..6a31229e4611 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -961,7 +961,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table 
*sgt, struct page **pages,

index++;
}

-   if (dma_len == 0) {
+   if (addrs && dma_len == 0) {
dma_sg = sg_next(dma_sg);
dma_len = sg_dma_len(dma_sg);
addr = sg_dma_address(dma_sg);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/3] drm/amdgpu: Allow dma_map_sg() coalescing

[Sinan Kaya]

On 4/27/2018 11:54 AM, Robin Murphy wrote:
> 
>> ubuntu@ubuntu:~/amdgpu$_./vectoradd_hip.exe
>> [  834.002206] create_process:620
>> [  837.413021] Unable to handle kernel NULL pointer dereference at virtual 
>> address 0018
> 
> £5 says that's sg_dma_len(NULL), which implies either that something's gone 
> horribly wrong with the scatterlist DMA mapping such that the lengths don't 
> match, or much more likely that ttm.dma_address is NULL and I've missed the 
> tiny subtlety below. Does that fix matters?

Turned out to be a null pointer problem after sg_next(). The following helped.

+   if (addrs && (dma_len == 0)) {
dma_sg = sg_next(dma_sg);
-   dma_len = sg_dma_len(dma_sg);
-   addr = sg_dma_address(dma_sg);
+   if (dma_sg) {
+   dma_len = sg_dma_len(dma_sg);
+   addr = sg_dma_address(dma_sg);
+   }
}
 

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/3] drm/amdgpu: Allow dma_map_sg() coalescing

[Sinan Kaya]

On 4/27/2018 11:54 AM, Robin Murphy wrote:
>> I'm taking this back. I observed a crash with the HSA applications:
> 
> FWIW, was this working before, or is this somewhere new that we're only 
> reaching now that pin_userpages can succeed?

Yes, HSA application is a different test. Previous DRM library unit test is
still passing. It must have some unique characteristic. 

I'll test your patch and report.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/3] drm/amdgpu: Allow dma_map_sg() coalescing

[Sinan Kaya]

On 4/17/2018 5:13 PM, Sinan Kaya wrote:
> Tested-by: Sinan Kaya 
> 
> using QDF2400 and XFX Vega64 GPU for the first two patches.
> 
> ./builddir/tests/amdgpu/amdgpu_test -s 1
> 
> Suite: Basic Tests
>   Test: Userptr Test ...passed
> 
> Userptr Test fails without this patch.


I'm taking this back. I observed a crash with the HSA applications:

ubuntu@ubuntu:~/amdgpu$_./vectoradd_hip.exe
[  834.002206] create_process:620
[  837.413021] Unable to handle kernel NULL pointer dereference at virtual 
address 0018
[  837.414097] user pgtable: 4k pages, 48-bit VAs, pgd = 8d448000
[  837.427034] [0018] *pgd=0a424003, *pud=0e0b3003, 
*pmd=
[  837.427414] Internal error: Oops: 9606 [#1] SMP
[  837.427744] Modules linked in:
[  837.457060] CPU: 3 PID: 2321 Comm: vectoradd_hip.e Not tainted 4.13.0 #5
[  837.463076] task: 8dfb0d80 task.stack: 8e17c000
[  837.473795] PC is at drm_prime_sg_to_page_addr_arrays+0xac/0xec
[  837.482877] LR is at drm_prime_sg_to_page_addr_arrays+0xac/0xec
[  837.491910] pc : [] lr : [] pstate: 
80400149
[  837.492022] sp : 8e17f850
[  837.492115] x29: 8e17f850 x28: 8d586700
[  837.516635] x27:  x26: e10410004000
[  837.526444] x25: 8cb91880 x24: 0002
[  837.534974] x23: 8cb91910 x22: 8cb91900
[  837.535178] x21: 0002 x20: 
[  837.535340] x19: 8cb91880 x18: d278
[  837.560498] x17: bef39240 x16: 081bb868
[  837.560684] x15: bf6fe000 x14: 
[  837.574764] x13:  x12: 
[  837.51] x11: 0001 x10: 8a449038
[  837.593181] x9 :  x8 : 8cb91980
[  837.604606] x7 :  x6 : 003f
[  837.612801] x5 : 0040 x4 : 
[  837.617425] x3 : 0002 x2 : 
[  837.625768] x1 : 0002 x0 : 
[  838.516100] [] drm_prime_sg_to_page_addr_arrays+0xac/0xec
[  838.516385] [] amdgpu_ttm_tt_populate+0x80/0xe8
[  838.545137] [] ttm_tt_bind+0x3c/0x7c
[  838.558468] [] ttm_bo_handle_move_mem+0x12c/0x340
[  838.562518] [] ttm_bo_validate+0x90/0x100
[  838.572370] [] ttm_bo_init_reserved+0x25c/0x324
[  838.582103] [] amdgpu_bo_do_create+0x140/0x3e4
[  838.591609] [] amdgpu_bo_create+0x40/0x15c
[  838.601034] [] 
amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu+0x36c/0x80c
[  838.609631] [] kfd_ioctl_alloc_memory_of_gpu+0xfc/0x180
[  838.621500] [] kfd_ioctl+0x144/0x1e8
[  838.632253] [] vfs_ioctl+0x18/0x40
[  838.641592] [] do_vfs_ioctl+0x5ac/0x6bc
[  838.649349] [] SyS_ioctl+0x5c/0x8c
[  838.649609] [] el0_svc_naked+0x24/0x28
[  838.649776] Code: 17f1 35d4 aa1903e0 97fab3f6 (b9401814)
[  838.672742] ---[ end trace fb2627bd4d4c9818 ]---

Robin, if you want to debug this; feel free to send me a debug patch.


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 2/3] drm/amdgpu: Allow dma_map_sg() coalescing

[Sinan Kaya]

On 4/17/2018 11:58 AM, Robin Murphy wrote:
> The amdgpu driver doesn't appear to directly use the scatterlist mapped
> by amdgpu_ttm_tt_pin_userptr(), it merely hands it off to
> drm_prime_sg_to_page_addr_arrays() to generate the dma_address array
> which it actually cares about. Now that the latter can cope with
> dma_map_sg() coalescing dma-contiguous segments such that it returns
> 0 < count < nents, we can relax the current count == nents check to
> only consider genuine failure as other drivers do.
> 
> This avoids a false negative on arm64 systems with an IOMMU.
> 
> Reported-by: Sinan Kaya 
> Signed-off-by: Robin Murphy 
> ---
> 
> v2: Expand commit message to clarify
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 205da3ff9cd0..f81e96a4242f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -813,7 +813,7 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_tt *ttm)
>  
>   r = -ENOMEM;
>   nents = dma_map_sg(adev->dev, ttm->sg->sgl, ttm->sg->nents, direction);
> - if (nents != ttm->sg->nents)
> + if (nents == 0)
>   goto release_sg;
>  
>   drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
> 

Tested-by: Sinan Kaya 

using QDF2400 and XFX Vega64 GPU for the first two patches.

./builddir/tests/amdgpu/amdgpu_test -s 1

Suite: Basic Tests
  Test: Userptr Test ...passed

Userptr Test fails without this patch.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx