| Message ID | 20221130053427.2207600-3-riana.tauro@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add hwmon support for dgfx selftests | expand |
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 >
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)) 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 >>
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 > >>
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 + * 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)) + 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__ */
Add an interface to obtain hwmon energy values. The function returns per-gt energy if register is valid else returns the device level energy. This is used by selftest to verify power consumption v2 : use i915_hwmon prefix (Anshuman) v3 : re-use is_visible function of energy to remove redundant code (Anshuman) v4 : fix kernel-doc (Anshuman) add per-gt hwmon support (Ashutosh) Signed-off-by: Riana Tauro <riana.tauro@intel.com> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 28 ++++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_hwmon.h | 3 +++ 2 files changed, 31 insertions(+)