<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Dec 5, 2012 at 11:40 AM, Inki Dae <span dir="ltr"><<a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>></span> wrote:<br> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br><br><div class="gmail_quote"><div><div>2012/12/5 Inki Dae <span dir="ltr"><<a href="mailto:inki.dae@samsung.com" target="_blank">inki.dae@samsung.com</a>></span><br> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><br><div class="gmail_quote"><div><div>2012/12/5 Prathyush K <span dir="ltr"><<a href="mailto:prathyush.k@samsung.com" target="_blank">prathyush.k@samsung.com</a>></span><br><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote"> This patchset fixes a few issues with use of wait for vblank in<br> exynos drm.<br> <br> Please apply these three patches without "drm/exynos: release fb pended by page flip"<br> patch.<br> <br> Patch 1: modify wait for vsync functions to use wait queues<br> This modifies the wait_for_vblank functions to use wait queues<br> thus ensuring that the current task goes to sleep without taking<br> up CPU while waiting.<br> <br> Patch 2: move wait_for_vblank to manager_ops<br> Currently, we do not call wait for vblank if encoder is in DPMS_OFF<br> state inside exynos_drm_encoder_complete_scanout function. This is<br> because wait for vblank is treated as an overlay op. </blockquote><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote"> This can be<br> modified to a manager_op thus removing the above check for DPMS_OFF.<br> This fixes a crash while removing a fb.<br> While removing the current fb (crtc->fb == fb) the encoder<br> dpms off is called and then the framebuffer is removed. So in<br> this case, the wait for vblank is not called thus leading to a crash<br> (since fb might get removed before dma is finished)<br> <br> Patch 3: do not disable crtc if already off<br> We should not disable the overlay if the crtc is in DPMS_OFF state.<br> Also, the disable function should not call DPMS_OFF of the crtc.<br> This is required to ensure we are able to wait for vblank<br> before freeing any framebuffers after disabling the crtc.<br> <br> With these three patches and without "drm/exynos: release fb pended by page flip"<br> I could not find any crash/page_fault in drm with fimd/hdmi during hotplug<br> and page flips.<br> <br></blockquote><div> </div></div></div><div>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.</div> <div> </div></div></blockquote></div></div><div><br>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,<br> 1. dma is accessing no current fb<br> 2. the fb to be released is not current fb so the fb would be released without disabling crtc when drm is released.<br>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.<br> <br></div></div></blockquote><div>Hi Mr. Dae,</div><div><br></div><div>But we would not release the fb till we get a vblank (i.e. wait for vblank is called before removing every framebuffer).</div><div><br></div><div>Taking your example itself:</div> <div><br></div><div>fb0 is current fb i.e. (crtc->fb = fb0).</div><div><br></div><div>But the dma is accessing from fb1.</div><div><br></div><div>We call remove fb1. </div><div><br></div><div>In this case, as you said, crtc will not be disabled as fb1 is not current fb.</div> <div><br></div><div>But we wait for vblank before removing fb1. Thus we ensure that dma from fb1 is complete and dma from fb0 has started.</div><div>So it is safe to remove fb1.</div><div><br></div><div>Hope I correctly understood the issue you mentioned.</div> <div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="gmail_quote"><div>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.<br><br>And I think it's better to separate your patch set into 4 patches like below,<br> 1. the patch including only exynos drm framework part.<br>2. the patch including only fimd driver part.<br>3. the patch including only hdmi driver part.<br>4. the patch including exception code.(previous Patch 3)<br><br> Please, re-send them.<br> <br></div></div></blockquote><div><div>Sure. I'll post them again as requested.</div><div><br></div><div>Regards,<br></div><div>Prathyush</div></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="gmail_quote"><div>Thanks,<br>Inki Dae <br><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="gmail_quote"> <div>Thanks,</div><div>Inki Dae</div><div><div> </div><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote"> Prathyush K (3)<br> drm/exynos: modify wait for vsync functions to use wait queues<br> drm/exynos: move wait_for_vblank to manager_ops<br> drm/exynos: do not disable crtc if already off<br> <br> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 +++-<br> drivers/gpu/drm/exynos/exynos_drm_drv.h | 20 ++------------<br> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 15 +++--------<br> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 37 ++++++++++++++++++--------<br> drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 22 ++++++++--------<br> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 2 +-<br> drivers/gpu/drm/exynos/exynos_mixer.c | 37 +++++++++++++++++---------<br> 7 files changed, 73 insertions(+), 66 deletions(-)<br> <br> _______________________________________________<br> dri-devel mailing list<br> <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a><br> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br> </blockquote></div></div><br> </blockquote></div></div><br> <br>_______________________________________________<br> dri-devel mailing list<br> <a href="mailto:dri-devel@lists.freedesktop.org" target="_blank">dri-devel@lists.freedesktop.org</a><br> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br> <br></blockquote></div><br></div>