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 307BEC38145 for ; Thu, 8 Sep 2022 05:28:20 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 217D210E959; Thu, 8 Sep 2022 05:28:19 +0000 (UTC) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id 17E3310E95B for ; Thu, 8 Sep 2022 05:28:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1662614897; x=1694150897; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=+G7OKYxchu7886qijw25/zISTxsIwCUro+cikjGx9Nk=; b=DZqjlwbA26WPtWX3QFtKke5eaL+eXTiW9S/oTut/z60z4tmr8RgH3JgK 3gt0NW2JAEsWxpCxA+7mO0p4192RnCW1gWCBNKt7x8k5VupCHTRHKB01s oxPa33+0OmdFH9SyzDu3gcwfg7QfkSpNIpvOBTRZVyRSusubR21MJkFS6 giLy5KfmVwMlr8fg62PUhia73nYt+a15HiTBU/lFZjODzRkiPa7C/rBgy hiFAZzDCeQQHXmchRDPj/zETzHfARSmSX0NltggIz/Wqyrv6J4sryBw5Y 43xCAEZhOHGWLL1VknyS+0Up35C8zWsM8ake/BGrk403O9BLp/m1znxf9 Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10463"; a="361034806" X-IronPort-AV: E=Sophos;i="5.93,299,1654585200"; d="scan'208";a="361034806" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Sep 2022 22:28:16 -0700 X-IronPort-AV: E=Sophos;i="5.93,299,1654585200"; d="scan'208";a="592014880" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.224.252]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Sep 2022 22:28:16 -0700 Date: Wed, 07 Sep 2022 22:28:15 -0700 Message-ID: <877d2exwnk.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Rodrigo Vivi In-Reply-To: References: <20220902235302.1112388-1-ashutosh.dixit@intel.com> <20220902235302.1112388-5-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.1 (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 4/6] drm/i915/debugfs: Add perf_limit_reasons in debugfs 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, 06 Sep 2022 07:13:03 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Fri, Sep 02, 2022 at 04:53:00PM -0700, Ashutosh Dixit wrote: > > From: Tilak Tangudu > > > > Add perf_limit_reasons in debugfs. Unlike the lower 16 perf_limit_reasons > > status bits, the upper 16 log bits remain set until cleared, thereby > > ensuring the throttling occurrence is not missed. The clear fop clears > > the upper 16 log bits, the get fop gets all 32 log and status bits. > > > > Signed-off-by: Tilak Tangudu > > --- > > drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 27 +++++++++++++++++++ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 28 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > > index 108b9e76c32e..5c95cba5e5df 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > > @@ -655,6 +655,32 @@ static bool rps_eval(void *data) > > > > DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(rps_boost); > > > > +static int perf_limit_reasons_get(void *data, u64 *val) > > +{ > > + struct intel_gt *gt = data; > > + intel_wakeref_t wakeref; > > + > > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) > > + *val = intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS); > > + > > + return 0; > > +} > > + > > +static int perf_limit_reasons_clear(void *data, u64 val) > > +{ > > + struct intel_gt *gt = data; > > + intel_wakeref_t wakeref; > > + > > + /* Clear the upper 16 log bits, the lower 16 status bits are read-only */ > > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) > > + intel_uncore_rmw(gt->uncore, GT0_PERF_LIMIT_REASONS, > > + GT0_PERF_LIMIT_REASONS_LOG_MASK, 0); > > + > > + return 0; > > +} > > +DEFINE_SIMPLE_ATTRIBUTE(perf_limit_reasons_fops, perf_limit_reasons_get, > > + perf_limit_reasons_clear, "%llu\n"); > > + > > void intel_gt_pm_debugfs_register(struct intel_gt *gt, struct dentry *root) > > { > > static const struct intel_gt_debugfs_file files[] = { > > @@ -664,6 +690,7 @@ void intel_gt_pm_debugfs_register(struct intel_gt *gt, struct dentry *root) > > { "forcewake_user", &forcewake_user_fops, NULL}, > > { "llc", &llc_fops, llc_eval }, > > { "rps_boost", &rps_boost_fops, rps_eval }, > > + { "perf_limit_reasons", &perf_limit_reasons_fops, NULL }, > > }; > > > > intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), gt); > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 5e6239864c35..10126995e1f6 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1802,6 +1802,7 @@ > > #define POWER_LIMIT_4_MASK REG_BIT(9) > > #define POWER_LIMIT_1_MASK REG_BIT(11) > > #define POWER_LIMIT_2_MASK REG_BIT(12) > > +#define GT0_PERF_LIMIT_REASONS_LOG_MASK REG_GENMASK(31, 16) > > Is this valid for all platforms? Yes, looks like it. > What does the bits are really telling us? The v1 commit message above hinted at what was happening, I've clarified the commit message in v2 as follows: Add perf_limit_reasons in debugfs. The upper 16 perf_limit_reasons RW "log" bits are identical to the lower 16 RO "status" bits except that the "log" bits remain set until cleared, thereby ensuring the throttling occurrence is not missed. The clear fop clears the upper 16 "log" bits, the get fop gets all 32 "log" and "status" bits. I've also expanded the comment in perf_limit_reasons_clear() to explain this. > Could we expand the reasons? The previous bits we know exactly > what kind of limits we are dealing of, but with this combined > one without any explanation I'm afraid this will bring more > confusion than help. We will get bugged by many folks trying > to debug this out there when bit 13, for instance, is set. > "What does bit 13 mean?" will be a recurrent question with > only a tribal knowledge kind of answer. I think the new commit message above and comment has the answer to this now. Also, won't there be a public copy of the Bspec where someone can look up the bit definitions? Also, are these "log" bits useful enough to expose them in sysfs like we have the lower "status" bits exposed today but that is probably the question for a different patch. Thanks. -- Ashutosh