[PATCH i-g-t 3/5] lib/igt_power: Add power_supply/BAT based measurement

[Kamil Konieczny]

Hi Ville,
On 2024-09-16 at 23:18:39 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Allow measuring total system power via the power_supply class
> stuff in sysfs (typically the information there comes from ACPI).
> 
> There are two types of batteries:
> - reports remaining capacity in uWh (energy_now)
> - reports remaining capacity in uAh (charge_now)
> 
> The first type is easier since we just have to convert to
> uJ. The second type is less convenient since we also need
> the voltage to determine energy. For that we just multiply
> with voltage_now (in uV) when sampling charge_now. Obviously
> this isn't entirely correct (should integrate instead), but
> I think it's close enough to be useful.
> 
> Also one should keep in mind that the battery reports remaining
> energy, not consumed energy. So we have to flip stuff around when
> calculating things. Another alternative would be flip already
> when measuring (eg. {charge,energy}_full_design - {charge,energy}_now),
> but that seems more of a hassle really.
> 
> TODO: maybe confirm that the AC adapter is unplugged and the
> battery is actually reporting to discharge before we start?
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  lib/igt_power.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_power.h |  5 ++++-
>  2 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_power.c b/lib/igt_power.c
> index e891da87acf3..a3e8ee588365 100644
> --- a/lib/igt_power.c
> +++ b/lib/igt_power.c
> @@ -85,6 +85,21 @@ static inline void rapl_close(struct rapl *r)
>  	r->fd = -1;
>  }
>  
> +static uint64_t bat_get_energy(int fd)
> +{
> +	if (igt_sysfs_has_attr(fd, "energy_now")) {
> +		/* uWh -> uJ */
> +		return 3600 *
> +			igt_sysfs_get_u64(fd, "energy_now");
> +	} else {
> +		/* uAh * uV -> uJ */
> +		return 3600 *
> +			igt_sysfs_get_u64(fd, "charge_now") *
> +			igt_sysfs_get_u64(fd, "voltage_now") /
> +			1000000;
> +	}
> +}
> +
>  /**
>   * igt_power_open:
>   * @fd : device fd
> @@ -103,6 +118,7 @@ int igt_power_open(int fd, struct igt_power *p, const char *domain)
>  	int i;
>  
>  	p->hwmon_fd = -1;
> +	p->bat_fd = -1;
>  	p->rapl.fd = -1;
>  
>  	if (fd >= 0 && is_intel_dgfx(fd)) {
> @@ -140,6 +156,8 @@ void igt_power_get_energy(struct igt_power *power, struct power_sample *s)
>  	if (power->hwmon_fd >= 0) {
>  		if (igt_sysfs_has_attr(power->hwmon_fd, "energy1_input"))
>  			s->energy = igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
> +	} else if (power->bat_fd >= 0) {
> +		s->energy = bat_get_energy(power->bat_fd);
>  	} else if (power->rapl.fd >= 0) {
>  		rapl_read(&power->rapl, s);
>  	}
> @@ -160,6 +178,8 @@ double igt_power_get_mJ(const struct igt_power *power,
>  {
>  	if (power->hwmon_fd >= 0)
>  		return (p1->energy - p0->energy) * 1e-3;
> +	else if (power->bat_fd >= 0)
> +		return (p0->energy - p1->energy) * 1e-3; /* battery measures remaining energy */
>  	else if (power->rapl.fd >= 0)
>  		return ((p1->energy - p0->energy) *  power->rapl.scale) * 1e3;
>  
> @@ -207,7 +227,47 @@ void igt_power_close(struct igt_power *power)
>  	if (power->hwmon_fd >= 0) {
>  		close(power->hwmon_fd);
>  		power->hwmon_fd = -1;
> +	} else if (power->bat_fd >= 0) {
> +		close(power->bat_fd);
> +		power->bat_fd = -1;
>  	} else if (power->rapl.fd >= 0) {
>  		rapl_close(&power->rapl);
>  	}
>  }
> +
> +/**
> + * igt_power_bat_open:
> + * @igt_power : power struct
> + * @index: battery index
> + *
> + * opens the power_supply fd based on battery index
> + *
> + * Returns
> + * 0 on success, errno otherwise

imho -1 in case of error?

> + */
> +int igt_power_bat_open(struct igt_power *p, int index)
> +{
> +	char path[64];
> +	int fd;
> +
> +	p->hwmon_fd = -1;
> +	p->bat_fd = -1;
> +	p->rapl.fd = -1;
> +
> +	snprintf(path, sizeof(path), "/sys/class/power_supply/BAT%d", index);
> +
> +	fd = open(path, O_RDONLY | O_DIRECTORY);
> +	if (fd < 0)
> +		return fd;
> +
> +	if (!igt_sysfs_has_attr(fd, "energy_now") &&
> +	    !(igt_sysfs_has_attr(fd, "charge_now") &&
> +	      igt_sysfs_has_attr(fd, "voltage_now"))) {
> +		close(fd);

Add newline before return.

With this fixed
Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

> +		return -1;
> +	}
> +
> +	p->bat_fd = fd;
> +
> +	return 0;
> +}
> diff --git a/lib/igt_power.h b/lib/igt_power.h
> index 68a05300eec2..853b0fae815a 100644
> --- a/lib/igt_power.h
> +++ b/lib/igt_power.h
> @@ -42,14 +42,17 @@ struct power_sample {
>  struct igt_power {
>  	struct rapl rapl;
>  	int hwmon_fd;
> +	int bat_fd;
>  };
>  
>  int igt_power_open(int i915, struct igt_power *p, const char *domain);
>  void igt_power_close(struct igt_power *p);
>  
> +int igt_power_bat_open(struct igt_power *p, int index);
> +
>  static inline bool igt_power_valid(struct igt_power *p)
>  {
> -	return (p->rapl.fd >= 0) || (p->hwmon_fd >= 0);
> +	return p->rapl.fd >= 0 || p->hwmon_fd >= 0 || p->bat_fd >= 0;
>  }
>  
>  void igt_power_get_energy(struct igt_power *p, struct power_sample *s);
> -- 
> 2.44.2
>