<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 2016年05月24日 18:11, Tomeu Vizoso
wrote:<br>
</div>
<blockquote
cite="mid:CAAObsKAj9Ja56buTixvp0EVBk=xE4JCXPPwXdviweHkmUQb2xg@mail.gmail.com"
type="cite">
<blockquote type="cite" style="color: #000000;">
<pre wrap="">Hi Tomeu
<span class="moz-txt-citetags">></span>
<span class="moz-txt-citetags">> </span>Sorry for reply late.
<span class="moz-txt-citetags">> </span>I don't agree the changes:
<span class="moz-txt-citetags">></span>
<span class="moz-txt-citetags">> </span>- if (!state->enable)
<span class="moz-txt-citetags">> </span>- return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
<span class="moz-txt-citetags">> </span>+ if (!state->enable &&
<span class="moz-txt-citetags">> </span>+ VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0)
<span class="moz-txt-citetags">> </span>+ return true;
<span class="moz-txt-citetags">></span>
<span class="moz-txt-citetags">> </span>This changes actually would lead a bug.
<span class="moz-txt-citetags">> </span>when we disable a plane, the vop_win_pending_is_complete would always return
<span class="moz-txt-citetags">> </span>true, That is not allowed, would cause fb free too early.
</pre>
</blockquote>
<pre wrap="">Ok, maybe I need to ask you first what the original block of code
intended to achieve. The TRM I have is very terse and I don't find any
explanation there. The battery of tests I have pass just fine without
it.
</pre>
<blockquote type="cite" style="color: #000000;">
<pre wrap=""><span class="moz-txt-citetags">> </span>Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for
<span class="moz-txt-citetags">> </span>pending events when disabling a CRTC"
</pre>
</blockquote>
<pre wrap="">Yes, this function is currently returning false when the pageflip has
been completed but the plan has been already disabled.
If you have any different idea of how to fix this bug, please share.
Thanks,
Tomeu
</pre>
</blockquote>
Hi Tomeu<br>
<br>
<pre wrap=""><i>@@ -504,6 +506,9 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
if (!vop->is_enabled)
return;
+ if (crtc->state->event || vop->event)
+ vop_crtc_wait_for_update(crtc);
+</i><i>
</i>
I think above change has some problem,
the function stack:
->drm swap state
->vop_crtc_disable
->vop_atomic_begin
->vop_atomic_flush
on vop_crtc_disable, crtc->state is new state, the crtc->state->event not yet update to vop, wait for crtc->state->event here is wrong.
So I think the patch should be:
<i>+ if (vop->event)
+ vop_crtc_wait_for_update(crtc);
+</i>
Then the patch "drm/rockchip: vop: Do check if an update is pending during disable" should be no needed.
Thanks.
</pre>
-- Ｍark Yao
</body>
</html>