On Thu, 25 Aug 2022 06:21:18 -0700, Badal Nilawar wrote:
>
> From: Dale B Stimson <dale.b.stim...@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.di...@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
