[PATCH v10 00/17] drm/exynos: atomic modesetting support

[Joonyoung Shim]

On 06/11/2015 11:01 PM, Gustavo Padovan wrote:
> Hi Joonyoung,
> 
> 2015-06-11 Joonyoung Shim <jy0922.shim at samsung.com>:
> 
>> On 06/10/2015 10:36 PM, Gustavo Padovan wrote:
>>> Hi Marek,
>>>
>>> 2015-06-10 Marek Szyprowski <m.szyprowski at samsung.com>:
>>>
>>>> Hello,
>>>>
>>>> On 2015-06-01 17:04, Gustavo Padovan wrote:
>>>>> From: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Here goes the full support for atomic modesetting on exynos. I've
>>>>> split the patches in the various phases of atomic support.
>>>>
>>>> Thanks for this patchses, however I've noticed a problem after applying
>>>> them.
>>>> The issue gets revealed when support for IOMMU is enabled. I've did my tests
>>>> with Exynos HDMI driver on Odroid U3 board.
>>>>
>>>> To demonstrated the issue I've added following additional debug in the
>>>> exynos
>>>> mixer driver in mixer_graph_buffer() function:
>>>> pr_info("dma addr %pad plane->src_width %d plane->src_height %d\n",
>>>> &plane->dma_addr[0], plane->src_width, plane->src_height);
>>>>
>>>> Before applying patches setting 640x480 mode and getting back to 1920x1080
>>>> console generates following log:
>>>>
>>>> # modetest -M exynos -s 23:640x480
>>>> setting mode 640x480-60Hz at XR24 on connectors 23, crtc 21
>>>> [ 3860.617151] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>>>> ^C
>>>> [ 3870.555232] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>> [ 3870.565696] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>>
>>>> After applying atomic modesetting patchset:
>>>> # modetest -M exynos -s 24:640x480
>>>> [  142.540122] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>> [  142.550726] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>> setting mode 640x480-60Hz at XR24 on connectors 24, crtc 22
>>>> [  142.643672] dma 0xbc500000 plane->src_width 1920 plane->src_height 1080
>>>> [  142.759982] dma 0xbc500000 plane->src_width 640 plane->src_height 480
>>>> ^C
>>>> [  154.986040] dma 0xbbd00000 plane->src_width 1920 plane->src_height 1080
>>>>
>>>> As you can see from the above log, mixer_graph_buffer function is called
>>>> several times. 0xbbd00000 is the DMA address of the 1920x1080 framebuffer
>>>> and 0xbc500000 is the DMA address of the allocated 640x480 buffer.
>>>> mixer_graph_buffer() is first called with the new DMA address of the
>>>> framebuffer, but with the old mode parameters (1920x1080 size) and then
>>>> in the next call it updates the plane parameters to the correct values
>>>> (size changed to 640x480). When IOMMU is not used, this can be easily
>>>> missed, but after enabling IOMMU support, any DMA access to unallocated
>>>> address causes IOMMU PAGE FAULT. Here it will happen after changing DMA
>>>> address of the buffer without changing the size.
>>>>
>>>> A quick workaround to resolve this multiple calls to mixer_graph_buffer()
>>>> with partially updated mode values is to remove calls to
>>>> mixer_window_suspend/mixer_window_resume from mixer_disable and
>>>> mixer_disable functions, but I expect that this is not the right
>>>> approach.
>>>>
>>>> Probably the same problem can be observed with Exynos FIMD driver.
>>>>
>>>> Gustavo: could you check if mixer_enable functions should really
>>>> call mixer_window_resume function, which in turn calls mixer_win_commit,
>>>> which calls mixer_graph_buffer with partially updated display buffer
>>>> data?
>>>
>>> It should not, you are right. This is actually the correct fix. Atomic
>>> modesetting should not do chained calls, e.g., crtc_disable calling
>>> plane_disable. This change was already in my plan actually, but as I had
>>> IOMMU disabled I didn't see it here, and I didn't create this patch yet.
>>>
>>
>> Right, but it needs that crtc_disable calls plane_disable in exynos
>> driver internally. Because crtc is disabled before plane is disabled,
>> it means plane_disable just returns without any register changes,
>> then we cannot be sure setting register to disable plane when crtc is
>> disable.
>>
>> I think a solution is enough only to eliminate calling xxx_resume
>> function in exynos driver function when enable plane, we can remove it
>> because it's called to enable plane from drm atomic modeset framework.
> 
> I think this should work. We really need to disable the planes before
> the crtc is disabled, for FIMD for example, the overlay planes are
> removed from the screen if we remove the _suspend call.
> 
> I'll sent a new patch to remove only the resume part from all our crtc
> drivers.
> 

I sent patchset related, is it same with which you try?

Thanks.