From patchwork Fri Nov 1 18:07:12 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rob Clark X-Patchwork-Id: 11223587 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id E07281390 for ; Fri, 1 Nov 2019 18:09:35 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 CA3AC21855 for ; Fri, 1 Nov 2019 18:09:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CA3AC21855 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7F4856E29E; Fri, 1 Nov 2019 18:09:33 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-pf1-x442.google.com (mail-pf1-x442.google.com [IPv6:2607:f8b0:4864:20::442]) by gabe.freedesktop.org (Postfix) with ESMTPS id 010CC6E29E for ; Fri, 1 Nov 2019 18:09:31 +0000 (UTC) Received: by mail-pf1-x442.google.com with SMTP id x195so4098657pfd.1 for ; Fri, 01 Nov 2019 11:09:31 -0700 (PDT) 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:mime-version :content-transfer-encoding; bh=YeTpB1Wx1lpy3awpQLYzDRRtlZuKfiLn87LhRSvje50=; b=J/CSd30JXkYxFKOol/yW7J0ijy+VinQORLntD+U0WHYDVihvF5YG6LgNHbqWMw36r3 U6RL0ztyD83E9Wmp5FYMToupiZzEV+QDUItHPQg4kWZiNFNaXyPtFR/D0PAhvM4RomdB EEYEsjiW7tGx857kDRtd1Cf/s8wwxfTCdN1nDiHTb+/UMibk4/hPpxwdvYDvNbbTV5TT MDlkxP3lNCAxKaZyiEBqhrog6q8jgMSz6xoVBKppTb2j/FkRvQ3ehqMHXQ25nBNJ15/V AxQ9fihZY2EH/l7LdMfFlHSUxtV2dzJzX9PpPDeBAM0pxJjV11UzcyfVn2LsQX+kSAAY ky5Q== X-Gm-Message-State: APjAAAU/d4il+Ps+dfQ2OChF1pGHx7SvPjJucz4N5Q+HVS45d/VsfDzw ZJNGdbom3eLrCpdH2YXWNYBsO9WKRJw= X-Google-Smtp-Source: APXvYqyGp7XRZmRiIlwGHykCJbawjeXomnH4aaKmoylErPEGGbORyqTyQ0wtAogksfMUB+aXofXNwQ== X-Received: by 2002:a63:f919:: with SMTP id h25mr6789478pgi.85.1572631771170; Fri, 01 Nov 2019 11:09:31 -0700 (PDT) Received: from localhost ([100.118.89.209]) by smtp.gmail.com with ESMTPSA id 16sm9364172pgd.0.2019.11.01.11.09.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Nov 2019 11:09:30 -0700 (PDT) From: Rob Clark To: dri-devel@lists.freedesktop.org Subject: [PATCH 1/2] drm/atomic: fix self-refresh helpers crtc state dereference Date: Fri, 1 Nov 2019 11:07:12 -0700 Message-Id: <20191101180713.5470-1-robdclark@gmail.com> X-Mailer: git-send-email 2.21.0 MIME-Version: 1.0 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=YeTpB1Wx1lpy3awpQLYzDRRtlZuKfiLn87LhRSvje50=; b=MEejXdvJ/xWDM7BvqOG6PxfoSYB1nWSgMQfn97OEzsbkg/GVwzIsbSUvmV1wsqV0Qs WIvpo+Z97RKj57DiBqRQ9AjJKfgkiH4umbmx1ElHdLveGg2pwZdKIuDU9PLSdWrvuvw4 uyxc+lD9UbDSxHAjICjQ3u49KgPDQQo91XCvY1kEv0uJ/6EYQKUzPEhiDG3iZ3NFM/e2 RL5Lo8JL0VlxAi+HXAiTTYkQVfY5L4ahPLsGyHsPtgF6UZl26yTQ/9RBeQ0XcCFfnWKc LI+sgSsbOjbhVKVez+ayoElCg9uvUjX2lOj4Cu0gDBVl5HuxyEe9HLh+1iDpOFrfB8Q1 PUsw== X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rob Clark , David Airlie , open list , Sean Paul , Sean Paul Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" From: Rob Clark drm_self_refresh_helper_update_avg_times() was incorrectly accessing the new incoming state after drm_atomic_helper_commit_hw_done(). But this state might have already been superceeded by an !nonblock atomic update resulting in dereferencing an already free'd crtc_state. Fixes: d4da4e33341c ("drm: Measure Self Refresh Entry/Exit times to avoid thrashing") Cc: Sean Paul Signed-off-by: Rob Clark --- TODO I *think* this will more or less do the right thing.. althought I'm not 100% sure if, for example, we enter psr in a nonblock commit, and then leave psr in a !nonblock commit that overtakes the completion of the nonblock commit. Not sure if this sort of scenario can happen in practice. But not crashing is better than crashing, so I guess we should either take this patch or rever the self-refresh helpers until Sean can figure out a better solution. drivers/gpu/drm/drm_atomic_helper.c | 2 ++ drivers/gpu/drm/drm_self_refresh_helper.c | 11 ++++++----- include/drm/drm_atomic.h | 8 ++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 3ef2ac52ce94..732bd0ce9241 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2240,6 +2240,8 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) int i; for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { + old_state->crtcs[i].new_self_refresh_active = new_crtc_state->self_refresh_active; + commit = new_crtc_state->commit; if (!commit) continue; diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c index 68f4765a5896..77b9079fa578 100644 --- a/drivers/gpu/drm/drm_self_refresh_helper.c +++ b/drivers/gpu/drm/drm_self_refresh_helper.c @@ -143,19 +143,20 @@ void drm_self_refresh_helper_update_avg_times(struct drm_atomic_state *state, unsigned int commit_time_ms) { struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state, *new_crtc_state; + struct drm_crtc_state *old_crtc_state; int i; - for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, - new_crtc_state, i) { + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { + bool new_self_refresh_active = + state->crtcs[i].new_self_refresh_active; struct drm_self_refresh_data *sr_data = crtc->self_refresh_data; struct ewma_psr_time *time; if (old_crtc_state->self_refresh_active == - new_crtc_state->self_refresh_active) + new_self_refresh_active) continue; - if (new_crtc_state->self_refresh_active) + if (new_self_refresh_active) time = &sr_data->entry_avg_ms; else time = &sr_data->exit_avg_ms; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 927e1205d7aa..86baf2b38bb3 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -155,6 +155,14 @@ struct __drm_crtcs_state { struct drm_crtc *ptr; struct drm_crtc_state *state, *old_state, *new_state; + /** + * @new_self_refresh_active: + * + * drm_atomic_helper_commit_hw_done() stashes new_crtc_state->self_refresh_active + * so that it can be accessed late in drm_self_refresh_helper_update_avg_times(). + */ + bool new_self_refresh_active; + /** * @commit: * From patchwork Fri Nov 1 18:07:13 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rob Clark X-Patchwork-Id: 11223591 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 50B571390 for ; Fri, 1 Nov 2019 18:11:45 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (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 3A91D205C9 for ; Fri, 1 Nov 2019 18:11:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3A91D205C9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7EC5B6F84B; Fri, 1 Nov 2019 18:11:44 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-pg1-x541.google.com (mail-pg1-x541.google.com [IPv6:2607:f8b0:4864:20::541]) by gabe.freedesktop.org (Postfix) with ESMTPS id ED0146F84B for ; Fri, 1 Nov 2019 18:11:42 +0000 (UTC) Received: by mail-pg1-x541.google.com with SMTP id j30so3471188pgn.5 for ; Fri, 01 Nov 2019 11:11:42 -0700 (PDT) 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=D6pp+waK1B6eoQZsfgu20T9S0S5gE50J59ZyY893XeU=; b=i/0fpdF1ACMb2c1ThnuK5jXH7eMfU3w5DEVN6y8xvbldvau3nsFofwMwCynr7dKCQL oOH6Cyz2oq79rG+4T9fjdtOgvgZO0k7q8CAeo4lPSXhPerpxgIkPYPCgglbbM75xMQLi SouOMFtchOoFTHOiGGjrFRPHFcR7EFz2qZ2tqCCqGW6ma9AVykAosfVKpaGRGV+ZAhBI wEQHHT4BgLPy8n8uJ3+TnXwye7ftPQuOS+jN1WIOCMcu6H3gI3ii7X7OU6R7rJ1gXNQL ZWP99Cfu2rtDl/Vq/xfolNwfDXSsXs+Xp0mroS4MP8bRaVj8Tu8lYR+XS/2aCEP4jAbY b5yA== X-Gm-Message-State: APjAAAXMbCi8gJP9ZP0Z615jVD8P1C9ppYBos3FaMc6oLum9Nxi3x8A7 TBkOkjhyxvlW8Zs2HUsmowLhulbIeRU= X-Google-Smtp-Source: APXvYqyntvvTNEB7b5YRQX6acrnW6EDOe32CdQyaoFyh8/yeQSOf08HCyibGS7j+0s+Mu2D+PSSeIg== X-Received: by 2002:a17:90a:8816:: with SMTP id s22mr12344442pjn.31.1572631902246; Fri, 01 Nov 2019 11:11:42 -0700 (PDT) Received: from localhost ([100.118.89.209]) by smtp.gmail.com with ESMTPSA id h5sm11525487pjc.9.2019.11.01.11.11.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Nov 2019 11:11:41 -0700 (PDT) From: Rob Clark To: dri-devel@lists.freedesktop.org Subject: [PATCH 2/2] drm/atomic: clear new_state pointers at hw_done Date: Fri, 1 Nov 2019 11:07:13 -0700 Message-Id: <20191101180713.5470-2-robdclark@gmail.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20191101180713.5470-1-robdclark@gmail.com> References: <20191101180713.5470-1-robdclark@gmail.com> MIME-Version: 1.0 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=D6pp+waK1B6eoQZsfgu20T9S0S5gE50J59ZyY893XeU=; b=ctnx2Mq7v0ZwoxZpgw8YqdbWutmx0MLZKmxmioikkvICWrTw1w/6AKRvR8X0KBJo1Q H0q5nPYMGRreXVKlEPeoh4VtEN1K97KsqpD13ts5v4lWQFJg4H2P5+JzG7oAlzLrVo1p oI5Os+HHsLwy+OUClFC8ZVGFwLA6KHN6/4DoMryIAOT7NsStmpHqwZFUtokc9oZL/kT+ cGvXVaiQTIs40ctu7V2vStfRIJlOGEpU2g9cmAKmuhy+UxHp9xJWz1vqEaJ+QNmFvjN+ y8MDer/CsYviNag0SnshQ6oPqAx3IpnuaI3HL54iKUW3BxQAZRGqCosbW1KAi6YtEudk jw3Q== X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rob Clark , David Airlie , open list , Sean Paul , Sean Paul Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" From: Rob Clark The new state should not be accessed after this point. Clear the pointers to make that explicit. This makes the error corrected in the previous patch more obvious. Signed-off-by: Rob Clark Reported-by: kernel test robot --- drivers/gpu/drm/drm_atomic_helper.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 732bd0ce9241..176831df8163 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2234,13 +2234,42 @@ EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); */ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) { + struct drm_connector *connector; + struct drm_connector_state *old_conn_state, *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state, *new_crtc_state; + struct drm_plane *plane; + struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_crtc_commit *commit; + struct drm_private_obj *obj; + struct drm_private_state *old_obj_state, *new_obj_state; int i; + /* + * After this point, drivers should not access the permanent modeset + * state, so we also clear the new_state pointers to make this + * restriction explicit. + * + * For the CRTC state, we do this in the same loop where we signal + * hw_done, since we still need to new_crtc_state to fish out the + * commit. + */ + + for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) { + old_state->connectors[i].new_state = NULL; + } + + for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) { + old_state->planes[i].new_state = NULL; + } + + for_each_oldnew_private_obj_in_state(old_state, obj, old_obj_state, new_obj_state, i) { + old_state->private_objs[i].new_state = NULL; + } + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { old_state->crtcs[i].new_self_refresh_active = new_crtc_state->self_refresh_active; + old_state->crtcs[i].new_state = NULL; commit = new_crtc_state->commit; if (!commit)