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: Thu, 2 Jun 2016 14:25:37 +0800 Message-ID: <574FD161.4050503@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> <5744FA7C.3080802@rock-chips.com> <574500FE.4050708@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 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 T24gMjAxNuW5tDA25pyIMDLml6UgMTM6NTcsIFRvbWV1IFZpem9zbyB3cm90ZToKPiBPbiAyNSBN YXkgMjAxNiBhdCAwMzozMywgTWFyayB5YW8gPG1hcmsueWFvQHJvY2stY2hpcHMuY29tPiB3cm90 ZToKPj4gT24gMjAxNuW5tDA15pyIMjXml6UgMDk6MDYsIE1hcmsgeWFvIHdyb3RlOgo+Pgo+PiBP biAyMDE25bm0MDXmnIgyNOaXpSAxODoxMSwgVG9tZXUgVml6b3NvIHdyb3RlOgo+Pgo+PiBIaSBU b21ldQo+Pj4gU29ycnkgZm9yIHJlcGx5IGxhdGUuCj4+PiBJIGRvbid0IGFncmVlIHRoZSBjaGFu Z2VzOgo+Pj4KPj4+IC0gaWYgKCFzdGF0ZS0+ZW5hYmxlKQo+Pj4gLSByZXR1cm4gVk9QX1dJTl9H RVQodm9wX3dpbi0+dm9wLCB2b3Bfd2luLT5kYXRhLCBlbmFibGUpID09IDA7Cj4+PiArIGlmICgh c3RhdGUtPmVuYWJsZSAmJgo+Pj4gKyAgICBWT1BfV0lOX0dFVCh2b3Bfd2luLT52b3AsIHZvcF93 aW4tPmRhdGEsIGVuYWJsZSkgPT0gMCkKPj4+ICsgcmV0dXJuIHRydWU7Cj4+Pgo+Pj4gVGhpcyBj aGFuZ2VzIGFjdHVhbGx5IHdvdWxkIGxlYWQgYSBidWcuCj4+PiB3aGVuIHdlIGRpc2FibGUgYSBw bGFuZSwgdGhlIHZvcF93aW5fcGVuZGluZ19pc19jb21wbGV0ZSB3b3VsZCBhbHdheXMKPj4+IHJl dHVybgo+Pj4gdHJ1ZSwgVGhhdCBpcyBub3QgYWxsb3dlZCwgd291bGQgY2F1c2UgZmIgZnJlZSB0 b28gZWFybHkuCj4+IE9rLCBtYXliZSBJIG5lZWQgdG8gYXNrIHlvdSBmaXJzdCB3aGF0IHRoZSBv cmlnaW5hbCBibG9jayBvZiBjb2RlCj4+IGludGVuZGVkIHRvIGFjaGlldmUuIFRoZSBUUk0gSSBo YXZlIGlzIHZlcnkgdGVyc2UgYW5kIEkgZG9uJ3QgZmluZCBhbnkKPj4gZXhwbGFuYXRpb24gdGhl cmUuIFRoZSBiYXR0ZXJ5IG9mIHRlc3RzIEkgaGF2ZSBwYXNzIGp1c3QgZmluZSB3aXRob3V0Cj4+ IGl0Lgo+Pgo+Pj4gRG9lcyB0aGlzIHBhdGNoIGlzIG5lZWRlZCBmb3IgIltQQVRDSCAyLzJdIGRy bS9yb2NrY2hpcDogdm9wOiBXYWl0IGZvcgo+Pj4gcGVuZGluZyBldmVudHMgd2hlbiBkaXNhYmxp bmcgYSBDUlRDIgo+PiBZZXMsIHRoaXMgZnVuY3Rpb24gaXMgY3VycmVudGx5IHJldHVybmluZyBm YWxzZSB3aGVuIHRoZSBwYWdlZmxpcCBoYXMKPj4gYmVlbiBjb21wbGV0ZWQgYnV0IHRoZSBwbGFu IGhhcyBiZWVuIGFscmVhZHkgZGlzYWJsZWQuCj4+Cj4+IElmIHlvdSBoYXZlIGFueSBkaWZmZXJl bnQgaWRlYSBvZiBob3cgdG8gZml4IHRoaXMgYnVnLCBwbGVhc2Ugc2hhcmUuCj4+Cj4+IFRoYW5r cywKPj4KPj4gVG9tZXUKPj4KPj4KPj4KPj4gSGkgVG9tZXUKPj4KPj4gQEAgLTUwNCw2ICs1MDYs OSBAQCBzdGF0aWMgdm9pZCB2b3BfY3J0Y19kaXNhYmxlKHN0cnVjdCBkcm1fY3J0YyAqY3J0YykK Pj4gICAgaWYgKCF2b3AtPmlzX2VuYWJsZWQpCj4+ICAgIHJldHVybjsKPj4KPj4gKyBpZiAoY3J0 Yy0+c3RhdGUtPmV2ZW50IHx8IHZvcC0+ZXZlbnQpCj4+ICsgdm9wX2NydGNfd2FpdF9mb3JfdXBk YXRlKGNydGMpOwo+PiArCj4+Cj4+IEkgdGhpbmsgYWJvdmUgY2hhbmdlIGhhcyBzb21lIHByb2Js ZW0sCj4+Cj4+IHRoZSBmdW5jdGlvbiBzdGFjazoKPj4gLT5kcm0gc3dhcCBzdGF0ZQo+PiAtPnZv cF9jcnRjX2Rpc2FibGUKPj4gLT52b3BfYXRvbWljX2JlZ2luCj4+IC0+dm9wX2F0b21pY19mbHVz aAo+Pgo+PiBvbiB2b3BfY3J0Y19kaXNhYmxlLCBjcnRjLT5zdGF0ZSBpcyBuZXcgc3RhdGUsIHRo ZSBjcnRjLT5zdGF0ZS0+ZXZlbnQgbm90Cj4+IHlldCB1cGRhdGUgdG8gdm9wLCB3YWl0IGZvciAg Y3J0Yy0+c3RhdGUtPmV2ZW50IGhlcmUgaXMgd3JvbmcuCj4+Cj4+IFNvIEkgdGhpbmsgdGhlIHBh dGNoIHNob3VsZCBiZToKPj4gKyBpZiAodm9wLT5ldmVudCkKPj4gKyB2b3BfY3J0Y193YWl0X2Zv cl91cGRhdGUoY3J0Yyk7Cj4+ICsKPj4KPj4KPj4gY2FsbCB2b3BfY3J0Y193YWl0X2Zvcl91cGRh dGUoY3J0YykgaGVyZSBhbHNvIGlzIHVuc2FmZSwgaXQgd2lsbCByZWluaXQgdGhlCj4+IHZvcC0+ d2FpdF91cGRhdGVfY29tcGxldGUuCj4+Cj4+IEkgZG91YnQgdGhhdCwgc2luY2UgdXNlIHRoZSBz ZXJpYWxpemUgb3V0c3RhbmRpbmcgbm9uYmxvY2tpbmcgY29tbWl0cywgb25seQo+PiBvbmUgcHJv Y2VzcyBjYW4gcnVuIGludG8gdGhlIHVwZGF0ZSBzdGFjaywgb2xkIHZvcC0+ZXZlbnQgc2hvdWxk IGJlIGZyZWUgb24KPj4gbGFzdCB0aW1lLCBpZiB3ZSBnZXQgdm9wLT5ldmVudCBoZXJlLCB0aGF0 IHNob3VsZCBiZSBhIGJ1Zy4KPj4KPj4KPj4gVGhlbiB0aGUgcGF0Y2ggImRybS9yb2NrY2hpcDog dm9wOiBEbyBjaGVjayBpZiBhbiB1cGRhdGUgaXMgcGVuZGluZyBkdXJpbmcKPj4gZGlzYWJsZSIg c2hvdWxkIGJlIG5vIG5lZWRlZC4KPiBIaSBNYXJrLAo+Cj4gd2l0aCBEYW5pZWwncyBzZXJpZXMg bGlua2VkIGJlbG93IHRoaXMgYW5kIHRoZSBvdGhlciBpc3N1ZXMgSSBmb3VuZCBpbgo+IHRoZSBS b2NrY2hpcCBkcml2ZXIgYXJlIGZpeGVkOgo+Cj4gaHR0cDovL3RocmVhZC5nbWFuZS5vcmcvZ21h bmUuY29tcC5mcmVlZGVza3RvcC54b3JnLmRyaXZlcnMuaW50ZWwvOTEwMjMvZm9jdXM9OTEwNTMK Ckdvb2QgbmV3cywgSSBhbHNvIHNlZSB0aGUgRGFuaWVsJ3Mgc2VyaWVzIHBhdGNoZXMsIHdvbmRl cmZ1bCB1cGRhdGUuCgpZb3UgY2FuIGFkZCBhIFRlc3RlZC1ieSBmb3IgRGFuaWVsJ3Mgcm9ja2No aXAgcGF0Y2hlcywgYW5kIEkgYWRkIGEgCkFja2VkLWJ5IGZvciB0aG9zZSByb2NrY2hpcCBwYXRj aGVzLgoKVGhhbmtzCgo+IFRoYW5rcywKPgo+IFRvbWV1Cj4KPj4gVGhhbmtzLgo+Pgo+PiAtLSDv vK1hcmsgWWFvCj4+Cj4+IF9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fCj4+IGRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKPj4gZHJpLWRldmVsQGxpc3RzLmZyZWVk ZXNrdG9wLm9yZwo+PiBodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3Rp bmZvL2RyaS1kZXZlbAo+Pgo+Pgo+Pgo+PiAtLQo+PiDvvK1hcmsgWWFvCj4+Cj4+Cj4+IF9fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCj4+IGRyaS1kZXZlbCBt YWlsaW5nIGxpc3QKPj4gZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwo+PiBodHRwczov L2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo+Pgo+Cj4K CgotLSAK77ytYXJrIFlhbwoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNr dG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2Ry aS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.yao@rock-chips.com (Mark yao) Date: Thu, 2 Jun 2016 14:25:37 +0800 Subject: [PATCH 1/2] drm/rockchip: vop: Do check if an update is pending during disable In-Reply-To: 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> Message-ID: <574FD161.4050503@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.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 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 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