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@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.
>
> 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
>>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
