From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
To: Badal Nilawar <badal.nilawar@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Add HWMON power sensor support
Date: Mon, 23 May 2022 20:10:46 -0700 [thread overview]
Message-ID: <877d6bvd55.wl-ashutosh.dixit@intel.com> (raw)
In-Reply-To: <20220523110841.1151431-2-badal.nilawar@intel.com>
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
next prev parent reply other threads:[~2022-05-24 3:10 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-23 11:08 [Intel-gfx] [PATCH 0/3] drm/i915: Add HWMON support Badal Nilawar
2022-05-23 11:08 ` [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Add HWMON power sensor support Badal Nilawar
2022-05-23 13:45 ` Jani Nikula
2022-05-23 13:46 ` Jani Nikula
2022-05-24 3:10 ` Dixit, Ashutosh [this message]
2022-05-23 11:08 ` [Intel-gfx] [PATCH 2/3] drm/i915/hwmon: Add HWMON energy support Badal Nilawar
2022-05-25 4:55 ` Dixit, Ashutosh
2022-05-25 4:56 ` Dixit, Ashutosh
2022-05-25 5:25 ` Gupta, Anshuman
2022-05-23 11:08 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Add HWMON current voltage support Badal Nilawar
2022-05-23 13:47 ` Jani Nikula
2022-05-25 5:01 ` Dixit, Ashutosh
2022-06-10 22:35 ` Dixit, Ashutosh
2022-06-11 0:45 ` Dixit, Ashutosh
2022-06-11 0:55 ` Dixit, Ashutosh
2022-05-23 17:03 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add HWMON support Patchwork
2022-05-23 17:26 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2022-05-24 1:36 ` [Intel-gfx] [PATCH 0/3] " Dixit, Ashutosh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877d6bvd55.wl-ashutosh.dixit@intel.com \
--to=ashutosh.dixit@intel.com \
--cc=badal.nilawar@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.