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: Mon, 23 May 2016 14:32:59 +0800 Message-ID: <5742A41B.8030308@rock-chips.com> References: <1459937686-9142-1-git-send-email-tomeu.vizoso@collabora.com> <5707043D.6060706@rock-chips.com> <570AFAC2.8080907@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: "open list:ARM/Rockchip SoC..." , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" List-Id: linux-rockchip.vger.kernel.org T24gMjAxNuW5tDA15pyIMDXml6UgMTc6MzQsIFRvbWV1IFZpem9zbyB3cm90ZToKPiBPbiAyMCBB cHJpbCAyMDE2IGF0IDE2OjIzLCBUb21ldSBWaXpvc28gPHRvbWV1LnZpem9zb0Bjb2xsYWJvcmEu Y29tPiB3cm90ZToKPj4gT24gMTEgQXByaWwgMjAxNiBhdCAwMzoxNSwgTWFyayB5YW8gPG1hcmsu eWFvQHJvY2stY2hpcHMuY29tPiB3cm90ZToKPj4+IE9uIDIwMTblubQwNOaciDA45pelIDE4OjU0 LCBUb21ldSBWaXpvc28gd3JvdGU6Cj4+Pj4gT24gOCBBcHJpbCAyMDE2IGF0IDAzOjA3LCBNYXJr IHlhbyA8bWFyay55YW9Acm9jay1jaGlwcy5jb20+IHdyb3RlOgo+Pj4+PiBPbiAyMDE25bm0MDTm nIgwNuaXpSAxODoxNCwgVG9tZXUgVml6b3NvIHdyb3RlOgo+Pj4+Pgo+Pj4+PiBXaGVuIGEgcGxh bmUgaXMgYmVpbmcgZGlzYWJsZWQgYnV0IGl0J3Mgc3RpbGwgZW5hYmxlZCwgZG8gY2hlY2sgaWYg dGhlCj4+Pj4+IHByZXZpb3VzIHVwZGF0ZSBoYXMgYmVlbiBjb21wbGV0ZWQgYnkgcmVhZGluZyB5 cmdiX21zdCBiYWNrLgo+Pj4+Pgo+Pj4+PiBPdGhlcndpc2UsIHBlbmRpbmcgcGFnZWZsaXBzIHdv dWxkIHJlbWFpbiBwZW5kaW5nIGFmdGVyIGEgQ1JUQyBpcwo+Pj4+PiBkaXNhYmxlZC4KPj4+Pj4K Pj4+Pj4gU2lnbmVkLW9mZi1ieTogVG9tZXUgVml6b3NvIDx0b21ldS52aXpvc29AY29sbGFib3Jh LmNvbT4KPj4+Pj4gLS0tCj4+Pj4+ICAgIGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hp cF9kcm1fdm9wLmMgfCA1ICsrKy0tCj4+Pj4+ICAgIDEgZmlsZSBjaGFuZ2VkLCAzIGluc2VydGlv bnMoKyksIDIgZGVsZXRpb25zKC0pCj4+Pj4+Cj4+Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dw dS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX3ZvcC5jCj4+Pj4+IGIvZHJpdmVycy9ncHUvZHJt L3JvY2tjaGlwL3JvY2tjaGlwX2RybV92b3AuYwo+Pj4+PiBpbmRleCBhOWIxZThiNWFjODUuLmY0 NmIxZmQxODg3YiAxMDA2NDQKPj4+Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3Jv Y2tjaGlwX2RybV92b3AuYwo+Pj4+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9j a2NoaXBfZHJtX3ZvcC5jCj4+Pj4+IEBAIC0xMDY0LDggKzEwNjQsOSBAQCBzdGF0aWMgYm9vbCB2 b3Bfd2luX3BlbmRpbmdfaXNfY29tcGxldGUoc3RydWN0Cj4+Pj4+IHZvcF93aW4KPj4+Pj4gKnZv cF93aW4pCj4+Pj4+ICAgICBzdHJ1Y3Qgdm9wX3BsYW5lX3N0YXRlICpzdGF0ZSA9IHRvX3ZvcF9w bGFuZV9zdGF0ZShwbGFuZS0+c3RhdGUpOwo+Pj4+PiAgICAgZG1hX2FkZHJfdCB5cmdiX21zdDsK Pj4+Pj4KPj4+Pj4gLSBpZiAoIXN0YXRlLT5lbmFibGUpCj4+Pj4+IC0gcmV0dXJuIFZPUF9XSU5f R0VUKHZvcF93aW4tPnZvcCwgdm9wX3dpbi0+ZGF0YSwgZW5hYmxlKSA9PSAwOwo+Pj4+PiArIGlm ICghc3RhdGUtPmVuYWJsZSAmJgo+Pj4+PiArICAgIFZPUF9XSU5fR0VUKHZvcF93aW4tPnZvcCwg dm9wX3dpbi0+ZGF0YSwgZW5hYmxlKSA9PSAwKQo+Pj4+PiArIHJldHVybiB0cnVlOwo+Pj4+Pgo+ Pj4+Pgo+Pj4+PiBJdCBpcyB3cm9uZywgdGhlIHBhdGNoIHdvdWxkIGNhdXNlIGEgYnVnLgo+Pj4+ Pgo+Pj4+PiB3aGVuIHN0YXRlLT5lbmFibGUgaXMgMCwgY2hlY2sgeXJnYl9tc3QgPT0gc3RhdGUt PnlyZ2JfbXN0IGFsd2F5cyBiZQo+Pj4+PiB0cnVlLAo+Pj4+PiBiZWNhdXNlIHN0YXRlLT55cmdi X21zdCBub3QgdXBkYXRlIG9uIHBsYW5lIGRpc2FibGVkIGZ1bnRpb24sIHRoYXQgd291bGQKPj4+ Pj4gY2F1c2UgaW9tbXUgY3Jhc2guCj4+Pj4gU29ycnksIGJ1dCBJIGRvbid0IHVuZGVyc3RhbmQg d2hlcmUncyB0aGUgYnVnIGFuZCB3aGF0IGNvdWxkIGNhdXNlCj4+Pj4gdGhhdCBjcmFzaC4gV2hh dCB0aGUgZXhpc3RpbmcgY29kZSB3YXMgZG9pbmcgaXMgc2F5aW5nIHRoYXQgYSBwYWdlZmxpcAo+ Pj4+IGV2ZW50IGlzIHN0aWxsIHBlbmRpbmcgaWYgd2UgaGF2ZSB0b2xkIHRoZSBwbGFuZSB0byBk aXNhYmxlIGJ1dCBmb3IKPj4+PiBzb21lIHJlYXNvbiBpdCBoYXNuJ3QgeWV0Lgo+Pj4+Cj4+Pj4g V2l0aCB0aGlzIG1vZGlmaWNhdGlvbiwgaWYgd2UgcmVhZCBiYWNrIHRoYXQgaXQncyBhbHJlYWR5 IGRpc2FibGVkLCB3ZQo+Pj4+IHJldHVybiB0cnVlIGFzIGJlZm9yZS4gQnV0IGlmIHdlIHJlYWQg YmFjayB0aGF0IGl0IGlzbid0IGRpc2FibGVkIHlldCwKPj4+PiB0aGVuIHdlIHN0aWxsIGNoZWNr IHRoZSBmYiBwb2ludGVycyBhbmQgY29tcGFyZSB0aGVtLgo+Pj4+Cj4+Pj4gVGhlIGlvbW11IG1h cHBpbmcgaXMgcmVtb3ZlZCB3aGVuIHRoZSBfQ1JUQ18gaXMgZGlzYWJsZWQsIGFuZCB3aGF0Cj4+ Pj4gdGhpcyBzZXJpZXMgZG9lcyBpcyB0byB3YWl0IGZvciB0aGUgcGVuZGluZyBwYWdlZmxpcCB0 byBmaW5pc2ggYmVmb3JlCj4+Pj4gY29uaXRudWluZyB3aXRoIENSVEMgZGlzYWJsZW1lbnQuCj4+ Pgo+Pj4gdGhlIGlvbW11IG1hcHBpbmcgd2lsbCB1bm1hcCBhZnRlciBwbGFuZSBkaXNhYmxlZCwg d2UgbmVlZCBzdXJlIHRoYXQgdGhlCj4+PiBwbGFuZSByZWFsbHkgZGlzYWJsZWQgYmVmb3JlIHVu bWFwLCBpZiBub3QsIHRoZSB1bm1hcCBtYXkgY2FsbCBiZWZvcmUgcGxhbmUKPj4+IHJlYWxseSBk aXNhYmxlLCB2b3AgbWF5IGFjY2VzcyB1bm1hcCBhZGRyZXNzLCB0aGVuIHdvdWxkIGdldCBpb21t dSBwYWdlCj4+PiBmYXVsdC4KPj4gU29ycnksIGJ1dCBJIHN0aWxsIGRvbid0IHNlZSB0aGUgZXJy b3IgY29uZGl0aW9uIHRoYXQgeW91IGFyZQo+PiBkZXNjcmliaW5nLiBBRkFJQ1MsIHRoZSB1bm1h cCB3aWxsIGhhcHBlbiBhZnRlciB0aGUgbGFzdCBwYWdlZmxpcCBoYXMKPj4gY29tcGxldGVkLgo+ Pgo+Pj4+PiBBYm91dCBwZW5kaW5nIHBhZ2VmbGlwcyB3b3VsZCByZW1haW4gcGVuZGluZywgY2Fu IHlvdSAgZGVzY3JpYmUgbW9yZSBpbmZvCj4+Pj4+IGFib3V0IGl0PyBJIHRoaW5rIHRob3NlIHBl bmRpbmcgcGFnZWZsaXBzIHNob3VsZCBiZSBpZ25vcmUgd2hlbiBDUlRDIGlzCj4+Pj4+IGRpc2Fi bGVkLgo+Pj4+IFdlbGwsIHJpZ2h0IG5vdyBpbiByb2NrY2hpcC1kcm0gcGVuZGluZyBwYWdlZmxp cHMgd29uJ3QgYmUgaWdub3JlZAo+Pj4+IHdoZW4gYSBDUlRDIGlzIGRpc2FibGVkLCBidXQgd2ls bCBiZSBkZWxpdmVyZWQgd2hlbiBpdCdzIHJlLWVuYWJsZWQuCj4+Pj4KPj4+PiBJZiB0aGV5IHdv dWxkIGJlIHRvIGJlIGlnbm9yZWQgKHVuZGVyc3RhbmRpbmcgdGhhdCBhcyBkcm9wcGVkKSwgdGhh dAo+Pj4+IHdvdWxkIHJlcXVpcmUgbW9kaWZpY2F0aW9ucyB0byBjbGllbnRzIHNvIHRoZXkga2Vl cCB0cmFjayBvZiB3aGljaCBmYnMKPj4+PiB3ZXJlIHVzZWQgaW4gYSBwYXJ0aWN1bGFyIGNydGMg YW5kIGRlc3Ryb3kgdGhlbSB3aGVuIHRoZSBjcnRjIGlzCj4+Pj4gZGlzYWJsZWQsIGJ1dCB0aGF0 IHdvdWxkIGJlIGluY29ycmVjdCB3aGVuIHVzaW5nIHRoZSBpOTE1IERSTSBkcml2ZXIKPj4+PiAo SSBhbHNvIGFzc3VtZSBvdGhlcnMgZG8gdGhlIHNhbWUpLiBHaXZlbiB0aGF0IHRoZSBwYWdlZmxp cCBpb2N0bAo+Pj4+IGlzbid0IGRyaXZlci1zcGVjaWZpYywgSSB0aGluayB0aGVyZSBjYW5ub3Qg YmUgc3VjaCBhIGRpZmZlcmVuY2UgaW4KPj4+PiBiZWhhdmlvciBiZXR3ZWVuIGRyaXZlcnMuCj4+ Pj4KPj4+PiBXaXRoIHRoZSBjdXJyZW50IGJlaGF2aW9yIChwZW5kaW5nIHBhZ2VmbGlwIGV2ZW50 cyBiZWluZyBkZWxheWVkIHVudGlsCj4+Pj4gdGhlIENSVEMgaXMgZW5hYmxlZCBhZ2FpbiksIGNv bXBvc2l0b3JzIGFuZCBvdGhlciBjbGllbnRzIHdpbGwgYmUKPj4+PiBob2xkaW5nIG9uIHRvIHRo ZSBmYiBpbiB0aGUgcGVuZGluZyBwYWdlZmxpcCB1bnRpbCBhbiBhcmJpdHJhcnkgcG9pbnQKPj4+ PiBpbiB0aGUgZnV0dXJlIHRoYXQgbWF5IG5vdCBldmVyIGNvbWUuIFRvIG1lIHRoYXQgc291bmRz IGxpa2UgYSBzZXJpb3VzCj4+Pj4gbW9kaWZpY2F0aW9uIG9mIHRoZSBhc3N1bXB0aW9ucyBvbiBm YiBsaWZlY3ljbGUgdGhhdCBtaWdodCBub3QgYmUKPj4+PiB3YXJyYW50ZWQuCj4+Pj4KPj4+PiBT byBpbiBzdW1tYXJ5LCBldmVuIGlmIEkgaGF2ZW4ndCBmb3VuZCBhbnkgZXhwbGljaXQgZG9jdW1l bnRhdGlvbiBvbgo+Pj4+IHRoaXMsIEkgdGhpbmsgdGhlIEFCSSBpcyB0aGF0IGFueSBwZW5kaW5n IHBhZ2VmbGlwcyBhcmUgdG8gYmUKPj4+PiBkZWxpdmVyZWQgd2hlbiB0aGF0IENSVEMgaXMgYmVp bmcgZGlzYWJsZWQgYW5kIG5vdCBsYXRlci4KPj4+Cj4+PiBvbiBkcml2ZXJzL2dwdS9kcm0vcm9j a2NoaXAvcm9ja2NoaXBfZHJtX2ZiLmMKPj4+Cj4+PiBkcm1fYXRvbWljX2hlbHBlcl9jb21taXRf cGxhbmVzKGRldiwgc3RhdGUsIHRydWUpOwo+Pj4gcm9ja2NoaXBfYXRvbWljX3dhaXRfZm9yX2Nv bXBsZXRlKGRldiwgc3RhdGUpOwo+Pj4KPj4+IFdlIHNldCBhY3RpdmVfb25seSA9IHRydWUsIEkg dGhpbmsgcGxhbmVzIGNhbiBvbmx5IHVwZGF0ZSB3aGVuIGNydGMgaXMKPj4+IGFjdGl2ZS4gYW5k IHJvY2tjaGlwX2F0b21pY193YWl0X2Zvcl9jb21wbGV0ZSBvbmx5IHdhaXQgd2hlbiBjcnRjIGlz IGFjdGl2ZS4KPj4gVGhhdCdzIGZpbmUsIGJ1dCBpZiBhIHBhZ2VmbGlwIGlzIHBlbmRpbmcgd2hl biB0aGUgY2xpZW50IHJlcXVlc3RzIHRoZQo+PiBDUlRDIHRvIGJlIGRpc2FibGVkLCB3ZSBuZWVk IHRvIHdhaXQgZm9yIHRoZSBldmVudCB0byBiZSBlbWl0dGVkCj4+IGJlZm9yZSB3ZSBhY3R1YWxs eSBkaXNhYmxlIHRoZSBIVy4KPiBIaSBNYXJrLAo+Cj4gY291bGQgeW91IHRlbGwgbWUgaWYgeW91 IGFncmVlIHRoYXQgdGhlcmUncyBhIGJ1ZyB0aGF0IG5lZWRzIHRvIGJlCj4gc29sdmVkLCBhbmQg aWYgc28sIHdoYXQgZG8geW91IHRoaW5rIHdlIHNob3VsZCBkbyBhYm91dCBpdD8KSGkgVG9tZXUK ClNvcnJ5IGZvciByZXBseSBsYXRlLgpJIGRvbid0IGFncmVlIHRoZSBjaGFuZ2VzOgoKLSBpZiAo IXN0YXRlLT5lbmFibGUpCi0gcmV0dXJuIFZPUF9XSU5fR0VUKHZvcF93aW4tPnZvcCwgdm9wX3dp bi0+ZGF0YSwgZW5hYmxlKSA9PSAwOworIGlmICghc3RhdGUtPmVuYWJsZSAmJgorICAgIFZPUF9X SU5fR0VUKHZvcF93aW4tPnZvcCwgdm9wX3dpbi0+ZGF0YSwgZW5hYmxlKSA9PSAwKQorIHJldHVy biB0cnVlOwoKVGhpcyBjaGFuZ2VzIGFjdHVhbGx5IHdvdWxkIGxlYWQgYSBidWcuCndoZW4gd2Ug ZGlzYWJsZSBhIHBsYW5lLCB0aGUgdm9wX3dpbl9wZW5kaW5nX2lzX2NvbXBsZXRlIHdvdWxkIGFs d2F5cyAKcmV0dXJuIHRydWUsIFRoYXQgaXMgbm90IGFsbG93ZWQsIHdvdWxkIGNhdXNlIGZiIGZy ZWUgdG9vIGVhcmx5LgoKRG9lcyB0aGlzIHBhdGNoIGlzIG5lZWRlZCBmb3IgIltQQVRDSCAyLzJd IGRybS9yb2NrY2hpcDogdm9wOiBXYWl0IGZvciAKcGVuZGluZyBldmVudHMgd2hlbiBkaXNhYmxp bmcgYSBDUlRDIgoKVGhhbmtzLgoKPiBUaGFua3MsCj4KPiBUb21ldQo+Cj4+IFJlZ2FyZHMsCj4+ Cj4+IFRvbWV1Cj4+Cj4+PiBUaGFua3MuCj4+Pgo+Pj4+IFRoYW5rcywKPj4+Pgo+Pj4+IFRvbWV1 Cj4+Pj4KPj4+Pj4gVGhhbmtzLgo+Pj4+Pgo+Pj4+Pgo+Pj4+PiAgICAgeXJnYl9tc3QgPSBWT1Bf V0lOX0dFVF9ZUkdCQUREUih2b3Bfd2luLT52b3AsIHZvcF93aW4tPmRhdGEpOwo+Pj4+Pgo+Pj4+ Pgo+Pj4+Pgo+Pj4+PiAtLQo+Pj4+PiDvvK1hcmsgWWFvCj4+Pj4+Cj4+Pj4+Cj4+Pj4+IF9fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCj4+Pj4+IGRyaS1kZXZl bCBtYWlsaW5nIGxpc3QKPj4+Pj4gZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwo+Pj4+ PiBodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZl bAo+Pj4+Pgo+Pj4+Cj4+Pgo+Pj4gLS0KPj4+IO+8rWFyayBZYW8KPj4+Cj4+Pgo+Pj4gX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KPj4+IGRyaS1kZXZlbCBt YWlsaW5nIGxpc3QKPj4+IGRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKPj4+IGh0dHBz Oi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCj4KPgoK Ci0tIArvvK1hcmsgWWFvCgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0 b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJp LWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.yao@rock-chips.com (Mark yao) Date: Mon, 23 May 2016 14:32:59 +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> Message-ID: <5742A41B.8030308@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2016?05?05? 17:34, Tomeu Vizoso wrote: > On 20 April 2016 at 16:23, Tomeu Vizoso wrote: >> On 11 April 2016 at 03:15, Mark yao wrote: >>> On 2016?04?08? 18:54, Tomeu Vizoso wrote: >>>> On 8 April 2016 at 03:07, Mark yao wrote: >>>>> On 2016?04?06? 18:14, Tomeu Vizoso wrote: >>>>> >>>>> When a plane is being disabled but it's still enabled, do check if the >>>>> previous update has been completed by reading yrgb_mst back. >>>>> >>>>> Otherwise, pending pageflips would remain pending after a CRTC is >>>>> disabled. >>>>> >>>>> Signed-off-by: Tomeu Vizoso >>>>> --- >>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++-- >>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> index a9b1e8b5ac85..f46b1fd1887b 100644 >>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> @@ -1064,8 +1064,9 @@ static bool vop_win_pending_is_complete(struct >>>>> vop_win >>>>> *vop_win) >>>>> struct vop_plane_state *state = to_vop_plane_state(plane->state); >>>>> dma_addr_t yrgb_mst; >>>>> >>>>> - 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; >>>>> >>>>> >>>>> It is wrong, the patch would cause a bug. >>>>> >>>>> when state->enable is 0, check yrgb_mst == state->yrgb_mst always be >>>>> true, >>>>> because state->yrgb_mst not update on plane disabled funtion, that would >>>>> cause iommu crash. >>>> Sorry, but I don't understand where's the bug and what could cause >>>> that crash. What the existing code was doing is saying that a pageflip >>>> event is still pending if we have told the plane to disable but for >>>> some reason it hasn't yet. >>>> >>>> With this modification, if we read back that it's already disabled, we >>>> return true as before. But if we read back that it isn't disabled yet, >>>> then we still check the fb pointers and compare them. >>>> >>>> The iommu mapping is removed when the _CRTC_ is disabled, and what >>>> this series does is to wait for the pending pageflip to finish before >>>> conitnuing with CRTC disablement. >>> >>> the iommu mapping will unmap after plane disabled, we need sure that the >>> plane really disabled before unmap, if not, the unmap may call before plane >>> really disable, vop may access unmap address, then would get iommu page >>> fault. >> Sorry, but I still don't see the error condition that you are >> describing. AFAICS, the unmap will happen after the last pageflip has >> completed. >> >>>>> About pending pageflips would remain pending, can you describe more info >>>>> about it? I think those pending pageflips should be ignore when CRTC is >>>>> disabled. >>>> Well, right now in rockchip-drm pending pageflips won't be ignored >>>> when a CRTC is disabled, but will be delivered when it's re-enabled. >>>> >>>> If they would be to be ignored (understanding that as dropped), that >>>> would require modifications to clients so they keep track of which fbs >>>> were used in a particular crtc and destroy them when the crtc is >>>> disabled, but that would be incorrect when using the i915 DRM driver >>>> (I also assume others do the same). Given that the pageflip ioctl >>>> isn't driver-specific, I think there cannot be such a difference in >>>> behavior between drivers. >>>> >>>> With the current behavior (pending pageflip events being delayed until >>>> the CRTC is enabled again), compositors and other clients will be >>>> holding on to the fb in the pending pageflip until an arbitrary point >>>> in the future that may not ever come. To me that sounds like a serious >>>> modification of the assumptions on fb lifecycle that might not be >>>> warranted. >>>> >>>> So in summary, even if I haven't found any explicit documentation on >>>> this, I think the ABI is that any pending pageflips are to be >>>> delivered when that CRTC is being disabled and not later. >>> >>> on drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> >>> drm_atomic_helper_commit_planes(dev, state, true); >>> rockchip_atomic_wait_for_complete(dev, state); >>> >>> We set active_only = true, I think planes can only update when crtc is >>> active. and rockchip_atomic_wait_for_complete only wait when crtc is active. >> That's fine, but if a pageflip is pending when the client requests the >> CRTC to be disabled, we need to wait for the event to be emitted >> before we actually disable the HW. > Hi Mark, > > could you tell me if you agree that there's a bug that needs to be > solved, and if so, what do you think we should do about it? 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. Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for pending events when disabling a CRTC" Thanks. > Thanks, > > Tomeu > >> Regards, >> >> Tomeu >> >>> Thanks. >>> >>>> Thanks, >>>> >>>> Tomeu >>>> >>>>> Thanks. >>>>> >>>>> >>>>> yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data); >>>>> >>>>> >>>>> >>>>> -- >>>>> ?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 S1753162AbcEWGdM (ORCPT ); Mon, 23 May 2016 02:33:12 -0400 Received: from regular1.263xmail.com ([211.150.99.139]:51470 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752975AbcEWGdL (ORCPT ); Mon, 23 May 2016 02:33:11 -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: dri-devel@lists.freedesktop.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: <3e9fdfc0771963593533940f7a170986> 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> Cc: "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" From: Mark yao Message-ID: <5742A41B.8030308@rock-chips.com> Date: Mon, 23 May 2016 14:32:59 +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年05月05日 17:34, Tomeu Vizoso wrote: > On 20 April 2016 at 16:23, Tomeu Vizoso wrote: >> On 11 April 2016 at 03:15, Mark yao wrote: >>> On 2016年04月08日 18:54, Tomeu Vizoso wrote: >>>> On 8 April 2016 at 03:07, Mark yao wrote: >>>>> On 2016年04月06日 18:14, Tomeu Vizoso wrote: >>>>> >>>>> When a plane is being disabled but it's still enabled, do check if the >>>>> previous update has been completed by reading yrgb_mst back. >>>>> >>>>> Otherwise, pending pageflips would remain pending after a CRTC is >>>>> disabled. >>>>> >>>>> Signed-off-by: Tomeu Vizoso >>>>> --- >>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++-- >>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> index a9b1e8b5ac85..f46b1fd1887b 100644 >>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>>>> @@ -1064,8 +1064,9 @@ static bool vop_win_pending_is_complete(struct >>>>> vop_win >>>>> *vop_win) >>>>> struct vop_plane_state *state = to_vop_plane_state(plane->state); >>>>> dma_addr_t yrgb_mst; >>>>> >>>>> - 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; >>>>> >>>>> >>>>> It is wrong, the patch would cause a bug. >>>>> >>>>> when state->enable is 0, check yrgb_mst == state->yrgb_mst always be >>>>> true, >>>>> because state->yrgb_mst not update on plane disabled funtion, that would >>>>> cause iommu crash. >>>> Sorry, but I don't understand where's the bug and what could cause >>>> that crash. What the existing code was doing is saying that a pageflip >>>> event is still pending if we have told the plane to disable but for >>>> some reason it hasn't yet. >>>> >>>> With this modification, if we read back that it's already disabled, we >>>> return true as before. But if we read back that it isn't disabled yet, >>>> then we still check the fb pointers and compare them. >>>> >>>> The iommu mapping is removed when the _CRTC_ is disabled, and what >>>> this series does is to wait for the pending pageflip to finish before >>>> conitnuing with CRTC disablement. >>> >>> the iommu mapping will unmap after plane disabled, we need sure that the >>> plane really disabled before unmap, if not, the unmap may call before plane >>> really disable, vop may access unmap address, then would get iommu page >>> fault. >> Sorry, but I still don't see the error condition that you are >> describing. AFAICS, the unmap will happen after the last pageflip has >> completed. >> >>>>> About pending pageflips would remain pending, can you describe more info >>>>> about it? I think those pending pageflips should be ignore when CRTC is >>>>> disabled. >>>> Well, right now in rockchip-drm pending pageflips won't be ignored >>>> when a CRTC is disabled, but will be delivered when it's re-enabled. >>>> >>>> If they would be to be ignored (understanding that as dropped), that >>>> would require modifications to clients so they keep track of which fbs >>>> were used in a particular crtc and destroy them when the crtc is >>>> disabled, but that would be incorrect when using the i915 DRM driver >>>> (I also assume others do the same). Given that the pageflip ioctl >>>> isn't driver-specific, I think there cannot be such a difference in >>>> behavior between drivers. >>>> >>>> With the current behavior (pending pageflip events being delayed until >>>> the CRTC is enabled again), compositors and other clients will be >>>> holding on to the fb in the pending pageflip until an arbitrary point >>>> in the future that may not ever come. To me that sounds like a serious >>>> modification of the assumptions on fb lifecycle that might not be >>>> warranted. >>>> >>>> So in summary, even if I haven't found any explicit documentation on >>>> this, I think the ABI is that any pending pageflips are to be >>>> delivered when that CRTC is being disabled and not later. >>> >>> on drivers/gpu/drm/rockchip/rockchip_drm_fb.c >>> >>> drm_atomic_helper_commit_planes(dev, state, true); >>> rockchip_atomic_wait_for_complete(dev, state); >>> >>> We set active_only = true, I think planes can only update when crtc is >>> active. and rockchip_atomic_wait_for_complete only wait when crtc is active. >> That's fine, but if a pageflip is pending when the client requests the >> CRTC to be disabled, we need to wait for the event to be emitted >> before we actually disable the HW. > Hi Mark, > > could you tell me if you agree that there's a bug that needs to be > solved, and if so, what do you think we should do about it? 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. Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for pending events when disabling a CRTC" Thanks. > Thanks, > > Tomeu > >> Regards, >> >> Tomeu >> >>> Thanks. >>> >>>> Thanks, >>>> >>>> Tomeu >>>> >>>>> Thanks. >>>>> >>>>> >>>>> yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data); >>>>> >>>>> >>>>> >>>>> -- >>>>> Ｍ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