Re: [PATCH v2] drm/prime: fix extracting of the DMA addresses from a scatterlist

[Marek Szyprowski]

Hi Michael,

On 2020-03-27 19:31, Ruhl, Michael J wrote:
>> -----Original Message-----
>> From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> Sent: Friday, March 27, 2020 12:21 PM
>> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx
>> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>;
>> stable@xxxxxxxxxxxxxxx; Bartlomiej Zolnierkiewicz
>> <b.zolnierkie@xxxxxxxxxxx>; Maarten Lankhorst
>> <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard
>> <mripard@xxxxxxxxxx>; Thomas Zimmermann <tzimmermann@xxxxxxx>;
>> David Airlie <airlied@xxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Alex Deucher
>> <alexander.deucher@xxxxxxx>; Shane Francis <bigbeeshane@xxxxxxxxx>;
>> Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>
>> Subject: [PATCH v2] drm/prime: fix extracting of the DMA addresses from a
>> scatterlist
>>
>> Scatterlist elements contains both pages and DMA addresses, but one
>> should not assume 1:1 relation between them. The sg->length is the size
>> of the physical memory chunk described by the sg->page, while
>> sg_dma_len(sg) is the size of the DMA (IO virtual) chunk described by
>> the sg_dma_address(sg).
>>
>> The proper way of extracting both: pages and DMA addresses of the whole
>> buffer described by a scatterlist it to iterate independently over the
>> sg->pages/sg->length and sg_dma_address(sg)/sg_dma_len(sg) entries.
>>
>> Fixes: 42e67b479eab ("drm/prime: use dma length macro when mapping sg")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
>> ---
>> drivers/gpu/drm/drm_prime.c | 37 +++++++++++++++++++++++++-----------
>> -
>> 1 file changed, 25 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 1de2cde2277c..282774e469ac 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -962,27 +962,40 @@ int drm_prime_sg_to_page_addr_arrays(struct
>> sg_table *sgt, struct page **pages,
>> 	unsigned count;
>> 	struct scatterlist *sg;
>> 	struct page *page;
>> -	u32 len, index;
>> +	u32 page_len, page_index;
>> 	dma_addr_t addr;
>> +	u32 dma_len, dma_index;
>>
>> -	index = 0;
>> +	/*
>> +	 * Scatterlist elements contains both pages and DMA addresses, but
>> +	 * one shoud not assume 1:1 relation between them. The sg->length
>> is
>> +	 * the size of the physical memory chunk described by the sg->page,
>> +	 * while sg_dma_len(sg) is the size of the DMA (IO virtual) chunk
>> +	 * described by the sg_dma_address(sg).
>> +	 */
> Is there an example of what the scatterlist would look like in this case?

DMA framework or IOMMU is allowed to join consecutive chunks while 
mapping if such operation is supported by the hw. Here is the example:

Lets assume that we have a scatterlist with 4 4KiB pages of the physical 
addresses: 0x12000000, 0x13011000, 0x13012000, 0x11011000. The total 
size of the buffer is 16KiB. After mapping this scatterlist to a device 
behind an IOMMU it may end up as a contiguous buffer in the DMA (IOVA) 
address space. at 0xf0010000. The scatterlist will look like this:

sg[0].page = 0x12000000
sg[0].len = 4096
sg[0].dma_addr = 0xf0010000
sg[0].dma_len = 16384
sg[1].page = 0x13011000
sg[1].len = 4096
sg[1].dma_addr = 0
sg[1].dma_len = 0
sg[2].page = 0x13012000
sg[2].len = 4096
sg[2].dma_addr = 0
sg[2].dma_len = 0
sg[3].page = 0x11011000
sg[3].len = 4096
sg[3].dma_addr = 0
sg[3].dma_len = 0

(I've intentionally wrote page as physical address to make it easier to 
understand, in real SGs it is stored a struct page pointer).

> Does each SG entry always have the page and dma info? or could you have
> entries that have page information only, and entries that have dma info only?
When SG is not mapped yet it contains only the ->pages and ->len 
entries. I'm not aware of the SGs with the DMA information only, but in 
theory it might be possible to have such.
> If the same entry has different size info (page_len = PAGE_SIZE,
> dma_len = 4 * PAGE_SIZE?), are we guaranteed that the arrays (page and addrs) have
> been sized correctly?

There are always no more DMA related entries than the phys pages. If 
there is 1:1 mapping between physical memory and DMA (IOVA) space, then 
each SG entry will have len == dma_len, and dma_addr will be describing 
the same as page entry. DMA mapping framework is allowed only to join 
entries while mapping to DMA (IOVA).

> Just trying to get my head wrapped around this.

Sure, I hope my explanation helps a bit.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland