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

[Gustavo Padovan]

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.

	Gustavo