[PATCH] drm/exynos: gem: Drop NONCONTIG flag for buffers allocated without IOMMU

[Marek Szyprowski]

Hi Inki,

On 2017-11-14 00:46, Inki Dae wrote:
> 2017년 11월 13일 23:28에 Marek Szyprowski 이(가) 쓴 글:
>> On 2017-11-13 02:24, Inki Dae wrote:
>>> 2017년 11월 10일 16:35에 Marek Szyprowski 이(가) 쓴 글:
>>>> On 2017-11-10 04:04, Inki Dae wrote:
>>>>> 2017년 11월 01일 01:28에 Marek Szyprowski 이(가) 쓴 글:
>>>>>> When no IOMMU is available, all GEM buffers allocated by Exynos DRM driver
>>>>>> are contiguous, because of the underlying dma_alloc_attrs() function
>>>>>> provides only such buffers. In such case it makes no sense to keep
>>>>> What if user disabled CMA support? In this case, it guarantees also to allocate physically contiguous memory?
>>>>> I think it depends on CMA support so wouldn't be true.
>>>> dma_alloc_attrs() always guarantees the contiguity of the allocated memory
>>>> in DMA address space. When NOIOMMU is available, this mean that the allocated
>>>> buffer is contiguous in the physical memory. When CMA is disabled,
>>>> dma_alloc_attrs() uses alloc_pages() allocator. The drawback of alloc_pages()
>>>> is that it easily fails if physical memory is fragmented and it cannot
>>>> allocate memory larger than MAX_ORDER (4MiB in case of ARM32bit).
>>> You are right. Without IOMMU suppot alloc_pages always tryies to allocate contiguous memory,
>>> order = get_order(size);
>>> page = alloc_pages(..., order);
>>> if (!page)
>>>      return NULL;
>>> ...
>> Right
>>
>>>>> Real problem I think is that user don't know whether the gem buffer allocated with CONTIG or NONCONTIG flag can be used as a SCANOUT buffer.
>>>>> So user can request a page flip with NONCONTIG buffer to kernel which doesn't support IOMMU.
>>>>>
>>>>> And another is that user may want to use NONCONTIG buffer for another purpose, not scanout.
>>>>> So if we enforce on using CONTIG buffer on kernel without IOMMU support, then it wouldn't be really what user intended.
>>>> When IOMMU support is disabled, ANY buffer allocated with dma_alloc_attrs()
>>>> will be contiguous, so I see no point propagating incorrect flag for it.
>>>>
>>>> The only way to create a NONCONTIG buffer with IOMMU disabled is to import
>>>> it from other driver and this path is already handled correctly.
>>>>
>>>>> My idea is to provide a new flag - i.e., EXYNOS_BO_SCANOUT - which can allocate a buffer with a different allocation type - CONTIG or NONCONTIG - according to IOMMU support.
>>>>> And any page flip request with NONCONTIG buffer to kernel without IOMMU support should fail and it has to return a error with a proper error message.
>>>> I don't think that we need it. With the proposed patch the same userspace will
>>>> work fine in both cases IOMMU enabled and disabled, even if it allocate GEM
>>>> with NONCONTIG flag. We can assume that CONTIG is a special case of NONCONTIG
>>>> then.
>>> The problem is really not whether user space will work fine or not but is that this may be not what user space wanted.
>>> I.e., user space expects the allocated buffer is NONCONTIG buffer because user space requested NONCONTIG buffer but kernel internally, this is changed to CONTIG buffer.
>> What's the problem when kernel allocated contiguous buffer but user
>> requested non-contiguous? Contiguous is a subclass of non-contiguous
>> in general. There is no driver nor scenario which will not work because
>> of such change (any code for handing n-segments should be fine with only
>> 1 segment). In some conditions, by luck, kernel might allocate
>> a contiguous buffer even with IOMMU enabled. When we know that the
>> buffer is contiguous, then flag it as such.
> I'd like to say what I experienced before here. I'd ever modified in-house kernel with similar issue that with IOMMU sometimes page fault happended. So tempararily I always made gem allocation request from user space to allocate CONTIG buffer and it worked well without page fault. Several days ago, a Platform guy reported one issue that gem allocation request failed even through it has free memory enough. The issus was as you guess fragementation issue and I talked to him memory fragmentation happended.
>
> However, that guy didn't understand why memory fragementation happended because he definitely requested NONCONTIG buffer allocation. Thus, if I provided a some hit - gem allocation way is changed to CONTIG due to some reason - to user space then he could understand the fragmentation issue without contacting me. This patch could fix the X Server issue but the X Server never know that the allocated buffer is contiguous because you changed the allocation type witout user's knowledge.

Okay, I will post a v2 with a warning.

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