Re: [Intel-gfx] [PATCH 7/7] drm/i915/hwmon: Extend power/energy for XEHPSDV

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

On Thu, 25 Aug 2022 06:21:18 -0700, Badal Nilawar wrote:
>
> From: Dale B Stimson <dale.b.stimson@intel.com>
>
> Extend hwmon power/energy for XEHPSDV especially per gt level energy
> usage.

A couple of very minor nits below which need to be fixed. But otherwise
this patch is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index b8ac52f07681..76d73216c54e 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -12,6 +12,7 @@
>  #include "i915_reg.h"
>  #include "intel_mchbar_regs.h"
>  #include "intel_pcode.h"
> +#include "gt/intel_gt.h"
>  #include "gt/intel_gt_regs.h"
>
>  /*
> @@ -21,7 +22,7 @@
>   * - curr   - milliamperes
>   * - energy - microjoules
>   */
> -#define SF_TIME            1000
> +#define SF_TIME		1000

This change should not be in this patch, it seems an unnecessary space has
been inserted.

> @@ -622,6 +690,10 @@ void i915_hwmon_register(struct drm_i915_private *i915)
>	struct i915_hwmon *hwmon;
>	struct device *hwmon_dev;
>	struct hwm_drvdata *ddat;
> +	struct hwm_drvdata *ddat_gt;
> +	struct intel_gt *gt;
> +	const char *ddname;

Variable is not needed, let's delete it and directly use ddat_gt->name.

>  void i915_hwmon_unregister(struct drm_i915_private *i915)
>  {
>	struct i915_hwmon *hwmon;
>	struct hwm_drvdata *ddat;
> +	struct intel_gt *gt;
> +	int i;
>
>	hwmon = fetch_and_zero(&i915->hwmon);
>	if (!hwmon)
>		return;
>
>	ddat = &hwmon->ddat;
> +
> +	for_each_gt(gt, i915, i) {
> +		struct hwm_drvdata *ddat_gt;
> +
> +		ddat_gt = hwmon->ddat_gt + i;
> +
> +		if (ddat_gt->hwmon_dev) {
> +			hwmon_device_unregister(ddat_gt->hwmon_dev);
> +			ddat_gt->hwmon_dev = NULL;

No need to set to NULL, we are doing 'kfree(hwmon)' below, let's just
delete this line.

Thanks.
--
Ashutosh