On 12/3/2022 3:42 AM, Dixit, Ashutosh wrote:
On Tue, 29 Nov 2022 21:34:26 -0800, Riana Tauro wrote:
Hi Riana,
Mostly looks good but I have a little nit below.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index c588a17f97e9..57d4e96d5c72 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -442,6 +442,34 @@ hwm_energy_read(struct hwm_drvdata *ddat, u32 attr, long *val)
}
}
+/**
+ * i915_hwmon_get_energy - obtains energy value
+ * @gt: intel_gt structure
+ * @energy: pointer to store energy in uJ
+ *
+ * This function checks for the validity of the underlying energy
+ * hardware register and obtains the per-gt/package level energy
Do we every use this function to find real package level energy? I don't
see it. I think what we mean here is that package level energy if there's
only one gt and gt level energy is not available, correct?
Yes. When gt level energy is not available function should return the
package energy.
So I think we should make this explicit in the code below. Also change the
comment above to say only per-gt level energy.
Will change this.
+ * values.
+ *
+ * Return: 0 on success, -EOPNOTSUPP if register is invalid
+ */
+int
+i915_hwmon_get_energy(struct intel_gt *gt, long *energy)
+{
+ struct i915_hwmon *hwmon = gt->i915->hwmon;
+ struct hwm_drvdata *ddat = &hwmon->ddat;
+ struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + gt->info.id;
+
+ if (hwm_energy_is_visible(ddat_gt, hwmon_energy_input))
+ hwm_energy(ddat_gt, energy);
+ else if (hwm_energy_is_visible(ddat, hwmon_energy_input))
So if we get here and we are finding gt level energy there must be only one
gt, correct?
So probably we need to do something like (maybe in intel_gt.h?):
static inline int intel_num_gt(struct drm_i915_private *i915)
{
struct intel_gt *gt;
int num_gt = 0, i;
for_each_gt(gt, i915, i)
num_gt++;
return num_gt;
}
And then the above check becomes:
else if (intel_num_gt() == 1 &&
hwm_energy_is_visible(ddat, hwmon_energy_input))
So this way we are basically always returning gt level energy from
i915_hwmon_get_energy.
Is it okay to use this macro instead of adding a new function?
if (!HAS_EXTRA_GT_LIST(gt->i915) && hwm_energy_is_visible(ddat,
hwmon_energy_input))
Thanks
Riana
If ever we need package level energy in the future we can add a new
function which takes a 'struct drm_i915_private *i915' arg (and uses
i915->hwmon->ddat).
Thanks.
--
Ashutosh
+ hwm_energy(ddat, energy);
+ else
+ return -EOPNOTSUPP;
+
+ return 0;
+}
+
static umode_t
hwm_curr_is_visible(const struct hwm_drvdata *ddat, u32 attr)
{
diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h
index 7ca9cf2c34c9..1c38cfdbb7e9 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.h
+++ b/drivers/gpu/drm/i915/i915_hwmon.h
@@ -8,13 +8,16 @@
#define __I915_HWMON_H__
struct drm_i915_private;
+struct intel_gt;
#if IS_REACHABLE(CONFIG_HWMON)
void i915_hwmon_register(struct drm_i915_private *i915);
void i915_hwmon_unregister(struct drm_i915_private *i915);
+int i915_hwmon_get_energy(struct intel_gt *gt, long *energy);
#else
static inline void i915_hwmon_register(struct drm_i915_private *i915) { };
static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { };
+static inline int i915_hwmon_get_energy(struct intel_gt *gt, long *energy) { return -EOPNOTSUPP; }
#endif
#endif /* __I915_HWMON_H__ */
--
2.25.1
