[RFC PATCH] drm/exynos: fix pending update handling
Tobias Jakobi
tjakobi at math.uni-bielefeld.de
Tue May 24 22:37:43 UTC 2016
Hello Andrzej,
sorry for the delay.
Andrzej Hajda wrote:
> On 05/07/2016 12:33 PM, Tobias Jakobi wrote:
>> Hello Andrzej,
>>
>> I've applied this to my 4.5.3 based tree, but I'm still seeing the page
>> fault.
>
> Thats pity, I hoped to solve two issues with one patch :)
>
>> In fact the calls leading to the fault haven't changed at all.
>
> That is not surprising.
>
>>
>>> [ 153.437877] [drm:fimd_update_plane] start addr = 0x20600000, end addr = 0x20a08000, size = 0x408000
>>> [ 153.446903] [drm:fimd_update_plane] ovl_width = 1366, ovl_height = 768
>>> [ 153.453406] [drm:fimd_update_plane] osd pos: tx = 0, ty = 0, bx = 1365, by = 767
>>> [ 153.460785] [drm:fimd_update_plane] osd size = 0x100200
>>> [ 153.466000] [drm:drm_vblank_enable] enabling vblank on crtc 0, ret: 0
>>> [ 153.472397] [drm:drm_update_vblank_count] updating vblank count on crtc 0: current=7, diff=0, hw=0 hw_last=0
>>> [ 153.527759] [drm:drm_atomic_state_default_clear] Clearing atomic state edef0c00
>>> [ 153.529428] [drm:drm_property_unreference_blob] edcd2000: blob ID: 44 (1)
>>> [ 153.536238] [drm:drm_framebuffer_unreference] edeaac00: FB ID: 42 (2)
>>> [ 153.542644] [drm:drm_atomic_state_free] Freeing atomic state edef0c00
>>> [ 153.549083] [drm:drm_framebuffer_reference] ecfee0c0: FB ID: 43 (3)
>>> [ 153.555315] [drm:drm_framebuffer_unreference] edeaac00: FB ID: 42 (1)
>>> [ 153.561776] [drm:exynos_drm_gem_destroy] handle count = 0
>>> [ 153.567120] [drm:exynos_drm_free_buf] dma_addr(0x20100000), size(0x408000)
>>> [ 153.574341] exynos-sysmmu 11e20000.sysmmu: PAGE FAULT occurred at 0x203f3d80 (page table base: 0x6e2dc000)
>>> [ 153.583586] exynos-sysmmu 11e20000.sysmmu: Lv1 entry: 0x6ea7c001
>>
>> I still need to test this with exynos-drm-next and [1].
>
> OK, in case of page fault let me know which modification you have made
> to dts / exynos_defconfig,
> I will try to reproduce/investigate it on similar hw.
I have tested this now on exynos-drm-next with [1] and a small patch
adding a FIMD node to my DT.
I haven't been able to trigger the pagefault yet, but I'm seeing a lot
of warning for timeouts:
> [ 1098.186100] WARNING: CPU: 3 PID: 3051 at drivers/gpu/drm/drm_atomic_helper.c:1108 drm_atomic_helper_wait_for_vblanks+0x238/0x23c
> [ 1098.186110] [CRTC:29] vblank wait timed out
> [ 1098.186119] Modules linked in: cmac ecb bridge stp llc bnep btrfs xor xor_neon zlib_inflate zlib_deflate raid6_pq btusb btbcm btintel bluetooth usb_storage s5p_jpeg videobuf2_dma_contig videobuf2_memops v4l2_mem2mem videobuf2_v4l2 videobuf2_core
> [ 1098.186221] CPU: 3 PID: 3051 Comm: X Tainted: G W 4.6.0-rc7-debug+ #3
> [ 1098.186229] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [ 1098.186238] Backtrace:
> [ 1098.186260] [<c010c4b0>] (dump_backtrace) from [<c010c6ac>] (show_stack+0x18/0x1c)
> [ 1098.186269] r6:c0a24560 r5:60010053 r4:00000000 r3:dc8ba400
> [ 1098.186301] [<c010c694>] (show_stack) from [<c035c5d0>] (dump_stack+0xb0/0xdc)
> [ 1098.186315] [<c035c520>] (dump_stack) from [<c0120f30>] (__warn+0xec/0x104)
> [ 1098.186323] r8:00000454 r7:00000009 r6:c086ac74 r5:00000000 r4:eb8ffc40 r3:dc8ba400
> [ 1098.186358] [<c0120e44>] (__warn) from [<c0120f88>] (warn_slowpath_fmt+0x40/0x48)
> [ 1098.186365] r9:00000000 r8:ee2b9000 r7:ee2aa800 r6:00000000 r5:eded3600 r4:c086b0d0
> [ 1098.186401] [<c0120f4c>] (warn_slowpath_fmt) from [<c03e49f8>] (drm_atomic_helper_wait_for_vblanks+0x238/0x23c)
> [ 1098.186409] r3:0000001d r2:c086b0d0
> [ 1098.186424] r4:edcd6c40
> [ 1098.186442] [<c03e47c0>] (drm_atomic_helper_wait_for_vblanks) from [<c0408108>] (exynos_atomic_commit_complete+0x54/0xa8)
> [ 1098.186450] r10:00000000 r9:00000000 r8:ee91afac r7:ee91af80 r6:edcd64c0 r5:ee2b9000
> [ 1098.186481] r4:edcd6c40
> [ 1098.186496] [<c04080b4>] (exynos_atomic_commit_complete) from [<c0408b48>] (exynos_atomic_commit+0x1b8/0x204)
> [ 1098.186504] r8:ee91af80 r7:ee2b9000 r6:edcd6c40 r5:ee91afac r4:edcd64c0 r3:00000000
> [ 1098.186537] [<c0408990>] (exynos_atomic_commit) from [<c0406620>] (drm_atomic_commit+0x4c/0x6c)
> [ 1098.186545] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:edcd6c40 r5:ee2b9000
> [ 1098.186574] r4:edcd6c40
> [ 1098.186591] [<c04065d4>] (drm_atomic_commit) from [<c03e5b70>] (drm_atomic_helper_disable_plane+0xbc/0xe0)
> [ 1098.186599] r5:ee8495c0 r4:00000000
> [ 1098.186620] [<c03e5ab4>] (drm_atomic_helper_disable_plane) from [<c03f6418>] (__setplane_internal+0x19c/0x26c)
> [ 1098.186628] r6:00000000 r5:ee8495c0 r4:00000000 r3:c03e5ab4
> [ 1098.186656] [<c03f627c>] (__setplane_internal) from [<c03f94e0>] (drm_mode_setplane+0x14c/0x1d8)
> [ 1098.186664] r8:00000000 r7:00000000 r6:ee8495c0 r5:ee849624 r4:00000000
> [ 1098.186697] [<c03f9394>] (drm_mode_setplane) from [<c03eb970>] (drm_ioctl+0x158/0x524)
> [ 1098.186705] r10:000000b7 r9:ee2b9000 r8:c1200c00 r7:c073e084 r6:c0a02448 r5:00000051
> [ 1098.186735] r4:eb8ffe4c
> [ 1098.186753] [<c03eb818>] (drm_ioctl) from [<c024864c>] (do_vfs_ioctl+0xa8/0x9b4)
> [ 1098.186761] r10:00000000 r9:eb8fe000 r8:00000006 r7:c0248f94 r6:ede64280 r5:ee028a20
> [ 1098.186790] r4:be8e0c50
> [ 1098.186806] [<c02485a4>] (do_vfs_ioctl) from [<c0248f94>] (SyS_ioctl+0x3c/0x64)
> [ 1098.186814] r10:00000000 r9:eb8fe000 r8:00000006 r7:c03064b7 r6:ede64280 r5:be8e0c50
> [ 1098.186843] r4:ede64281
> [ 1098.186860] [<c0248f58>] (SyS_ioctl) from [<c0107ea0>] (ret_fast_syscall+0x0/0x1c)
> [ 1098.186868] r8:c0108044 r7:00000036 r6:c03064b7 r5:be8e0c50 r4:00000000 r3:00000000
> [ 1098.186898] ---[ end trace a4b1ce119f2a94bc ]---
As I already suspected the FIMD block doesn't get any vblank interrupts
when there is no display attached to the connector, hence the wait times
out. I'm unsure if the current code does the correct thing, i.e.
retaining a ref to the framebuffer that is currently scanned out.
With best wishes,
Tobias
> Regards
> Andrzej
>
>>
>> With best wishes,
>> Tobias
>>
>>
>>
>> Andrzej Hajda wrote:
>>> Exynos DRM devices update their registers at vblank time. Exynos-DRM uses
>>> custom mechanism to wait for vblank. This mechanism is error prone -
>>> variables are not updated atomically. As a result in certain circumstances
>>> user space can try to free buffers which are still in use by hardware,
>>> in such cases IOMMU can throw OOPS.
>>> The patch instead of fixing the mechanism replaces it with drm core helper.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda at samsung.com>
>>> ---
>>> Hi Tobias,
>>>
>>> This is a quick/dirty patch just for checking if it solves your issue.
>>> Successfully tested on decon5433/dsi/i80 path.
>>>
>>> Please verify if it helps in your case.
>>>
>>> The patch is based on exynos-drm-next and
>>> "drm/exynos: fix cancel page flip code" [1].
>>>
>>> [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/53801
>>>
>>> Regards
>>> Andrzej
>>> ---
>>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 +-----------
>>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 44 +-------------------------------
>>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 2 --
>>> 3 files changed, 2 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index 785ffa6..5b6845b 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -134,8 +134,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
>>> exynos_crtc->ops = ops;
>>> exynos_crtc->ctx = ctx;
>>>
>>> - init_waitqueue_head(&exynos_crtc->wait_update);
>>> -
>>> crtc = &exynos_crtc->base;
>>>
>>> private->crtc[pipe] = crtc;
>>> @@ -175,13 +173,6 @@ void exynos_drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>>> exynos_crtc->ops->disable_vblank(exynos_crtc);
>>> }
>>>
>>> -void exynos_drm_crtc_wait_pending_update(struct exynos_drm_crtc *exynos_crtc)
>>> -{
>>> - wait_event_timeout(exynos_crtc->wait_update,
>>> - (atomic_read(&exynos_crtc->pending_update) == 0),
>>> - msecs_to_jiffies(50));
>>> -}
>>> -
>>> void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc,
>>> struct exynos_drm_plane *exynos_plane)
>>> {
>>> @@ -190,9 +181,6 @@ void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc,
>>>
>>> exynos_plane->pending_fb = NULL;
>>>
>>> - if (atomic_dec_and_test(&exynos_crtc->pending_update))
>>> - wake_up(&exynos_crtc->wait_update);
>>> -
>>> spin_lock_irqsave(&crtc->dev->event_lock, flags);
>>> if (exynos_crtc->event)
>>> drm_crtc_send_vblank_event(crtc, exynos_crtc->event);
>>> @@ -235,10 +223,8 @@ void exynos_drm_crtc_cancel_page_flip(struct drm_crtc *crtc,
>>> spin_lock_irqsave(&crtc->dev->event_lock, flags);
>>>
>>> e = exynos_crtc->event;
>>> - if (e && e->base.file_priv == file) {
>>> + if (e && e->base.file_priv == file)
>>> exynos_crtc->event = NULL;
>>> - atomic_dec(&exynos_crtc->pending_update);
>>> - }
>>>
>>> spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> index 0281b30..cc96e85 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>>> @@ -45,37 +45,11 @@ struct exynos_atomic_commit {
>>> u32 crtcs;
>>> };
>>>
>>> -static void exynos_atomic_wait_for_commit(struct drm_atomic_state *state)
>>> -{
>>> - struct drm_crtc_state *crtc_state;
>>> - struct drm_crtc *crtc;
>>> - int i, ret;
>>> -
>>> - for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> - struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>> -
>>> - if (!crtc->state->enable)
>>> - continue;
>>> -
>>> - ret = drm_crtc_vblank_get(crtc);
>>> - if (ret)
>>> - continue;
>>> -
>>> - exynos_drm_crtc_wait_pending_update(exynos_crtc);
>>> - drm_crtc_vblank_put(crtc);
>>> - }
>>> -}
>>> -
>>> static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit)
>>> {
>>> struct drm_device *dev = commit->dev;
>>> struct exynos_drm_private *priv = dev->dev_private;
>>> struct drm_atomic_state *state = commit->state;
>>> - struct drm_plane *plane;
>>> - struct drm_crtc *crtc;
>>> - struct drm_plane_state *plane_state;
>>> - struct drm_crtc_state *crtc_state;
>>> - int i;
>>>
>>> drm_atomic_helper_commit_modeset_disables(dev, state);
>>>
>>> @@ -89,25 +63,9 @@ static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit)
>>> * have the relevant clocks enabled to perform the update.
>>> */
>>>
>>> - for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> - struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>> -
>>> - atomic_set(&exynos_crtc->pending_update, 0);
>>> - }
>>> -
>>> - for_each_plane_in_state(state, plane, plane_state, i) {
>>> - struct exynos_drm_crtc *exynos_crtc =
>>> - to_exynos_crtc(plane->crtc);
>>> -
>>> - if (!plane->crtc)
>>> - continue;
>>> -
>>> - atomic_inc(&exynos_crtc->pending_update);
>>> - }
>>> -
>>> drm_atomic_helper_commit_planes(dev, state, false);
>>>
>>> - exynos_atomic_wait_for_commit(state);
>>> + drm_atomic_helper_wait_for_vblanks(dev, state);
>>>
>>> drm_atomic_helper_cleanup_planes(dev, state);
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> index 561925f..0bb3766 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> @@ -174,8 +174,6 @@ struct exynos_drm_crtc {
>>> enum exynos_drm_output_type type;
>>> unsigned int pipe;
>>> struct drm_pending_vblank_event *event;
>>> - wait_queue_head_t wait_update;
>>> - atomic_t pending_update;
>>> const struct exynos_drm_crtc_ops *ops;
>>> void *ctx;
>>> struct exynos_drm_clk *pipe_clk;
>>>
>>
>>
>
More information about the dri-devel
mailing list