Re: [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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
>     Reviewed-by: Sachin Kamat <sachin.kamat@xxxxxxxxxx>
>     Signed-off-by: Inki Dae <inki.dae@xxxxxxxxxxx>
> 
> 
> commit ea6d66c3a797376d21b23dc8261733ce35970014
> Author: Inki Dae <inki.dae@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
>     Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> 
> commit 2b35892e9da672df40ce890bffc4f9f6119c57e0
> Author: Inki Dae <inki.dae@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
>     Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>     Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
> 
> -- Shuah
>