From patchwork Sun Aug 20 12:59:20 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 9911159 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 5B4D1601D4 for ; Sun, 20 Aug 2017 13:16:43 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4D78828462 for ; Sun, 20 Aug 2017 13:16:43 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 428E9287C9; Sun, 20 Aug 2017 13:16:43 +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.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED 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 44E3728462 for ; Sun, 20 Aug 2017 13:16:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C61456E183; Sun, 20 Aug 2017 13:16:41 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from smtpq3.mnd.mail.iss.as9143.net (smtpq3.mnd.mail.iss.as9143.net [212.54.34.166]) by gabe.freedesktop.org (Postfix) with ESMTPS id 46E8D6E183; Sun, 20 Aug 2017 13:16:40 +0000 (UTC) Received: from [212.54.34.114] (helo=smtp6.mnd.mail.iss.as9143.net) by smtpq3.mnd.mail.iss.as9143.net with esmtp (Exim 4.86_2) (envelope-from ) id 1djPpG-0001xH-Ms; Sun, 20 Aug 2017 14:59:34 +0200 Received: from 546a5441.cm-12-3b.dynamic.ziggo.nl ([84.106.84.65] helo=shalem.localdomain.com) by smtp6.mnd.mail.iss.as9143.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.86_2) (envelope-from ) id 1djPpK-0001D8-Pu; Sun, 20 Aug 2017 14:59:38 +0200 From: Hans de Goede To: Daniel Vetter , Jani Nikula , =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= , Imre Deak Date: Sun, 20 Aug 2017 14:59:20 +0200 Message-Id: <20170820125920.27347-4-hdegoede@redhat.com> X-Mailer: git-send-email 2.13.4 In-Reply-To: <20170820125920.27347-1-hdegoede@redhat.com> References: <20170820125920.27347-1-hdegoede@redhat.com> X-SourceIP: 84.106.84.65 X-Authenticated-Sender: jwrdegoede@ziggo.nl (via SMTP) X-Ziggo-spambar: / X-Ziggo-spamscore: 0.0 X-Ziggo-spamreport: CMAE Analysis: v=2.2 cv=ZsWdE5zG c=1 sm=1 tr=0 a=YRvH8WGRgbOb+hydPbgS/w==:17 a=9+rZDBEiDlHhcck0kWbJtElFXBc=:19 a=KeKAF7QvOSUA:10 a=20KFwNOVAAAA:8 a=QyXUC8HyAAAA:8 a=cIkWXtm7fOU6T3GVvTEA:9 none X-Ziggo-Spam-Status: No Cc: Hans de Goede , intel-gfx , dri-devel@lists.freedesktop.org Subject: [Intel-gfx] [PATCH v4 4/4] drm/i915: Acquire PUNIT->PMIC bus for intel_uncore_forcewake_reset() X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP intel_uncore_forcewake_reset() does forcewake puts and gets as such we need to make sure that no-one tries to access the PUNIT->PMIC bus (on systems where this bus is shared) while it runs, otherwise bad things happen. Normally this is taken care of by the i915_pmic_bus_access_notifier() which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other driver tries to access the PMIC bus, so that later forcewake gets are no-ops (for the duration of the bus access). But intel_uncore_forcewake_reset gets called in 3 cases: 1) Before registering the pmic_bus_access_notifier 2) After unregistering the pmic_bus_access_notifier 3) To reset forcewake state on a GPU reset In all 3 cases the i915_pmic_bus_access_notifier() protection is insufficient. This commit fixes this race by calling iosf_mbi_punit_acquire() before calling intel_uncore_forcewake_reset(). In the case where it is called directly after unregistering the pmic_bus_access_notifier, we need to hold the punit-lock over both calls to avoid a race where intel_uncore_fw_release_timer() may execute between the 2 calls. To allow holding the lock over both calls we need an unlocked variant of iosf_mbi_unregister_pmic_bus_access_notifier. Since intel_uncore.c is the only user of this function, we simply modify it in this commit. Doing this in a separate commit would require first adding an unlocked variant, then this commit and then removing the unused normal variant. Signed-off-by: Hans de Goede Reviewed-by: Imre Deak --- Changes in v2: -Rebase on current (July 6th 2017) drm-next Changes in v3: -Keep punit acquired / locked over the unregister + forcewake_reset call combo to avoid a race hitting between the 2 calls -This requires modifying iosf_mbi_unregister_pmic_bus_access_notifier to not take the lock itself, since we are the only users this is done in this same commit Changes in v4: -Fix missing rename in doc-comment -Add and use iosf_mbi_assert_punit_acquired() helper -Add missing acquiqre surrounding intel_uncore_forcewake_reset() inside intel_uncore_check_forcewake_domains() -Add Imre's Reviewed-by --- arch/x86/include/asm/iosf_mbi.h | 20 +++++++++++++++++--- arch/x86/platform/intel/iosf_mbi.c | 20 +++++++++++--------- drivers/gpu/drm/i915/intel_uncore.c | 17 +++++++++++++---- drivers/gpu/drm/i915/selftests/intel_uncore.c | 3 +++ 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h index c313cac36f56..0f0de4303180 100644 --- a/arch/x86/include/asm/iosf_mbi.h +++ b/arch/x86/include/asm/iosf_mbi.h @@ -139,11 +139,17 @@ void iosf_mbi_punit_release(void); int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb); /** - * iosf_mbi_register_pmic_bus_access_notifier - Unregister PMIC bus notifier + * iosf_mbi_register_pmic_bus_access_notifier_unlocked - Unregister PMIC bus + * notifier + * + * Note the caller must call iosf_mbi_punit_acquire() before calling this + * to ensure the bus is inactive before unregistering (and call + * iosf_mbi_punit_release() afterwards). * * @nb: notifier_block to unregister */ -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb); /** * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain @@ -153,6 +159,11 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb); */ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v); +/** + * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired. + */ +void iosf_mbi_assert_punit_acquired(void); + #else /* CONFIG_IOSF_MBI is not enabled */ static inline bool iosf_mbi_available(void) @@ -191,7 +202,8 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) } static inline -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb) { return 0; } @@ -202,6 +214,8 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) return 0; } +static inline void iosf_mbi_assert_punit_acquired(void) {} + #endif /* CONFIG_IOSF_MBI */ #endif /* IOSF_MBI_SYMS_H */ diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c index a952ac199741..a5361fd11e6e 100644 --- a/arch/x86/platform/intel/iosf_mbi.c +++ b/arch/x86/platform/intel/iosf_mbi.c @@ -218,19 +218,15 @@ int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb) } EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier); -int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb) +int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + struct notifier_block *nb) { - int ret; + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); - /* Wait for the bus to go inactive before unregistering */ - mutex_lock(&iosf_mbi_punit_mutex); - ret = blocking_notifier_chain_unregister( + return blocking_notifier_chain_unregister( &iosf_mbi_pmic_bus_access_notifier, nb); - mutex_unlock(&iosf_mbi_punit_mutex); - - return ret; } -EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier); +EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier_unlocked); int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) { @@ -239,6 +235,12 @@ int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v) } EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain); +void iosf_mbi_assert_punit_acquired(void) +{ + WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex)); +} +EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired); + #ifdef CONFIG_IOSF_MBI_DEBUG static u32 dbg_mdr; static u32 dbg_mcr; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index e9ed02518406..80e75c029e59 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -229,6 +229,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) return HRTIMER_NORESTART; } +/* Note callers must have acquired the PUNIT->PMIC bus, before calling this. */ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, bool restore) { @@ -237,6 +238,8 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, int retry_count = 100; enum forcewake_domains fw, active_domains; + iosf_mbi_assert_punit_acquired(); + /* Hold uncore.lock across reset to prevent any register access * with forcewake not set correctly. Wait until all pending * timers are run before holding. @@ -416,14 +419,18 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv, GT_FIFO_CTL_RC6_POLICY_STALL); } + iosf_mbi_punit_acquire(); intel_uncore_forcewake_reset(dev_priv, restore_forcewake); + iosf_mbi_punit_release(); } void intel_uncore_suspend(struct drm_i915_private *dev_priv) { - iosf_mbi_unregister_pmic_bus_access_notifier( + iosf_mbi_punit_acquire(); + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( &dev_priv->uncore.pmic_bus_access_nb); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); } void intel_uncore_resume_early(struct drm_i915_private *dev_priv) @@ -1246,12 +1253,14 @@ void intel_uncore_init(struct drm_i915_private *dev_priv) void intel_uncore_fini(struct drm_i915_private *dev_priv) { - iosf_mbi_unregister_pmic_bus_access_notifier( - &dev_priv->uncore.pmic_bus_access_nb); - /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev_priv); + + iosf_mbi_punit_acquire(); + iosf_mbi_unregister_pmic_bus_access_notifier_unlocked( + &dev_priv->uncore.pmic_bus_access_nb); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); } #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1) diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c index 2d0fef2cfca6..55d0ef4fbcef 100644 --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c @@ -148,7 +148,10 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri for_each_set_bit(offset, valid, FW_RANGE) { i915_reg_t reg = { offset }; + iosf_mbi_punit_acquire(); intel_uncore_forcewake_reset(dev_priv, false); + iosf_mbi_punit_release(); + check_for_unclaimed_mmio(dev_priv); (void)I915_READ(reg);