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

[Karol Herbst]

> 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