On 2016?06?02? 13:57, Tomeu Vizoso wrote: > On 25 May 2016 at 03:33, Mark yao <mark.yao at rock-chips.com> 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 Good news, I also see the Daniel's series patches, wonderful update. You can add a Tested-by for Daniel's rockchip patches, and I add a Acked-by for those rockchip patches. Thanks > Thanks, > > Tomeu > >> Thanks. >> >> -- ?ark Yao >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> >> >> -- >> ?ark Yao >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > > -- ?ark Yao
