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

[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).
> 
> Best regards
>