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

Commit Message

Robin Murphy April 27, 2018, 3:54 p.m. UTC

On 25/04/18 21:44, Sinan Kaya wrote:
> On 4/17/2018 5:13 PM, Sinan Kaya wrote:
>> Tested-by: Sinan Kaya <[e-mail]>
>>
>> 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 00000018

£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-----
  			addr = sg_dma_address(dma_sg);

Comments

Sinan Kaya April 27, 2018, 4:20 p.m. UTC | #1

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 April 27, 2018, 7:42 p.m. UTC | #2

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 00000018
> 
> £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);
+                       }
                }

Robin Murphy April 30, 2018, 1 p.m. UTC | #3

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 00000018
>>
>> £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);
> +                       }
>                  }
>   
>

Patch

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);