[v3,1/3] drm/prime: Iterate SG DMA addresses separately

Commit Message

Robin Murphy April 30, 2018, 1:54 p.m. UTC

For dma_map_sg(), DMA API implementations are free to merge consecutive
segments into a single DMA mapping if conditions are suitable, thus the
resulting DMA addresses which drm_prime_sg_to_page_addr_arrays()
iterates over may be packed into fewer entries than sgt->nents implies.

The current implementation does not account for this, meaning that its
callers either have to reject the 0 < count < nents case or risk getting
bogus DMA addresses beyond the first segment. Fortunately this is quite
easy to handle without having to rejig structures to also store the
mapped count, since the total DMA length should still be equal to the
total buffer length. All we need is a second scatterlist cursor to
iterate through the DMA addresses independently of the page addresses.

Reviewed-by: Christian König <[e-mail]>
Signed-off-by: Robin Murphy <[e-mail]>
---

v3: Move dma_len == 0 logic earlier to avoid iterating dma_sg too far

 drivers/gpu/drm/drm_prime.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Sinan Kaya April 30, 2018, 5:59 p.m. UTC | #1

On 4/30/2018 9:54 AM, Robin Murphy wrote:
> For dma_map_sg(), DMA API implementations are free to merge consecutive
> segments into a single DMA mapping if conditions are suitable, thus the
> resulting DMA addresses which drm_prime_sg_to_page_addr_arrays()
> iterates over may be packed into fewer entries than sgt->nents implies.
> 
> The current implementation does not account for this, meaning that its
> callers either have to reject the 0 < count < nents case or risk getting
> bogus DMA addresses beyond the first segment. Fortunately this is quite
> easy to handle without having to rejig structures to also store the
> mapped count, since the total DMA length should still be equal to the
> total buffer length. All we need is a second scatterlist cursor to
> iterate through the DMA addresses independently of the page addresses.
> 
> Reviewed-by: Christian König <[e-mail]>
> Signed-off-by: Robin Murphy <[e-mail]>
> ---

Much better 

Tested-by: Sinan Kaya <[e-mail]>

for the first two patches. (1/3 and 2/3)

Robin Murphy May 25, 2018, 1:33 p.m. UTC | #2

On 30/04/18 18:59, Sinan Kaya wrote:
> On 4/30/2018 9:54 AM, Robin Murphy wrote:
>> For dma_map_sg(), DMA API implementations are free to merge consecutive
>> segments into a single DMA mapping if conditions are suitable, thus the
>> resulting DMA addresses which drm_prime_sg_to_page_addr_arrays()
>> iterates over may be packed into fewer entries than sgt->nents implies.
>>
>> The current implementation does not account for this, meaning that its
>> callers either have to reject the 0 < count < nents case or risk getting
>> bogus DMA addresses beyond the first segment. Fortunately this is quite
>> easy to handle without having to rejig structures to also store the
>> mapped count, since the total DMA length should still be equal to the
>> total buffer length. All we need is a second scatterlist cursor to
>> iterate through the DMA addresses independently of the page addresses.
>>
>> Reviewed-by: Christian König <[e-mail]>
>> Signed-off-by: Robin Murphy <[e-mail]>
>> ---
> 
> Much better
> 
> Tested-by: Sinan Kaya <[e-mail]>
> 
> for the first two patches. (1/3 and 2/3)

Cheers Sinan.

Alex, Christian, David; is the AMD GPU tree the right target for these 
patches, or is there a wider audience I should consider resending them 
to? (before I forget about them again...)

Thanks,
Robin.

Patch

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7856a9b3f8a8..3e74c84d0baf 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -933,16 +933,24 @@  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
 				     dma_addr_t *addrs, int max_entries)
 {
 	unsigned count;
-	struct scatterlist *sg;
+	struct scatterlist *sg, *dma_sg;
 	struct page *page;
-	u32 len, index;
+	u32 len, dma_len, index;
 	dma_addr_t addr;
 
 	index = 0;
+	dma_sg = sgt->sgl;
+	dma_len = sg_dma_len(dma_sg);
+	addr = sg_dma_address(dma_sg);
 	for_each_sg(sgt->sgl, sg, sgt->nents, count) {
 		len = sg->length;
 		page = sg_page(sg);
-		addr = sg_dma_address(sg);
+
+		if (addrs && dma_len == 0) {
+			dma_sg = sg_next(dma_sg);
+			dma_len = sg_dma_len(dma_sg);
+			addr = sg_dma_address(dma_sg);
+		}
 
 		while (len > 0) {
 			if (WARN_ON(index >= max_entries))
@@ -955,6 +963,7 @@  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
 			page++;
 			addr += PAGE_SIZE;
 			len -= PAGE_SIZE;
+			dma_len -= PAGE_SIZE;
 			index++;
 		}
 	}