| Message ID | 1400258565-28941-1-git-send-email-imirkin@alum.mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17 May 2014 02:43, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: > > Adds a NvReclock boolean option to allow the user to enable (or disable) > reclocking. All chipsets default to off, except NVAA/NVAC, which are > reportedly complete. Hey Ilia, I think I've expressed my thoughts on this previously via IRC, but let me stick them here too so there's a record of the current state... For nvaa/nvac, yes, let's enable it by default. It should, apparently, be good enough that it has a decent chance of working. It's not like we're attempting anything automatic yet, so, this won't break anything for people who aren't trying.. I'm on the fence about Kepler. It actually might work to some extent in a decent number of cases already, there's potentially some severe issues even with engine clocks on some boards that I'm aware of, so it's not just a memory reclocking worry here. That said, it has a good chance of working for some people. So. Thoughts? I'm also talking making "NvMemExec" default on here too. Again, causing a fuck-up will still require direct user action. For the rest (Hm, except maybe nv40, a lot will probably be ok..) There's *very* little chance memory reclocking will work, even on the systems where it used to. The code is far less complete, as it was broken in general, and I haven't yet had the time to *properly* reverse engineer the sequence needed to stably reclock memory. Kepler is the only implementation where that's even been started. Tl;dr - unless you're working on the code for Tesla/Fermi, there's zero point even trying it. So, the block should stay. Personally, as you know, I'm more comfortable leaving it developer-only still (except nvaa/nvac) until it at least works on all our own boards without any major known missing bits.. But yeah, for the ones mentioned above, I guess it's a possibility if people *really* want... I can only envision that if we allow this even just in the places it's known to be partially broken, certain sensationalist, er, people, will feel the need to test and complain about how broken it all really is... And then retest on a regular basis, despite there having been *zero* work done because no-one has the time, and then complain about the exact same thing AGAIN! (WHOA.. I'm done ranting now :p) Anyways, that's my thoughts on the matter :) Comments, suggestions? Thanks, Ben. > > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> > --- > > Ben, I know you've been saying that reclocking is in a pretty bad state, but I > do think that there are going to be groups of people for whom the current code > can work at least a little bit. May as well let them try it. The memory script > execution is still behind the extra-special flag as well... > > Also this provides a way forward to enable reclocking on some chips and leave > it off for others, by default. > > nvkm/include/subdev/clock.h | 8 +++++--- > nvkm/subdev/clock/base.c | 8 ++++++-- > nvkm/subdev/clock/nv04.c | 3 ++- > nvkm/subdev/clock/nv40.c | 3 ++- > nvkm/subdev/clock/nv50.c | 2 +- > nvkm/subdev/clock/nva3.c | 3 ++- > nvkm/subdev/clock/nvaa.c | 3 ++- > nvkm/subdev/clock/nvc0.c | 3 ++- > nvkm/subdev/clock/nve0.c | 3 ++- > 9 files changed, 24 insertions(+), 12 deletions(-) > > diff --git a/nvkm/include/subdev/clock.h b/nvkm/include/subdev/clock.h > index 8f4ced7..c01e29c 100644 > --- a/nvkm/include/subdev/clock.h > +++ b/nvkm/include/subdev/clock.h > @@ -77,6 +77,8 @@ struct nouveau_clock { > int tstate; /* thermal adjustment (max-) */ > int dstate; /* display adjustment (min+) */ > > + bool allow_reclock; > + > int (*read)(struct nouveau_clock *, enum nv_clk_src); > int (*calc)(struct nouveau_clock *, struct nouveau_cstate *); > int (*prog)(struct nouveau_clock *); > @@ -106,8 +108,8 @@ struct nouveau_clocks { > int mdiv; > }; > > -#define nouveau_clock_create(p,e,o,i,d) \ > - nouveau_clock_create_((p), (e), (o), (i), sizeof(**d), (void **)d) > +#define nouveau_clock_create(p,e,o,i,r,d) \ > + nouveau_clock_create_((p), (e), (o), (i), (r), sizeof(**d), (void **)d) > #define nouveau_clock_destroy(p) ({ \ > struct nouveau_clock *clk = (p); \ > _nouveau_clock_dtor(nv_object(clk)); \ > @@ -121,7 +123,7 @@ struct nouveau_clocks { > > int nouveau_clock_create_(struct nouveau_object *, struct nouveau_object *, > struct nouveau_oclass *, > - struct nouveau_clocks *, int, void **); > + struct nouveau_clocks *, bool, int, void **); > void _nouveau_clock_dtor(struct nouveau_object *); > int _nouveau_clock_init(struct nouveau_object *); > #define _nouveau_clock_fini _nouveau_subdev_fini > diff --git a/nvkm/subdev/clock/base.c b/nvkm/subdev/clock/base.c > index dd62bae..80ad9d3 100644 > --- a/nvkm/subdev/clock/base.c > +++ b/nvkm/subdev/clock/base.c > @@ -346,8 +346,8 @@ nouveau_clock_ustate_update(struct nouveau_clock *clk, int req) > struct nouveau_pstate *pstate; > int i = 0; > > - /* YKW repellant */ > - return -ENOSYS; > + if (!clk->allow_reclock) > + return -ENOSYS; > > if (req != -1 && req != -2) { > list_for_each_entry(pstate, &clk->states, head) { > @@ -456,6 +456,7 @@ nouveau_clock_create_(struct nouveau_object *parent, > struct nouveau_object *engine, > struct nouveau_oclass *oclass, > struct nouveau_clocks *clocks, > + bool allow_reclock, > int length, void **object) > { > struct nouveau_device *device = nv_device(parent); > @@ -478,6 +479,9 @@ nouveau_clock_create_(struct nouveau_object *parent, > ret = nouveau_pstate_new(clk, idx++); > } while (ret == 0); > > + clk->allow_reclock = > + nouveau_boolopt(device->cfgopt, "NvReclock", allow_reclock); > + > mode = nouveau_stropt(device->cfgopt, "NvClkMode", &arglen); > if (mode) { > if (!strncasecmpz(mode, "disabled", arglen)) { > diff --git a/nvkm/subdev/clock/nv04.c b/nvkm/subdev/clock/nv04.c > index b74db6c..eb2d442 100644 > --- a/nvkm/subdev/clock/nv04.c > +++ b/nvkm/subdev/clock/nv04.c > @@ -82,7 +82,8 @@ nv04_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, > struct nv04_clock_priv *priv; > int ret; > > - ret = nouveau_clock_create(parent, engine, oclass, nv04_domain, &priv); > + ret = nouveau_clock_create(parent, engine, oclass, nv04_domain, false, > + &priv); > *pobject = nv_object(priv); > if (ret) > return ret; > diff --git a/nvkm/subdev/clock/nv40.c b/nvkm/subdev/clock/nv40.c > index db7346f..651e299 100644 > --- a/nvkm/subdev/clock/nv40.c > +++ b/nvkm/subdev/clock/nv40.c > @@ -213,7 +213,8 @@ nv40_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, > struct nv40_clock_priv *priv; > int ret; > > - ret = nouveau_clock_create(parent, engine, oclass, nv40_domain, &priv); > + ret = nouveau_clock_create(parent, engine, oclass, nv40_domain, false, > + &priv); > *pobject = nv_object(priv); > if (ret) > return ret; > diff --git a/nvkm/subdev/clock/nv50.c b/nvkm/subdev/clock/nv50.c > index 250a6d9..8c13277 100644 > --- a/nvkm/subdev/clock/nv50.c > +++ b/nvkm/subdev/clock/nv50.c > @@ -507,7 +507,7 @@ nv50_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, > int ret; > > ret = nouveau_clock_create(parent, engine, oclass, pclass->domains, > - &priv); > + false, &priv); > *pobject = nv_object(priv); > if (ret) > return ret; > diff --git a/nvkm/subdev/clock/nva3.c b/nvkm/subdev/clock/nva3.c > index 4f5a137..9fb5835 100644 > --- a/nvkm/subdev/clock/nva3.c > +++ b/nvkm/subdev/clock/nva3.c > @@ -302,7 +302,8 @@ nva3_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, > struct nva3_clock_priv *priv; > int ret; > > - ret = nouveau_clock_create(parent, engine, oclass, nva3_domain, &priv); > + ret = nouveau_clock_create(parent, engine, oclass, nva3_domain, false, > + &priv); > *pobject = nv_object(priv); > if (ret) > return ret; > diff --git a/nvkm/subdev/clock/nvaa.c b/nvkm/subdev/clock/nvaa.c > index 7a723b4..6a65fc9 100644 > --- a/nvkm/subdev/clock/nvaa.c > +++ b/nvkm/subdev/clock/nvaa.c > @@ -421,7 +421,8 @@ nvaa_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, > struct nvaa_clock_priv *priv; > int ret; > > - ret = nouveau_clock_create(parent, engine, oclass, nvaa_domains, &priv); > + ret = nouveau_clock_create(parent, engine, oclass, nvaa_domains, true, > + &priv); > *pobject = nv_object(priv); > if (ret) > return ret; > diff --git a/nvkm/subdev/clock/nvc0.c b/nvkm/subdev/clock/nvc0.c > index c310572..dbf8517 100644 > --- a/nvkm/subdev/clock/nvc0.c > +++ b/nvkm/subdev/clock/nvc0.c > @@ -437,7 +437,8 @@ nvc0_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, > struct nvc0_clock_priv *priv; > int ret; > > - ret = nouveau_clock_create(parent, engine, oclass, nvc0_domain, &priv); > + ret = nouveau_clock_create(parent, engine, oclass, nvc0_domain, false, > + &priv); > *pobject = nv_object(priv); > if (ret) > return ret; > diff --git a/nvkm/subdev/clock/nve0.c b/nvkm/subdev/clock/nve0.c > index d3c37c9..860aa73 100644 > --- a/nvkm/subdev/clock/nve0.c > +++ b/nvkm/subdev/clock/nve0.c > @@ -473,7 +473,8 @@ nve0_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, > struct nve0_clock_priv *priv; > int ret; > > - ret = nouveau_clock_create(parent, engine, oclass, nve0_domain, &priv); > + ret = nouveau_clock_create(parent, engine, oclass, nve0_domain, false, > + &priv); > *pobject = nv_object(priv); > if (ret) > return ret; > -- > 1.8.5.5 > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau
On Fri, May 16, 2014 at 11:17 PM, Ben Skeggs <skeggsb@gmail.com> wrote: > On 17 May 2014 02:43, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: >> >> Adds a NvReclock boolean option to allow the user to enable (or disable) >> reclocking. All chipsets default to off, except NVAA/NVAC, which are >> reportedly complete. > Hey Ilia, > > I think I've expressed my thoughts on this previously via IRC, but let me > stick them here too so there's a record of the current state... > > For nvaa/nvac, yes, let's enable it by default. It should, apparently, be > good enough that it has a decent chance of working. It's not like we're > attempting anything automatic yet, so, this won't break anything for people > who aren't trying.. > > I'm on the fence about Kepler. It actually might work to some extent in a > decent number of cases already, there's potentially some severe issues even > with engine clocks on some boards that I'm aware of, so it's not just a > memory reclocking worry here. > > That said, it has a good chance of working for some people. So. Thoughts? > I'm also talking making "NvMemExec" default on here too. Again, causing a > fuck-up will still require direct user action. > > For the rest (Hm, except maybe nv40, a lot will probably be ok..) There's > *very* little chance memory reclocking will work, even on the systems where > it used to. The code is far less complete, as it was broken in general, and > I haven't yet had the time to *properly* reverse engineer the sequence > needed to stably reclock memory. Kepler is the only implementation where > that's even been started. Tl;dr - unless you're working on the code for > Tesla/Fermi, there's zero point even trying it. So, the block should stay. Meh. It works on my G98, for one of the perf levels. I'm sure there are lots of tesla's where it totally wouldn't work, but as long as it works on some of them, why not let people try? > > Personally, as you know, I'm more comfortable leaving it developer-only > still (except nvaa/nvac) until it at least works on all our own boards > without any major known missing bits.. But yeah, for the ones mentioned > above, I guess it's a possibility if people *really* want... I'm of the opposite opinion... if it works on _some_ of our boards, we should let people play with it. Why lock it away? Unless there's a real danger of it bricking the card. I've never heard of our code doing that, and given the way that you were RE'ing this stuff, I doubt that there's anything we can do (within reason) to brick the card. > > I can only envision that if we allow this even just in the places it's known > to be partially broken, certain sensationalist, er, people, will feel the > need to test and complain about how broken it all really is... And then > retest on a regular basis, despite there having been *zero* work done > because no-one has the time, and then complain about the exact same thing > AGAIN! (WHOA.. I'm done ranting now :p) I would prefer to avoid our decisions being directed by a small number of loud complaining users, and instead to try to do things that will serve the real users. Those complaints are only as loud as you think they are -- you can also think of them as an automated tester that puts its results into prose. Prior to 3.13, we allowed people to try reclocking on nv40 and nv50, and I didn't see some huge quantity of complaints about how it didn't work perfectly. Perhaps you saw those at first, but I think the expectation by now is that it won't work. Especially if it's behind a config option. I have no idea what NvMemExec _really_ is doing, so I left it alone. I assume that the majority of what my patch enables is actually engine reclocking, not memory reclocking. So to get both, people would have to flip both flags? Or is there more to it? -ilia
On 17 May 2014 13:39, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: > > On Fri, May 16, 2014 at 11:17 PM, Ben Skeggs <skeggsb@gmail.com> wrote: > > On 17 May 2014 02:43, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: > >> > >> Adds a NvReclock boolean option to allow the user to enable (or disable) > >> reclocking. All chipsets default to off, except NVAA/NVAC, which are > >> reportedly complete. > > Hey Ilia, > > > > I think I've expressed my thoughts on this previously via IRC, but let me > > stick them here too so there's a record of the current state... > > > > For nvaa/nvac, yes, let's enable it by default. It should, apparently, be > > good enough that it has a decent chance of working. It's not like we're > > attempting anything automatic yet, so, this won't break anything for people > > who aren't trying.. > > > > I'm on the fence about Kepler. It actually might work to some extent in a > > decent number of cases already, there's potentially some severe issues even > > with engine clocks on some boards that I'm aware of, so it's not just a > > memory reclocking worry here. > > > > That said, it has a good chance of working for some people. So. Thoughts? > > I'm also talking making "NvMemExec" default on here too. Again, causing a > > fuck-up will still require direct user action. > > > > For the rest (Hm, except maybe nv40, a lot will probably be ok..) There's > > *very* little chance memory reclocking will work, even on the systems where > > it used to. The code is far less complete, as it was broken in general, and > > I haven't yet had the time to *properly* reverse engineer the sequence > > needed to stably reclock memory. Kepler is the only implementation where > > that's even been started. Tl;dr - unless you're working on the code for > > Tesla/Fermi, there's zero point even trying it. So, the block should stay. > > Meh. It works on my G98, for one of the perf levels. I'm sure there > are lots of tesla's where it totally wouldn't work, but as long as it > works on some of them, why not let people try? Because we don't even *try* to handle timing changes, which, for the vast majority perflvl changes, will not end well. This is more than a "mere" bug or slight bit of missing handling. It's a *very* important chunk of the process that's simply missing. This is the difference between the Kepler code. Kepler might work, this probably will not, unless you're very very lucky. > > > > > Personally, as you know, I'm more comfortable leaving it developer-only > > still (except nvaa/nvac) until it at least works on all our own boards > > without any major known missing bits.. But yeah, for the ones mentioned > > above, I guess it's a possibility if people *really* want... > > I'm of the opposite opinion... if it works on _some_ of our boards, we > should let people play with it. Why lock it away? Unless there's a > real danger of it bricking the card. I've never heard of our code > doing that, and given the way that you were RE'ing this stuff, I doubt > that there's anything we can do (within reason) to brick the card. This is why I'm "Hmm, maybe" on Kepler's code. Again, the rest isn't even expected to work at all yet. I'm surprised it did for you.. Were the clocks you chose close to the boot clocks by chance? > > > > > I can only envision that if we allow this even just in the places it's known > > to be partially broken, certain sensationalist, er, people, will feel the > > need to test and complain about how broken it all really is... And then > > retest on a regular basis, despite there having been *zero* work done > > because no-one has the time, and then complain about the exact same thing > > AGAIN! (WHOA.. I'm done ranting now :p) > > I would prefer to avoid our decisions being directed by a small number > of loud complaining users, and instead to try to do things that will > serve the real users. Those complaints are only as loud as you think > they are -- you can also think of them as an automated tester that > puts its results into prose. Yes, this was more of a rant than an argument against the idea. Prior to 3.13, we allowed people to try > reclocking on nv40 and nv50, and I didn't see some huge quantity of > complaints about how it didn't work perfectly. Perhaps you saw those > at first, but I think the expectation by now is that it won't work. > Especially if it's behind a config option. Yes, but now, the code is lacking the huge chunk of functionality and probably won't work. Nv40 is probably in a similar state to before, however. So, again with the "maybe" there too. > > I have no idea what NvMemExec _really_ is doing, so I left it alone. I > assume that the majority of what my patch enables is actually engine > reclocking, not memory reclocking. So to get both, people would have > to flip both flags? Or is there more to it? I use it during development to allow independent testing or engine clock code without the more faulty memory code causing problems. The option prevents the memory recording scripts from being executed. Ben. > > -ilia
On Fri, May 16, 2014 at 11:54 PM, Ben Skeggs <skeggsb@gmail.com> wrote: > On 17 May 2014 13:39, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: >> >> On Fri, May 16, 2014 at 11:17 PM, Ben Skeggs <skeggsb@gmail.com> wrote: >> > On 17 May 2014 02:43, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: >> >> >> >> Adds a NvReclock boolean option to allow the user to enable (or >> >> disable) >> >> reclocking. All chipsets default to off, except NVAA/NVAC, which are >> >> reportedly complete. >> > Hey Ilia, >> > >> > I think I've expressed my thoughts on this previously via IRC, but let >> > me >> > stick them here too so there's a record of the current state... >> > >> > For nvaa/nvac, yes, let's enable it by default. It should, apparently, >> > be >> > good enough that it has a decent chance of working. It's not like we're >> > attempting anything automatic yet, so, this won't break anything for >> > people >> > who aren't trying.. >> > >> > I'm on the fence about Kepler. It actually might work to some extent in >> > a >> > decent number of cases already, there's potentially some severe issues >> > even >> > with engine clocks on some boards that I'm aware of, so it's not just a >> > memory reclocking worry here. >> > >> > That said, it has a good chance of working for some people. So. >> > Thoughts? >> > I'm also talking making "NvMemExec" default on here too. Again, causing >> > a >> > fuck-up will still require direct user action. >> > >> > For the rest (Hm, except maybe nv40, a lot will probably be ok..) >> > There's >> > *very* little chance memory reclocking will work, even on the systems >> > where >> > it used to. The code is far less complete, as it was broken in general, >> > and >> > I haven't yet had the time to *properly* reverse engineer the sequence >> > needed to stably reclock memory. Kepler is the only implementation >> > where >> > that's even been started. Tl;dr - unless you're working on the code for >> > Tesla/Fermi, there's zero point even trying it. So, the block should >> > stay. >> >> Meh. It works on my G98, for one of the perf levels. I'm sure there >> are lots of tesla's where it totally wouldn't work, but as long as it >> works on some of them, why not let people try? > Because we don't even *try* to handle timing changes, which, for the vast > majority perflvl changes, will not end well. > > This is more than a "mere" bug or slight bit of missing handling. It's a > *very* important chunk of the process that's simply missing. > > This is the difference between the Kepler code. Kepler might work, this > probably will not, unless you're very very lucky. > >> >> > >> > Personally, as you know, I'm more comfortable leaving it developer-only >> > still (except nvaa/nvac) until it at least works on all our own boards >> > without any major known missing bits.. But yeah, for the ones mentioned >> > above, I guess it's a possibility if people *really* want... >> >> I'm of the opposite opinion... if it works on _some_ of our boards, we >> should let people play with it. Why lock it away? Unless there's a >> real danger of it bricking the card. I've never heard of our code >> doing that, and given the way that you were RE'ing this stuff, I doubt >> that there's anything we can do (within reason) to brick the card. > This is why I'm "Hmm, maybe" on Kepler's code. Again, the rest isn't even > expected to work at all yet. I'm surprised it did for you.. Were the clocks > you chose close to the boot clocks by chance? Yep, they were. > >> >> > >> > I can only envision that if we allow this even just in the places it's >> > known >> > to be partially broken, certain sensationalist, er, people, will feel >> > the >> > need to test and complain about how broken it all really is... And then >> > retest on a regular basis, despite there having been *zero* work done >> > because no-one has the time, and then complain about the exact same >> > thing >> > AGAIN! (WHOA.. I'm done ranting now :p) >> >> I would prefer to avoid our decisions being directed by a small number >> of loud complaining users, and instead to try to do things that will >> serve the real users. Those complaints are only as loud as you think >> they are -- you can also think of them as an automated tester that >> puts its results into prose. > Yes, this was more of a rant than an argument against the idea. > > Prior to 3.13, we allowed people to try >> reclocking on nv40 and nv50, and I didn't see some huge quantity of >> complaints about how it didn't work perfectly. Perhaps you saw those >> at first, but I think the expectation by now is that it won't work. >> Especially if it's behind a config option. > Yes, but now, the code is lacking the huge chunk of functionality and > probably won't work. Nv40 is probably in a similar state to before, > however. So, again with the "maybe" there too. > >> >> I have no idea what NvMemExec _really_ is doing, so I left it alone. I >> assume that the majority of what my patch enables is actually engine >> reclocking, not memory reclocking. So to get both, people would have >> to flip both flags? Or is there more to it? > I use it during development to allow independent testing or engine clock > code without the more faulty memory code causing problems. The option > prevents the memory recording scripts from being executed. OK, so it really should be getting enabled in order for reclocking to "work". If you're really sure that it won't work for the majority of tesla cases --fine. I definitely know that it _used_ to work before 3.13 too, e.g. some ~nva8 user needed to clock it up to get video decoding engines to be fast enough. So... fine. Let's do the full enable for nvaa, override-able enable for nv40, nve0 (including defaulting NvMemExec to on if NvReclock is set), and leave nv50/nva3/nvc0 alone. Thoughts? -ilia
On Sat, May 17, 2014 at 2:06 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: > On Fri, May 16, 2014 at 11:54 PM, Ben Skeggs <skeggsb@gmail.com> wrote: >> On 17 May 2014 13:39, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: >>> >>> On Fri, May 16, 2014 at 11:17 PM, Ben Skeggs <skeggsb@gmail.com> wrote: >>> > On 17 May 2014 02:43, "Ilia Mirkin" <imirkin@alum.mit.edu> wrote: >>> >> >>> >> Adds a NvReclock boolean option to allow the user to enable (or >>> >> disable) >>> >> reclocking. All chipsets default to off, except NVAA/NVAC, which are >>> >> reportedly complete. >>> > Hey Ilia, >>> > >>> > I think I've expressed my thoughts on this previously via IRC, but let >>> > me >>> > stick them here too so there's a record of the current state... >>> > >>> > For nvaa/nvac, yes, let's enable it by default. It should, apparently, >>> > be >>> > good enough that it has a decent chance of working. It's not like we're >>> > attempting anything automatic yet, so, this won't break anything for >>> > people >>> > who aren't trying.. >>> > >>> > I'm on the fence about Kepler. It actually might work to some extent in >>> > a >>> > decent number of cases already, there's potentially some severe issues >>> > even >>> > with engine clocks on some boards that I'm aware of, so it's not just a >>> > memory reclocking worry here. >>> > >>> > That said, it has a good chance of working for some people. So. >>> > Thoughts? >>> > I'm also talking making "NvMemExec" default on here too. Again, causing >>> > a >>> > fuck-up will still require direct user action. >>> > >>> > For the rest (Hm, except maybe nv40, a lot will probably be ok..) >>> > There's >>> > *very* little chance memory reclocking will work, even on the systems >>> > where >>> > it used to. The code is far less complete, as it was broken in general, >>> > and >>> > I haven't yet had the time to *properly* reverse engineer the sequence >>> > needed to stably reclock memory. Kepler is the only implementation >>> > where >>> > that's even been started. Tl;dr - unless you're working on the code for >>> > Tesla/Fermi, there's zero point even trying it. So, the block should >>> > stay. >>> >>> Meh. It works on my G98, for one of the perf levels. I'm sure there >>> are lots of tesla's where it totally wouldn't work, but as long as it >>> works on some of them, why not let people try? >> Because we don't even *try* to handle timing changes, which, for the vast >> majority perflvl changes, will not end well. >> >> This is more than a "mere" bug or slight bit of missing handling. It's a >> *very* important chunk of the process that's simply missing. >> >> This is the difference between the Kepler code. Kepler might work, this >> probably will not, unless you're very very lucky. >> >>> >>> > >>> > Personally, as you know, I'm more comfortable leaving it developer-only >>> > still (except nvaa/nvac) until it at least works on all our own boards >>> > without any major known missing bits.. But yeah, for the ones mentioned >>> > above, I guess it's a possibility if people *really* want... >>> >>> I'm of the opposite opinion... if it works on _some_ of our boards, we >>> should let people play with it. Why lock it away? Unless there's a >>> real danger of it bricking the card. I've never heard of our code >>> doing that, and given the way that you were RE'ing this stuff, I doubt >>> that there's anything we can do (within reason) to brick the card. >> This is why I'm "Hmm, maybe" on Kepler's code. Again, the rest isn't even >> expected to work at all yet. I'm surprised it did for you.. Were the clocks >> you chose close to the boot clocks by chance? > > Yep, they were. ;) > >> >>> >>> > >>> > I can only envision that if we allow this even just in the places it's >>> > known >>> > to be partially broken, certain sensationalist, er, people, will feel >>> > the >>> > need to test and complain about how broken it all really is... And then >>> > retest on a regular basis, despite there having been *zero* work done >>> > because no-one has the time, and then complain about the exact same >>> > thing >>> > AGAIN! (WHOA.. I'm done ranting now :p) >>> >>> I would prefer to avoid our decisions being directed by a small number >>> of loud complaining users, and instead to try to do things that will >>> serve the real users. Those complaints are only as loud as you think >>> they are -- you can also think of them as an automated tester that >>> puts its results into prose. >> Yes, this was more of a rant than an argument against the idea. >> >> Prior to 3.13, we allowed people to try >>> reclocking on nv40 and nv50, and I didn't see some huge quantity of >>> complaints about how it didn't work perfectly. Perhaps you saw those >>> at first, but I think the expectation by now is that it won't work. >>> Especially if it's behind a config option. >> Yes, but now, the code is lacking the huge chunk of functionality and >> probably won't work. Nv40 is probably in a similar state to before, >> however. So, again with the "maybe" there too. >> >>> >>> I have no idea what NvMemExec _really_ is doing, so I left it alone. I >>> assume that the majority of what my patch enables is actually engine >>> reclocking, not memory reclocking. So to get both, people would have >>> to flip both flags? Or is there more to it? >> I use it during development to allow independent testing or engine clock >> code without the more faulty memory code causing problems. The option >> prevents the memory recording scripts from being executed. > > OK, so it really should be getting enabled in order for reclocking to > "work". If you're really sure that it won't work for the majority of > tesla cases --fine. I definitely know that it _used_ to work before > 3.13 too, e.g. some ~nva8 user needed to clock it up to get video > decoding engines to be fast enough. > > So... fine. Let's do the full enable for nvaa, override-able enable > for nv40, nve0 (including defaulting NvMemExec to on if NvReclock is > set), and leave nv50/nva3/nvc0 alone. > > Thoughts? Let's just go full enable (so, make the -ENOSYS >= 0x50 || < 0xe0) on nv40/nvaa/nve0 implementations. IIRC the default clock mode is "disabled" still, so, nothing bad will happen unless people try it. And yes, I'd vote for just making NvMemExec default "true" everywhere. The option should stay though, it's useful for development/debugging. Ben. > > -ilia
diff --git a/nvkm/include/subdev/clock.h b/nvkm/include/subdev/clock.h index 8f4ced7..c01e29c 100644 --- a/nvkm/include/subdev/clock.h +++ b/nvkm/include/subdev/clock.h @@ -77,6 +77,8 @@ struct nouveau_clock { int tstate; /* thermal adjustment (max-) */ int dstate; /* display adjustment (min+) */ + bool allow_reclock; + int (*read)(struct nouveau_clock *, enum nv_clk_src); int (*calc)(struct nouveau_clock *, struct nouveau_cstate *); int (*prog)(struct nouveau_clock *); @@ -106,8 +108,8 @@ struct nouveau_clocks { int mdiv; }; -#define nouveau_clock_create(p,e,o,i,d) \ - nouveau_clock_create_((p), (e), (o), (i), sizeof(**d), (void **)d) +#define nouveau_clock_create(p,e,o,i,r,d) \ + nouveau_clock_create_((p), (e), (o), (i), (r), sizeof(**d), (void **)d) #define nouveau_clock_destroy(p) ({ \ struct nouveau_clock *clk = (p); \ _nouveau_clock_dtor(nv_object(clk)); \ @@ -121,7 +123,7 @@ struct nouveau_clocks { int nouveau_clock_create_(struct nouveau_object *, struct nouveau_object *, struct nouveau_oclass *, - struct nouveau_clocks *, int, void **); + struct nouveau_clocks *, bool, int, void **); void _nouveau_clock_dtor(struct nouveau_object *); int _nouveau_clock_init(struct nouveau_object *); #define _nouveau_clock_fini _nouveau_subdev_fini diff --git a/nvkm/subdev/clock/base.c b/nvkm/subdev/clock/base.c index dd62bae..80ad9d3 100644 --- a/nvkm/subdev/clock/base.c +++ b/nvkm/subdev/clock/base.c @@ -346,8 +346,8 @@ nouveau_clock_ustate_update(struct nouveau_clock *clk, int req) struct nouveau_pstate *pstate; int i = 0; - /* YKW repellant */ - return -ENOSYS; + if (!clk->allow_reclock) + return -ENOSYS; if (req != -1 && req != -2) { list_for_each_entry(pstate, &clk->states, head) { @@ -456,6 +456,7 @@ nouveau_clock_create_(struct nouveau_object *parent, struct nouveau_object *engine, struct nouveau_oclass *oclass, struct nouveau_clocks *clocks, + bool allow_reclock, int length, void **object) { struct nouveau_device *device = nv_device(parent); @@ -478,6 +479,9 @@ nouveau_clock_create_(struct nouveau_object *parent, ret = nouveau_pstate_new(clk, idx++); } while (ret == 0); + clk->allow_reclock = + nouveau_boolopt(device->cfgopt, "NvReclock", allow_reclock); + mode = nouveau_stropt(device->cfgopt, "NvClkMode", &arglen); if (mode) { if (!strncasecmpz(mode, "disabled", arglen)) { diff --git a/nvkm/subdev/clock/nv04.c b/nvkm/subdev/clock/nv04.c index b74db6c..eb2d442 100644 --- a/nvkm/subdev/clock/nv04.c +++ b/nvkm/subdev/clock/nv04.c @@ -82,7 +82,8 @@ nv04_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, struct nv04_clock_priv *priv; int ret; - ret = nouveau_clock_create(parent, engine, oclass, nv04_domain, &priv); + ret = nouveau_clock_create(parent, engine, oclass, nv04_domain, false, + &priv); *pobject = nv_object(priv); if (ret) return ret; diff --git a/nvkm/subdev/clock/nv40.c b/nvkm/subdev/clock/nv40.c index db7346f..651e299 100644 --- a/nvkm/subdev/clock/nv40.c +++ b/nvkm/subdev/clock/nv40.c @@ -213,7 +213,8 @@ nv40_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, struct nv40_clock_priv *priv; int ret; - ret = nouveau_clock_create(parent, engine, oclass, nv40_domain, &priv); + ret = nouveau_clock_create(parent, engine, oclass, nv40_domain, false, + &priv); *pobject = nv_object(priv); if (ret) return ret; diff --git a/nvkm/subdev/clock/nv50.c b/nvkm/subdev/clock/nv50.c index 250a6d9..8c13277 100644 --- a/nvkm/subdev/clock/nv50.c +++ b/nvkm/subdev/clock/nv50.c @@ -507,7 +507,7 @@ nv50_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, int ret; ret = nouveau_clock_create(parent, engine, oclass, pclass->domains, - &priv); + false, &priv); *pobject = nv_object(priv); if (ret) return ret; diff --git a/nvkm/subdev/clock/nva3.c b/nvkm/subdev/clock/nva3.c index 4f5a137..9fb5835 100644 --- a/nvkm/subdev/clock/nva3.c +++ b/nvkm/subdev/clock/nva3.c @@ -302,7 +302,8 @@ nva3_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, struct nva3_clock_priv *priv; int ret; - ret = nouveau_clock_create(parent, engine, oclass, nva3_domain, &priv); + ret = nouveau_clock_create(parent, engine, oclass, nva3_domain, false, + &priv); *pobject = nv_object(priv); if (ret) return ret; diff --git a/nvkm/subdev/clock/nvaa.c b/nvkm/subdev/clock/nvaa.c index 7a723b4..6a65fc9 100644 --- a/nvkm/subdev/clock/nvaa.c +++ b/nvkm/subdev/clock/nvaa.c @@ -421,7 +421,8 @@ nvaa_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, struct nvaa_clock_priv *priv; int ret; - ret = nouveau_clock_create(parent, engine, oclass, nvaa_domains, &priv); + ret = nouveau_clock_create(parent, engine, oclass, nvaa_domains, true, + &priv); *pobject = nv_object(priv); if (ret) return ret; diff --git a/nvkm/subdev/clock/nvc0.c b/nvkm/subdev/clock/nvc0.c index c310572..dbf8517 100644 --- a/nvkm/subdev/clock/nvc0.c +++ b/nvkm/subdev/clock/nvc0.c @@ -437,7 +437,8 @@ nvc0_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, struct nvc0_clock_priv *priv; int ret; - ret = nouveau_clock_create(parent, engine, oclass, nvc0_domain, &priv); + ret = nouveau_clock_create(parent, engine, oclass, nvc0_domain, false, + &priv); *pobject = nv_object(priv); if (ret) return ret; diff --git a/nvkm/subdev/clock/nve0.c b/nvkm/subdev/clock/nve0.c index d3c37c9..860aa73 100644 --- a/nvkm/subdev/clock/nve0.c +++ b/nvkm/subdev/clock/nve0.c @@ -473,7 +473,8 @@ nve0_clock_ctor(struct nouveau_object *parent, struct nouveau_object *engine, struct nve0_clock_priv *priv; int ret; - ret = nouveau_clock_create(parent, engine, oclass, nve0_domain, &priv); + ret = nouveau_clock_create(parent, engine, oclass, nve0_domain, false, + &priv); *pobject = nv_object(priv); if (ret) return ret;
Adds a NvReclock boolean option to allow the user to enable (or disable) reclocking. All chipsets default to off, except NVAA/NVAC, which are reportedly complete. Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> --- Ben, I know you've been saying that reclocking is in a pretty bad state, but I do think that there are going to be groups of people for whom the current code can work at least a little bit. May as well let them try it. The memory script execution is still behind the extra-special flag as well... Also this provides a way forward to enable reclocking on some chips and leave it off for others, by default. nvkm/include/subdev/clock.h | 8 +++++--- nvkm/subdev/clock/base.c | 8 ++++++-- nvkm/subdev/clock/nv04.c | 3 ++- nvkm/subdev/clock/nv40.c | 3 ++- nvkm/subdev/clock/nv50.c | 2 +- nvkm/subdev/clock/nva3.c | 3 ++- nvkm/subdev/clock/nvaa.c | 3 ++- nvkm/subdev/clock/nvc0.c | 3 ++- nvkm/subdev/clock/nve0.c | 3 ++- 9 files changed, 24 insertions(+), 12 deletions(-)