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 1D46AC433FE for ; Wed, 9 Nov 2022 01:07:46 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CD7D910E551; Wed, 9 Nov 2022 01:07:44 +0000 (UTC) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 64DEF10E551 for ; Wed, 9 Nov 2022 01:07:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1667956061; x=1699492061; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=zbBr57ulor9ynRTO7fYkcdHWjmRYMDxdqR65zdCEzlE=; b=fVDynbuO0qUyw2XZGtxnIQgJ4QcYa1b18RFBZRA6mDBEF8Yi9i+eMPjv puzTpPt55kIyXgjGIQsWA61TnVwddxLdenusn0f29XFVlQ4FDC7ZnY7US TbsRawsnUUdoLRC+hCx8l9yOdOUiKoPS1/wCWbtjHBYDvgHLJnYr55BaD HJ7JCW1Svm3JNdv4DMXxo/9qPutil+3UdcnnA5ZqKlvZJfJgOolyU4uNU lfVCG3RdEo8fzXiy3DmViKjjHRJTyV630KWGfNpOiUrH4WsGrYkAPBUeh Cd42BPNdBGfB0iley4vUHe5OZuiDE95elecbhJH+1zhyEmNggJZfXxivz w==; X-IronPort-AV: E=McAfee;i="6500,9779,10525"; a="310850637" X-IronPort-AV: E=Sophos;i="5.96,149,1665471600"; d="scan'208";a="310850637" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 17:07:40 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10525"; a="705507303" X-IronPort-AV: E=Sophos;i="5.96,149,1665471600"; d="scan'208";a="705507303" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.74.81]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 17:07:39 -0800 Date: Tue, 08 Nov 2022 17:07:38 -0800 Message-ID: <87h6z90wbp.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Sujaritha Sundaresan In-Reply-To: <20221103123723.18063-1-sujaritha.sundaresan@intel.com> References: <20221103123723.18063-1-sujaritha.sundaresan@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 v1] drm/i915/gt: Add sysfs RAPL PL1 interface 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 Thu, 03 Nov 2022 05:37:23 -0700, Sujaritha Sundaresan wrote: > Hi Suja, > Adding the rapl_pl1_freq_mhz sysfs attribute. > > Signed-off-by: Sujaritha Sundaresan > Cc: Ashutosh Dixit > --- > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 20 ++++++++++ > drivers/gpu/drm/i915/gt/intel_rps.c | 44 +++++++++++++++++++++ > drivers/gpu/drm/i915/gt/intel_rps.h | 3 ++ > drivers/gpu/drm/i915/i915_reg.h | 4 ++ > 4 files changed, 71 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > index 904160952369..e7f00ec252f8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c > @@ -496,6 +496,17 @@ static DEVICE_ATTR_RO(vlv_rpe_freq_mhz); > static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR; > static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR; > > +static ssize_t rapl_pl1_freq_mhz_show(struct device *dev, > + struct device_attribute *attr, > + char *buff) > +{ > + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); > + u32 rapl_pl1 = intel_rps_read_rapl_pl1_frequency(>->rps); > + > + return sysfs_emit(buff, "%u\n", rapl_pl1); > +} > + > + > static ssize_t punit_req_freq_mhz_show(struct device *dev, > struct device_attribute *attr, > char *buff) > @@ -534,6 +545,7 @@ struct intel_gt_bool_throttle_attr attr_##sysfs_func__ = { \ > .mask = mask__, \ > } > > +static DEVICE_ATTR_RO(rapl_pl1_freq_mhz); > static DEVICE_ATTR_RO(punit_req_freq_mhz); Is this patch against old code? Since this is now INTEL_GT_ATTR_RO. Yes the build failed. So rapl_pl1_freq_mhz will need to follow punit_req_freq_mhz. > static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_status, GT0_PERF_LIMIT_REASONS_MASK); > static INTEL_GT_RPS_BOOL_ATTR_RO(throttle_reason_pl1, POWER_LIMIT_1_MASK); > @@ -790,12 +802,20 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) > if (!is_object_gt(kobj)) > return; > > + ret = sysfs_create_file(kobj, &dev_attr_rapl_pl1_freq_mhz.attr); The convention here is to create sysfs files only for platforms on which a feature (in this case RAPL PL1 freq) is supported. Also are we sure this is only available on MTL and XEHPSDV and not on DG2? Since generally a feature appears first on a platform and then is available for all successive products. If it's available on DG2 too then we can use something like: if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) See GRAPHICS_VER_FULL for various platforms in i915_pci.c. > + if (ret) > + drm_warn(>->i915->drm, > + "failed to create gt%u rapl_pl1_freq_mhz sysfs(%pe)", > + gt->info.id, ERR_PTR(ret)); > + > + > ret = sysfs_create_file(kobj, &dev_attr_punit_req_freq_mhz.attr); > if (ret) > drm_warn(>->i915->drm, > "failed to create gt%u punit_req_freq_mhz sysfs (%pe)", > gt->info.id, ERR_PTR(ret)); > > + Remove empty line. > if (i915_mmio_reg_valid(intel_gt_perf_limit_reasons_reg(gt))) { > ret = sysfs_create_files(kobj, throttle_reason_attrs); > if (ret) > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c > index 17b40b625e31..0e89b941e3be 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.c > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c > @@ -9,6 +9,7 @@ > > #include "i915_drv.h" > #include "i915_irq.h" > +#include "i915_reg.h" Not needed, see below. > #include "intel_breadcrumbs.h" > #include "intel_gt.h" > #include "intel_gt_clock_utils.h" > @@ -2422,6 +2423,49 @@ bool rps_read_mask_mmio(struct intel_rps *rps, > return rps_read_mmio(rps, reg32) & mask; > } > > +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps) > +{ > + struct drm_i915_private *i915 = rps_to_i915(rps); > + i915_reg_t rgadr; > + u32 rapl_pl1; > + > + if (IS_METEORLAKE(i915)) { > + rgadr = MTL_RAPL_PL1_FREQ_LIMIT; > + } else if (IS_XEHPSDV(i915)) { > + rgadr = XEHPSDV_RAPL_PL1_FREQ_LIMIT; > + } else { > + MISSING_CASE(GRAPHICS_VER(i915)); > + rgadr = INVALID_MMIO_REG; No need for this, the sysfs file will only be visible for platforms on which this is supported so this will never be hit. > + } > + > + if (!i915_mmio_reg_valid(rgadr)) > + rapl_pl1 = 0; No need for this either. > + else > + rapl_pl1 = rps_read_mmio(rps, rgadr); > + > + return rapl_pl1; > +} > + > +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1) > +{ > + struct drm_i915_private *i915 = rps_to_i915(rps); > + u32 rapl = 0; > + > + if (IS_METEORLAKE(i915) || IS_XEHPSDV(i915)) > + rapl = rapl_pl1 & RAPL_PL1_FREQ_LIMIT_MASK; > + else > + MISSING_CASE(GRAPHICS_VER(i915)); No need for this either. > + > + return rapl; > +} > + > +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps) > +{ > + u32 rapl_freq = intel_rps_get_rapl(rps, intel_rps_read_rapl_pl1(rps)); > + > + return (rapl_freq >> 8) * GT_FREQUENCY_MULTIPLIER; Use REG_FIELD_GET to extract the freq. There doesn't seem any need to have 3 functions here, let's combine them into a single function and use REG_FIELD_GET. > +} > + > /* External interface for intel_ips.ko */ > > static struct drm_i915_private __rcu *ips_mchdev; > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h > index 4509dfdc52e0..4adc6aaedba0 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.h > +++ b/drivers/gpu/drm/i915/gt/intel_rps.h > @@ -34,6 +34,7 @@ void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive); > int intel_gpu_freq(struct intel_rps *rps, int val); > int intel_freq_opcode(struct intel_rps *rps, int val); > u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1); > +u32 intel_rps_get_rapl(struct intel_rps *rps, u32 rapl_pl1); > u32 intel_rps_read_actual_frequency(struct intel_rps *rps); > u32 intel_rps_get_requested_frequency(struct intel_rps *rps); > u32 intel_rps_get_min_frequency(struct intel_rps *rps); > @@ -47,6 +48,8 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps); > u32 intel_rps_get_rpn_frequency(struct intel_rps *rps); > u32 intel_rps_read_punit_req(struct intel_rps *rps); > u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); > +u32 intel_rps_read_rapl_pl1(struct intel_rps *rps); > +u32 intel_rps_read_rapl_pl1_frequency(struct intel_rps *rps); These should be static and not in the .h file. Anyway I think as pointed out above these 3 functions should probably be combined into just one. > void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps); > void intel_rps_raise_unslice(struct intel_rps *rps); > void intel_rps_lower_unslice(struct intel_rps *rps); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 5003a5ffbc6a..68ff98e27b8d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1818,6 +1818,10 @@ > #define GT0_PERF_LIMIT_REASONS_LOG_MASK REG_GENMASK(31, 16) > #define MTL_MEDIA_PERF_LIMIT_REASONS _MMIO(0x138030) > > +#define XEHPSDV_RAPL_PL1_FREQ_LIMIT _MMIO(0x250070) > +#define MTL_RAPL_PL1_FREQ_LIMIT _MMIO(0x281070) > +#define RAPL_PL1_FREQ_LIMIT_MASK 0xffff Use REG_GENMASK(15, 0) here. Also: * The registers now need to be sorted by offset. Let's move these #defines to gt/intel_gt_regs.h * RAPL_PL1_FREQ_LIMIT_MASK will need to be indented a couple of spaces as done for other registers > #define CHV_CLK_CTL1 _MMIO(0x101100) > #define VLV_CLK_CTL2 _MMIO(0x101104) > #define CLK_CTL2_CZCOUNT_30NS_SHIFT 28 > -- > 2.34.1 > Thanks. -- Ashutosh