From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752553AbcFBGZx (ORCPT ); Thu, 2 Jun 2016 02:25:53 -0400 Received: from regular1.263xmail.com ([211.150.99.132]:58338 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750754AbcFBGZv (ORCPT ); Thu, 2 Jun 2016 02:25:51 -0400 X-263anti-spam: KSV:0;BIG:0;ABS:1;DNS:0;ATT:0;SPF:S; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ADDR-CHECKED: 0 X-RL-SENDER: mark.yao@rock-chips.com X-FST-TO: linux-arm-kernel@lists.infradead.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: <17a6f7eee55fc5cbda37a7345060c8f2> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH 1/2] drm/rockchip: vop: Do check if an update is pending during disable To: Tomeu Vizoso References: <1459937686-9142-1-git-send-email-tomeu.vizoso@collabora.com> <5707043D.6060706@rock-chips.com> <570AFAC2.8080907@rock-chips.com> <5742A41B.8030308@rock-chips.com> <5744FA7C.3080802@rock-chips.com> <574500FE.4050708@rock-chips.com> Cc: "open list:ARM/Rockchip SoC..." , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" From: Mark yao Message-ID: <574FD161.4050503@rock-chips.com> Date: Thu, 2 Jun 2016 14:25:37 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016年06月02日 13:57, Tomeu Vizoso wrote: > On 25 May 2016 at 03:33, Mark yao 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> >> >> -- >> Ｍark Yao >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > > -- Ｍark Yao