Re: [Intel-gfx] [PATCH v5 2/3] drm/i915/hwmon: Add helper function to obtain energy values

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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
>