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=-8.5 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,USER_AGENT_MUTT 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 5F453C43381 for ; Tue, 12 Mar 2019 11:05:14 +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 2E057206DF for ; Tue, 12 Mar 2019 11:05:14 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="FnLgtS1B"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="UBN60xhe" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2E057206DF Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch 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:In-Reply-To:MIME-Version:References: 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=6wIIdxovPf22PO8K1GOivbUsNhZWBUR5uyEhzYxWeYE=; b=FnLgtS1BkbXxOX g8L8BCYZpYiZYZ5DBMd7cqf3MgQG1c3Y5wedpVdrOHe/X07gd9eZC+nEs8lww1iZSJpzFVfIpvOxb +9YgQSWmgUh9hpKLClAgiA8F7DflRocal9BhKDFTbabK39jK3HDW1TZwz478TLgdFp7zarSbMQ26D c+r+EO9YFQ4NDnycUNPCg2iSWKbSBYYKwo0aWN9tPCnHUWoSIVTCJVlY3pUFDGfH0JxPf5iDJfgqm Zs9j23+/05wsBe6h8du6HaW0wDaF74xTX1e6/6zb+qMcJg19Fof5CTOVqSTZxq1tJDuH8zq5kXUh9 RfPONSUh8Kcgzl+DtMWg==; 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 1h3fDW-0002Bn-2G; Tue, 12 Mar 2019 11:05:06 +0000 Received: from mail-ed1-x543.google.com ([2a00:1450:4864:20::543]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h3fDQ-0001kD-8r for linux-arm-kernel@lists.infradead.org; Tue, 12 Mar 2019 11:05:02 +0000 Received: by mail-ed1-x543.google.com with SMTP id n17so1860372edt.13 for ; Tue, 12 Mar 2019 04:04:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=sender:date:from:to:cc:subject:message-id:mail-followup-to :references:mime-version:content-disposition:in-reply-to:user-agent; bh=DO+BpYD1HiVHvDw8PFyEy3WyNntjkNlUp9cufIP9CE8=; b=UBN60xheZLGXdcEwKYd7AfH1LPNA4cXSgiEdcc/YEW5Aa4WQXCFsuaSFOBxEqOh24h xNCzDwux9UdLr9cLp6uVjkajkMRZbxKXqY5yNq17QXH0ILrB/npQpji2v54CWsfcO9ao omvxLWiPy6WJJ2TzR49ykqmGn5zn3JLDRE5Xo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to:user-agent; bh=DO+BpYD1HiVHvDw8PFyEy3WyNntjkNlUp9cufIP9CE8=; b=e4Z3NAi/3N06Kd9gtFm9F/n2AquNbaJYW+rygAzGgPUtNJbodGO38V2nRiRuuYGcxk UPmWAWkO6SeTHLSQFcl5U0qy8x1VgIkkqv02AJ8vNpC03+fM8zxg6AmSBFV5+7/3zTXs g+2DaU6i7nydAPSEo1lGn+3LUpbo3Oic9hBGDgF4fmQMbuq+U+backKk6O6NaElqciFS A50eXExvm1lpQM+hnjFbnNHmwGAybTDn/gZg5gjYPpji4LoSghk1jT3/oAmax1y0fYHd bBvDSLA/wl9C8nuVTu3UHYmJI+PBT+U0pWj2X9TlEWxdGAjCzXwGb4SRLtA5BvYK82PM WR7g== X-Gm-Message-State: APjAAAVdqiYCP8mEcwUeP/sBlObe6BfC1kziaW0H9T3sh5J6gZsM4ib5 nNO8bsgZquddOVSQ/uR6IxO23Q== X-Google-Smtp-Source: APXvYqy+pzte6vBAu5XHlWTNNy4WrR29lu+XoZ7GRxem0CgH3XtdQniMzMmA4Wd9gCKFGkNPJGdDcA== X-Received: by 2002:aa7:d987:: with SMTP id u7mr2885409eds.194.1552388696744; Tue, 12 Mar 2019 04:04:56 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:569e:0:3106:d637:d723:e855]) by smtp.gmail.com with ESMTPSA id 31sm5367324edr.7.2019.03.12.04.04.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 12 Mar 2019 04:04:55 -0700 (PDT) Date: Tue, 12 Mar 2019 12:04:53 +0100 From: Daniel Vetter To: Boris Brezillon Subject: Re: [PATCH v2 1/5] drm/rockchip: fix fb references in async update Message-ID: <20190312110453.GB2665@phenom.ffwll.local> Mail-Followup-To: Boris Brezillon , Helen Koike , dri-devel@lists.freedesktop.org, nicholas.kazlauskas@amd.com, andrey.grodzovsky@amd.com, linux-kernel@vger.kernel.org, Tomasz Figa , David Airlie , Sean Paul , kernel@collabora.com, harry.wentland@amd.com, =?iso-8859-1?Q?St=E9phane?= Marchesin , Sandy Huang , linux-rockchip@lists.infradead.org, Heiko =?iso-8859-1?Q?St=FCbner?= , linux-arm-kernel@lists.infradead.org References: <20190312022204.2775-1-helen.koike@collabora.com> <20190312022204.2775-2-helen.koike@collabora.com> <20190312073438.05ad8173@collabora.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190312073438.05ad8173@collabora.com> X-Operating-System: Linux phenom 4.19.0-1-amd64 User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190312_040500_311212_42270C45 X-CRM114-Status: GOOD ( 33.82 ) 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, =?iso-8859-1?Q?St=E9phane?= Marchesin , Heiko =?iso-8859-1?Q?St=FCbner?= , 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, Helen Koike , 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, Mar 12, 2019 at 07:34:38AM +0100, 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. Scanning out garbage because the old buffer is unpinned too quickly is one of the long-term "features" of async updates. At least for features. It's another one of these things we need to fix. Which might become easier if we switch to usual state switching, since then we can punt the cleanup_plane phase to a worker. Note that depending upon the gpu this might not just result in garbage but hangs, usually when there's an iommu and the chip dies if it accesses an unmapped page. Probably something to fix later on in async framework. -Daniel > > > + */ > > + if (vop->is_enabled && > > + plane->state->fb && plane->state->fb != new_state->fb) { > > drm_framebuffer_get(plane->state->fb); > > WARN_ON(drm_crtc_vblank_get(plane->state->crtc) != 0); > > drm_flip_work_queue(&vop->fb_unref_work, plane->state->fb); > > set_bit(VOP_PENDING_FB_UNREF, &vop->pending); > > } > > In any case, I think this should be called after > vop_plane_atomic_update() to prevent the situation where the VBLANK > event happens between this point and the following > vop_plane_atomic_update() call. > > > > > + 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; > > + plane->state->fb = new_state->fb; > > Any reason not to use swap() here and reference plane->state->fb > instead of new_state->fb after this point? > > > + > > if (vop->is_enabled) { > > rockchip_drm_psr_inhibit_get_state(new_state->state); > > vop_plane_atomic_update(plane, plane->state); > > @@ -945,7 +946,12 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane, > > rockchip_drm_psr_inhibit_put_state(new_state->state); > > } > > > > - plane->funcs->atomic_destroy_state(plane, plane_state); > > + /* > > + * In async update we perform inplace modifications and release the > > + * new_state. The following is required so we release the reference of > > + * the old framebuffer. > > + */ > > + new_state->fb = old_fb; > > } > > > > static const struct drm_plane_helper_funcs plane_helper_funcs = { > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel