Re: [PATCH v4 37/37] volt: add NvVoltOffsetmV option

[Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>]

On 18/04/16 22:14, Karol Herbst wrote:
> This option can be used to adjust the calculated voltage or the cstate voltage
> calculation
>
> Signed-off-by: Karol Herbst <nouveau@karolherbst.de>
> ---
>   bin/nv_cmp_volt.c                      |  2 +-
>   drm/nouveau/include/nvkm/subdev/volt.h |  4 +++-
>   drm/nouveau/nvkm/subdev/clk/base.c     |  8 ++++----
>   drm/nouveau/nvkm/subdev/volt/base.c    | 25 ++++++++++++++++++++-----
>   4 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/bin/nv_cmp_volt.c b/bin/nv_cmp_volt.c
> index e61056c..d1a0402 100644
> --- a/bin/nv_cmp_volt.c
> +++ b/bin/nv_cmp_volt.c
> @@ -117,7 +117,7 @@ main(int argc, char **argv)
>   
>   		new_voltage = nvkm_volt_get(volt);
>   		new_temp = nvkm_rd32(device, 0x20400);//nvkm_therm_temp_get(therm);
> -		new_nouveau_voltage = max(nvkm_volt_map(volt, best_cstate->voltage, new_temp), nvkm_volt_map(volt, best_pstate->base.voltage, new_temp));
> +		new_nouveau_voltage = max(nvkm_volt_map(volt, best_cstate->voltage, new_temp, true), nvkm_volt_map(volt, best_pstate->base.voltage, new_temp, false));
>   		new_pstate = best_pstate->pstate;
>   		new_cstate = best_cstate->cstate;
>   
> diff --git a/drm/nouveau/include/nvkm/subdev/volt.h b/drm/nouveau/include/nvkm/subdev/volt.h
> index 25588c7..f34fd39 100644
> --- a/drm/nouveau/include/nvkm/subdev/volt.h
> +++ b/drm/nouveau/include/nvkm/subdev/volt.h
> @@ -22,9 +22,11 @@ struct nvkm_volt {
>   	u8 max2_id;
>   
>   	int speedo;
> +
> +	int volt_offset_mv;
>   };
>   
> -int nvkm_volt_map(struct nvkm_volt *volt, u8 id, u8 temperature);
> +int nvkm_volt_map(struct nvkm_volt *volt, u8 id, u8 temperature, bool enableOffset);
>   int nvkm_volt_map_min(struct nvkm_volt *volt, u8 id);
>   int nvkm_volt_get(struct nvkm_volt *);
>   int nvkm_volt_set_id(struct nvkm_volt *, u8 id, u8 min_id, int condition);
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index d5440a9..7937155 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -100,7 +100,7 @@ nvkm_cstate_valid(struct nvkm_clk *clk, struct nvkm_cstate *cstate, u32 max_volt
>   	if (!volt)
>   		return true;
>   
> -	voltage = nvkm_volt_map(volt, cstate->voltage, temp);
> +	voltage = nvkm_volt_map(volt, cstate->voltage, temp, true);
>   	if (voltage < 0)
>   		return false;
>   	return voltage <= min(max_volt, volt->max_uv) &&
> @@ -131,13 +131,13 @@ nvkm_cstate_find_best(struct nvkm_clk *clk, struct nvkm_pstate *pstate,
>   	max_volt = volt->max_uv;
>   	if (volt->max0_id != 0xff)
>   		max_volt = min(max_volt,
> -			       nvkm_volt_map(volt, volt->max0_id, temp));
> +			       nvkm_volt_map(volt, volt->max0_id, temp, false));
>   	if (volt->max1_id != 0xff)
>   		max_volt = min(max_volt,
> -			       nvkm_volt_map(volt, volt->max1_id, temp));
> +			       nvkm_volt_map(volt, volt->max1_id, temp, false));
>   	if (volt->max2_id != 0xff)
>   		max_volt = min(max_volt,
> -			       nvkm_volt_map(volt, volt->max2_id, temp));
> +			       nvkm_volt_map(volt, volt->max2_id, temp, false));

So, this ugly dance of having true or flase depending on whether we are 
looking for a
pstate or a cstate looks really ugly, but it does make some sense. Given 
how useful this
may be to debug voltage-related issues, I would say it is worth the 
extra complexity (that
we really did not need).

