[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

Fix exynos_drm_gem_create_ioctl() attempts to allocate non-contiguous GEM
memory without IOMMU. In this case, there is no point in attempting to
allocate non-contiguous memory, only to return error during the next step
from exynos_drm_framebuffer_init() which leads to display manager failing
to start.

Check if non-contiguous GEM memory is requested without IOMMU. If so,
allocate contiguous GEM memory to help display manager start.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 4c4cb0e..4719116 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -266,6 +266,20 @@ int exynos_drm_gem_create_ioctl(struct drm_device *dev, void *data,
 	struct exynos_drm_gem *exynos_gem;
 	int ret;
 
+	/*
+	 * Check if non-contiguous GEM memory is requested without IOMMU.
+	 * If so, allocate contiguous GEM memory.
+	 *
+	 * There is no point in attempting to allocate non-contiguous memory,
+	 * only to return error from exynos_drm_framebuffer_init() which leads
+	 * to display manager failing to start.
+	*/
+	if (!is_drm_iommu_supported(dev) &&
+	    (args->flags & EXYNOS_BO_NONCONTIG)) {
+		args->flags &= ~EXYNOS_BO_NONCONTIG;
+		args->flags |= EXYNOS_BO_CONTIG;
+	}
+
 	exynos_gem = exynos_drm_gem_create(dev, args->flags, args->size);
 	if (IS_ERR(exynos_gem))
 		return PTR_ERR(exynos_gem);
-- 
2.7.4

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Javier Martinez Canillas]

Hello Shuah,

On Wed, Aug 10, 2016 at 1:30 PM, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> Fix exynos_drm_gem_create_ioctl() attempts to allocate non-contiguous GEM
> memory without IOMMU. In this case, there is no point in attempting to
> allocate non-contiguous memory, only to return error during the next step
> from exynos_drm_framebuffer_init() which leads to display manager failing
> to start.
>
> Check if non-contiguous GEM memory is requested without IOMMU. If so,
> allocate contiguous GEM memory to help display manager start.
>
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 4c4cb0e..4719116 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -266,6 +266,20 @@ int exynos_drm_gem_create_ioctl(struct drm_device *dev, void *data,
>         struct exynos_drm_gem *exynos_gem;
>         int ret;
>
> +       /*
> +        * Check if non-contiguous GEM memory is requested without IOMMU.
> +        * If so, allocate contiguous GEM memory.
> +        *
> +        * There is no point in attempting to allocate non-contiguous memory,
> +        * only to return error from exynos_drm_framebuffer_init() which leads
> +        * to display manager failing to start.
> +       */
> +       if (!is_drm_iommu_supported(dev) &&
> +           (args->flags & EXYNOS_BO_NONCONTIG)) {
> +               args->flags &= ~EXYNOS_BO_NONCONTIG;
> +               args->flags |= EXYNOS_BO_CONTIG;
> +       }
> +

I'm not a DRM expert so I don't know if this is the correct way to
handle this. But the approach sounds sensible to me.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
Javier

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Inki Dae]

Hi Shuah,

2016? 08? 11? 02:30? Shuah Khan ?(?) ? ?:
> Fix exynos_drm_gem_create_ioctl() attempts to allocate non-contiguous GEM
> memory without IOMMU. In this case, there is no point in attempting to

DRM gem can be used for Non-DRM drivers such as GPU, V4L2 based Multimedia device and other DMA devices.
Even though IOMMU support is disabled, other framework based DMA drivers can use IOMMU - i.e., GPU driver -
and they can use non-contiguous GEM buffer through UMM. (DMABUF) 

So GEM allocation type is not dependent on IOMMU.

Thanks,
Inki Dae

> allocate non-contiguous memory, only to return error during the next step
> from exynos_drm_framebuffer_init() which leads to display manager failing
> to start.
> 
> Check if non-contiguous GEM memory is requested without IOMMU. If so,
> allocate contiguous GEM memory to help display manager start.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 4c4cb0e..4719116 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -266,6 +266,20 @@ int exynos_drm_gem_create_ioctl(struct drm_device *dev, void *data,
>  	struct exynos_drm_gem *exynos_gem;
>  	int ret;
>  
> +	/*
> +	 * Check if non-contiguous GEM memory is requested without IOMMU.
> +	 * If so, allocate contiguous GEM memory.
> +	 *
> +	 * There is no point in attempting to allocate non-contiguous memory,
> +	 * only to return error from exynos_drm_framebuffer_init() which leads
> +	 * to display manager failing to start.
> +	*/
> +	if (!is_drm_iommu_supported(dev) &&
> +	    (args->flags & EXYNOS_BO_NONCONTIG)) {
> +		args->flags &= ~EXYNOS_BO_NONCONTIG;
> +		args->flags |= EXYNOS_BO_CONTIG;
> +	}
> +
>  	exynos_gem = exynos_drm_gem_create(dev, args->flags, args->size);
>  	if (IS_ERR(exynos_gem))
>  		return PTR_ERR(exynos_gem);
> 

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

On 08/10/2016 04:59 PM, Inki Dae wrote:
> Hi Shuah,
> 
> 2016? 08? 11? 02:30? Shuah Khan ?(?) ? ?:
>> Fix exynos_drm_gem_create_ioctl() attempts to allocate non-contiguous GEM
>> memory without IOMMU. In this case, there is no point in attempting to
> 
> DRM gem can be used for Non-DRM drivers such as GPU, V4L2 based Multimedia device and other DMA devices.
> Even though IOMMU support is disabled, other framework based DMA drivers can use IOMMU - i.e., GPU driver -
> and they can use non-contiguous GEM buffer through UMM. (DMABUF) 
> 
> So GEM allocation type is not dependent on IOMMU.

Hi Inki,

I am seeing the following failure without IOMMU and light dm fails
to start:

[drm:exynos_drm_framebuffer_init] *ERROR* Non-continguous GEM memory is not supported.

The change I made fixed that problem and light dm starts without IOMMU.
Is there a better way to fix this problem? Currently without IOMMU,
light dm doesn't start.

This is on linux_next

thanks,
-- Shuah

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

On 08/10/2016 05:05 PM, Shuah Khan wrote:
> On 08/10/2016 04:59 PM, Inki Dae wrote:
>> Hi Shuah,
>>
>> 2016? 08? 11? 02:30? Shuah Khan ?(?) ? ?:
>>> Fix exynos_drm_gem_create_ioctl() attempts to allocate non-contiguous GEM
>>> memory without IOMMU. In this case, there is no point in attempting to
>>
>> DRM gem can be used for Non-DRM drivers such as GPU, V4L2 based Multimedia device and other DMA devices.
>> Even though IOMMU support is disabled, other framework based DMA drivers can use IOMMU - i.e., GPU driver -
>> and they can use non-contiguous GEM buffer through UMM. (DMABUF) 
>>
>> So GEM allocation type is not dependent on IOMMU.
> 
> Hi Inki,
> 
> I am seeing the following failure without IOMMU and light dm fails
> to start:
> 
> [drm:exynos_drm_framebuffer_init] *ERROR* Non-continguous GEM memory is not supported.
> 
> The change I made fixed that problem and light dm starts without IOMMU.
> Is there a better way to fix this problem? Currently without IOMMU,
> light dm doesn't start.
> 
> This is on linux_next

Hi Inki,

I am looking into this further and I am finding inconsistent
commits with regards to GEM contiguous and non-contiguous
buffers.

Okay what you said is that:

exymod-drm should support non-continguous and contiguous GEM memory
type with or without IOMMU

However, the code currently isn't doing that. The following
commit allocates non-contiguous buffers when IOMMU is enabled
to handle contiguous allocation failures.

There are other commits that removed checks for non-contig type.
Let's look at the following cases to see what should be the driver
behavior in these cases:

IOMMU is disabled:

exynos_drm_gem_create_ioctl() gets called with NONCONTIG
- driver should try to allocate non-contig
- if it can't allocate non-contig, allocate contig
  ( this will allow avoid failure like the one I am seeing)

exynos_drm_gem_create_ioctl() gets called with CONTIG
- driver should try to allocate contig
- if it can't allocate contig, allocate non-contig

What is confusing is there are several code paths in the
GEN allocation and checking memory types are enforcing
non-contig with IOMMU. Check this routine:

exynos_drm_framebuffer_init() will reject non-contig
memory type when check_fb_gem_memory_type() rejects
non-contig GEM memory type without IOMMU.

So there is inconsistency in the non-contig vs. contig
GEM support in exynos-drm. I think this needs to be cleaned
up to get the desired behavior.

The following commit allocates non-contiguous buffers when IOMMU is
enabled to handle contiguous allocation failures.

There are other commits that removed checks for non-contig type.
Let's look at the following cases to see what should be the driver
behavior in these cases:

commit 122beea84bb90236b1ae545f08267af58591c21b
Author: Rahul Sharma <Rahul.Sharma@samsung.com>
Date:   Wed May 7 17:21:29 2014 +0530

    drm/exynos: allocate non-contigous buffers when iommu is enabled
    
    Allow to allocate non-contigous buffers when iommu is enabled.
    Currently, it tries to allocates contigous buffer which consistently
    fail for large buffers and then fall back to non contigous. Apart
    from being slow, this implementation is also very noisy and fills
    the screen with alloc fail logs.
    
    Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
    Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
    Signed-off-by: Inki Dae <inki.dae@samsung.com>


commit ea6d66c3a797376d21b23dc8261733ce35970014
Author: Inki Dae <inki.dae@samsung.com>
Date:   Fri Nov 2 16:10:39 2012 +0900

    drm/exynos: remove EXYNOS_BO_NONCONTIG type checking.
    
    With iommu support, non-continuous buffer also is supported so
    this patch removes these checking from exynos_drm_gem_get/put_dma_addr
    funciton.
    
    This patch is based on the below patch set, "drm/exynos: add
    iommu support for -next".
        http://www.spinics.net/lists/dri-devel/msg29041.html
    
    Signed-off-by: Inki Dae <inki.dae@samsung.com>
    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

commit 2b35892e9da672df40ce890bffc4f9f6119c57e0
Author: Inki Dae <inki.dae@samsung.com>
Date:   Fri Mar 16 18:47:05 2012 +0900

    drm/exynos: update gem and buffer framework.
    
    with this patch, we can allocate physically continuous or non-continuous
    memory and also it creates scatterlist for iommu support so allocated
    memory region can be mapped to iommu page table using scatterlist.
    
    Signed-off-by: Inki Dae <inki.dae@samsung.com>
    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

-- Shuah

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

On 08/12/2016 11:28 AM, Shuah Khan wrote:
> On 08/10/2016 05:05 PM, Shuah Khan wrote:
>> On 08/10/2016 04:59 PM, Inki Dae wrote:
>>> Hi Shuah,
>>>
>>> 2016? 08? 11? 02:30? Shuah Khan ?(?) ? ?:
>>>> Fix exynos_drm_gem_create_ioctl() attempts to allocate non-contiguous GEM
>>>> memory without IOMMU. In this case, there is no point in attempting to
>>>
>>> DRM gem can be used for Non-DRM drivers such as GPU, V4L2 based Multimedia device and other DMA devices.
>>> Even though IOMMU support is disabled, other framework based DMA drivers can use IOMMU - i.e., GPU driver -
>>> and they can use non-contiguous GEM buffer through UMM. (DMABUF) 
>>>
>>> So GEM allocation type is not dependent on IOMMU.
>>
>> Hi Inki,
>>
>> I am seeing the following failure without IOMMU and light dm fails
>> to start:
>>
>> [drm:exynos_drm_framebuffer_init] *ERROR* Non-continguous GEM memory is not supported.
>>
>> The change I made fixed that problem and light dm starts without IOMMU.
>> Is there a better way to fix this problem? Currently without IOMMU,
>> light dm doesn't start.
>>
>> This is on linux_next
> 
> Hi Inki,
> 
> I am looking into this further and I am finding inconsistent
> commits with regards to GEM contiguous and non-contiguous
> buffers.
> 
> Okay what you said is that:
> 
> exymod-drm should support non-continguous and contiguous GEM memory
> type with or without IOMMU
> 
> However, the code currently isn't doing that. The following
> commit allocates non-contiguous buffers when IOMMU is enabled
> to handle contiguous allocation failures.
> 
> There are other commits that removed checks for non-contig type.
> Let's look at the following cases to see what should be the driver
> behavior in these cases:
> 
> IOMMU is disabled:
> 
> exynos_drm_gem_create_ioctl() gets called with NONCONTIG
> - driver should try to allocate non-contig
> - if it can't allocate non-contig, allocate contig
>   ( this will allow avoid failure like the one I am seeing)
> 
> exynos_drm_gem_create_ioctl() gets called with CONTIG
> - driver should try to allocate contig
> - if it can't allocate contig, allocate non-contig
> 
> What is confusing is there are several code paths in the
> GEN allocation and checking memory types are enforcing
> non-contig with IOMMU. Check this routine:
> 
> exynos_drm_framebuffer_init() will reject non-contig
> memory type when check_fb_gem_memory_type() rejects
> non-contig GEM memory type without IOMMU.


okay the very first commit that added IOMMU support
introduced the code that rejects non-contig gem memory
type without IOMMU.

commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
Author: Inki Dae <inki.dae@samsung.com>
Date:   Sat Oct 20 07:53:42 2012 -0700

    drm/exynos: add iommu support for exynos drm framework

Anyway, if it is th right change to fix check_fb_gem_memory_type()
to not reject NONCONTIG_BUFFER, then I can make that change
instead of this patch I sent.

> 
> So there is inconsistency in the non-contig vs. contig
> GEM support in exynos-drm. I think this needs to be cleaned
> up to get the desired behavior.
> 
> The following commit allocates non-contiguous buffers when IOMMU is
> enabled to handle contiguous allocation failures.
> 
> There are other commits that removed checks for non-contig type.
> Let's look at the following cases to see what should be the driver
> behavior in these cases:
> 
> commit 122beea84bb90236b1ae545f08267af58591c21b
> Author: Rahul Sharma <Rahul.Sharma@samsung.com>
> Date:   Wed May 7 17:21:29 2014 +0530
> 
>     drm/exynos: allocate non-contigous buffers when iommu is enabled
>     
>     Allow to allocate non-contigous buffers when iommu is enabled.
>     Currently, it tries to allocates contigous buffer which consistently
>     fail for large buffers and then fall back to non contigous. Apart
>     from being slow, this implementation is also very noisy and fills
>     the screen with alloc fail logs.
>     
>     Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>     Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
> 
> 
> commit ea6d66c3a797376d21b23dc8261733ce35970014
> Author: Inki Dae <inki.dae@samsung.com>
> Date:   Fri Nov 2 16:10:39 2012 +0900
> 
>     drm/exynos: remove EXYNOS_BO_NONCONTIG type checking.
>     
>     With iommu support, non-continuous buffer also is supported so
>     this patch removes these checking from exynos_drm_gem_get/put_dma_addr
>     funciton.
>     
>     This patch is based on the below patch set, "drm/exynos: add
>     iommu support for -next".
>         http://www.spinics.net/lists/dri-devel/msg29041.html
>     
>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> commit 2b35892e9da672df40ce890bffc4f9f6119c57e0
> Author: Inki Dae <inki.dae@samsung.com>
> Date:   Fri Mar 16 18:47:05 2012 +0900
> 
>     drm/exynos: update gem and buffer framework.
>     
>     with this patch, we can allocate physically continuous or non-continuous
>     memory and also it creates scatterlist for iommu support so allocated
>     memory region can be mapped to iommu page table using scatterlist.
>     
>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>     Signed-off-by: Dave Airlie <airlied@redhat.com>
> 
> -- Shuah
> 

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Inki Dae]

Hi Shuah,

2016? 08? 13? 02:52? Shuah Khan ?(?) ? ?:
> On 08/12/2016 11:28 AM, Shuah Khan wrote:
>> On 08/10/2016 05:05 PM, Shuah Khan wrote:
>>> On 08/10/2016 04:59 PM, Inki Dae wrote:
>>>> Hi Shuah,
>>>>
>>>> 2016? 08? 11? 02:30? Shuah Khan ?(?) ? ?:
>>>>> Fix exynos_drm_gem_create_ioctl() attempts to allocate non-contiguous GEM
>>>>> memory without IOMMU. In this case, there is no point in attempting to
>>>>
>>>> DRM gem can be used for Non-DRM drivers such as GPU, V4L2 based Multimedia device and other DMA devices.
>>>> Even though IOMMU support is disabled, other framework based DMA drivers can use IOMMU - i.e., GPU driver -
>>>> and they can use non-contiguous GEM buffer through UMM. (DMABUF) 
>>>>
>>>> So GEM allocation type is not dependent on IOMMU.
>>>
>>> Hi Inki,
>>>
>>> I am seeing the following failure without IOMMU and light dm fails
>>> to start:
>>>
>>> [drm:exynos_drm_framebuffer_init] *ERROR* Non-continguous GEM memory is not supported.
>>>
>>> The change I made fixed that problem and light dm starts without IOMMU.
>>> Is there a better way to fix this problem? Currently without IOMMU,
>>> light dm doesn't start.
>>>
>>> This is on linux_next
>>
>> Hi Inki,
>>
>> I am looking into this further and I am finding inconsistent
>> commits with regards to GEM contiguous and non-contiguous
>> buffers.
>>
>> Okay what you said is that:
>>
>> exymod-drm should support non-continguous and contiguous GEM memory
>> type with or without IOMMU

Right.

>>
>> However, the code currently isn't doing that. The following
>> commit allocates non-contiguous buffers when IOMMU is enabled
>> to handle contiguous allocation failures.
>>
>> There are other commits that removed checks for non-contig type.
>> Let's look at the following cases to see what should be the driver
>> behavior in these cases:
>>
>> IOMMU is disabled:
>>
>> exynos_drm_gem_create_ioctl() gets called with NONCONTIG
>> - driver should try to allocate non-contig
>> - if it can't allocate non-contig, allocate contig
>>   ( this will allow avoid failure like the one I am seeing)
>>
>> exynos_drm_gem_create_ioctl() gets called with CONTIG
>> - driver should try to allocate contig
>> - if it can't allocate contig, allocate non-contig
>>
>> What is confusing is there are several code paths in the
>> GEN allocation and checking memory types are enforcing
>> non-contig with IOMMU. Check this routine:
>>
>> exynos_drm_framebuffer_init() will reject non-contig
>> memory type when check_fb_gem_memory_type() rejects
>> non-contig GEM memory type without IOMMU.

Only in case that the gem buffer is used for framebuffer, gem memory type should be checked because this means the DMA of Display controller accesses the gem buffer so without IOMMU the DMA device cannot access non-contiguous memory region.
That is why exynos_drm_framebuffer_init checks gem memory type for fb not when gem is created.

> 
> 
> okay the very first commit that added IOMMU support
> introduced the code that rejects non-contig gem memory
> type without IOMMU.
> 
> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
> Author: Inki Dae <inki.dae@samsung.com>
> Date:   Sat Oct 20 07:53:42 2012 -0700
> 
>     drm/exynos: add iommu support for exynos drm framework
> 
> Anyway, if it is th right change to fix check_fb_gem_memory_type()
> to not reject NONCONTIG_BUFFER, then I can make that change

No, as I mentioned above, the gem buffer for fb is dependent on IOMMU because the gem buffer for fb is used by DMA device - FIMD, DECON or Mixer.
You would need to understand that gem buffer can be used for other purposes - 2D/3D or post process devices which don't use framebuffer - not display controller which uses framebuffer to scanout

Thanks,
Inki Dae

> instead of this patch I sent.
> 
>>
>> So there is inconsistency in the non-contig vs. contig
>> GEM support in exynos-drm. I think this needs to be cleaned
>> up to get the desired behavior.
>>
>> The following commit allocates non-contiguous buffers when IOMMU is
>> enabled to handle contiguous allocation failures.
>>
>> There are other commits that removed checks for non-contig type.
>> Let's look at the following cases to see what should be the driver
>> behavior in these cases:
>>
>> commit 122beea84bb90236b1ae545f08267af58591c21b
>> Author: Rahul Sharma <Rahul.Sharma@samsung.com>
>> Date:   Wed May 7 17:21:29 2014 +0530
>>
>>     drm/exynos: allocate non-contigous buffers when iommu is enabled
>>     
>>     Allow to allocate non-contigous buffers when iommu is enabled.
>>     Currently, it tries to allocates contigous buffer which consistently
>>     fail for large buffers and then fall back to non contigous. Apart
>>     from being slow, this implementation is also very noisy and fills
>>     the screen with alloc fail logs.
>>     
>>     Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>>     Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
>>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>
>>
>> commit ea6d66c3a797376d21b23dc8261733ce35970014
>> Author: Inki Dae <inki.dae@samsung.com>
>> Date:   Fri Nov 2 16:10:39 2012 +0900
>>
>>     drm/exynos: remove EXYNOS_BO_NONCONTIG type checking.
>>     
>>     With iommu support, non-continuous buffer also is supported so
>>     this patch removes these checking from exynos_drm_gem_get/put_dma_addr
>>     funciton.
>>     
>>     This patch is based on the below patch set, "drm/exynos: add
>>     iommu support for -next".
>>         http://www.spinics.net/lists/dri-devel/msg29041.html
>>     
>>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>
>> commit 2b35892e9da672df40ce890bffc4f9f6119c57e0
>> Author: Inki Dae <inki.dae@samsung.com>
>> Date:   Fri Mar 16 18:47:05 2012 +0900
>>
>>     drm/exynos: update gem and buffer framework.
>>     
>>     with this patch, we can allocate physically continuous or non-continuous
>>     memory and also it creates scatterlist for iommu support so allocated
>>     memory region can be mapped to iommu page table using scatterlist.
>>     
>>     Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>     Signed-off-by: Dave Airlie <airlied@redhat.com>
>>
>> -- Shuah
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

Hi Inki,

On 08/15/2016 10:40 PM, Inki Dae wrote:

>>
>> okay the very first commit that added IOMMU support
>> introduced the code that rejects non-contig gem memory
>> type without IOMMU.
>>
>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
>> Author: Inki Dae <inki.dae@samsung.com>
>> Date:   Sat Oct 20 07:53:42 2012 -0700
>>
>>     drm/exynos: add iommu support for exynos drm framework
>>

I haven't given up on this yet. I am still seeing the following failure:

Additional debug messages I added:
[   15.287403] exynos_drm_gem_create_ioctl() 1
[   15.287419] exynos_drm_gem_create() flags 1

[   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.

Additional debug message I added:
[   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer

This is what happens:

1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
3. exynos_user_fb_create() tries to associate GEM to fb and fails during
   check_fb_gem_memory_type()

At this point, there is no recovery and lightdm fails

xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
allocations are not supported in some exynos drm versions: The following
commit introduced this change:

https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9

excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
-               create_exynos.flags = EXYNOS_BO_CONTIG;
-       else
-               create_exynos.flags = EXYNOS_BO_NONCONTIG;
+
+       /* Contiguous allocations are not supported in some exynos drm versions.
+        * When they are supported all allocations are effectively contiguous
+        * anyway, so for simplicity we always request non contiguous buffers.
+        */
+       create_exynos.flags = EXYNOS_BO_NONCONTIG;

There might have been logic on exynos_drm that forced Contig when it coudn't
support NONCONTIG. At least, that is what this comment suggests. This assumption
doesn't appear to be a good one and not sure if this change was made to fix a bug.

After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
support, latest kernels have a mismatch with the installed xf86-video-armsoc

This is what I am running into. This leads to the following question:

1. How do we ensure exynos_drm kernel changes don't break user-space
   specifically xf86-video-armsoc
2. This seems to have gone undetected for a while. I see a change in
   exynos_drm_gem_dumb_create() that is probably addressing this type
   of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
   handling for IOMMU NONCONTIG case.

Anyway, I am interested in getting the exynos_drm kernel side code
and xf86-video-armsoc in sync to resolve the issue.

Could you recommend a going forward plan?

I can submit a patch to xf86-video-armsoc. I am also looking ahead to
see if we can avoid such breaks in the future by keeping kernel and
xf86-video-armsoc in sync.

thanks,
-- Shuah

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

On 10/12/2016 05:11 PM, Shuah Khan wrote:

+ Fixing Krzysztof Kozlowski address.

> Hi Inki,
> 
> On 08/15/2016 10:40 PM, Inki Dae wrote:
> 
>>>
>>> okay the very first commit that added IOMMU support
>>> introduced the code that rejects non-contig gem memory
>>> type without IOMMU.
>>>
>>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
>>> Author: Inki Dae <inki.dae@samsung.com>
>>> Date:   Sat Oct 20 07:53:42 2012 -0700
>>>
>>>     drm/exynos: add iommu support for exynos drm framework
>>>
> 
> I haven't given up on this yet. I am still seeing the following failure:
> 
> Additional debug messages I added:
> [   15.287403] exynos_drm_gem_create_ioctl() 1
> [   15.287419] exynos_drm_gem_create() flags 1
> 
> [   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.
> 
> Additional debug message I added:
> [   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer
> 
> This is what happens:
> 
> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during
>    check_fb_gem_memory_type()
> 
> At this point, there is no recovery and lightdm fails
> 
> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
> allocations are not supported in some exynos drm versions: The following
> commit introduced this change:
> 
> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9
> 
> excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
> -               create_exynos.flags = EXYNOS_BO_CONTIG;
> -       else
> -               create_exynos.flags = EXYNOS_BO_NONCONTIG;
> +
> +       /* Contiguous allocations are not supported in some exynos drm versions.
> +        * When they are supported all allocations are effectively contiguous
> +        * anyway, so for simplicity we always request non contiguous buffers.
> +        */
> +       create_exynos.flags = EXYNOS_BO_NONCONTIG;
> 
> There might have been logic on exynos_drm that forced Contig when it coudn't
> support NONCONTIG. At least, that is what this comment suggests. This assumption
> doesn't appear to be a good one and not sure if this change was made to fix a bug.
> 
> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
> support, latest kernels have a mismatch with the installed xf86-video-armsoc
> 
> This is what I am running into. This leads to the following question:
> 
> 1. How do we ensure exynos_drm kernel changes don't break user-space
>    specifically xf86-video-armsoc
> 2. This seems to have gone undetected for a while. I see a change in
>    exynos_drm_gem_dumb_create() that is probably addressing this type
>    of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
>    handling for IOMMU NONCONTIG case.
> 
> Anyway, I am interested in getting the exynos_drm kernel side code
> and xf86-video-armsoc in sync to resolve the issue.
> 
> Could you recommend a going forward plan?
> 
> I can submit a patch to xf86-video-armsoc. I am also looking ahead to
> see if we can avoid such breaks in the future by keeping kernel and
> xf86-video-armsoc in sync.
> 
> thanks,
> -- Shuah
> 

exynos-drm: display manager fails to start without IOMMU problem

[Tobias Jakobi]

Hello Shuah,

just a short note that more misleading comments about default allocation
flags can be found in libdrm.

https://cgit.freedesktop.org/mesa/drm/tree/exynos/exynos_drm.c

See e.g. the comment for exynos_bo_create().

In my opinion, the whole approach to _set_ a bit to get non-contigious
memory is messed up. It would make more sense to me to set a bit to
request an additional property (here "being contiguous") of the memory.

Anyway, clearing up this situation is highly appreciated!

More comments below.

With best wishes,
Tobias



Shuah Khan wrote:
> Restarting the thread with a different subject line:
> 
> I haven't given up on this yet. I am still seeing the following failure:
> 
> Additional debug messages I added:
> [   15.287403] exynos_drm_gem_create_ioctl() 1
> [   15.287419] exynos_drm_gem_create() flags 1
> 
> [   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.
> 
> Additional debug message I added:
> [   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer
> 
> This is what happens:
> 
> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during
>    check_fb_gem_memory_type()
> 
> At this point, there is no recovery and lightdm fails
> 
> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
> allocations are not supported in some exynos drm versions: The following
> commit introduced this change:
> 
> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9
> 
> excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
> -               create_exynos.flags = EXYNOS_BO_CONTIG;
> -       else
> -               create_exynos.flags = EXYNOS_BO_NONCONTIG;
> +
> +       /* Contiguous allocations are not supported in some exynos drm versions.
> +        * When they are supported all allocations are effectively contiguous
> +        * anyway, so for simplicity we always request non contiguous buffers.
> +        */
> +       create_exynos.flags = EXYNOS_BO_NONCONTIG;
> 
> There might have been logic on exynos_drm that forced Contig when it coudn't
> support NONCONTIG. At least, that is what this comment suggests. This assumption
> doesn't appear to be a good one and not sure if this change was made to fix a bug.
> 
> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
> support, latest kernels have a mismatch with the installed xf86-video-armsoc
> 
> This is what I am running into. This leads to the following question:
> 
> 1. How do we ensure exynos_drm kernel changes don't break user-space
>    specifically xf86-video-armsoc
> 2. This seems to have gone undetected for a while. I see a change in
>    exynos_drm_gem_dumb_create() that is probably addressing this type
>    of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
>    handling for IOMMU NONCONTIG case.
I don't think that this commit is related to the issue, since it is only
used for the generic dumb buffer ioctl, while armsoc is using an Exynos
specific ioctl.

So in particular you shouldn't see the issue with
xf86-video-modesetting. Might be worth trying that one out?


> 
> Anyway, I am interested in getting the exynos_drm kernel side code
> and xf86-video-armsoc in sync to resolve the issue.
> 
> Could you recommend a going forward plan?
> 
> I can submit a patch to xf86-video-armsoc. I am also looking ahead to
> see if we can avoid such breaks in the future by keeping kernel and
> xf86-video-armsoc in sync.
> 
> thanks,
> -- Shuah
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

exynos-drm: display manager fails to start without IOMMU problem

[Shuah Khan]

On 10/19/2016 10:23 AM, Tobias Jakobi wrote:
> Hello Shuah,
> 
> just a short note that more misleading comments about default allocation
> flags can be found in libdrm.
> 
> https://cgit.freedesktop.org/mesa/drm/tree/exynos/exynos_drm.c
> 
> See e.g. the comment for exynos_bo_create().
> 
> In my opinion, the whole approach to _set_ a bit to get non-contigious
> memory is messed up. It would make more sense to me to set a bit to
> request an additional property (here "being contiguous") of the memory.
> 
> Anyway, clearing up this situation is highly appreciated!
> 
> More comments below.
> 
> With best wishes,
> Tobias
> 

Hi Tobias,

Thanks for the note. Yes the comments are confusing. It seems like
some old assumptions are persisting. I will fix these as well.

-- Shuah

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Inki Dae]

Hi Shuah,

2016-10-13 8:11 GMT+09:00 Shuah Khan <shuahkh@osg.samsung.com>:
> Hi Inki,
>
> On 08/15/2016 10:40 PM, Inki Dae wrote:
>
>>>
>>> okay the very first commit that added IOMMU support
>>> introduced the code that rejects non-contig gem memory
>>> type without IOMMU.
>>>
>>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
>>> Author: Inki Dae <inki.dae@samsung.com>
>>> Date:   Sat Oct 20 07:53:42 2012 -0700
>>>
>>>     drm/exynos: add iommu support for exynos drm framework
>>>
>
> I haven't given up on this yet. I am still seeing the following failure:
>
> Additional debug messages I added:
> [   15.287403] exynos_drm_gem_create_ioctl() 1
> [   15.287419] exynos_drm_gem_create() flags 1
>
> [   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.
>
> Additional debug message I added:
> [   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer
>
> This is what happens:
>
> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during
>    check_fb_gem_memory_type()
>
> At this point, there is no recovery and lightdm fails
>
> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
> allocations are not supported in some exynos drm versions: The following
> commit introduced this change:
>
> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9
>
> excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
> -               create_exynos.flags = EXYNOS_BO_CONTIG;
> -       else
> -               create_exynos.flags = EXYNOS_BO_NONCONTIG;
> +
> +       /* Contiguous allocations are not supported in some exynos drm versions.
> +        * When they are supported all allocations are effectively contiguous
> +        * anyway, so for simplicity we always request non contiguous buffers.
> +        */
> +       create_exynos.flags = EXYNOS_BO_NONCONTIG;
>

Above comment, "Contiguous allocations are not supported in some
exynos drm versions.", seems wrong assumption.
The root cause, contiguous allocation is not supported, would be that
they used Linux kernel which didn't have CMA region enough - as
default 16MB, or didn't declare CMA region enough for the DMA device.
So I think they should not force to flag EXYNOS_BO_NONCONTIG and they
should manage the error case if the allocation failed.

> There might have been logic on exynos_drm that forced Contig when it coudn't
> support NONCONTIG. At least, that is what this comment suggests. This assumption
> doesn't appear to be a good one and not sure if this change was made to fix a bug.
>
> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
> support, latest kernels have a mismatch with the installed xf86-video-armsoc
>
> This is what I am running into. This leads to the following question:
>
> 1. How do we ensure exynos_drm kernel changes don't break user-space
>    specifically xf86-video-armsoc
> 2. This seems to have gone undetected for a while. I see a change in
>    exynos_drm_gem_dumb_create() that is probably addressing this type
>    of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
>    handling for IOMMU NONCONTIG case.

Seems this patch has a problem. This patch forces to flag NONCONTIG if
iommu is enabled. The flag should be depend on the argument from
user-space.
I.e., if user-space requested a gem allocation with CONTIG flag, then
Exynos drm driver should allocate contiguous memory even though iommu
is enabled.

>
> Anyway, I am interested in getting the exynos_drm kernel side code
> and xf86-video-armsoc in sync to resolve the issue.
>
> Could you recommend a going forward plan?

My opinion are,

1. Do not force to flag EXYNOS_BO_NONCONTIG at xf86-video-armsoc
2. Do not force to flag NONCONTIG at Exynos drm driver even though
iommu is enabled and flag allocation type with the argument from
user-space.

I think you could try to post above patches.

Thanks,
Inki Dae

>
> I can submit a patch to xf86-video-armsoc. I am also looking ahead to
> see if we can avoid such breaks in the future by keeping kernel and
> xf86-video-armsoc in sync.
>
> thanks,
> -- Shuah
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

On 10/19/2016 08:16 AM, Inki Dae wrote:
> Hi Shuah,
> 
> 2016-10-13 8:11 GMT+09:00 Shuah Khan <shuahkh@osg.samsung.com>:
>> Hi Inki,
>>
>> On 08/15/2016 10:40 PM, Inki Dae wrote:
>>
>>>>
>>>> okay the very first commit that added IOMMU support
>>>> introduced the code that rejects non-contig gem memory
>>>> type without IOMMU.
>>>>
>>>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
>>>> Author: Inki Dae <inki.dae@samsung.com>
>>>> Date:   Sat Oct 20 07:53:42 2012 -0700
>>>>
>>>>     drm/exynos: add iommu support for exynos drm framework
>>>>
>>
>> I haven't given up on this yet. I am still seeing the following failure:
>>
>> Additional debug messages I added:
>> [   15.287403] exynos_drm_gem_create_ioctl() 1
>> [   15.287419] exynos_drm_gem_create() flags 1
>>
>> [   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.
>>
>> Additional debug message I added:
>> [   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer
>>
>> This is what happens:
>>
>> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
>> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
>> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during
>>    check_fb_gem_memory_type()
>>
>> At this point, there is no recovery and lightdm fails
>>
>> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
>> allocations are not supported in some exynos drm versions: The following
>> commit introduced this change:
>>
>> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9
>>
>> excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
>> -               create_exynos.flags = EXYNOS_BO_CONTIG;
>> -       else
>> -               create_exynos.flags = EXYNOS_BO_NONCONTIG;
>> +
>> +       /* Contiguous allocations are not supported in some exynos drm versions.
>> +        * When they are supported all allocations are effectively contiguous
>> +        * anyway, so for simplicity we always request non contiguous buffers.
>> +        */
>> +       create_exynos.flags = EXYNOS_BO_NONCONTIG;
>>
> 
> Above comment, "Contiguous allocations are not supported in some
> exynos drm versions.", seems wrong assumption.
> The root cause, contiguous allocation is not supported, would be that
> they used Linux kernel which didn't have CMA region enough - as
> default 16MB, or didn't declare CMA region enough for the DMA device.
> So I think they should not force to flag EXYNOS_BO_NONCONTIG and they
> should manage the error case if the allocation failed.

This assumption doesn't sound correct and forcing NONCONTIG isn't right
either. 

> 
>> There might have been logic on exynos_drm that forced Contig when it coudn't
>> support NONCONTIG. At least, that is what this comment suggests. This assumption
>> doesn't appear to be a good one and not sure if this change was made to fix a bug.
>>
>> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
>> support, latest kernels have a mismatch with the installed xf86-video-armsoc
>>
>> This is what I am running into. This leads to the following question:
>>
>> 1. How do we ensure exynos_drm kernel changes don't break user-space
>>    specifically xf86-video-armsoc
>> 2. This seems to have gone undetected for a while. I see a change in
>>    exynos_drm_gem_dumb_create() that is probably addressing this type
>>    of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
>>    handling for IOMMU NONCONTIG case.
> 
> Seems this patch has a problem. This patch forces to flag NONCONTIG if
> iommu is enabled. The flag should be depend on the argument from
> user-space.
> I.e., if user-space requested a gem allocation with CONTIG flag, then
> Exynos drm driver should allocate contiguous memory even though iommu
> is enabled.
> 
>>
>> Anyway, I am interested in getting the exynos_drm kernel side code
>> and xf86-video-armsoc in sync to resolve the issue.
>>
>> Could you recommend a going forward plan?
> 
> My opinion are,
> 
> 1. Do not force to flag EXYNOS_BO_NONCONTIG at xf86-video-armsoc
> 2. Do not force to flag NONCONTIG at Exynos drm driver even though
> iommu is enabled and flag allocation type with the argument from
> user-space.
> 
> I think you could try to post above patches.
> 

Sounds good. I will work on the above two patches.

thanks,
-- Shuah

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

On 10/19/2016 04:27 PM, Shuah Khan wrote:
> On 10/19/2016 08:16 AM, Inki Dae wrote:
>> Hi Shuah,
>>
>> 2016-10-13 8:11 GMT+09:00 Shuah Khan <shuahkh@osg.samsung.com>:
>>> Hi Inki,
>>>
>>> On 08/15/2016 10:40 PM, Inki Dae wrote:
>>>
>>>>>
>>>>> okay the very first commit that added IOMMU support
>>>>> introduced the code that rejects non-contig gem memory
>>>>> type without IOMMU.
>>>>>
>>>>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
>>>>> Author: Inki Dae <inki.dae@samsung.com>
>>>>> Date:   Sat Oct 20 07:53:42 2012 -0700
>>>>>
>>>>>     drm/exynos: add iommu support for exynos drm framework
>>>>>
>>>
>>> I haven't given up on this yet. I am still seeing the following failure:
>>>
>>> Additional debug messages I added:
>>> [   15.287403] exynos_drm_gem_create_ioctl() 1
>>> [   15.287419] exynos_drm_gem_create() flags 1
>>>
>>> [   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.
>>>
>>> Additional debug message I added:
>>> [   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer
>>>
>>> This is what happens:
>>>
>>> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
>>> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
>>> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during
>>>    check_fb_gem_memory_type()
>>>
>>> At this point, there is no recovery and lightdm fails
>>>
>>> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
>>> allocations are not supported in some exynos drm versions: The following
>>> commit introduced this change:
>>>
>>> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9
>>>
>>> excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
>>> -               create_exynos.flags = EXYNOS_BO_CONTIG;
>>> -       else
>>> -               create_exynos.flags = EXYNOS_BO_NONCONTIG;
>>> +
>>> +       /* Contiguous allocations are not supported in some exynos drm versions.
>>> +        * When they are supported all allocations are effectively contiguous
>>> +        * anyway, so for simplicity we always request non contiguous buffers.
>>> +        */
>>> +       create_exynos.flags = EXYNOS_BO_NONCONTIG;
>>>
>>
>> Above comment, "Contiguous allocations are not supported in some
>> exynos drm versions.", seems wrong assumption.
>> The root cause, contiguous allocation is not supported, would be that
>> they used Linux kernel which didn't have CMA region enough - as
>> default 16MB, or didn't declare CMA region enough for the DMA device.
>> So I think they should not force to flag EXYNOS_BO_NONCONTIG and they
>> should manage the error case if the allocation failed.
> 
> This assumption doesn't sound correct and forcing NONCONTIG isn't right
> either. 
> 
>>
>>> There might have been logic on exynos_drm that forced Contig when it coudn't
>>> support NONCONTIG. At least, that is what this comment suggests. This assumption
>>> doesn't appear to be a good one and not sure if this change was made to fix a bug.
>>>
>>> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
>>> support, latest kernels have a mismatch with the installed xf86-video-armsoc
>>>
>>> This is what I am running into. This leads to the following question:
>>>
>>> 1. How do we ensure exynos_drm kernel changes don't break user-space
>>>    specifically xf86-video-armsoc
>>> 2. This seems to have gone undetected for a while. I see a change in
>>>    exynos_drm_gem_dumb_create() that is probably addressing this type
>>>    of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
>>>    handling for IOMMU NONCONTIG case.
>>
>> Seems this patch has a problem. This patch forces to flag NONCONTIG if
>> iommu is enabled. The flag should be depend on the argument from
>> user-space.
>> I.e., if user-space requested a gem allocation with CONTIG flag, then
>> Exynos drm driver should allocate contiguous memory even though iommu
>> is enabled.
>>
>>>
>>> Anyway, I am interested in getting the exynos_drm kernel side code
>>> and xf86-video-armsoc in sync to resolve the issue.
>>>
>>> Could you recommend a going forward plan?
>>
>> My opinion are,
>>
>> 1. Do not force to flag EXYNOS_BO_NONCONTIG at xf86-video-armsoc

Okay more on this. I fixed xf86-video-armso to ask for EXYNOS_BO_CONTIG
for ARMSOC_BO_SCANOUT and EXYNOS_BO_NONCONTIG in all other cases.

Revert of the commit: 3be1f6273441fe95dd442f44064387322e16b7e9

With this change, now display manager starts just fine. However, it turns
out xf86-video-armsoc is obsoleted in favor of xf86-video-modesetting. The
last update to xf86-video-armsoc git was 3 years ago.

I am not sure where to send the fix and doesn't look like it is being
maintained actively. I can pursue it further and try to get this into
xf86-video-armsoc provided I can find the maintainer for this seemingly
inactive project.

I brought in the latest xf86-video-modesetting bits from

https://cgit.freedesktop.org/xorg/driver/xf86-video-modesetting

I removed xf86-video-armsoc and installed xf86-video-modesetting and that
worked just fine. xf86-video-modesetting uses dumb_create interface instead
of DRM_IOCTL_EXYNOS_GEM_CREATE.

dumb_create interface eliminates the need for DRM_IOCTL_EXYNOS_GEM_CREATE
in userspace. libdrm-2.4.71 does call DRM_IOCTL_EXYNOS_GEM_CREATE.

The question is do we still need to continue to support the custom gem
create interface DRM_IOCTL_EXYNOS_GEM_CREATE? Some drm drivers seem to
support it and some don't. Unfortunately, if userspace requests noncontig
for scanout, we will continue to see problems with display manager when
iommu is disabled. dumb create hides all of that, are there good reasons
to continue to support it in exynos drm?

Exposing CONTIG and NONCONTIG to userspace appears to be causing problems
when exynos drm determines it can't support non-contig GEM buffers in fb
init after userspace allocates them.

>> 2. Do not force to flag NONCONTIG at Exynos drm driver even though
>> iommu is enabled and flag allocation type with the argument from
>> user-space.
>>

exynos_drm_gem_dumb_create() doesn't take any flags, there is no need
to change the above.

thanks,
-- Shuah

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Tobias Jakobi]

Hello Shuah,


Shuah Khan wrote:
> On 10/19/2016 04:27 PM, Shuah Khan wrote:
>> On 10/19/2016 08:16 AM, Inki Dae wrote:
>>> Hi Shuah,
>>>
>>> 2016-10-13 8:11 GMT+09:00 Shuah Khan <shuahkh@osg.samsung.com>:
>>>> Hi Inki,
>>>>
>>>> On 08/15/2016 10:40 PM, Inki Dae wrote:
>>>>
>>>>>>
>>>>>> okay the very first commit that added IOMMU support
>>>>>> introduced the code that rejects non-contig gem memory
>>>>>> type without IOMMU.
>>>>>>
>>>>>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
>>>>>> Author: Inki Dae <inki.dae@samsung.com>
>>>>>> Date:   Sat Oct 20 07:53:42 2012 -0700
>>>>>>
>>>>>>     drm/exynos: add iommu support for exynos drm framework
>>>>>>
>>>>
>>>> I haven't given up on this yet. I am still seeing the following failure:
>>>>
>>>> Additional debug messages I added:
>>>> [   15.287403] exynos_drm_gem_create_ioctl() 1
>>>> [   15.287419] exynos_drm_gem_create() flags 1
>>>>
>>>> [   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.
>>>>
>>>> Additional debug message I added:
>>>> [   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer
>>>>
>>>> This is what happens:
>>>>
>>>> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
>>>> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
>>>> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during
>>>>    check_fb_gem_memory_type()
>>>>
>>>> At this point, there is no recovery and lightdm fails
>>>>
>>>> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
>>>> allocations are not supported in some exynos drm versions: The following
>>>> commit introduced this change:
>>>>
>>>> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9
>>>>
>>>> excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
>>>> -               create_exynos.flags = EXYNOS_BO_CONTIG;
>>>> -       else
>>>> -               create_exynos.flags = EXYNOS_BO_NONCONTIG;
>>>> +
>>>> +       /* Contiguous allocations are not supported in some exynos drm versions.
>>>> +        * When they are supported all allocations are effectively contiguous
>>>> +        * anyway, so for simplicity we always request non contiguous buffers.
>>>> +        */
>>>> +       create_exynos.flags = EXYNOS_BO_NONCONTIG;
>>>>
>>>
>>> Above comment, "Contiguous allocations are not supported in some
>>> exynos drm versions.", seems wrong assumption.
>>> The root cause, contiguous allocation is not supported, would be that
>>> they used Linux kernel which didn't have CMA region enough - as
>>> default 16MB, or didn't declare CMA region enough for the DMA device.
>>> So I think they should not force to flag EXYNOS_BO_NONCONTIG and they
>>> should manage the error case if the allocation failed.
>>
>> This assumption doesn't sound correct and forcing NONCONTIG isn't right
>> either. 
>>
>>>
>>>> There might have been logic on exynos_drm that forced Contig when it coudn't
>>>> support NONCONTIG. At least, that is what this comment suggests. This assumption
>>>> doesn't appear to be a good one and not sure if this change was made to fix a bug.
>>>>
>>>> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
>>>> support, latest kernels have a mismatch with the installed xf86-video-armsoc
>>>>
>>>> This is what I am running into. This leads to the following question:
>>>>
>>>> 1. How do we ensure exynos_drm kernel changes don't break user-space
>>>>    specifically xf86-video-armsoc
>>>> 2. This seems to have gone undetected for a while. I see a change in
>>>>    exynos_drm_gem_dumb_create() that is probably addressing this type
>>>>    of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
>>>>    handling for IOMMU NONCONTIG case.
>>>
>>> Seems this patch has a problem. This patch forces to flag NONCONTIG if
>>> iommu is enabled. The flag should be depend on the argument from
>>> user-space.
>>> I.e., if user-space requested a gem allocation with CONTIG flag, then
>>> Exynos drm driver should allocate contiguous memory even though iommu
>>> is enabled.
>>>
>>>>
>>>> Anyway, I am interested in getting the exynos_drm kernel side code
>>>> and xf86-video-armsoc in sync to resolve the issue.
>>>>
>>>> Could you recommend a going forward plan?
>>>
>>> My opinion are,
>>>
>>> 1. Do not force to flag EXYNOS_BO_NONCONTIG at xf86-video-armsoc
> 
> Okay more on this. I fixed xf86-video-armso to ask for EXYNOS_BO_CONTIG
> for ARMSOC_BO_SCANOUT and EXYNOS_BO_NONCONTIG in all other cases.
> 
> Revert of the commit: 3be1f6273441fe95dd442f44064387322e16b7e9
> 
> With this change, now display manager starts just fine. However, it turns
> out xf86-video-armsoc is obsoleted in favor of xf86-video-modesetting. The
> last update to xf86-video-armsoc git was 3 years ago.
IIRC xf86-video-armsoc was created to facilitate integration with the
proprietary Mali userspace blob. I don't think that can be done with the
modesetting DDX.



> I am not sure where to send the fix and doesn't look like it is being
> maintained actively. I can pursue it further and try to get this into
> xf86-video-armsoc provided I can find the maintainer for this seemingly
> inactive project.
I was wondering if anyone has actually complained about this issue?



> I brought in the latest xf86-video-modesetting bits from
> 
> https://cgit.freedesktop.org/xorg/driver/xf86-video-modesetting
> 
> I removed xf86-video-armsoc and installed xf86-video-modesetting and that
> worked just fine. xf86-video-modesetting uses dumb_create interface instead
> of DRM_IOCTL_EXYNOS_GEM_CREATE.
> 
> dumb_create interface eliminates the need for DRM_IOCTL_EXYNOS_GEM_CREATE
> in userspace. libdrm-2.4.71 does call DRM_IOCTL_EXYNOS_GEM_CREATE.
> 
> The question is do we still need to continue to support the custom gem
> create interface DRM_IOCTL_EXYNOS_GEM_CREATE?
I'd say yes. With just the dumb_create interface it is not possible to
change the type of buffer you get. And if you want to support any kind
of hw acceleration in the DDX, you probably want to control at least the
caching behaviour of the buffer.



> Some drm drivers seem to support it and some don't.
Not sure I understand this. I don't think any other DRM driver except
for exynos supports this ioctl. But all drivers have their own ioctls to
request memory (DRM_IOCTL_ETNAVIV_GEM_NEW, DRM_IOCTL_VC4_CREATE_BO, etc.)



>  Unfortunately, if userspace requests noncontig
> for scanout, we will continue to see problems with display manager when
> iommu is disabled. dumb create hides all of that, are there good reasons
> to continue to support it in exynos drm?
In addition to the stuff I said above, you can use it for render nodes.
That doesn't work for dumb_create.


> Exposing CONTIG and NONCONTIG to userspace appears to be causing problems
> when exynos drm determines it can't support non-contig GEM buffers in fb
> init after userspace allocates them.
Might be missing something here, but this whole thing just looks like a
bug in xf86-video-armsoc. No matter from which perspective you look, the
commit that changed all allocations to noncontig was plain wrong.

My guess: This was done to paper over some bug in some vendor kernel for
a product using an Exynos SoC.


With best wishes,
Tobias



> 
>>> 2. Do not force to flag NONCONTIG at Exynos drm driver even though
>>> iommu is enabled and flag allocation type with the argument from
>>> user-space.
>>>
> 
> exynos_drm_gem_dumb_create() doesn't take any flags, there is no need
> to change the above.
> 
> thanks,
> -- Shuah
> 

[PATCH] exynos-drm: Fix display manager failing to start without IOMMU problem

[Shuah Khan]

On 10/25/2016 11:57 AM, Tobias Jakobi wrote:
> Hello Shuah,
> 
> 
> Shuah Khan wrote:
>> On 10/19/2016 04:27 PM, Shuah Khan wrote:
>>> On 10/19/2016 08:16 AM, Inki Dae wrote:
>>>> Hi Shuah,
>>>>
>>>> 2016-10-13 8:11 GMT+09:00 Shuah Khan <shuahkh@osg.samsung.com>:
>>>>> Hi Inki,
>>>>>
>>>>> On 08/15/2016 10:40 PM, Inki Dae wrote:
>>>>>
>>>>>>>
>>>>>>> okay the very first commit that added IOMMU support
>>>>>>> introduced the code that rejects non-contig gem memory
>>>>>>> type without IOMMU.
>>>>>>>
>>>>>>> commit 0519f9a12d0113caab78980c48a7902d2bd40c2c
>>>>>>> Author: Inki Dae <inki.dae@samsung.com>
>>>>>>> Date:   Sat Oct 20 07:53:42 2012 -0700
>>>>>>>
>>>>>>>     drm/exynos: add iommu support for exynos drm framework
>>>>>>>
>>>>>
>>>>> I haven't given up on this yet. I am still seeing the following failure:
>>>>>
>>>>> Additional debug messages I added:
>>>>> [   15.287403] exynos_drm_gem_create_ioctl() 1
>>>>> [   15.287419] exynos_drm_gem_create() flags 1
>>>>>
>>>>> [   15.311511] [drm:exynos_drm_framebuffer_init] *ERROR* Non-contiguous GEM memory is not supported.
>>>>>
>>>>> Additional debug message I added:
>>>>> [   15.318981] [drm:exynos_user_fb_create] *ERROR* failed to initialize framebuffer
>>>>>
>>>>> This is what happens:
>>>>>
>>>>> 1. exynos_drm_gem_create_ioctl() gets called with EXYNOS_BO_NONCONTIG request
>>>>> 2. exynos_drm_gem_create(0 goes ahead and creates the GEM buffers
>>>>> 3. exynos_user_fb_create() tries to associate GEM to fb and fails during
>>>>>    check_fb_gem_memory_type()
>>>>>
>>>>> At this point, there is no recovery and lightdm fails
>>>>>
>>>>> xf86-video-armsoc/src/drmmode_exynos/drmmode_exynos.c assumes contiguous
>>>>> allocations are not supported in some exynos drm versions: The following
>>>>> commit introduced this change:
>>>>>
>>>>> https://git.linaro.org/arm/xorg/driver/xf86-video-armsoc.git/commitdiff/3be1f6273441fe95dd442f44064387322e16b7e9
>>>>>
>>>>> excerpts from the diff:-       if (create_gem->buf_type == ARMSOC_BO_SCANOUT)
>>>>> -               create_exynos.flags = EXYNOS_BO_CONTIG;
>>>>> -       else
>>>>> -               create_exynos.flags = EXYNOS_BO_NONCONTIG;
>>>>> +
>>>>> +       /* Contiguous allocations are not supported in some exynos drm versions.
>>>>> +        * When they are supported all allocations are effectively contiguous
>>>>> +        * anyway, so for simplicity we always request non contiguous buffers.
>>>>> +        */
>>>>> +       create_exynos.flags = EXYNOS_BO_NONCONTIG;
>>>>>
>>>>
>>>> Above comment, "Contiguous allocations are not supported in some
>>>> exynos drm versions.", seems wrong assumption.
>>>> The root cause, contiguous allocation is not supported, would be that
>>>> they used Linux kernel which didn't have CMA region enough - as
>>>> default 16MB, or didn't declare CMA region enough for the DMA device.
>>>> So I think they should not force to flag EXYNOS_BO_NONCONTIG and they
>>>> should manage the error case if the allocation failed.
>>>
>>> This assumption doesn't sound correct and forcing NONCONTIG isn't right
>>> either. 
>>>
>>>>
>>>>> There might have been logic on exynos_drm that forced Contig when it coudn't
>>>>> support NONCONTIG. At least, that is what this comment suggests. This assumption
>>>>> doesn't appear to be a good one and not sure if this change was made to fix a bug.
>>>>>
>>>>> After the IOMMU support, this assumption is no longer true. Hence, with IOMMU
>>>>> support, latest kernels have a mismatch with the installed xf86-video-armsoc
>>>>>
>>>>> This is what I am running into. This leads to the following question:
>>>>>
>>>>> 1. How do we ensure exynos_drm kernel changes don't break user-space
>>>>>    specifically xf86-video-armsoc
>>>>> 2. This seems to have gone undetected for a while. I see a change in
>>>>>    exynos_drm_gem_dumb_create() that is probably addressing this type
>>>>>    of breakage. Commit 122beea84bb90236b1ae545f08267af58591c21b adds
>>>>>    handling for IOMMU NONCONTIG case.
>>>>
>>>> Seems this patch has a problem. This patch forces to flag NONCONTIG if
>>>> iommu is enabled. The flag should be depend on the argument from
>>>> user-space.
>>>> I.e., if user-space requested a gem allocation with CONTIG flag, then
>>>> Exynos drm driver should allocate contiguous memory even though iommu
>>>> is enabled.
>>>>
>>>>>
>>>>> Anyway, I am interested in getting the exynos_drm kernel side code
>>>>> and xf86-video-armsoc in sync to resolve the issue.
>>>>>
>>>>> Could you recommend a going forward plan?
>>>>
>>>> My opinion are,
>>>>
>>>> 1. Do not force to flag EXYNOS_BO_NONCONTIG at xf86-video-armsoc
>>
>> Okay more on this. I fixed xf86-video-armso to ask for EXYNOS_BO_CONTIG
>> for ARMSOC_BO_SCANOUT and EXYNOS_BO_NONCONTIG in all other cases.
>>
>> Revert of the commit: 3be1f6273441fe95dd442f44064387322e16b7e9
>>
>> With this change, now display manager starts just fine. However, it turns
>> out xf86-video-armsoc is obsoleted in favor of xf86-video-modesetting. The
>> last update to xf86-video-armsoc git was 3 years ago.
> IIRC xf86-video-armsoc was created to facilitate integration with the
> proprietary Mali userspace blob. I don't think that can be done with the
> modesetting DDX.
> 
> 
> 
>> I am not sure where to send the fix and doesn't look like it is being
>> maintained actively. I can pursue it further and try to get this into
>> xf86-video-armsoc provided I can find the maintainer for this seemingly
>> inactive project.
> I was wondering if anyone has actually complained about this issue?

I sent email to the last person that modified the xf86-video-armsoc.
I will pursue fix to this.

> 
> 
> 
>> I brought in the latest xf86-video-modesetting bits from
>>
>> https://cgit.freedesktop.org/xorg/driver/xf86-video-modesetting
>>
>> I removed xf86-video-armsoc and installed xf86-video-modesetting and that
>> worked just fine. xf86-video-modesetting uses dumb_create interface instead
>> of DRM_IOCTL_EXYNOS_GEM_CREATE.
>>
>> dumb_create interface eliminates the need for DRM_IOCTL_EXYNOS_GEM_CREATE
>> in userspace. libdrm-2.4.71 does call DRM_IOCTL_EXYNOS_GEM_CREATE.
>>
>> The question is do we still need to continue to support the custom gem
>> create interface DRM_IOCTL_EXYNOS_GEM_CREATE?
> I'd say yes. With just the dumb_create interface it is not possible to
> change the type of buffer you get. And if you want to support any kind
> of hw acceleration in the DDX, you probably want to control at least the
> caching behaviour of the buffer.

Right. Okay that makes sense.

> 
> 
> 
>> Some drm drivers seem to support it and some don't.
> Not sure I understand this. I don't think any other DRM driver except
> for exynos supports this ioctl. But all drivers have their own ioctls to
> request memory (DRM_IOCTL_ETNAVIV_GEM_NEW, DRM_IOCTL_VC4_CREATE_BO, etc.)
> 
> 
> 
>>  Unfortunately, if userspace requests noncontig
>> for scanout, we will continue to see problems with display manager when
>> iommu is disabled. dumb create hides all of that, are there good reasons
>> to continue to support it in exynos drm?
> In addition to the stuff I said above, you can use it for render nodes.
> That doesn't work for dumb_create.
> 
> 
>> Exposing CONTIG and NONCONTIG to userspace appears to be causing problems
>> when exynos drm determines it can't support non-contig GEM buffers in fb
>> init after userspace allocates them.
> Might be missing something here, but this whole thing just looks like a
> bug in xf86-video-armsoc. No matter from which perspective you look, the
> commit that changed all allocations to noncontig was plain wrong.
> 
> My guess: This was done to paper over some bug in some vendor kernel for
> a product using an Exynos SoC.
> 

Yeah. The right fix in this case is fixing xf86-video-armsoc as you explained
dump_create is too generic for some use-cases.

thanks,
-- Shuah