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?
So I think we should make this explicit in the code below. Also change the
comment above to say only per-gt level energy.
> + * 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.
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
>