Anyway: Reviewed-by: Martin Peres <martin.peres@free.fr>
>   
>   	for (cstate = start; &cstate->head != &pstate->list;
>   	     cstate = list_entry(cstate->head.prev, typeof(*cstate), head)) {
> diff --git a/drm/nouveau/nvkm/subdev/volt/base.c b/drm/nouveau/nvkm/subdev/volt/base.c
> index 5e35d96..7d5e6c8 100644
> --- a/drm/nouveau/nvkm/subdev/volt/base.c
> +++ b/drm/nouveau/nvkm/subdev/volt/base.c
> @@ -23,6 +23,8 @@
>    */
>   #include "priv.h"
>   
> +#include <core/option.h>
> +
>   #include <subdev/bios.h>
>   #include <subdev/bios/vmap.h>
>   #include <subdev/bios/volt.h>
> @@ -100,8 +102,8 @@ nvkm_volt_map_min(struct nvkm_volt *volt, u8 id)
>   	return id ? id * 10000 : -ENODEV;
>   }
>   
> -int
> -nvkm_volt_map(struct nvkm_volt *volt, u8 id, u8 temp)
> +static int
> +nvkm_volt_map_impl(struct nvkm_volt *volt, u8 id, u8 temp)
>   {
>   	struct nvkm_bios *bios = volt->subdev.device->bios;
>   	struct nvbios_vmap_entry info;
> @@ -145,7 +147,7 @@ nvkm_volt_map(struct nvkm_volt *volt, u8 id, u8 temp)
>   		result = min(max(result, (s64)info.min), (s64)info.max);
>   
>   		if (info.link != 0xff) {
> -			int ret = nvkm_volt_map(volt, info.link, temp);
> +			int ret = nvkm_volt_map_impl(volt, info.link, temp);
>   			if (ret < 0)
>   				return ret;
>   			result += ret;
> @@ -157,6 +159,15 @@ nvkm_volt_map(struct nvkm_volt *volt, u8 id, u8 temp)
>   }
>   
>   int
> +nvkm_volt_map(struct nvkm_volt *volt, u8 id, u8 temp, bool enable_offset)
> +{
> +	int res = nvkm_volt_map_impl(volt, id, temp);
> +	if (enable_offset)
> +		res += volt->volt_offset_mv * 1000;
> +	return res;
> +}
> +
> +int
>   nvkm_volt_set_id(struct nvkm_volt *volt, u8 id, u8 min_id, int condition)
>   {
>   	struct nvkm_therm *therm = volt->subdev.device->therm;
> @@ -169,13 +180,13 @@ nvkm_volt_set_id(struct nvkm_volt *volt, u8 id, u8 min_id, int condition)
>   	if (therm)
>   		temp = nvkm_therm_temp_get(therm);
>   
> -	ret = nvkm_volt_map(volt, id, max(temp, 0));
> +	ret = nvkm_volt_map(volt, id, max(temp, 0), true);
>   	if (ret >= 0) {
>   		int prev = nvkm_volt_get(volt);
>   		if (!condition || prev < 0 ||
>   		    (condition < 0 && ret < prev) ||
>   		    (condition > 0 && ret > prev)) {
> -			int min = nvkm_volt_map(volt, min_id, max(temp, 0));
> +			int min = nvkm_volt_map(volt, min_id, max(temp, 0), false);
>   			if (min >= 0)
>   				ret = max(min, ret);
>   			ret = nvkm_volt_set(volt, ret);
> @@ -308,6 +319,10 @@ nvkm_volt_ctor(const struct nvkm_volt_func *func, struct nvkm_device *device,
>   	volt->speedo = nvkm_volt_speedo_read(volt);
>   	if (volt->speedo > 0)
>   		nvkm_debug(&volt->subdev, "speedo %x\n", volt->speedo);
> +
> +	volt->volt_offset_mv = nvkm_longopt(device->cfgopt, "NvVoltOffsetmV", 0);
> +	if (volt->volt_offset_mv)
> +		nvkm_info(&volt->subdev, "Volt Offset applied: %i\n", volt->volt_offset_mv);
>   }
>   
>   int

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau