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.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED 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 B9B0EC43381 for ; Wed, 13 Mar 2019 03:43:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 76ED9217F5 for ; Wed, 13 Mar 2019 03:43:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="ltDHoa7q" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726869AbfCMDnC (ORCPT ); Tue, 12 Mar 2019 23:43:02 -0400 Received: from mail-vk1-f196.google.com ([209.85.221.196]:45387 "EHLO mail-vk1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726849AbfCMDnB (ORCPT ); Tue, 12 Mar 2019 23:43:01 -0400 Received: by mail-vk1-f196.google.com with SMTP id v187so132304vkf.12 for ; Tue, 12 Mar 2019 20:43:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=H0dIcw6m1Pzr0Ps1GAlM43xbFMS3sCb4y/K1DlTFxi4=; b=ltDHoa7qX/+p2IbyixR+qVL8X/IQ5+LXGIEF7HHMo6Zz/2CRXlG1orsfb4AVbh+E96 JxQYs7ZAzwWzmKWjmpoPPT7TqC9D/EydtrF00rpPjYQXNhinOg3hI2eMCaRHmp+ARDq2 ilu+vIqgk1etAFlwHihsw4fGkiaLwNUcuDXpg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=H0dIcw6m1Pzr0Ps1GAlM43xbFMS3sCb4y/K1DlTFxi4=; b=eb81H0GkK4wsTtIGh6+NtXrbZiZ8/RxmfQ1N69uSROZGIkkJf33EE1pTKQuxUEPHYw 5VSkwnJFBOK5uhKLoV1KUgMalZiGPGAdbCVnGct7QZbYjyKOGcpRWTQw3joRFFLEitzM M+iqwSiSQlbFiN8Vke2qYLSKnwj08Zd8FbDgCAtAGv6uZ9YMgzEqR74tyPsiM/tSqeXC z0MXJ8lBtQQIvN+f2CfhX/ImL1NHecjfS5G90JD7WRaJ9+bcYlCqI3WY1105gfLBcXs7 XUk0r8dC+Jlrr+jMd5Rmugnb2cpmNWHxyMatwFHdcpyXkcZC+e8gCJ09UyV+0+qoe8MV 0JuQ== X-Gm-Message-State: APjAAAWi3be+AlliiPV8ZtNqcpV9QJWS9LhHg7eeUtWgfjzzxgb1e9L/ tR4Uy+Dz3Hb49maa8pVQ0LUYQeLNVfc= X-Google-Smtp-Source: APXvYqwPDT/1oTY2llVg/4dhBaGPXUzvtiG8Ue+QIrQNRC0St2Z+ZayLCVyA+/k1FwQU6mreifoUjg== X-Received: by 2002:a1f:a0d7:: with SMTP id j206mr21466165vke.37.1552448579951; Tue, 12 Mar 2019 20:42:59 -0700 (PDT) Received: from mail-vs1-f43.google.com (mail-vs1-f43.google.com. [209.85.217.43]) by smtp.gmail.com with ESMTPSA id l12sm65909uao.15.2019.03.12.20.42.56 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 12 Mar 2019 20:42:57 -0700 (PDT) Received: by mail-vs1-f43.google.com with SMTP id c189so191491vsd.9 for ; Tue, 12 Mar 2019 20:42:56 -0700 (PDT) X-Received: by 2002:a67:f744:: with SMTP id w4mr22501451vso.16.1552448576321; Tue, 12 Mar 2019 20:42:56 -0700 (PDT) MIME-Version: 1.0 References: <20190312022204.2775-1-helen.koike@collabora.com> <20190312022204.2775-2-helen.koike@collabora.com> <20190312073438.05ad8173@collabora.com> <20190312165243.5b771e4a@collabora.com> In-Reply-To: <20190312165243.5b771e4a@collabora.com> From: Tomasz Figa Date: Wed, 13 Mar 2019 12:42:45 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update To: Boris Brezillon Cc: Helen Koike , dri-devel , nicholas.kazlauskas@amd.com, andrey.grodzovsky@amd.com, Daniel Vetter , Linux Kernel Mailing List , David Airlie , Sean Paul , kernel@collabora.com, harry.wentland@amd.com, =?UTF-8?Q?St=C3=A9phane_Marchesin?= , Sandy Huang , "open list:ARM/Rockchip SoC..." , =?UTF-8?Q?Heiko_St=C3=BCbner?= , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , Daniel Vetter Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 13, 2019 at 12:52 AM Boris Brezillon wrote: > > 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). The hardware switches framebuffers on vblank, so whatever framebuffer is currently being scanned out from needs to stay there until the hardware switches to the new one in shadow registers. If that doesn't happen, you get IOMMU faults and the display controller stops working since we don't have any fault handling currently, just printing a message. Best regards, Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update Date: Wed, 13 Mar 2019 12:42:45 +0900 Message-ID: References: <20190312022204.2775-1-helen.koike@collabora.com> <20190312022204.2775-2-helen.koike@collabora.com> <20190312073438.05ad8173@collabora.com> <20190312165243.5b771e4a@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20190312165243.5b771e4a@collabora.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Boris Brezillon Cc: =?UTF-8?Q?St=C3=A9phane_Marchesin?= , Sean Paul , David Airlie , Daniel Vetter , Linux Kernel Mailing List , dri-devel , "open list:ARM/Rockchip SoC..." , Helen Koike , kernel@collabora.com, nicholas.kazlauskas@amd.com, "list@263.net:IOMMU DRIVERS , Joerg Roedel , " List-Id: linux-rockchip.vger.kernel.org T24gV2VkLCBNYXIgMTMsIDIwMTkgYXQgMTI6NTIgQU0gQm9yaXMgQnJlemlsbG9uCjxib3Jpcy5i cmV6aWxsb25AY29sbGFib3JhLmNvbT4gd3JvdGU6Cj4KPiBPbiBUdWUsIDEyIE1hciAyMDE5IDEy OjM0OjQ1IC0wMzAwCj4gSGVsZW4gS29pa2UgPGhlbGVuLmtvaWtlQGNvbGxhYm9yYS5jb20+IHdy b3RlOgo+Cj4gPiBPbiAzLzEyLzE5IDM6MzQgQU0sIEJvcmlzIEJyZXppbGxvbiB3cm90ZToKPiA+ ID4gT24gTW9uLCAxMSBNYXIgMjAxOSAyMzoyMTo1OSAtMDMwMAo+ID4gPiBIZWxlbiBLb2lrZSA8 aGVsZW4ua29pa2VAY29sbGFib3JhLmNvbT4gd3JvdGU6Cj4gPiA+Cj4gPiA+PiBJbiB0aGUgY2Fz ZSBvZiBhc3luYyB1cGRhdGUsIG1vZGlmaWNhdGlvbnMgYXJlIGRvbmUgaW4gcGxhY2UsIGkuZS4g aW4gdGhlCj4gPiA+PiBjdXJyZW50IHBsYW5lIHN0YXRlLCBzbyB0aGUgbmV3X3N0YXRlIGlzIHBy ZXBhcmVkIGFuZCB0aGUgbmV3X3N0YXRlIGlzCj4gPiA+PiBjbGVhbnVwIHVwIChpbnN0ZWFkIG9m IHRoZSBvbGRfc3RhdGUsIGRpZmVycmVudGx5IG9uIHdoYXQgaGFwcGVuIGluIGEKPiA+ID4KPiA+ ID4gICBeIGNsZWFuZWQgdXAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBeIGRpZmZlcmVu dGx5IChidXQgbWF5YmUKPiA+ID4gInVubGlrZSB3aGF0IGhhcHBlbnMiIGlzIG1vcmUgYXBwcm9w cmlhdGUgaGVyZSkuCj4gPiA+Cj4gPiA+PiBub3JtYWwgc3luYyB1cGRhdGUpLgo+ID4gPj4gVG8g Y2xlYW51cCB0aGUgb2xkX2ZiIHByb3Blcmx5LCBpdCBuZWVkcyB0byBiZSBwbGFjZWQgaW4gdGhl IG5ld19zdGF0ZQo+ID4gPj4gaW4gdGhlIGVuZCBvZiBhc3luY191cGRhdGUsIHNvIGNsZWFudXAg Y2FsbCB3aWxsIHVucmVmZXJlbmNlIHRoZSBvbGRfZmIKPiA+ID4+IGNvcnJlY3RseS4KPiA+ID4+ Cj4gPiA+PiBBbHNvLCB0aGUgcHJldmlvdXMgY29kZSBoYWQgYToKPiA+ID4+Cj4gPiA+PiAgICBw bGFuZV9zdGF0ZSA9IHBsYW5lLT5mdW5jcy0+YXRvbWljX2R1cGxpY2F0ZV9zdGF0ZShwbGFuZSk7 Cj4gPiA+PiAgICAuLi4KPiA+ID4+ICAgIHN3YXAocGxhbmVfc3RhdGUsIHBsYW5lLT5zdGF0ZSk7 Cj4gPiA+Pgo+ID4gPj4gICAgaWYgKHBsYW5lLT5zdGF0ZS0+ZmIgJiYgcGxhbmUtPnN0YXRlLT5m YiAhPSBuZXdfc3RhdGUtPmZiKSB7Cj4gPiA+PiAgICAuLi4KPiA+ID4+ICAgIH0KPiA+ID4+Cj4g PiA+PiBXaGljaCB3YXMgd3JvbmcsIGFzIHRoZSBmYiB3ZXJlIGp1c3QgYXNzaWduZWQgdG8gYmUg ZXF1YWwsIHNvIHRoaXMgaWYKPiA+ID4+IHN0YXRlbWVudCBuZXZlcnMgZXZhbHVhdGVzIHRvIHRy dWUuCj4gPiA+Pgo+ID4gPj4gQW5vdGhlciBkZXRhaWxzIGlzIHRoYXQgdGhlIGZ1bmN0aW9uIGRy bV9jcnRjX3ZibGFua19nZXQoKSBjYW4gb25seSBiZQo+ID4gPj4gY2FsbGVkIHdoZW4gdm9wLT5p c19lbmFibGVkIGlzIHRydWUsIG90aGVyd2lzZSBpdCBoYXMgbm8gZWZmZWN0IGFuZAo+ID4gPj4g dHJvd3MgYSBXQVJOX09OKCkuCj4gPiA+Pgo+ID4gPj4gQ2FsbGluZyBkcm1fYXRvbWljX3NldF9m Yl9mb3JfcGxhbmUoKSAod2hpY2ggZ2V0IGEgcmVmZXJlbnQgb2YgdGhlIG5ldwo+ID4gPj4gZmIg YW5kIHB1cyB0aGUgb2xkIGZiKSBpcyBub3QgcmVxdWlyZWQsIGFzIGl0IGlzIHRha2VuIGNhcmUg YnkKPiA+ID4+IGRybV9tb2RlX2N1cnNvcl91bml2ZXJzYWwoKSB3aGVuIGNhbGxpbmcKPiA+ID4+ IGRybV9hdG9taWNfaGVscGVyX3VwZGF0ZV9wbGFuZSgpLgo+ID4gPj4KPiA+ID4+IFNpZ25lZC1v ZmYtYnk6IEhlbGVuIEtvaWtlIDxoZWxlbi5rb2lrZUBjb2xsYWJvcmEuY29tPgo+ID4gPj4KPiA+ ID4+IC0tLQo+ID4gPj4gSGVsbG8sCj4gPiA+Pgo+ID4gPj4gSSB0ZXN0ZWQgb24gdGhlIHJvY2tj aGlwIGZpY3VzIHYxLjEgdXNpbmcgaWd0IHBsYW5lX2N1cnNvcl9sZWdhY3kgYW5kCj4gPiA+PiBr bXNfY3Vyc29yX2xlZ2FjeSBhbmQgSSBkaWRuJ3Qgc2VlIGFueSByZWdyZXNzaW9ucy4KPiA+ID4+ Cj4gPiA+PiBDaGFuZ2VzIGluIHYyOiBOb25lCj4gPiA+Pgo+ID4gPj4gIGRyaXZlcnMvZ3B1L2Ry bS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fdm9wLmMgfCA0MiArKysrKysrKysrKystLS0tLS0tLS0K PiA+ID4+ICAxIGZpbGUgY2hhbmdlZCwgMjQgaW5zZXJ0aW9ucygrKSwgMTggZGVsZXRpb25zKC0p Cj4gPiA+Pgo+ID4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2Nr Y2hpcF9kcm1fdm9wLmMgYi9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX3Zv cC5jCj4gPiA+PiBpbmRleCBjN2Q0YzYwNzNlYTUuLmExZWU4YzE1NmE3YiAxMDA2NDQKPiA+ID4+ IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fdm9wLmMKPiA+ID4+ ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fdm9wLmMKPiA+ID4+ IEBAIC05MTIsMzAgKzkxMiwzMSBAQCBzdGF0aWMgdm9pZCB2b3BfcGxhbmVfYXRvbWljX2FzeW5j X3VwZGF0ZShzdHJ1Y3QgZHJtX3BsYW5lICpwbGFuZSwKPiA+ID4+ICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICBzdHJ1Y3QgZHJtX3BsYW5lX3N0YXRlICpuZXdfc3RhdGUpCj4g PiA+PiAgewo+ID4gPj4gICAgc3RydWN0IHZvcCAqdm9wID0gdG9fdm9wKHBsYW5lLT5zdGF0ZS0+ Y3J0Yyk7Cj4gPiA+PiAtICBzdHJ1Y3QgZHJtX3BsYW5lX3N0YXRlICpwbGFuZV9zdGF0ZTsKPiA+ ID4+ICsgIHN0cnVjdCBkcm1fZnJhbWVidWZmZXIgKm9sZF9mYiA9IHBsYW5lLT5zdGF0ZS0+ZmI7 Cj4gPiA+Pgo+ID4gPj4gLSAgcGxhbmVfc3RhdGUgPSBwbGFuZS0+ZnVuY3MtPmF0b21pY19kdXBs aWNhdGVfc3RhdGUocGxhbmUpOwo+ID4gPj4gLSAgcGxhbmVfc3RhdGUtPmNydGNfeCA9IG5ld19z dGF0ZS0+Y3J0Y194Owo+ID4gPj4gLSAgcGxhbmVfc3RhdGUtPmNydGNfeSA9IG5ld19zdGF0ZS0+ Y3J0Y195Owo+ID4gPj4gLSAgcGxhbmVfc3RhdGUtPmNydGNfaCA9IG5ld19zdGF0ZS0+Y3J0Y19o Owo+ID4gPj4gLSAgcGxhbmVfc3RhdGUtPmNydGNfdyA9IG5ld19zdGF0ZS0+Y3J0Y193Owo+ID4g Pj4gLSAgcGxhbmVfc3RhdGUtPnNyY194ID0gbmV3X3N0YXRlLT5zcmNfeDsKPiA+ID4+IC0gIHBs YW5lX3N0YXRlLT5zcmNfeSA9IG5ld19zdGF0ZS0+c3JjX3k7Cj4gPiA+PiAtICBwbGFuZV9zdGF0 ZS0+c3JjX2ggPSBuZXdfc3RhdGUtPnNyY19oOwo+ID4gPj4gLSAgcGxhbmVfc3RhdGUtPnNyY193 ID0gbmV3X3N0YXRlLT5zcmNfdzsKPiA+ID4+IC0KPiA+ID4+IC0gIGlmIChwbGFuZV9zdGF0ZS0+ ZmIgIT0gbmV3X3N0YXRlLT5mYikKPiA+ID4+IC0gICAgICAgICAgZHJtX2F0b21pY19zZXRfZmJf Zm9yX3BsYW5lKHBsYW5lX3N0YXRlLCBuZXdfc3RhdGUtPmZiKTsKPiA+ID4+IC0KPiA+ID4+IC0g IHN3YXAocGxhbmVfc3RhdGUsIHBsYW5lLT5zdGF0ZSk7Cj4gPiA+PiAtCj4gPiA+PiAtICBpZiAo cGxhbmUtPnN0YXRlLT5mYiAmJiBwbGFuZS0+c3RhdGUtPmZiICE9IG5ld19zdGF0ZS0+ZmIpIHsK PiA+ID4+ICsgIC8qCj4gPiA+PiArICAgKiBBIHNjYW5vdXQgY2FuIHN0aWxsIGJlIG9jY3Vycmlu Zywgc28gd2UgY2FuJ3QgZHJvcCB0aGUgcmVmZXJlbmNlIHRvCj4gPiA+PiArICAgKiB0aGUgb2xk IGZyYW1lYnVmZmVyLiBUbyBzb2x2ZSB0aGlzIHdlIGdldCBhIHJlZmVyZW5jZSB0byBvbGRfZmIg YW5kCj4gPiA+PiArICAgKiBzZXQgYSB3b3JrZXIgdG8gcmVsZWFzZSBpdCBsYXRlci4KPiA+ID4K PiA+ID4gSG0sIGRvZXNuJ3QgbG9vayBsaWtlIGFuIGFzeW5jIHVwZGF0ZSB0byBtZSBpZiB3ZSBo YXZlIHRvIHdhaXQgZm9yIHRoZQo+ID4gPiBuZXh0IFZCTEFOSyB0byBoYXBwZW4gdG8gZ2V0IHRo ZSBuZXcgY29udGVudCBvbiB0aGUgc2NyZWVuLiBNYXliZSB3ZQo+ID4gPiBzaG91bGQgcmVqZWN0 IGFzeW5jIHVwZGF0ZXMgd2hlbiBvbGRfZmIgIT0gbmV3X2ZiIGluIHRoZSByawo+ID4gPiAtPmFz eW5jX2NoZWNrKCkgaG9vay4KPiA+Cj4gPiBVbmxlc3MgSSBhbSBtaXN1bmRlcnN0YW5kaW5nIHRo aXMsIHdlIGRvbid0IHdhaXQgaGVyZSwgd2UganVzdCBncmFiIGEKPiA+IHJlZmVyZW5jZSB0byB0 aGUgZmIgaW4gY2FzZSBpdCBpcyBiZWluZyBzdGlsbCB1c2VkIGJ5IHRoZSBodywgc28gaXQKPiA+ IGRvZXNuJ3QgZ2V0IHJlbGVhc2VkIHByZW1hdHVyZWx5Lgo+Cj4gSSB3YXMganVzdCByZWFjdGlu ZyB0byB0aGUgY29tbWVudCB0aGF0IHNheXMgdGhlIG5ldyBGQiBzaG91bGQgc3RheQo+IGFyb3Vu ZCB1bnRpbCB0aGUgbmV4dCBWQkxBTksgZXZlbnQgaGFwcGVucy4gSWYgdGhlIEZCIG11c3Qgc3Rh eSBhcm91bmQKPiB0aGF0IHByb2JhYmx5IG1lYW5zIHRoZSBIVyBpcyBzdGlsbCB1c2luZywgd2hp Y2ggbWFkZSBtZSB3b25kZXIgaWYgdGhpcwo+IEhXIGFjdHVhbGx5IHN1cHBvcnRzIGFzeW5jIHVw ZGF0ZSAod2hlcmUgYXN5bmMgbWVhbnMgInVwZGF0ZSBub3cgYW5kCj4gZG9uJ3QgY2FyZSBhYm91 dCBhYm91dCB0ZWFyaW5nIikuIE9yIG1heWJlIGl0IHRha2VzIHNvbWUgdGltZSB0byBzd2l0Y2gK PiB0byB0aGUgbmV3IEZCIGFuZCB3YWl0aW5nIGZvciB0aGUgbmV4dCBWQkxBTksgdG8gcmVsZWFz ZSB0aGUgb2xkIEZCIHdhcwo+IGFuIGVhc3kgc29sdXRpb24gdG8gbm90IHdhaXQgZm9yIHRoZSBm bGlwIHRvIGFjdHVhbGx5IGhhcHBlbiBpbgo+IC0+YXN5bmNfdXBkYXRlKCkgKHdoaWNoIGlzIGtp bmQgb2YgYSBjb21iaW5hdGlvbiBvZiBhc3luYytub24tYmxvY2tpbmcpLgoKVGhlIGhhcmR3YXJl IHN3aXRjaGVzIGZyYW1lYnVmZmVycyBvbiB2YmxhbmssIHNvIHdoYXRldmVyIGZyYW1lYnVmZmVy CmlzIGN1cnJlbnRseSBiZWluZyBzY2FubmVkIG91dCBmcm9tIG5lZWRzIHRvIHN0YXkgdGhlcmUg dW50aWwgdGhlCmhhcmR3YXJlIHN3aXRjaGVzIHRvIHRoZSBuZXcgb25lIGluIHNoYWRvdyByZWdp c3RlcnMuIElmIHRoYXQgZG9lc24ndApoYXBwZW4sIHlvdSBnZXQgSU9NTVUgZmF1bHRzIGFuZCB0 aGUgZGlzcGxheSBjb250cm9sbGVyIHN0b3BzIHdvcmtpbmcKc2luY2Ugd2UgZG9uJ3QgaGF2ZSBh bnkgZmF1bHQgaGFuZGxpbmcgY3VycmVudGx5LCBqdXN0IHByaW50aW5nIGEKbWVzc2FnZS4KCkJl c3QgcmVnYXJkcywKVG9tYXN6Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNr dG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2Ry aS1kZXZlbA== 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,URIBL_BLOCKED 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 4475FC43381 for ; Wed, 13 Mar 2019 11:19:29 +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 108132171F for ; Wed, 13 Mar 2019 11:19:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="cCKNTpOc"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="ltDHoa7q" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 108132171F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org 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:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ZDO+Cb2L7hEFPuhhknIerqrtzRYZT8oA1ab203DfXZU=; b=cCKNTpOcAt17RR SAv0nG5RIVW42D9WlD6G4BzENJfA3czH7iq6UFaMD0ls98tY64fOCc09W6fFAFrntTf8SRRFabZq3 YOrQp6sN1j7Fs1EWssA0VXnhVJuPYUiY4zzVL4SG6kBsRHGk0jEqCl7cipXxfdxgXXrr6zC9+F8Bx nhTlF9FaERKCNdVVf/BS8f6aHNQ5MKscXiu2kisBsQOn+JXYzm89i3MbjkleAizME4njkCVZ+nGAl OnWK59uH97oK+9jLqpH4YFk7WnoFxlbxNbMSZ+3qY+k+ebdr9z8EhalRuXITyQf0+M2dcDU27qLSY WHUDFGoiCVd0oTkanayA==; 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 1h41uq-0000zi-FW; Wed, 13 Mar 2019 11:19:20 +0000 Received: from mail-vk1-xa43.google.com ([2607:f8b0:4864:20::a43]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h41um-0000zB-4p for linux-arm-kernel@lists.infradead.org; Wed, 13 Mar 2019 11:19:17 +0000 Received: by mail-vk1-xa43.google.com with SMTP id b6so366656vkf.8 for ; Wed, 13 Mar 2019 04:19:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=H0dIcw6m1Pzr0Ps1GAlM43xbFMS3sCb4y/K1DlTFxi4=; b=ltDHoa7qX/+p2IbyixR+qVL8X/IQ5+LXGIEF7HHMo6Zz/2CRXlG1orsfb4AVbh+E96 JxQYs7ZAzwWzmKWjmpoPPT7TqC9D/EydtrF00rpPjYQXNhinOg3hI2eMCaRHmp+ARDq2 ilu+vIqgk1etAFlwHihsw4fGkiaLwNUcuDXpg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=H0dIcw6m1Pzr0Ps1GAlM43xbFMS3sCb4y/K1DlTFxi4=; b=aFXPlwo0tk+f34YR8IV35Qb8MkDjW6q+mjdpGT3k7wgvof5fGyVjNpIh5eyWJvA3YW XtCBG6jZTJd2rYVhzjCzbx2N9atpvmI87PROq/mK27kLliI9f65QpVS8cWn1AkqSJT7i Iy++YR9ZOYLVNzOC0Q4sscI/kGYZT6wjMSjlI4ZRy19NKJXtlwgf4L9pWOjzYmRIspJT nd/CGoLCPtr+epfW0CxKcIAOlwZkqHKdf5NeGi12Iwb0vEt8r6Nw6tV6A1uAsWiAMtah c8TDuA6fAzGG428gFZlG3SbEMWEz2Ui5at5TO4cJx8qqgUhZqdKwnil+2fOVBm+eV9pz VTZA== X-Gm-Message-State: APjAAAVRdkI21j838DZbebFDzCHS0rhO2nkHIhY0vmL0TxiWYrfJXQbx gAs/4sQoDXit5wNmoqBaDPec3ETGuGeq/cx1 X-Google-Smtp-Source: APXvYqwXCs/sjrA0UaWMep7fvxj6AZB7+D7qEZebgcv9cmjr8NN9FkX72TyIc0U3W7vD2UFqTUp64Q== X-Received: by 2002:a1f:d007:: with SMTP id h7mr112417vkg.35.1552448898675; Tue, 12 Mar 2019 20:48:18 -0700 (PDT) Received: from mail-vs1-f48.google.com (mail-vs1-f48.google.com. [209.85.217.48]) by smtp.gmail.com with ESMTPSA id u200sm2933127vku.13.2019.03.12.20.48.18 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 12 Mar 2019 20:48:18 -0700 (PDT) Received: by mail-vs1-f48.google.com with SMTP id u6so190792vso.10 for ; Tue, 12 Mar 2019 20:48:18 -0700 (PDT) X-Received: by 2002:a67:f744:: with SMTP id w4mr22501451vso.16.1552448576321; Tue, 12 Mar 2019 20:42:56 -0700 (PDT) MIME-Version: 1.0 References: <20190312022204.2775-1-helen.koike@collabora.com> <20190312022204.2775-2-helen.koike@collabora.com> <20190312073438.05ad8173@collabora.com> <20190312165243.5b771e4a@collabora.com> In-Reply-To: <20190312165243.5b771e4a@collabora.com> From: Tomasz Figa Date: Wed, 13 Mar 2019 12:42:45 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update To: Boris Brezillon X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190313_041916_214688_051DFFDD X-CRM114-Status: GOOD ( 32.26 ) 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?Q?St=C3=A9phane_Marchesin?= , =?UTF-8?Q?Heiko_St=C3=BCbner?= , Sean Paul , David Airlie , Daniel Vetter , Linux Kernel Mailing List , dri-devel , Sandy Huang , "open list:ARM/Rockchip SoC..." , Helen Koike , Daniel Vetter , kernel@collabora.com, harry.wentland@amd.com, nicholas.kazlauskas@amd.com, "list@263.net:IOMMU DRIVERS , Joerg Roedel , " 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 Wed, Mar 13, 2019 at 12:52 AM Boris Brezillon wrote: > > 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). The hardware switches framebuffers on vblank, so whatever framebuffer is currently being scanned out from needs to stay there until the hardware switches to the new one in shadow registers. If that doesn't happen, you get IOMMU faults and the display controller stops working since we don't have any fault handling currently, just printing a message. Best regards, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel