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 86153C433F5 for ; Tue, 24 May 2022 03:10:50 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DBA9A10E041; Tue, 24 May 2022 03:10:49 +0000 (UTC) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4058210E041 for ; Tue, 24 May 2022 03:10:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1653361848; x=1684897848; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=2dWzEq8hSZ5orxby9a3GdX0RyMKf5PzWkCWDRaBLoTY=; b=H0Ug2RsoIIOAVtUtbODNKNERdkz7aZaXqnKKwRO1cImmh3AgmqBQVrMi 5v+EIXplRdggh7ylw50UIplM4FHC7DR+VnSDnqOBH+zSirjQpONG+4OvQ Nnv9Fwwz8Y3zP7tZoDp5sWQ7G7oOrlPty4TY5mJ41y69CjnCn6cIDlaua JrkP5krdfDGvnLhdmhMaMPwkvuYGi8Ct+Ka5ZwGyUtJOazNShGBcgdPlq GkNaz05ztePp6Rjrx4g+rHLvvXBUGi9YPSIT3GOjOBu0P1JcHLW2Syc6J idVCNQoZcbRijBKLYsoixXRF91Fj4Iqw1UXnOcPgdxHeSP2QHQRQlAl6R A==; X-IronPort-AV: E=McAfee;i="6400,9594,10356"; a="272235911" X-IronPort-AV: E=Sophos;i="5.91,247,1647327600"; d="scan'208";a="272235911" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 May 2022 20:10:47 -0700 X-IronPort-AV: E=Sophos;i="5.91,247,1647327600"; d="scan'208";a="641759842" Received: from adixit-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.8.157]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 May 2022 20:10:47 -0700 Date: Mon, 23 May 2022 20:10:46 -0700 Message-ID: <877d6bvd55.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Badal Nilawar In-Reply-To: <20220523110841.1151431-2-badal.nilawar@intel.com> References: <20220523110841.1151431-1-badal.nilawar@intel.com> <20220523110841.1151431-2-badal.nilawar@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 1/3] drm/i915/hwmon: Add HWMON power sensor support 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 Mon, 23 May 2022 04:08:39 -0700, Badal Nilawar wrote: > A few initial comments. > +static void > +i915_hwmon_get_preregistration_info(struct drm_i915_private *i915) > +{ > + struct i915_hwmon *hwmon = i915->hwmon; > + struct intel_uncore *uncore = &i915->uncore; > + struct i915_hwmon_drvdata *ddat = &hwmon->ddat; > + intel_wakeref_t wakeref; > + u32 val_sku_unit; > + __le32 le_sku_unit; > + > + if (IS_DG1(i915) || IS_DG2(i915)) { > + hwmon->rg.pkg_power_sku_unit = PCU_PACKAGE_POWER_SKU_UNIT; > + hwmon->rg.pkg_power_sku = INVALID_MMIO_REG; > + hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT; > + } else { > + hwmon->rg.pkg_power_sku_unit = INVALID_MMIO_REG; > + hwmon->rg.pkg_power_sku = INVALID_MMIO_REG; > + hwmon->rg.pkg_rapl_limit = INVALID_MMIO_REG; > + } > + > + wakeref = intel_runtime_pm_get(uncore->rpm); Let's just use with_intel_runtime_pm(). > + > + /* > + * The contents of register hwmon->rg.pkg_power_sku_unit do not change, > + * so read it once and store the shift values. > + * > + * For some platforms, this value is defined as available "for all > + * tiles", with the values consistent across all tiles. > + * In this case, use the tile 0 value for all. If we are going to introduce multiple tiles "later", let's introduce this comment later too. > + */ > + if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku_unit)) > + val_sku_unit = intel_uncore_read(uncore, > + hwmon->rg.pkg_power_sku_unit); > + else > + val_sku_unit = 0; > + > + intel_runtime_pm_put(uncore->rpm, wakeref); > + > + le_sku_unit = cpu_to_le32(val_sku_unit); > + hwmon->scl_shift_power = le32_get_bits(le_sku_unit, PKG_PWR_UNIT); Do we need such endianness conversions, typically we don't do them? > + > + /* > + * The value of power1_max is reset to the default on reboot, but is > + * not reset by a module unload/load sequence. To allow proper > + * functioning after a module reload, the value for power1_max is > + * restored to its original value at module unload time in > + * i915_hwmon_unregister(). > + */ Because on production systems typically modules are not reloaded, I am not sure if we need to take care of this save/restore. Also there may be other ways of doing this e.g.: https://patchwork.freedesktop.org/patch/483988/?series=102665&rev=3 That is above we just expose the default or init values but don't expose them. In order for such issues to be reviewed/debated, I would submit this save/restore part as a separate patch, so decouple it from the hwmon_power_max patch. > +void i915_hwmon_register(struct drm_i915_private *i915); > +void i915_hwmon_unregister(struct drm_i915_private *i915); > +#endif > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index d8579ab9384c..1c570c706ff8 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1866,6 +1866,21 @@ > #define POWER_LIMIT_4_MASK REG_BIT(9) > #define POWER_LIMIT_1_MASK REG_BIT(11) > #define POWER_LIMIT_2_MASK REG_BIT(12) > +/* > + * *_PACKAGE_POWER_SKU - SKU power and timing parameters. > + * Used herein as a 64-bit register. > + * These masks are defined using GENMASK_ULL as REG_GENMASK is limited to u32 > + * and as GENMASK is "long" and therefore 32-bits on a 32-bit system. > + * PKG_PKG_TDP, PKG_MIN_PWR, and PKG_MAX_PWR are scaled in the same way as > + * PKG_PWR_LIM_*, above. > + * PKG_MAX_WIN has sub-fields for x and y, and has the value: is 1.x * 2^y. > + */ > +#define PKG_PKG_TDP GENMASK_ULL(14, 0) > +#define PKG_MIN_PWR GENMASK_ULL(30, 16) > +#define PKG_MAX_PWR GENMASK_ULL(46, 32) > +#define PKG_MAX_WIN GENMASK_ULL(54, 48) > +#define PKG_MAX_WIN_Y GENMASK_ULL(54, 53) > +#define PKG_MAX_WIN_X GENMASK_ULL(52, 48) > > #define CHV_CLK_CTL1 _MMIO(0x101100) > #define VLV_CLK_CTL2 _MMIO(0x101104) > diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h b/drivers/gpu/drm/i915/intel_mchbar_regs.h > index 2aad2f0cc8db..247561d7974c 100644 > --- a/drivers/gpu/drm/i915/intel_mchbar_regs.h > +++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h > @@ -191,11 +191,20 @@ > > #define GEN6_GT_PERF_STATUS _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5948) > #define GEN6_RP_STATE_LIMITS _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5994) > +#define PCU_PACKAGE_POWER_SKU_UNIT _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5938) > +#define PKG_PWR_UNIT REG_GENMASK(3, 0) > +#define PKG_TIME_UNIT REG_GENMASK(19, 16) > + > #define GEN6_RP_STATE_CAP _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5998) > #define RP0_CAP_MASK REG_GENMASK(7, 0) > #define RP1_CAP_MASK REG_GENMASK(15, 8) > #define RPN_CAP_MASK REG_GENMASK(23, 16) > > +#define PCU_PACKAGE_RAPL_LIMIT _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x59a0) > +#define PKG_PWR_LIM_1 REG_GENMASK(14, 0) > +#define PKG_PWR_LIM_1_EN REG_BIT(15) > +#define PKG_PWR_LIM_1_TIME REG_GENMASK(23, 17) > + I think we should remove #define's which are not actually used in the patch and introduce them in the patches in which they are used. > /* snb MCH registers for priority tuning */ > #define MCH_SSKPD _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5d10) > #define SSKPD_NEW_WM0_MASK_HSW REG_GENMASK64(63, 56) > -- > 2.25.1 > Thanks. -- Ashutosh