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