2012/12/5 Inki Dae <inki.dae at samsung.com> > > > 2012/12/5 Inki Dae <inki.dae at samsung.com> > >> >> >> 2012/12/5 Prathyush K <prathyush at chromium.org> >> >>> >>> >>> >>> On Wed, Dec 5, 2012 at 11:40 AM, Inki Dae <inki.dae at samsung.com> wrote: >>> >>>> >>>> >>>> 2012/12/5 Inki Dae <inki.dae at samsung.com> >>>> >>>>> >>>>> >>>>> 2012/12/5 Prathyush K <prathyush.k at samsung.com> >>>>> >>>>>> This patchset fixes a few issues with use of wait for vblank in >>>>>> exynos drm. >>>>>> >>>>>> Please apply these three patches without "drm/exynos: release fb >>>>>> pended by page flip" >>>>>> patch. >>>>>> >>>>>> Patch 1: modify wait for vsync functions to use wait queues >>>>>> This modifies the wait_for_vblank functions to use wait queues >>>>>> thus ensuring that the current task goes to sleep without taking >>>>>> up CPU while waiting. >>>>>> >>>>>> Patch 2: move wait_for_vblank to manager_ops >>>>>> Currently, we do not call wait for vblank if encoder is in DPMS_OFF >>>>>> state inside exynos_drm_encoder_complete_scanout function. This is >>>>>> because wait for vblank is treated as an overlay op. >>>>> >>>>> This can be >>>>>> modified to a manager_op thus removing the above check for DPMS_OFF. >>>>>> This fixes a crash while removing a fb. >>>>>> While removing the current fb (crtc->fb == fb) the encoder >>>>>> dpms off is called and then the framebuffer is removed. So in >>>>>> this case, the wait for vblank is not called thus leading to a crash >>>>>> (since fb might get removed before dma is finished) >>>>>> >>>>>> Patch 3: do not disable crtc if already off >>>>>> We should not disable the overlay if the crtc is in DPMS_OFF state. >>>>>> Also, the disable function should not call DPMS_OFF of the crtc. >>>>>> This is required to ensure we are able to wait for vblank >>>>>> before freeing any framebuffers after disabling the crtc. >>>>>> >>>>>> With these three patches and without "drm/exynos: release fb pended >>>>>> by page flip" >>>>>> I could not find any crash/page_fault in drm with fimd/hdmi during >>>>>> hotplug >>>>>> and page flips. >>>>>> >>>>>> >>>>> It seems better than old one and also including some exception codes, >>>>> Patch 3 we did't consider. Ok, we will test these patches and merge them >>>>> instead of old one if no problem. >>>>> >>>>> >>>> >>>> Tested and worked fine. But with this patch and without "drm/exynos: >>>> release fb pended by page flip", we would still have potential issue to >>>> page fault like below, >>>> 1. dma is accessing no current fb >>>> 2. the fb to be released is not current fb so the fb would be released >>>> without disabling crtc when drm is released. >>>> 3. but the memory region to no current fb is being accessed by the dma >>>> so the page fault would be induced. This is a rare case but possible issue. >>>> >>>> Hi Mr. Dae, >>> >>> But we would not release the fb till we get a vblank (i.e. wait for >>> vblank is called before removing every framebuffer). >>> >>> Taking your example itself: >>> >>> fb0 is current fb i.e. (crtc->fb = fb0). >>> >>> But the dma is accessing from fb1. >>> >>> We call remove fb1. >>> >>> In this case, as you said, crtc will not be disabled as fb1 is not >>> current fb. >>> >>> But we wait for vblank before removing fb1. Thus we ensure that dma from >>> fb1 is complete and dma from fb0 has started. >>> So it is safe to remove fb1. >>> >> >> Please, see the below codes, >> >> drm_release() >> drm_fb_release() >> drm_framebuffer_remove() >> /* assume that current fb is fb1 and dma is >> accessing fb0 but trying to release fb0 at here */ >> /* this situation could be reproduced with >> pageflip test. */ >> fb0 is not crtc->fb so the crtc isn't disabled >> ... >> drm_framebuffer_unreference(fb0); <- fb0 will be >> released without wait_for_vblank(). >> >> The wait_for_vblank() is called by exynos_drm_encoder_complete_scanout() >> when fb->func->destroy() is called so the wait_for_vblank() will be called >> after fb0 is released. >> Is there another place that wait_for_vblank() is called? >> >> > Ah, sorry~. exynos_drm_encoder_completescanout() is called prior to gem > releasing. Right, no problem. :) > > Thanks, > Inki Dae > >
In addition, I had added exynos_drm_encoder_complete_scanout() function to resolve this issue. but it seems like that previous patch didn't perform wait_for_vblank correctly. Anyway this is just exynos specific code. so I hope maybe Daniel's fixup can resolve this issue in drm core. Thanks, Inki Dae > >> Thanks, >> Inki Dae >> >> >>> Hope I correctly understood the issue you mentioned. >>> >>> >>> >>>> Anyway I removed the patch, "drm/exynos: release fb pended by page >>>> flip". And I hope this issue can be resolved by Daniel's fixup later. >>>> >>>> And I think it's better to separate your patch set into 4 patches like >>>> below, >>>> 1. the patch including only exynos drm framework part. >>>> 2. the patch including only fimd driver part. >>>> 3. the patch including only hdmi driver part. >>>> 4. the patch including exception code.(previous Patch 3) >>>> >>>> Please, re-send them. >>>> >>>> Sure. I'll post them again as requested. >>> >>> Regards, >>> Prathyush >>> >>> >>> >>>> Thanks, >>>> Inki Dae >>>> >>>> Thanks, >>>>> Inki Dae >>>>> >>>>> >>>>>> Prathyush K (3) >>>>>> drm/exynos: modify wait for vsync functions to use wait queues >>>>>> drm/exynos: move wait_for_vblank to manager_ops >>>>>> drm/exynos: do not disable crtc if already off >>>>>> >>>>>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 +++- >>>>>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 20 ++------------ >>>>>> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 15 +++-------- >>>>>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 37 >>>>>> ++++++++++++++++++-------- >>>>>> drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 22 ++++++++-------- >>>>>> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 2 +- >>>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 37 >>>>>> +++++++++++++++++--------- >>>>>> 7 files changed, 73 insertions(+), 66 deletions(-) >>>>>> >>>>>> _______________________________________________ >>>>>> dri-devel mailing list >>>>>> dri-devel at lists.freedesktop.org >>>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>>> >>>>> >>>>> >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel at lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>>> >>>> >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel at lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >>> >> > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121205/64bd8d16/attachment.html>
