[Nouveau] [RFC PATCH 5/5] clk: allow boosting only when NvBoost is set

[Pierre Moreau]

Hi Karol,

I have some comments below.

On 05:42 PM - Dec 01 2015, Karol Herbst wrote:
> 0: disable boosting (cap to base clock from the vbios)
> 1: boost only to boost clock from the vbios
> 2: boost to max clock available
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h | 10 +++++++++-
>  drm/nouveau/nvkm/subdev/clk/base.c    | 17 ++++++++++++++++-
>  drm/nouveau/nvkm/subdev/clk/gf100.c   |  2 +-
>  drm/nouveau/nvkm/subdev/clk/gk104.c   |  2 +-
>  4 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index 8708f0a..8085d81 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -64,7 +64,8 @@ struct nvkm_pstate {
>  struct nvkm_domain {
>  	enum nv_clk_src name;
>  	u8 bios; /* 0xff for none */
> -#define NVKM_CLK_DOM_FLAG_CORE 0x01
> +#define NVKM_CLK_DOM_FLAG_CORE            0x01
> +#define NVKM_CLK_DOM_FLAG_BASE_CLOCK_CORE 0x02
>  	u8 flags;
>  	const char *mname;
>  	int mdiv;
> @@ -94,6 +95,13 @@ struct nvkm_clk {
>  	int dstate; /* display adjustment (min+) */
>  
>  	bool allow_reclock;
> +#define NVKM_CLK_BOOST_MODE_NONE 0x0
> +#define NVKM_CLK_BOOST_MODE_AVG  0x1
> +#define NVKM_CLK_BOOST_MODE_FULL 0x2
> +	u8 boost_mode;
> +
> +	u32 base_clock;
> +	u32 boost_clock;
>  
>  	/*XXX: die, these are here *only* to support the completely
>  	 *     bat-shit insane what-was-nouveau_hw.c code
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index df9173e..ae76601 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -166,6 +166,12 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
>  		if (domain->flags & NVKM_CLK_DOM_FLAG_CORE) {
>  			u32 freq = nvkm_clk_adjust(clk, true, pstate->pstate,
>  						   domain->bios, cstepX.freq);
> +			if (domain->flags & NVKM_CLK_DOM_FLAG_BASE_CLOCK_CORE) {
> +				if (clk->boost_mode == 0 && freq > clk->base_clock)
> +					goto err;
> +				if (clk->boost_mode == 1 && freq > clk->boost_clock)
> +					goto err;
> +			}
>  			cstate->domain[domain->name] = freq;
>  		}
>  		domain++;
> @@ -173,6 +179,9 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
>  
>  	list_add(&cstate->head, &pstate->list);
>  	return 0;
> +err:
> +	kfree(cstate);
> +	return -EINVAL;
>  }
>  
>  /******************************************************************************
> @@ -573,13 +582,19 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>  
>  	if (bios && !nvbios_baseclock_parse(bios, &header)) {
>  		struct nvbios_baseclock_entry base_entry, boost_entry;
> +		clk->boost_mode = nvkm_longopt(device->cfgopt, "NvBoost", 0);
>  		if (nvbios_baseclock_get_entry(bios, &header, header.base_entry, &base_entry))

If `boost_mode == -1` is some "went wrong, don't use" value, shouldn't you set
`clk->boost_mode = -1;` here too?

>  			nvkm_error(&clk->subdev, "couldn't parse base clock\n");
>  		else if (nvbios_baseclock_get_entry(bios, &header, header.boost_entry, &boost_entry))

Same comment as above

>  			nvkm_error(&clk->subdev, "couldn't parse boost clock\n");
> -		else
> +		else {
> +			clk->base_clock = base_entry.clock_mhz * 1000;
> +			clk->boost_clock = boost_entry.clock_mhz * 1000;
>  			nvkm_info(&clk->subdev, "base: %i MHz, boost: %i MHz\n",
>  				base_entry.clock_mhz / 2, boost_entry.clock_mhz / 2);
> +		}
> +	} else {
> +		clk->boost_mode = -1;

You only listed values 0, 1, 2 in the commit message, so what is -1 for?
Disabling nvboost is already taken care by 0, so it's something else. runpm
uses it for auto mode, but here you set it if the bios wasn't found or couldn't
be parsed, so not an auto mode. :-D


Regards,
Pierre


>  	}
>  
>  	clk->func = func;
> diff --git a/drm/nouveau/nvkm/subdev/clk/gf100.c b/drm/nouveau/nvkm/subdev/clk/gf100.c
> index a52b7e7..eaf4f83 100644
> --- a/drm/nouveau/nvkm/subdev/clk/gf100.c
> +++ b/drm/nouveau/nvkm/subdev/clk/gf100.c
> @@ -443,7 +443,7 @@ gf100_clk = {
>  		{ nv_clk_src_hubk06 , 0x00 },
>  		{ nv_clk_src_hubk01 , 0x01 },
>  		{ nv_clk_src_copy   , 0x02 },
> -		{ nv_clk_src_gpc    , 0x03, 0, "core", 2000 },
> +		{ nv_clk_src_gpc    , 0x03, NVKM_CLK_DOM_FLAG_BASE_CLOCK_CORE, "core", 2000 },
>  		{ nv_clk_src_rop    , 0x04 },
>  		{ nv_clk_src_mem    , 0x05, 0, "memory", 1000 },
>  		{ nv_clk_src_vdec   , 0x06 },
> diff --git a/drm/nouveau/nvkm/subdev/clk/gk104.c b/drm/nouveau/nvkm/subdev/clk/gk104.c
> index 396f7e4..f194112 100644
> --- a/drm/nouveau/nvkm/subdev/clk/gk104.c
> +++ b/drm/nouveau/nvkm/subdev/clk/gk104.c
> @@ -485,7 +485,7 @@ gk104_clk = {
>  	.domains = {
>  		{ nv_clk_src_crystal, 0xff },
>  		{ nv_clk_src_href   , 0xff },
> -		{ nv_clk_src_gpc    , 0x00, NVKM_CLK_DOM_FLAG_CORE, "core", 2000 },
> +		{ nv_clk_src_gpc    , 0x00, NVKM_CLK_DOM_FLAG_CORE | NVKM_CLK_DOM_FLAG_BASE_CLOCK_CORE, "core", 2000 },
>  		{ nv_clk_src_hubk07 , 0x01, NVKM_CLK_DOM_FLAG_CORE },
>  		{ nv_clk_src_rop    , 0x02, NVKM_CLK_DOM_FLAG_CORE },
>  		{ nv_clk_src_mem    , 0x03, 0, "memory", 500 },
> -- 
> 2.6.3
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau