On 25 May 2016 at 03:33, Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote: > On 2016年05月25日 09:06, Mark yao wrote: > > On 2016年05月24日 18:11, Tomeu Vizoso wrote: > > Hi Tomeu >> >> Sorry for reply late. >> I don't agree the changes: >> >> - if (!state->enable) >> - return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0; >> + if (!state->enable && >> + VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0) >> + return true; >> >> This changes actually would lead a bug. >> when we disable a plane, the vop_win_pending_is_complete would always >> return >> true, That is not allowed, would cause fb free too early. > > 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. > >> Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for >> pending events when disabling a CRTC" > > 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 > > > > Hi Tomeu > > @@ -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 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: > + if (vop->event) > + vop_crtc_wait_for_update(crtc); > + > > > call vop_crtc_wait_for_update(crtc) here also is unsafe, it will reinit the > vop->wait_update_complete. > > I doubt that, since use the serialize outstanding nonblocking commits, only > one process can run into the update stack, old vop->event should be free on > last time, if we get vop->event here, that should be a bug. > > > Then the patch "drm/rockchip: vop: Do check if an update is pending during > disable" should be no needed. Hi Mark, with Daniel's series linked below this and the other issues I found in the Rockchip driver are fixed: http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/91023/focus=91053 Thanks, Tomeu > Thanks. > > -- Ｍark Yao > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Ｍark Yao > > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
