<br><br><div class="gmail_quote">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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote"><div><div class="h5">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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote"><div><div>2012/12/5 Prathyush K <span dir="ltr"><<a href="mailto:prathyush@chromium.org" target="_blank">prathyush@chromium.org</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div>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></div><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></div></blockquote></div></div>
<div><br>Please, see the below codes,<br>
<br>drm_release()<br> drm_fb_release()<br> drm_framebuffer_remove()<br> /* assume that current fb is fb1 and dma is accessing fb0 but trying to release fb0 at here */<br> /* this situation could be reproduced with pageflip test. */<br>
fb0 is not crtc->fb so the crtc isn't disabled<br> ...<br> drm_framebuffer_unreference(fb0); <- fb0 will be released without wait_for_vblank().<br>
<br>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.<br>Is there another place that wait_for_vblank() is called?<br>
<br></div></div></blockquote></div></div><div><br>Ah, sorry~. exynos_drm_encoder_completescanout() is called prior to gem releasing. Right, no problem. :)<br><br>Thanks,<br>Inki Dae<br><br></div></div></blockquote><div><br>
<br>In addition,<br><br>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.<br>
<br>Thanks,<br>Inki Dae<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div> </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>Thanks,<br>Inki Dae<br><br></div><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>Hope I correctly understood the issue you mentioned.</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>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><div>Sure. I'll post them again as requested.</div><div><br></div><div>Regards,<br></div><div>Prathyush</div></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></div><br></div>
<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></div></div><br>
</blockquote></div></div></div><br>
</blockquote></div><br>