[PATCH 2/3] drm/exynos: gem: Rework scatter-list contiguity check on Prime import

[Inki Dae]

20. 4. 22. 오후 12:37에 Inki Dae 이(가) 쓴 글:
> Hi Marek,
> 
> 20. 4. 21. 오후 5:09에 Marek Szyprowski 이(가) 쓴 글:
>> Hi Inki,
>>
>> On 21.04.2020 09:38, Inki Dae wrote:
>>> 20. 4. 7. 오후 10:42에 Marek Szyprowski 이(가) 쓴 글:
>>>> Explicitly check if the imported buffer has been mapped as contiguous in
>>>> the DMA address space, what is required by all Exynos DRM CRTC drivers.
>>>> While touching this, set buffer flags depending on the availability of
>>>> the IOMMU.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
>>>> ---
>>>>   drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
>>>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>> index 40514d3dcf60..9d4e4d321bda 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>> @@ -458,6 +458,23 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>>>>   	int npages;
>>>>   	int ret;
>>>>   
>>> (Optional) could you leave one comment here?
>>> i.e., /* Check if DMA memory region from a exporter are mapped contiguously or not. */
>>
>> Okay.
>>
>>>> +	if (sgt->nents != 1) {
>>> How about using below condition instead?
>>> if (sgt->nents > 0) {
>>
>> This is not the same. My check for != 1 is intended. Checking contiguity 
>> of the scatterlist if it has only 1 element doesn't have much sense.
> 
> Oops sorry. My intention was 'if (sgt->nents > 1)' because if (sgt->nents != 1) allows
> - sgt->nents < 1
> - sgt->nents > 1
> 
> I think the checking would be valid only in case of having multiple entries - sgt->nents > 1.
> 
> Thanks,
> Inki Dae
> 
>>
>>>> +		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
>>>> +		struct scatterlist *s;
>>>> +		unsigned int i;
>>>> +
>>>> +		for_each_sg(sgt->sgl, s, sgt->nents, i) {
>>>> +			if (!sg_dma_len(s))
>>>> +				continue;
>>> Isn't it an error case if sg_dma_len(s) is 0? I think length of s is 0 then it would be invalid because all entries of sgt should be mapped before sg_dma_len() is called.
>>
>> Well, it is a grey area. Some code incorrectly sets nents as orig_nents, 
>> thus this version is simply safe for both approaches. sg entries unused 
>> for DMA chunks have sg_dma_len() == 0.
>>
>> The above check is more or less copied from 
>> drm_gem_cma_prime_import_sg_table() (drivers/gpu/drm/drm_gem_cma_helper.c).

I looked into above original code but it seems that it allows sgt->nents to have negative value, 'sgt->nents < 1', which could incur some bugs.
So I think the original code can be fixed like below,
	if (sgt->nents > 1) {  // <- here
		/* check if the entries in the sg_table are contiguous */
		dma_addr_t next_addr = sg_dma_address(sgt->sgl);
		struct scatterlist *s;
		unsigned int i;

		for_each_sg(sgt->sgl, s, sgt->nents, i) {
			/*
			 * sg_dma_address(s) is only valid for entries
			 * that have sg_dma_len(s) > 0 // <- here
			 */
			if (!sg_dma_len(s))
				continue;

			if (sg_dma_address(s) != next_addr)
				return ERR_PTR(-EINVAL);

			next_addr = sg_dma_address(s) + sg_dma_len(s);
		}
	}

So if you agree with me then we don't have to copy and paste this code as-is.

Regarding 'if (!sg_dma_len(s))' condition here, I'm not clear whether we are using it correctly because scatterlist.h header description says,
/*                                                                              
 * These macros should be used after a dma_map_sg call has been done            
 * to get bus addresses of each of the SG entries and their lengths.            
 * You should only work with the number of sg entries dma_map_sg                
 * returns, or alternatively stop on the first sg_dma_len(sg) which             
 * is 0.                                                                        
 */      

According to above description, sg_dma_len() should be called after dma_map_sg() call. Even it says to stop on the first sg_dma_len(sg) which is 0.
And we could avoid the checking function code from being duplicated by introducing one function which checks if the entries in the sg_table are contiguous or not as a separate patch later.

Thanks,
Inki Dae

>>
>> Best regards
>>
> 
>