From patchwork Mon Feb 13 09:38:14 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Russell King (Oracle)" X-Patchwork-Id: 9569499 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D1BCB60572 for ; Mon, 13 Feb 2017 12:53:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A5B9F237A5 for ; Mon, 13 Feb 2017 12:53:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9841F27D0E; Mon, 13 Feb 2017 12:53:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 35C0A237A5 for ; Mon, 13 Feb 2017 12:53:34 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E5D306E3CB; Mon, 13 Feb 2017 12:53:05 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by gabe.freedesktop.org (Postfix) with ESMTPS id C9FD46E35D for ; Mon, 13 Feb 2017 09:38:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2014; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=vXCNL8gD/mh0euMp2aBGmOywnSxG0NIm5fu1Escfmvo=; b=YKsprf8dx2Mr5trUOxke4k/S+RO3/mebD4TyQ84BT1U65ApkUrijB+2YII4BN33yVJYs2ppU1Wvc7RL5bVCqdpiMJK/FnXj961coVsmHhqdY1g4Xdo3Q3VaEwsNEf9SNyMSBiu2ByirRu69SdkKSH34m06RE9eBQJ1lggCdPmZ0=; Received: from n2100.armlinux.org.uk ([2001:4d48:ad52:3201:214:fdff:fe10:4f86]:44980) by pandora.armlinux.org.uk with esmtpsa (TLSv1:DHE-RSA-AES256-SHA:256) (Exim 4.82_1-5b7a7c0-XX) (envelope-from ) id 1cdD5O-0001eF-2z; Mon, 13 Feb 2017 09:38:18 +0000 Received: from linux by n2100.armlinux.org.uk with local (Exim 4.76) (envelope-from ) id 1cdD5L-0001AH-5w; Mon, 13 Feb 2017 09:38:15 +0000 Date: Mon, 13 Feb 2017 09:38:14 +0000 From: Russell King - ARM Linux To: Chris Wilson Subject: Re: imxdrm issue on SABRE Lite Message-ID: <20170213093814.GW27312@n2100.armlinux.org.uk> References: <20170212001546.GR27312@n2100.armlinux.org.uk> <20170213080533.GC13451@ulmo.ba.sec> <20170213085553.GE14326@nuc-i3427.alporthouse.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170213085553.GE14326@nuc-i3427.alporthouse.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Mailman-Approved-At: Mon, 13 Feb 2017 12:52:59 +0000 Cc: dri-devel@lists.freedesktop.org, Dan MacDonald X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Feb 13, 2017 at 08:55:53AM +0000, Chris Wilson wrote: > On Mon, Feb 13, 2017 at 09:05:33AM +0100, Thierry Reding wrote: > > On Sun, Feb 12, 2017 at 12:15:46AM +0000, Russell King - ARM Linux wrote: > > > On Sat, Feb 11, 2017 at 09:09:34PM +0000, Dan MacDonald wrote: > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > index 21f992605541..46668d071d6a 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -1201,9 +1201,8 @@ static void commit_tail(struct drm_atomic_state *state) > > > else > > > drm_atomic_helper_commit_tail(state); > > > > > > - drm_atomic_helper_commit_cleanup_done(state); > > > - > > > - drm_atomic_state_free(state); > > > + if (drm_atomic_helper_commit_cleanup_done(state) == 0) > > > + drm_atomic_state_free(state); > > > > Chris (Cc'ed) added reference counting to atomic state for v4.10, maybe > > that already fixes the issue? > > I'm not confident it will, as there is not an independent ref on the > state for the phases, and so a forced timeout still leaves a dangling > pointer. The above chunk goes the opposite way and leaks the state to > avoid the invalid deref, what we need is a ref around its existence on > the dependency queue if that is outside the lifetime of the commit. I said as much in my email - unfortunately, Thierry cut all that context. Right now, we oops the kernel, which causes: (a) the death of the calling process (b) leaking of all memory associated with the modeset What I'm proposing for the -stable kernels is to _improve_ the situation by eliminating part of the problem, so it's possible to get a better idea of which bit went wrong and which outputs have failed. Fixing it properly is likely to be very invasive, since you'll need to add reference counting to the drm_crtc_commit structure, a pointer to that in the drm_pending_event structure, and ensure that the reference count gets incremented at the appropriate time. Incrementing the reference count in drm_atomic_helper_setup_commit() certainly isn't the right place, that would be at the sites which queue the event, but they are scattered amongst all the atomic modeset drivers. For reference, here's my complete patch I posted yesterday: drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++------ include/drm/drm_atomic_helper.h | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 21f992605541..46668d071d6a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1201,9 +1201,8 @@ static void commit_tail(struct drm_atomic_state *state) else drm_atomic_helper_commit_tail(state); - drm_atomic_helper_commit_cleanup_done(state); - - drm_atomic_state_free(state); + if (drm_atomic_helper_commit_cleanup_done(state) == 0) + drm_atomic_state_free(state); } static void commit_work(struct work_struct *work) @@ -1591,12 +1590,12 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); * This is part of the atomic helper support for nonblocking commits, see * drm_atomic_helper_setup_commit() for an overview. */ -void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state) +int drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state) { struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_crtc_commit *commit; - int i; + int i, failed = 0; long ret; for_each_crtc_in_state(state, crtc, crtc_state, i) { @@ -1621,15 +1620,19 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state) * not hold a reference of its own. */ ret = wait_for_completion_timeout(&commit->flip_done, 10*HZ); - if (ret == 0) - DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", + if (ret == 0) { + DRM_ERROR("[CRTC:%d:%s] flip_done timed out, memory leaked\n", crtc->base.id, crtc->name); + failed = -ETIMEDOUT; + } spin_lock(&crtc->commit_lock); del_commit: list_del(&commit->commit_entry); spin_unlock(&crtc->commit_lock); } + + return failed; } EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 7ff92b09fd9c..ee3d642c1feb 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -88,7 +88,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); -void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state); +int drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state); /* implementations for legacy interfaces */ int drm_atomic_helper_update_plane(struct drm_plane *plane,