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 > 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/8bd0d812/attachment-0001.html>
