Pierre Moreau
2015-Dec-02 01:26 UTC
[Nouveau] [RFC PATCH 5/5] clk: allow boosting only when NvBoost is set
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
Karol Herbst
2015-Dec-02 12:24 UTC
[Nouveau] [RFC PATCH 5/5] clk: allow boosting only when NvBoost is set
> Pierre Moreau <pierre.morrow at free.fr> hat am 2. Dezember 2015 um 02:26 > geschrieben: > > 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 >yeah right, I think I stick with 0 then> 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
Emil Velikov
2015-Dec-02 12:32 UTC
[Nouveau] [RFC PATCH 5/5] clk: allow boosting only when NvBoost is set
On 2 December 2015 at 12:24, Karol Herbst <nouveau at karolherbst.de> wrote:>> Pierre Moreau <pierre.morrow at free.fr> hat am 2. Dezember 2015 um 02:26 >> geschrieben:>> 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 >> > yeah right, I think I stick with 0 then >Speaking of which why don't you use the symbolic names ? It feels a bit strange to define them and then check against 0,1... Regards, Emil
Apparently Analagous Threads
- [RFC PATCH 5/5] clk: allow boosting only when NvBoost is set
- [RFC PATCH 5/5] clk: allow boosting only when NvBoost is set
- [PATCH 05/19] clk: allow boosting only when NvBoost is set
- [PATCH v4 15/37] clk: allow boosting only when NvBoost is set
- [RFC PATCH 0/5] stabilize kepler reclocking