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

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@samsung.com>

---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 36 +++++++++++++++++--------
 1 file changed, 25 insertions(+), 11 deletions(-)

-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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@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. */ 

> +	if (sgt->nents != 1) {

How about using below condition instead?
if (sgt->nents > 0) {

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

Thanks,
Inki Dae

> +			if (sg_dma_address(s) != next_addr) {
> +				DRM_ERROR("buffer chunks must be mapped contiguously");
> +				return ERR_PTR(-EINVAL);
> +			}
> +			next_addr = sg_dma_address(s) + sg_dma_len(s);
> +		}
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
>  	if (IS_ERR(exynos_gem)) {
>  		ret = PTR_ERR(exynos_gem);
> @@ -480,18 +497,15 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device *dev,
>  
>  	exynos_gem->sgt = sgt;
>  
> -	if (sgt->nents == 1) {
> -		/* always physically continuous memory if sgt->nents is 1. */
> -		exynos_gem->flags |= EXYNOS_BO_CONTIG;
> -	} else {
> -		/*
> -		 * this case could be CONTIG or NONCONTIG type but for now
> -		 * sets NONCONTIG.
> -		 * TODO. we have to find a way that exporter can notify
> -		 * the type of its own buffer to importer.
> -		 */
> +	/*
> +	 * Buffer has been mapped as contiguous into DMA address space,
> +	 * but if there is IOMMU, it can be either CONTIG or NONCONTIG.
> +	 * We assume a simplified logic below:
> +	 */
> +	if (is_drm_iommu_supported(dev))
>  		exynos_gem->flags |= EXYNOS_BO_NONCONTIG;
> -	}
> +	else
> +		exynos_gem->flags |= EXYNOS_BO_CONTIG;
>  
>  	return &exynos_gem->base;
>  
>

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

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

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

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

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