From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update Date: Tue, 12 Mar 2019 16:52:43 +0100 Message-ID: <20190312165243.5b771e4a@collabora.com> References: <20190312022204.2775-1-helen.koike@collabora.com> <20190312022204.2775-2-helen.koike@collabora.com> <20190312073438.05ad8173@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" 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: Helen Koike Cc: =?UTF-8?B?U3TDqXBoYW5l?= Marchesin , Sean Paul , David Airlie , daniel.vetter@ffwll.ch, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Tomasz Figa , linux-rockchip@lists.infradead.org, kernel@collabora.com, nicholas.kazlauskas@amd.com, linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org T24gVHVlLCAxMiBNYXIgMjAxOSAxMjozNDo0NSAtMDMwMApIZWxlbiBLb2lrZSA8aGVsZW4ua29p a2VAY29sbGFib3JhLmNvbT4gd3JvdGU6Cgo+IE9uIDMvMTIvMTkgMzozNCBBTSwgQm9yaXMgQnJl emlsbG9uIHdyb3RlOgo+ID4gT24gTW9uLCAxMSBNYXIgMjAxOSAyMzoyMTo1OSAtMDMwMAo+ID4g SGVsZW4gS29pa2UgPGhlbGVuLmtvaWtlQGNvbGxhYm9yYS5jb20+IHdyb3RlOgo+ID4gICAKPiA+ PiBJbiB0aGUgY2FzZSBvZiBhc3luYyB1cGRhdGUsIG1vZGlmaWNhdGlvbnMgYXJlIGRvbmUgaW4g cGxhY2UsIGkuZS4gaW4gdGhlCj4gPj4gY3VycmVudCBwbGFuZSBzdGF0ZSwgc28gdGhlIG5ld19z dGF0ZSBpcyBwcmVwYXJlZCBhbmQgdGhlIG5ld19zdGF0ZSBpcwo+ID4+IGNsZWFudXAgdXAgKGlu c3RlYWQgb2YgdGhlIG9sZF9zdGF0ZSwgZGlmZXJyZW50bHkgb24gd2hhdCBoYXBwZW4gaW4gYSAg Cj4gPiAKPiA+ICAgXiBjbGVhbmVkIHVwCQkJCV4gZGlmZmVyZW50bHkgKGJ1dCBtYXliZQo+ID4g InVubGlrZSB3aGF0IGhhcHBlbnMiIGlzIG1vcmUgYXBwcm9wcmlhdGUgaGVyZSkuCj4gPiAgIAo+ ID4+IG5vcm1hbCBzeW5jIHVwZGF0ZSkuCj4gPj4gVG8gY2xlYW51cCB0aGUgb2xkX2ZiIHByb3Bl cmx5LCBpdCBuZWVkcyB0byBiZSBwbGFjZWQgaW4gdGhlIG5ld19zdGF0ZQo+ID4+IGluIHRoZSBl bmQgb2YgYXN5bmNfdXBkYXRlLCBzbyBjbGVhbnVwIGNhbGwgd2lsbCB1bnJlZmVyZW5jZSB0aGUg b2xkX2ZiCj4gPj4gY29ycmVjdGx5Lgo+ID4+Cj4gPj4gQWxzbywgdGhlIHByZXZpb3VzIGNvZGUg aGFkIGE6Cj4gPj4KPiA+PiAJcGxhbmVfc3RhdGUgPSBwbGFuZS0+ZnVuY3MtPmF0b21pY19kdXBs aWNhdGVfc3RhdGUocGxhbmUpOwo+ID4+IAkuLi4KPiA+PiAJc3dhcChwbGFuZV9zdGF0ZSwgcGxh bmUtPnN0YXRlKTsKPiA+Pgo+ID4+IAlpZiAocGxhbmUtPnN0YXRlLT5mYiAmJiBwbGFuZS0+c3Rh dGUtPmZiICE9IG5ld19zdGF0ZS0+ZmIpIHsKPiA+PiAJLi4uCj4gPj4gCX0KPiA+Pgo+ID4+IFdo aWNoIHdhcyB3cm9uZywgYXMgdGhlIGZiIHdlcmUganVzdCBhc3NpZ25lZCB0byBiZSBlcXVhbCwg c28gdGhpcyBpZgo+ID4+IHN0YXRlbWVudCBuZXZlcnMgZXZhbHVhdGVzIHRvIHRydWUuCj4gPj4K PiA+PiBBbm90aGVyIGRldGFpbHMgaXMgdGhhdCB0aGUgZnVuY3Rpb24gZHJtX2NydGNfdmJsYW5r X2dldCgpIGNhbiBvbmx5IGJlCj4gPj4gY2FsbGVkIHdoZW4gdm9wLT5pc19lbmFibGVkIGlzIHRy dWUsIG90aGVyd2lzZSBpdCBoYXMgbm8gZWZmZWN0IGFuZAo+ID4+IHRyb3dzIGEgV0FSTl9PTigp Lgo+ID4+Cj4gPj4gQ2FsbGluZyBkcm1fYXRvbWljX3NldF9mYl9mb3JfcGxhbmUoKSAod2hpY2gg Z2V0IGEgcmVmZXJlbnQgb2YgdGhlIG5ldwo+ID4+IGZiIGFuZCBwdXMgdGhlIG9sZCBmYikgaXMg bm90IHJlcXVpcmVkLCBhcyBpdCBpcyB0YWtlbiBjYXJlIGJ5Cj4gPj4gZHJtX21vZGVfY3Vyc29y X3VuaXZlcnNhbCgpIHdoZW4gY2FsbGluZwo+ID4+IGRybV9hdG9taWNfaGVscGVyX3VwZGF0ZV9w bGFuZSgpLgo+ID4+Cj4gPj4gU2lnbmVkLW9mZi1ieTogSGVsZW4gS29pa2UgPGhlbGVuLmtvaWtl QGNvbGxhYm9yYS5jb20+Cj4gPj4KPiA+PiAtLS0KPiA+PiBIZWxsbywKPiA+Pgo+ID4+IEkgdGVz dGVkIG9uIHRoZSByb2NrY2hpcCBmaWN1cyB2MS4xIHVzaW5nIGlndCBwbGFuZV9jdXJzb3JfbGVn YWN5IGFuZAo+ID4+IGttc19jdXJzb3JfbGVnYWN5IGFuZCBJIGRpZG4ndCBzZWUgYW55IHJlZ3Jl c3Npb25zLgo+ID4+Cj4gPj4gQ2hhbmdlcyBpbiB2MjogTm9uZQo+ID4+Cj4gPj4gIGRyaXZlcnMv Z3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fdm9wLmMgfCA0MiArKysrKysrKysrKystLS0t LS0tLS0KPiA+PiAgMSBmaWxlIGNoYW5nZWQsIDI0IGluc2VydGlvbnMoKyksIDE4IGRlbGV0aW9u cygtKQo+ID4+Cj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2Nr Y2hpcF9kcm1fdm9wLmMgYi9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX3Zv cC5jCj4gPj4gaW5kZXggYzdkNGM2MDczZWE1Li5hMWVlOGMxNTZhN2IgMTAwNjQ0Cj4gPj4gLS0t IGEvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV92b3AuYwo+ID4+ICsrKyBi L2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fdm9wLmMKPiA+PiBAQCAtOTEy LDMwICs5MTIsMzEgQEAgc3RhdGljIHZvaWQgdm9wX3BsYW5lX2F0b21pY19hc3luY191cGRhdGUo c3RydWN0IGRybV9wbGFuZSAqcGxhbmUsCj4gPj4gIAkJCQkJICBzdHJ1Y3QgZHJtX3BsYW5lX3N0 YXRlICpuZXdfc3RhdGUpCj4gPj4gIHsKPiA+PiAgCXN0cnVjdCB2b3AgKnZvcCA9IHRvX3ZvcChw bGFuZS0+c3RhdGUtPmNydGMpOwo+ID4+IC0Jc3RydWN0IGRybV9wbGFuZV9zdGF0ZSAqcGxhbmVf c3RhdGU7Cj4gPj4gKwlzdHJ1Y3QgZHJtX2ZyYW1lYnVmZmVyICpvbGRfZmIgPSBwbGFuZS0+c3Rh dGUtPmZiOwo+ID4+ICAKPiA+PiAtCXBsYW5lX3N0YXRlID0gcGxhbmUtPmZ1bmNzLT5hdG9taWNf ZHVwbGljYXRlX3N0YXRlKHBsYW5lKTsKPiA+PiAtCXBsYW5lX3N0YXRlLT5jcnRjX3ggPSBuZXdf c3RhdGUtPmNydGNfeDsKPiA+PiAtCXBsYW5lX3N0YXRlLT5jcnRjX3kgPSBuZXdfc3RhdGUtPmNy dGNfeTsKPiA+PiAtCXBsYW5lX3N0YXRlLT5jcnRjX2ggPSBuZXdfc3RhdGUtPmNydGNfaDsKPiA+ PiAtCXBsYW5lX3N0YXRlLT5jcnRjX3cgPSBuZXdfc3RhdGUtPmNydGNfdzsKPiA+PiAtCXBsYW5l X3N0YXRlLT5zcmNfeCA9IG5ld19zdGF0ZS0+c3JjX3g7Cj4gPj4gLQlwbGFuZV9zdGF0ZS0+c3Jj X3kgPSBuZXdfc3RhdGUtPnNyY195Owo+ID4+IC0JcGxhbmVfc3RhdGUtPnNyY19oID0gbmV3X3N0 YXRlLT5zcmNfaDsKPiA+PiAtCXBsYW5lX3N0YXRlLT5zcmNfdyA9IG5ld19zdGF0ZS0+c3JjX3c7 Cj4gPj4gLQo+ID4+IC0JaWYgKHBsYW5lX3N0YXRlLT5mYiAhPSBuZXdfc3RhdGUtPmZiKQo+ID4+ IC0JCWRybV9hdG9taWNfc2V0X2ZiX2Zvcl9wbGFuZShwbGFuZV9zdGF0ZSwgbmV3X3N0YXRlLT5m Yik7Cj4gPj4gLQo+ID4+IC0Jc3dhcChwbGFuZV9zdGF0ZSwgcGxhbmUtPnN0YXRlKTsKPiA+PiAt Cj4gPj4gLQlpZiAocGxhbmUtPnN0YXRlLT5mYiAmJiBwbGFuZS0+c3RhdGUtPmZiICE9IG5ld19z dGF0ZS0+ZmIpIHsKPiA+PiArCS8qCj4gPj4gKwkgKiBBIHNjYW5vdXQgY2FuIHN0aWxsIGJlIG9j Y3VycmluZywgc28gd2UgY2FuJ3QgZHJvcCB0aGUgcmVmZXJlbmNlIHRvCj4gPj4gKwkgKiB0aGUg b2xkIGZyYW1lYnVmZmVyLiBUbyBzb2x2ZSB0aGlzIHdlIGdldCBhIHJlZmVyZW5jZSB0byBvbGRf ZmIgYW5kCj4gPj4gKwkgKiBzZXQgYSB3b3JrZXIgdG8gcmVsZWFzZSBpdCBsYXRlci4gIAo+ID4g Cj4gPiBIbSwgZG9lc24ndCBsb29rIGxpa2UgYW4gYXN5bmMgdXBkYXRlIHRvIG1lIGlmIHdlIGhh dmUgdG8gd2FpdCBmb3IgdGhlCj4gPiBuZXh0IFZCTEFOSyB0byBoYXBwZW4gdG8gZ2V0IHRoZSBu ZXcgY29udGVudCBvbiB0aGUgc2NyZWVuLiBNYXliZSB3ZQo+ID4gc2hvdWxkIHJlamVjdCBhc3lu YyB1cGRhdGVzIHdoZW4gb2xkX2ZiICE9IG5ld19mYiBpbiB0aGUgcmsgIAo+ID4gLT5hc3luY19j aGVjaygpIGhvb2suICAKPiAKPiBVbmxlc3MgSSBhbSBtaXN1bmRlcnN0YW5kaW5nIHRoaXMsIHdl IGRvbid0IHdhaXQgaGVyZSwgd2UganVzdCBncmFiIGEKPiByZWZlcmVuY2UgdG8gdGhlIGZiIGlu IGNhc2UgaXQgaXMgYmVpbmcgc3RpbGwgdXNlZCBieSB0aGUgaHcsIHNvIGl0Cj4gZG9lc24ndCBn ZXQgcmVsZWFzZWQgcHJlbWF0dXJlbHkuCgpJIHdhcyBqdXN0IHJlYWN0aW5nIHRvIHRoZSBjb21t ZW50IHRoYXQgc2F5cyB0aGUgbmV3IEZCIHNob3VsZCBzdGF5CmFyb3VuZCB1bnRpbCB0aGUgbmV4 dCBWQkxBTksgZXZlbnQgaGFwcGVucy4gSWYgdGhlIEZCIG11c3Qgc3RheSBhcm91bmQKdGhhdCBw cm9iYWJseSBtZWFucyB0aGUgSFcgaXMgc3RpbGwgdXNpbmcsIHdoaWNoIG1hZGUgbWUgd29uZGVy IGlmIHRoaXMKSFcgYWN0dWFsbHkgc3VwcG9ydHMgYXN5bmMgdXBkYXRlICh3aGVyZSBhc3luYyBt ZWFucyAidXBkYXRlIG5vdyBhbmQKZG9uJ3QgY2FyZSBhYm91dCBhYm91dCB0ZWFyaW5nIikuIE9y IG1heWJlIGl0IHRha2VzIHNvbWUgdGltZSB0byBzd2l0Y2gKdG8gdGhlIG5ldyBGQiBhbmQgd2Fp dGluZyBmb3IgdGhlIG5leHQgVkJMQU5LIHRvIHJlbGVhc2UgdGhlIG9sZCBGQiB3YXMKYW4gZWFz eSBzb2x1dGlvbiB0byBub3Qgd2FpdCBmb3IgdGhlIGZsaXAgdG8gYWN0dWFsbHkgaGFwcGVuIGlu Ci0+YXN5bmNfdXBkYXRlKCkgKHdoaWNoIGlzIGtpbmQgb2YgYSBjb21iaW5hdGlvbiBvZiBhc3lu Yytub24tYmxvY2tpbmcpLgoKQW55d2F5LCBsZXQncyBrZWVwIGl0IGxpa2UgdGhhdC4KX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AF843C43381 for ; Tue, 12 Mar 2019 15:52:57 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 800E520693 for ; Tue, 12 Mar 2019 15:52:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="W7kR8EPx" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 800E520693 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tHRZPDj4eMlzJ+pO+TLE4AkwA/JMUbQFTUu6zPbbgeU=; b=W7kR8EPxCK29/D R+JaIvBhXWdLEOeaJc+r3quMZH+PMPflx3LwdecUNKmHCpUZ7Af5jgmigxP8Al3Yl3rMTrgHCX8OW qf7Q+8c66iX8r9CqbDDD4UQCj8gH27CBb6xktp+xEjbQK+ta+4zXNtOcz6wrhGo50FFmFHeVCfbms vPOTZAQ0jA+TpTHP1J16KsgJ2A2dQWfgP0G0YroQS6JHhui7buyi5EgUrBv6mz4KsRDbfp742OevB DILR1ByC0662Gxrclv23d6N07ycLnmZ7MUu2rpQDjypnjEqGSFS3ptfGlFdfORevj5N5ORj/RRAto kN4HO7lKkDjxPU2h2lAQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3ji0-0001CD-AN; Tue, 12 Mar 2019 15:52:52 +0000 Received: from bhuna.collabora.co.uk ([46.235.227.227]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3jhw-0001Bl-UL; Tue, 12 Mar 2019 15:52:50 +0000 Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 84B2A26D7C8; Tue, 12 Mar 2019 15:52:46 +0000 (GMT) Date: Tue, 12 Mar 2019 16:52:43 +0100 From: Boris Brezillon To: Helen Koike Subject: Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update Message-ID: <20190312165243.5b771e4a@collabora.com> In-Reply-To: References: <20190312022204.2775-1-helen.koike@collabora.com> <20190312022204.2775-2-helen.koike@collabora.com> <20190312073438.05ad8173@collabora.com> Organization: Collabora X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190312_085249_233830_CAABBE0E X-CRM114-Status: GOOD ( 28.53 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: andrey.grodzovsky@amd.com, =?UTF-8?B?U3TDqXBoYW5l?= Marchesin , Heiko =?UTF-8?B?U3TDvGJuZXI=?= , Sean Paul , David Airlie , daniel.vetter@ffwll.ch, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Tomasz Figa , linux-rockchip@lists.infradead.org, Sandy Huang , Daniel Vetter , kernel@collabora.com, harry.wentland@amd.com, nicholas.kazlauskas@amd.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 12 Mar 2019 12:34:45 -0300 Helen Koike wrote: > On 3/12/19 3:34 AM, Boris Brezillon wrote: > > On Mon, 11 Mar 2019 23:21:59 -0300 > > Helen Koike wrote: > > > >> In the case of async update, modifications are done in place, i.e. in the > >> current plane state, so the new_state is prepared and the new_state is > >> cleanup up (instead of the old_state, diferrently on what happen in a > > > > ^ cleaned up ^ differently (but maybe > > "unlike what happens" is more appropriate here). > > > >> normal sync update). > >> To cleanup the old_fb properly, it needs to be placed in the new_state > >> in the end of async_update, so cleanup call will unreference the old_fb > >> correctly. > >> > >> Also, the previous code had a: > >> > >> plane_state = plane->funcs->atomic_duplicate_state(plane); > >> ... > >> swap(plane_state, plane->state); > >> > >> if (plane->state->fb && plane->state->fb != new_state->fb) { > >> ... > >> } > >> > >> Which was wrong, as the fb were just assigned to be equal, so this if > >> statement nevers evaluates to true. > >> > >> Another details is that the function drm_crtc_vblank_get() can only be > >> called when vop->is_enabled is true, otherwise it has no effect and > >> trows a WARN_ON(). > >> > >> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new > >> fb and pus the old fb) is not required, as it is taken care by > >> drm_mode_cursor_universal() when calling > >> drm_atomic_helper_update_plane(). > >> > >> Signed-off-by: Helen Koike > >> > >> --- > >> Hello, > >> > >> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and > >> kms_cursor_legacy and I didn't see any regressions. > >> > >> Changes in v2: None > >> > >> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++--------- > >> 1 file changed, 24 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > >> index c7d4c6073ea5..a1ee8c156a7b 100644 > >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > >> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane, > >> struct drm_plane_state *new_state) > >> { > >> struct vop *vop = to_vop(plane->state->crtc); > >> - struct drm_plane_state *plane_state; > >> + struct drm_framebuffer *old_fb = plane->state->fb; > >> > >> - plane_state = plane->funcs->atomic_duplicate_state(plane); > >> - plane_state->crtc_x = new_state->crtc_x; > >> - plane_state->crtc_y = new_state->crtc_y; > >> - plane_state->crtc_h = new_state->crtc_h; > >> - plane_state->crtc_w = new_state->crtc_w; > >> - plane_state->src_x = new_state->src_x; > >> - plane_state->src_y = new_state->src_y; > >> - plane_state->src_h = new_state->src_h; > >> - plane_state->src_w = new_state->src_w; > >> - > >> - if (plane_state->fb != new_state->fb) > >> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb); > >> - > >> - swap(plane_state, plane->state); > >> - > >> - if (plane->state->fb && plane->state->fb != new_state->fb) { > >> + /* > >> + * A scanout can still be occurring, so we can't drop the reference to > >> + * the old framebuffer. To solve this we get a reference to old_fb and > >> + * set a worker to release it later. > > > > Hm, doesn't look like an async update to me if we have to wait for the > > next VBLANK to happen to get the new content on the screen. Maybe we > > should reject async updates when old_fb != new_fb in the rk > > ->async_check() hook. > > Unless I am misunderstanding this, we don't wait here, we just grab a > reference to the fb in case it is being still used by the hw, so it > doesn't get released prematurely. I was just reacting to the comment that says the new FB should stay around until the next VBLANK event happens. If the FB must stay around that probably means the HW is still using, which made me wonder if this HW actually supports async update (where async means "update now and don't care about about tearing"). Or maybe it takes some time to switch to the new FB and waiting for the next VBLANK to release the old FB was an easy solution to not wait for the flip to actually happen in ->async_update() (which is kind of a combination of async+non-blocking). Anyway, let's keep it like that. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 98174C43381 for ; Tue, 12 Mar 2019 15:52:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7336D20693 for ; Tue, 12 Mar 2019 15:52:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726728AbfCLPwv (ORCPT ); Tue, 12 Mar 2019 11:52:51 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:60194 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725917AbfCLPwu (ORCPT ); Tue, 12 Mar 2019 11:52:50 -0400 Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 84B2A26D7C8; Tue, 12 Mar 2019 15:52:46 +0000 (GMT) Date: Tue, 12 Mar 2019 16:52:43 +0100 From: Boris Brezillon To: Helen Koike Cc: dri-devel@lists.freedesktop.org, nicholas.kazlauskas@amd.com, andrey.grodzovsky@amd.com, daniel.vetter@ffwll.ch, linux-kernel@vger.kernel.org, Tomasz Figa , David Airlie , Sean Paul , kernel@collabora.com, harry.wentland@amd.com, =?UTF-8?B?U3TDqXBoYW5l?= Marchesin , Sandy Huang , linux-rockchip@lists.infradead.org, Heiko =?UTF-8?B?U3TDvGJuZXI=?= , linux-arm-kernel@lists.infradead.org, Daniel Vetter Subject: Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update Message-ID: <20190312165243.5b771e4a@collabora.com> In-Reply-To: References: <20190312022204.2775-1-helen.koike@collabora.com> <20190312022204.2775-2-helen.koike@collabora.com> <20190312073438.05ad8173@collabora.com> Organization: Collabora X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 12 Mar 2019 12:34:45 -0300 Helen Koike wrote: > On 3/12/19 3:34 AM, Boris Brezillon wrote: > > On Mon, 11 Mar 2019 23:21:59 -0300 > > Helen Koike wrote: > > > >> In the case of async update, modifications are done in place, i.e. in the > >> current plane state, so the new_state is prepared and the new_state is > >> cleanup up (instead of the old_state, diferrently on what happen in a > > > > ^ cleaned up ^ differently (but maybe > > "unlike what happens" is more appropriate here). > > > >> normal sync update). > >> To cleanup the old_fb properly, it needs to be placed in the new_state > >> in the end of async_update, so cleanup call will unreference the old_fb > >> correctly. > >> > >> Also, the previous code had a: > >> > >> plane_state = plane->funcs->atomic_duplicate_state(plane); > >> ... > >> swap(plane_state, plane->state); > >> > >> if (plane->state->fb && plane->state->fb != new_state->fb) { > >> ... > >> } > >> > >> Which was wrong, as the fb were just assigned to be equal, so this if > >> statement nevers evaluates to true. > >> > >> Another details is that the function drm_crtc_vblank_get() can only be > >> called when vop->is_enabled is true, otherwise it has no effect and > >> trows a WARN_ON(). > >> > >> Calling drm_atomic_set_fb_for_plane() (which get a referent of the new > >> fb and pus the old fb) is not required, as it is taken care by > >> drm_mode_cursor_universal() when calling > >> drm_atomic_helper_update_plane(). > >> > >> Signed-off-by: Helen Koike > >> > >> --- > >> Hello, > >> > >> I tested on the rockchip ficus v1.1 using igt plane_cursor_legacy and > >> kms_cursor_legacy and I didn't see any regressions. > >> > >> Changes in v2: None > >> > >> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 42 ++++++++++++--------- > >> 1 file changed, 24 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > >> index c7d4c6073ea5..a1ee8c156a7b 100644 > >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > >> @@ -912,30 +912,31 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane, > >> struct drm_plane_state *new_state) > >> { > >> struct vop *vop = to_vop(plane->state->crtc); > >> - struct drm_plane_state *plane_state; > >> + struct drm_framebuffer *old_fb = plane->state->fb; > >> > >> - plane_state = plane->funcs->atomic_duplicate_state(plane); > >> - plane_state->crtc_x = new_state->crtc_x; > >> - plane_state->crtc_y = new_state->crtc_y; > >> - plane_state->crtc_h = new_state->crtc_h; > >> - plane_state->crtc_w = new_state->crtc_w; > >> - plane_state->src_x = new_state->src_x; > >> - plane_state->src_y = new_state->src_y; > >> - plane_state->src_h = new_state->src_h; > >> - plane_state->src_w = new_state->src_w; > >> - > >> - if (plane_state->fb != new_state->fb) > >> - drm_atomic_set_fb_for_plane(plane_state, new_state->fb); > >> - > >> - swap(plane_state, plane->state); > >> - > >> - if (plane->state->fb && plane->state->fb != new_state->fb) { > >> + /* > >> + * A scanout can still be occurring, so we can't drop the reference to > >> + * the old framebuffer. To solve this we get a reference to old_fb and > >> + * set a worker to release it later. > > > > Hm, doesn't look like an async update to me if we have to wait for the > > next VBLANK to happen to get the new content on the screen. Maybe we > > should reject async updates when old_fb != new_fb in the rk > > ->async_check() hook. > > Unless I am misunderstanding this, we don't wait here, we just grab a > reference to the fb in case it is being still used by the hw, so it > doesn't get released prematurely. I was just reacting to the comment that says the new FB should stay around until the next VBLANK event happens. If the FB must stay around that probably means the HW is still using, which made me wonder if this HW actually supports async update (where async means "update now and don't care about about tearing"). Or maybe it takes some time to switch to the new FB and waiting for the next VBLANK to release the old FB was an easy solution to not wait for the flip to actually happen in ->async_update() (which is kind of a combination of async+non-blocking). Anyway, let's keep it like that.