From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark yao Subject: Re: [PATCH 1/2] drm/rockchip: vop: Do check if an update is pending during disable Date: Wed, 25 May 2016 09:06:04 +0800 Message-ID: <5744FA7C.3080802@rock-chips.com> 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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1487749255==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Tomeu Vizoso Cc: "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" List-Id: linux-rockchip.vger.kernel.org --===============1487749255== Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
On 2016=E5=B9=B405=E6=9C=8824=E6=97=A5= 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) =3D=3D 0;
> + if (!state->enable &=
;&
> +    VOP_WIN_GET(vop_win->=
;vop, vop_win->data, enable) =3D=3D 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, w=
ould 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 disablin=
g 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(st=
ruct drm_crtc *crtc)
 	if (!vop->is_enabled)
 		return;
=20
+	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 w=
rong.

So I think the patch should be:
+	if (vop->event)
+		vop_crtc_wait_for_update(crtc);
+

Then the patch "drm/rockchip: vop: Do check if an update is pending durin=
g disable" should be no needed.

Thanks.
-- =EF=BC=ADark Yao --===============1487749255== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1487749255==--