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

[Marek Szyprowski]

Dear Inki,

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

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

>> BO_NONCONTIG flag for the allocated GEM buffers. This allows to avoid
>> failures for buffer contiguity checks in the subsequent operations on GEM
>> objects.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
>> CC: stable at vger.kernel.org # v4.4+
>> ---
>> This issue is there since commit 0519f9a12d011 ("drm/exynos: add iommu
>> support for exynos drm framework"), but this patch applies cleanly
>> only to v4.4+ kernel releases due changes in the surrounding code.
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_gem.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> index 02f978bb9261..476c00fe1998 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> @@ -227,6 +227,13 @@ struct exynos_drm_gem *exynos_drm_gem_create(struct drm_device *dev,
>>   	if (IS_ERR(exynos_gem))
>>   		return exynos_gem;
>>   
>> +	/*
>> +	 * when no IOMMU is available, all allocated buffers are contiguous
>> +	 * anyway, so drop EXYNOS_BO_NONCONTIG flag
>> +	 */
>> +	if (!is_drm_iommu_supported(dev))
>> +		flags &= ~EXYNOS_BO_NONCONTIG;
> So this could be a tempararily solution until the new flag is added, and you may need to modify above comments.
>
> Thanks,
> Inki Dae
>
>> +
>>   	/* set memory type and cache attribute from user side. */
>>   	exynos_gem->flags = flags;
>>   
>>
>

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