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

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

[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
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@samsung.com>
CC: stable@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;
+
 	/* set memory type and cache attribute from user side. */
 	exynos_gem->flags = flags;
 
-- 
2.14.2

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

[Inki Dae]

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
> 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@samsung.com>
> CC: stable@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;

Even through iommu isn't supported, there could be 3D GPU or other DMA devices which support IOMMU. Of course, in this case, at least one memory copy will be required to pass it to Display controller.
So I'm not clear yet and it would bring more dicussions here.

Thanks,
Inki Dae

> +
>  	/* set memory type and cache attribute from user side. */
>  	exynos_gem->flags = flags;
>  
> 

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

[Marek Szyprowski]

Hi Inki,

On 2017-11-01 07:31, 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
>> 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@samsung.com>
>> CC: stable@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;
> Even through iommu isn't supported, there could be 3D GPU or other DMA devices which support IOMMU. Of course, in this case, at least one memory copy will be required to pass it to Display controller.
> So I'm not clear yet and it would bring more dicussions here.

This is NONCONTIG flag drop is done only in exynos_drm_gem_create() 
function,
which always allocates memory using dma_alloc_attrs() (a replacement for
dma_alloc_coherent). There is no point keeping this flag, when we KNOW that
the buffer is contiguous.

For buffers shared from 3D GPU or other DMA device, GEM object will be 
created
using exynos_drm_gem_prime_import() and 
exynos_drm_gem_prime_import_sg_table()
functions, which checks for buffer contiguity and sets proper flags.

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

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

[Inki Dae]

Hi Marek,

2017년 11월 02일 15:51에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 2017-11-01 07:31, 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
>>> 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@samsung.com>
>>> CC: stable@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;
>> Even through iommu isn't supported, there could be 3D GPU or other DMA devices which support IOMMU. Of course, in this case, at least one memory copy will be required to pass it to Display controller.
>> So I'm not clear yet and it would bring more dicussions here.
> 
> This is NONCONTIG flag drop is done only in exynos_drm_gem_create() function,

Ah, sorry. it's NONCONTIG, not CONTIG. there was my mistaken.

Thanks,
Inki Dae

> which always allocates memory using dma_alloc_attrs() (a replacement for
> dma_alloc_coherent). There is no point keeping this flag, when we KNOW that
> the buffer is contiguous.
> 
> For buffers shared from 3D GPU or other DMA device, GEM object will be created
> using exynos_drm_gem_prime_import() and exynos_drm_gem_prime_import_sg_table()
> functions, which checks for buffer contiguity and sets proper flags.
> 
> Best regards

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

[Inki Dae]

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.

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.

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.

> 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@samsung.com>
> CC: stable@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;
>  
> 

Re: [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@samsung.com>
>> CC: stable@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

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

[Inki Dae]

Hi Marek,

2017년 11월 10일 16:35에 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).

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

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

So you could provide some information - maybe warning message?? - to user space the buffer type is changed to CONTIG buffer due to NO IOMMU support.

Regarding the buffer type Exynos DRM has, I'm not sure that providing CONTIG and NONCONTIG buffer types to user space is reasonable because I think such buffer types - physically contiguous or non contiguous - should be transparent to user space.
And I looked into other ARM DRM drivers - omap and msm provides SCANOUT buffer type, tegra and rockchip have no such buffer types (contiguous or non-contiguous).

This is also a thing I felt while working on kernel in collaborating with Platform guys - we made user spaces to care about the memory allocation way.

I wonder why you say SCANOUT type is incorrect flag.

Thanks,
Inki Dae

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

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

[Marek Szyprowski]

Hi Inki,

On 2017-11-13 02:24, Inki Dae wrote:
> Hi Marek,
>
> 2017년 11월 10일 16:35에 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).
> 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.

> So you could provide some information - maybe warning message?? - to user space the buffer type is changed to CONTIG buffer due to NO IOMMU support.

I don't think this needs a warning. I just think that when we know that
the allocated buffer IS contiguous there is no point flagging it as
non-contiguous and fail a few calls later although the driver allocated
the contiguous buffer in fact. That's all.

> Regarding the buffer type Exynos DRM has, I'm not sure that providing CONTIG and NONCONTIG buffer types to user space is reasonable because I think such buffer types - physically contiguous or non contiguous - should be transparent to user space.
> And I looked into other ARM DRM drivers - omap and msm provides SCANOUT buffer type, tegra and rockchip have no such buffer types (contiguous or non-contiguous).
>
> This is also a thing I felt while working on kernel in collaborating with Platform guys - we made user spaces to care about the memory allocation way.
>
> I wonder why you say SCANOUT type is incorrect flag.

I'm not against adding SCANOUT flag. It is a good idea in general,
especially if the driver can simply select which set of flags will
be best for such use case on the given hardware (IOMMU is practically
always used on newer Exynos SoCs, IOMMU has enough TLB cache and
handling of non-contiguous buffers is cheap there).

I would only separate it from fixing the current code.

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

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

[Inki Dae]

2017년 11월 13일 23:28에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 2017-11-13 02:24, Inki Dae wrote:
>> Hi Marek,
>>
>> 2017년 11월 10일 16:35에 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).
>> 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. 

Thanks,
Inki Dae

> 
>> So you could provide some information - maybe warning message?? - to user space the buffer type is changed to CONTIG buffer due to NO IOMMU support.
> 
> I don't think this needs a warning. I just think that when we know that
> the allocated buffer IS contiguous there is no point flagging it as
> non-contiguous and fail a few calls later although the driver allocated
> the contiguous buffer in fact. That's all.
> 
>> Regarding the buffer type Exynos DRM has, I'm not sure that providing CONTIG and NONCONTIG buffer types to user space is reasonable because I think such buffer types - physically contiguous or non contiguous - should be transparent to user space.
>> And I looked into other ARM DRM drivers - omap and msm provides SCANOUT buffer type, tegra and rockchip have no such buffer types (contiguous or non-contiguous).
>>
>> This is also a thing I felt while working on kernel in collaborating with Platform guys - we made user spaces to care about the memory allocation way.
>>
>> I wonder why you say SCANOUT type is incorrect flag.
> 
> I'm not against adding SCANOUT flag. It is a good idea in general,
> especially if the driver can simply select which set of flags will
> be best for such use case on the given hardware (IOMMU is practically
> always used on newer Exynos SoCs, IOMMU has enough TLB cache and
> handling of non-contiguous buffers is cheap there).
> 
> I would only separate it from fixing the current code.
> 
> Best regards

Re: [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