On 2 June 2016 at 08:25, Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote: > On 2016年06月02日 13:57, Tomeu Vizoso wrote: >> >> 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 > > > 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. Yup, should be already there. Regards, Tomeu > Thanks > > >> 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 >>> >> >> > > > -- > Ｍ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
