On Sun, 04 Dec 2022 23:44:57 -0800, Tauro, Riana wrote:
>
> 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))
Yes, good find, looks like that's what this is. Thanks.
>
> 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
> >>
