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

[Marek Szyprowski]

Hi Inki,

On 22.04.2020 06:36, Inki Dae wrote:
> 20. 4. 22. 오후 12:37에 Inki Dae 이(가) 쓴 글:
>> 20. 4. 21. 오후 5:09에 Marek Szyprowski 이(가) 쓴 글:
>>> 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.

Okay, I can change the check to  sgt->nents > 1.

>>>>> +		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.

Okay, I will add a check for negative or zero nents.

> 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.

Okay, I will rework this, then I will prepare a patch which will unify 
scatterlist contiguity checks for all DRM drivers.

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