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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 5924AC05027 for ; Tue, 14 Feb 2023 20:48:39 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BA4C010E999; Tue, 14 Feb 2023 20:48:38 +0000 (UTC) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8F08610E999 for ; Tue, 14 Feb 2023 20:48:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1676407717; x=1707943717; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=2a1Kieuj0atD3vtIXP5FktxIzG6B03xi5Dl6Ya2Agjo=; b=Hn9TeJdNPpB7dFPODZ+KkvaSu3kSudE2ApA7wCTsqIQdqHLO8njyvg+C BG3+a/z2xL63Gklpa8dDrrC8++8goObrbpIu3vPvdqHEtMnwb44moU28t hDhsRtzH1O+eJ+A6QhyWSy8k4ebDbH/YJj68HfFhr/N7m58XAl26HqpX4 n75Uj8krHEVZdTvGjQaIB54tY3flo3OKLF+3Knszs3wykhhWPcSD3naf/ q2GtffRNlhMXz06CZbvu9r206bhVVcOVgqrgvjFazD6CR2+BRAcHxUJ2d ACC0aA5+hd1Rv6zXR2WFdqRDjODCGnBLshBwr3FdLSVlGA/T0egMK5eC7 w==; X-IronPort-AV: E=McAfee;i="6500,9779,10621"; a="395884990" X-IronPort-AV: E=Sophos;i="5.97,297,1669104000"; d="scan'208";a="395884990" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Feb 2023 12:47:25 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10621"; a="671354547" X-IronPort-AV: E=Sophos;i="5.97,297,1669104000"; d="scan'208";a="671354547" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.220.101]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Feb 2023 12:47:24 -0800 Date: Tue, 14 Feb 2023 12:47:23 -0800 Message-ID: <87ttzot1no.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Rodrigo Vivi In-Reply-To: References: <20230213210049.1900681-1-ashutosh.dixit@intel.com> <20230213210049.1900681-3-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/hwmon: Enable PL1 limit when writing limit value to HW X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, 14 Feb 2023 06:51:37 -0800, Rodrigo Vivi wrote: > Hi Rodrigo, > On Mon, Feb 13, 2023 at 01:00:48PM -0800, Ashutosh Dixit wrote: > > Previous documentation suggested that the PL1 power limit is always enabled > > in HW. However we now find this not to be the case on some platforms (such > > as ATSM). Therefore enable the PL1 power limit (by setting the enable bit) > > when writing the PL1 limit value to HW. > > > > Bspec: 51864 > > > > Signed-off-by: Ashutosh Dixit > > --- > > drivers/gpu/drm/i915/i915_hwmon.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 85195d61f89c7..7c20a6f47b92e 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -385,10 +385,11 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > > > /* Computation in 64-bits to avoid overflow. Round to nearest. */ > > nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); > > + nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval); > > > > hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, > > - PKG_PWR_LIM_1, > > - REG_FIELD_PREP(PKG_PWR_LIM_1, nval)); > > + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, > > + nval); > > This patch looks right to me. But could you please open up on what exactly > failed on that reverted patch? Why do we need to set the bits together? I had explained a bit here: https://gitlab.freedesktop.org/drm/intel/-/issues/8062#note_1761070 But will repeat. On ATSM, at power-up, PCODE sets the PL1 power limit to 0 but disables the PL1 power limit. The earlier patch had enabled the the PL1 power limit during module load itself but had left the PL1 power limit set to 0 (since there is no easy way to find out what it should be set to, on ATSM PCODE sets even the max power (which could have been used to set the PL1 limit) to 0). You can see that patch here: https://patchwork.freedesktop.org/patch/521321/?series=113578&rev=4 Now the PL1 power limit being 0 (and enabled) implies that HW will work with minimum power and therefore the lowest effective frequency. This means all workloads will run slower and this was resulting in various IGT tests timing out and GuC FW load (on resets) timing out on ATSM and that is why we had to revert that patch. In this patch I have changed the strategy and instead of enabling the PL1 power limit on module load we now enable it only when the limit is set by userspace. So at least the default CI will not be affected in this case. We can see that there no regressions on ATSM this time here: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113984v1/bat-all.html? Thanks. -- Ashutosh > > > return 0; > > } > > > > -- > > 2.38.0 > >