From patchwork Fri Jul 14 22:46:54 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 9841783 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 8F4FF60212 for ; Fri, 14 Jul 2017 22:47:08 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 81680287C0 for ; Fri, 14 Jul 2017 22:47:08 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7403A287B9; Fri, 14 Jul 2017 22:47:08 +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=unavailable 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 34063287B9 for ; Fri, 14 Jul 2017 22:47:08 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6844F6E8BB; Fri, 14 Jul 2017 22:47:06 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id AB0436E8BB for ; Fri, 14 Jul 2017 22:47:04 +0000 (UTC) Received: by mail-wm0-x241.google.com with SMTP id j85so13035476wmj.0 for ; Fri, 14 Jul 2017 15:47:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id; bh=3nyS4GspiCTKD3glULe+oa2dK+9MXf5FR9h6ebnhs0E=; b=XwnE/jRBocofyreVrcEydQ+RzR2VauboiE/AAZlItz9n5LydR9f6lMq4roVnALfgGs FtQBkZLISfO1rGDaJS6ZiLb47sWSBpj8mJgB5Je430mutGBcS+7Sg4uMkI0QuiWvURKP IqGCRBdQ1ULa/CNgp/aCOLAaBTrvL9hEcxfXU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=3nyS4GspiCTKD3glULe+oa2dK+9MXf5FR9h6ebnhs0E=; b=H4WVQkzDWHTZnTebS+utCD3AaOrCu0dYbJ1IwBCUVlOevzydU8j55jdqzMhU1Zti1Z C5lBhv75wXtmbGBk51bMr5UHiGjKMZ/sxfsiKD8dpOB3cWHInMxMaWY4piJsHwRbqIvF T0ixtR6p3QHZM96O+p+5ZuiAXA0IdNuus87xO1del4OaGt9DZcJonZ6Oik6IBzpgnpPF rqKaVKxxBgAsJz7LJwFtgtnYOg2/OudaQ5V6PaMrIclyLTRaiteUE5M4eqK/K6kmdwy2 Za8PMmUkB9sL4p5lx7CFZzeT5em+lQBSM966gAJuCEwrGFOcnkjxE++T8FogDdZNUXD7 5TbQ== X-Gm-Message-State: AIVw110cX4u54bI0f6EIRCO31vDzcz+mzYFgdkr8Ut17uOvT53QIl/zL ogNmU7iYuOz4N+3bS7s= X-Received: by 10.80.182.90 with SMTP id c26mr8540050ede.55.1500072423023; Fri, 14 Jul 2017 15:47:03 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:5640:0:960b:2678:e223:c1c6]) by smtp.gmail.com with ESMTPSA id f5sm4453427ede.17.2017.07.14.15.47.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 14 Jul 2017 15:47:02 -0700 (PDT) From: Daniel Vetter To: DRI Development Subject: [PATCH 1/3] drm/atomic-helper: Fix leak in disable_all Date: Sat, 15 Jul 2017 00:46:54 +0200 Message-Id: <20170714224656.6431-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.13.2 Cc: Daniel Vetter , Daniel Vetter , Intel Graphics Development 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: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP The legacy plane->fb pointer is refcounted by calling drm_atomic_clean_old_fb(). In practice this isn't a real problem because: - The caller in the i915 gpu reset code restores the original state again, which means the plane->fb pointer won't change, hence can't leak. - Drivers using drm_atomic_helper_shutdown call the fbdev cleanup first, and that usually cleans up the fb through drm_remove_framebuffer, which does this correctly. - Without fbdev the only framebuffers are from userspace, and those get cleaned up (again using drm_remove_framebuffer) befor the driver can even be unloaded. But in i915 I've switched the cleanup sequence around so that the _shutdown() calls happens after the drm_remove_framebuffer(), which is how I discovered this issue. v2: My analysis why the current code was ok for gpu reset and suspend/resume was correct, but then I totally failed to realize that we better keep this symmetric. Thanksfully CI noticed that for balance, a refcounting bug must exist at 2 places if previously there was no issue ... Cc: martin.peres@free.fr Cc: chris@chris-wilson.co.uk Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b07fc30372d3..3a04619d3bec 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2726,6 +2726,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; + unsigned plane_mask = 0; int ret, i; state = drm_atomic_state_alloc(dev); @@ -2768,10 +2769,14 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, goto free; drm_atomic_set_fb_for_plane(plane_state, NULL); + plane_mask |= BIT(drm_plane_index(plane)); + plane->old_fb = plane->fb; } ret = drm_atomic_commit(state); free: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); drm_atomic_state_put(state); return ret; } @@ -2902,6 +2907,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state; + struct drm_device *dev = state->dev; + int ret; state->acquire_ctx = ctx; @@ -2914,7 +2921,10 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, for_each_new_connector_in_state(state, connector, new_conn_state, i) state->connectors[i].old_state = connector->state; - return drm_atomic_commit(state); + ret = drm_atomic_commit(state); + drm_atomic_clean_old_fb(dev, ~0U, ret); + + return ret; } EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);