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

["Tauro, Riana" <riana.tauro@intel.com>]

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
>>