Re: [rethinking] Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 04/07/2012 18:04 Andriy Gapon said the following:
> Our current approach of using C-state indexes as C-state identifiers/names
> may be
> not so bad after all if we remove confusion by properly documenting the
> meaning
> (index vs "type").
> Here is a link to a past discussion on this topic:
> http://thread.gmane.org/gmane.os.freebsd.current/127860/focus=6373
> I think that messages in a sub-thread starting at the highlighted message may
> provide some useful information.
> Here are couple of links to the code in Linux and OpenSolaris:
> http://lxr.linux.no/#linux+v3.4.4/drivers/acpi/processor_idle.c#L354
> http://lxr.linux.no/#linux+v3.4.4/drivers/acpi/processor_idle.c#L1097
> http://fxr.watson.org/fxr/source/i86pc/os/cpupm/cpu_acpi.c?v=OPENSOLARIS#L683
> http://fxr.watson.org/fxr/source/i86pc/os/cpupm/cpu_idle.c?v=OPENSOLARIS;im=bigexcerpts#L678
>
> So, now as then I still don't have any preference for either of methods. But
> the
> decision is not straightforward as you might see now, thus we need some
> convincing
> that any change is needed actually :-)
>
> For more information about each C-state I would go a way of creating a sysctl
> forest with a tree at each C-state under each CPU (for potential asymmetric
> C-state support), which would describe all available parameters of each
> C-state
> (power, latency, ACPI (or HW-specific[*]) type, I/O-vs-MWAIT-vs-etc type of
> entrance, etc).
>
> [*] E.g. for intel_idle it could be C-state numbers from Intel specs.
>
Here is an example based on a real i7 system (enslaved by Linux) to make the
situation more easy to comprehend.
The system seems to provide 5 C-states including C0, but uninteresting C0 and C1
will be omitted in the further data.
So:
ACPI (_CST) index 2 3 4
ACPI (_CST) type2 3 3
ACPI I/O register 0x414 0x415 0x416
Intel name (intel_idle) C3 C6 C7
MWAIT hint 0x100x200x30
[*] ACPI tables can advertise C-states either with I/O entry or with MWAIT entry
based on declared OS capabilities (what FreeBSD defines as
ACPI_CAP_SMP_C3_NATIVE,
to be precise).
MWAIT hints are the same both in ACPI tables and in intel_idle driver.
I make the following observations and conclusions.
All C-states are really different despite two of them having the same ACPI type.
So collapsing C-states based on ACPI type like (Open)Solaris does (did) is
clearly
a wrong approach.
Using ACPI type as a name (identity) of a C-state also seems to be unhelpful
("C3
- which C3, the first C3 or the second C3?").
Restoring the vendor designated names of the states (C3, C6, C7) is impossible
from ACPI provided information alone.
Where does this leave us? I think that it leaves us exactly where we are now
(and
Linux too).
The only thing that I would perhaps change would be to drop 'C' from FreeBSD
C-state names to minimize confusion with "real" C-state name/types/whatevers
(whichever one prefers to consider to be the real ones) and to emphasize that
they
are just indexes/handles/FreeBSD IDs.
But even such a change, being purely cosmetic, probably has too little benefit
considering potential pain of POLA violation.
So I urge you to give up your C-state renaming patch.
At least for me the above example decides the choice.
--
Andriy Gapon
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [rethinking] Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 03/07/2012 00:17 Sean Bruno said the following: > Agreed. We shall have to discuss further and see where this leads us. > Its starting to look like some amount of further overhaul may be > required. [Just replying to a high-level thing here.] Or maybe no overhaul at all?.. :-) Our current approach of using C-state indexes as C-state identifiers/names may be not so bad after all if we remove confusion by properly documenting the meaning (index vs "type"). Here is a link to a past discussion on this topic: http://thread.gmane.org/gmane.os.freebsd.current/127860/focus=6373 I think that messages in a sub-thread starting at the highlighted message may provide some useful information. Here are couple of links to the code in Linux and OpenSolaris: http://lxr.linux.no/#linux+v3.4.4/drivers/acpi/processor_idle.c#L354 http://lxr.linux.no/#linux+v3.4.4/drivers/acpi/processor_idle.c#L1097 http://fxr.watson.org/fxr/source/i86pc/os/cpupm/cpu_acpi.c?v=OPENSOLARIS#L683 http://fxr.watson.org/fxr/source/i86pc/os/cpupm/cpu_idle.c?v=OPENSOLARIS;im=bigexcerpts#L678 So, now as then I still don't have any preference for either of methods. But the decision is not straightforward as you might see now, thus we need some convincing that any change is needed actually :-) For more information about each C-state I would go a way of creating a sysctl forest with a tree at each C-state under each CPU (for potential asymmetric C-state support), which would describe all available parameters of each C-state (power, latency, ACPI (or HW-specific[*]) type, I/O-vs-MWAIT-vs-etc type of entrance, etc). [*] E.g. for intel_idle it could be C-state numbers from Intel specs. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
[rethinking] Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
> > > > http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt > > Some sort of a review... > > First, going over what seems to be cosmetic changes in the patch. > > The change to etc/rc.d/power_profile seems to be a NOP, is it needed? > Ugh, well ... this was *supposed* to be a change to cx_lowest, not dev.cpu.0.freq. So, let me change that. +1 pointyhat to me. > New comment in the first hunk of acpi_cpu.c seems to be redundant and its > placement violates style(9). Right. *sigh* I'll adjust that. > > What do we achieve by renaming cpu_cx_lowest static file-scope variable to > global_lowest_cstate? I am not necessarily against the change, but it needs > to > be justified and it should likely be made in a separate commit. Besides it > breaks > an apparent (and perhaps useless) naming convention of prefixing everything > with > "cpu_". > Nothing gained other than my sanity. I suspect what I really want to do, is change the naming of all the global override values for ease of reading, but for now this is trivial and I'll revert it. > Other comments that you seem to have added as notes to self while working on > the > code - they are good, but better be done in a separate comments-only commit. > > Now to the essence of the patch. > > acpi_cpu_notify hunk - I am not sure if I completely follow the logic of the > current code in that function. For some reason that I can not grasp it > doesn't > update per-CPU cpu_cx_lowest if it is (was) equal or greater than the global > cpu_cx_lowest limit. Otherwise the per-CPU limit is set to the shallower of > the > global limit and the deepest available state. If I'm reading the code correctly, I'm assuming that each CPU will be notified via the ACPI handler here (denoted by the ACPI_SERIAL_BEGIN) and update its own cpu_cx_lowest via acpi_cpu_cx_cst/list() If the *new* lowest is greater than the global value, then we assume that the *old* global lowest is still valid and move on. I don't *think* that this is a problem currently. However if the per cpu list of cstates no longer has a value that corresponds to cpu_cx_lowest, I think that this *could* be a problem. > The patch seems to do something that make less sense in the latter case, it > tries > to set per-CPU cpu_cx_lowest based on a new C-state that resides at old > per-CPU > cpu_cx_lowest index. Essentially this means that (1) cpu_cx_lowest maybe > point to > a junk portion of cpu_cx_states after C-states change; (2) whatever the > change, if > we do not run into (1), then cpu_cx_lowest value won't change and the lowest > C-state would be whichever happens to be at that index. Ok, so the current method of using an index will always work to do something valid as opposed to my proposal which has the potential to go into a garbage part of the array and use values that are meaningless. I suspect that if I went from AC adapter to battery I would see some odd behavior on my laptop if I was to put the proper debug flags into place as the notify handler messed with the cpu_cx_states[] array per cpu. *sigh* ... back to the drawing board. > > About change in acpi_cpu_set_cx_lowest - I am not sure that it should fail if > the > exact match can not be found. Maybe cx_lowest should be set to the best > match in > that case? Where best match is the deepest state that is not deeper than the > requested state. I would prefer it to error in that case. I personally wouldn't want the code to do something "smart" in this case on my servers. I'd rather know that the Cstate isn't supported and look to see why. Most users aren't tweaking this directly, but it is being adjusted by devd/power_profile, so it should *know* the correct Cx state. > > Also, the ACPI spec permits multiple _CST entries with the same C-state type. > Not > sure if this ever happens in practice but wouldn't rule it out. > Previously they would be disambiguated by their indexes (perhaps incorrectly > from > ACPI point of view). I think that with the proposed change we need to have > some > policy on which of e.g. four C3 states to select as _the_ C3 state. This never occurred to me to be honest. I had to go back and read chap 8.4.2 of the ACPI spec to see what they were up to. I see no good reason for this to exist, but it is a possible configuration according to 8.4.2.1 ... This pains me a bit. :-) So, with that being said. This pretty much kills my entire implementation. :-) I didn't expect there to be multiple _CST entries for a single Cx state. Amusing. This also means that the control of the cx_lowest value is a bit trickier. If the user asks for C3, which one do we give them? :-) Also, I'm not clear that the code as it exists can handle this case gracefully. I'll ponder this one a bit. It would be good to find a box that does this to see how it behaves. > > acpi_cpu_global_cx_lowest_sysctl change - unlike the current code where > acpi_cpu_set_cx_lowest never fails, the new code may
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 27/06/2012 03:23 Sean Bruno said the following: > Ok, version 2 now in effect. I removed the return of BUS_PROBE_DEFAULT > and this seems to do the right thing in the cases I have. > > If there are no objections to this, I'll chuck it over into -head soonish. > > > http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt Some sort of a review... First, going over what seems to be cosmetic changes in the patch. The change to etc/rc.d/power_profile seems to be a NOP, is it needed? New comment in the first hunk of acpi_cpu.c seems to be redundant and its placement violates style(9). What do we achieve by renaming cpu_cx_lowest static file-scope variable to global_lowest_cstate? I am not necessarily against the change, but it needs to be justified and it should likely be made in a separate commit. Besides it breaks an apparent (and perhaps useless) naming convention of prefixing everything with "cpu_". Other comments that you seem to have added as notes to self while working on the code - they are good, but better be done in a separate comments-only commit. Now to the essence of the patch. acpi_cpu_notify hunk - I am not sure if I completely follow the logic of the current code in that function. For some reason that I can not grasp it doesn't update per-CPU cpu_cx_lowest if it is (was) equal or greater than the global cpu_cx_lowest limit. Otherwise the per-CPU limit is set to the shallower of the global limit and the deepest available state. The patch seems to do something that make less sense in the latter case, it tries to set per-CPU cpu_cx_lowest based on a new C-state that resides at old per-CPU cpu_cx_lowest index. Essentially this means that (1) cpu_cx_lowest maybe point to a junk portion of cpu_cx_states after C-states change; (2) whatever the change, if we do not run into (1), then cpu_cx_lowest value won't change and the lowest C-state would be whichever happens to be at that index. About change in acpi_cpu_set_cx_lowest - I am not sure that it should fail if the exact match can not be found. Maybe cx_lowest should be set to the best match in that case? Where best match is the deepest state that is not deeper than the requested state. Also, the ACPI spec permits multiple _CST entries with the same C-state type. Not sure if this ever happens in practice but wouldn't rule it out. Previously they would be disambiguated by their indexes (perhaps incorrectly from ACPI point of view). I think that with the proposed change we need to have some policy on which of e.g. four C3 states to select as _the_ C3 state. acpi_cpu_global_cx_lowest_sysctl change - unlike the current code where acpi_cpu_set_cx_lowest never fails, the new code may change cx_lowest on some CPUs and leave other CPUs un-updated in the case of an error. Also, it seems that with this patch cpu_cx_count becomes a write-only variable. Maybe it should be removed or maybe it could be re-purposed/replaced with a deepest Cx type available. So that the sysctl handlers could check against this value like they did before (instead of MAX_CX_STATES). I think that the patch is not ready yet to be committed. Perhaps we need to discuss some things before the next review request. E.g. how the following three should interact: manually set global Cx limit, manually set Cx level for an individual CPU (thus overriding global limit) and ACPI platform change of available Cx levels. Thank you very much for your work on this driver! It really needs a bit of do-over. P.S. A diff produced with svn diff -x -p is much more convenient to review than plain svn diff. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Wed, 2012-06-27 at 05:12 -0700, John Baldwin wrote: > On Tuesday, June 26, 2012 8:23:23 pm Sean Bruno wrote: > > Ok, version 2 now in effect. I removed the return of BUS_PROBE_DEFAULT > > and this seems to do the right thing in the cases I have. > > > > If there are no objections to this, I'll chuck it over into -head soonish. > > > > > > http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt > > Is this for a BIOS that is reporting non-contiguous Cx states or is this a > patch to let your custom Intel Cx driver work? If the latter, I would rather > have the driver export a contiguous range of virtual Cx states (what ACPI > actually does under the covers) rather than change our code to allow sparse > ACPI Cx states. > This patch is for the BIOS that reports non-contiguous ACPI Cx states. e.g. C1 and C3 with no C2. The Intel Mwaits/Cx State thing is still a work in progress and not dependent on this. Sean ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Tuesday, June 26, 2012 8:23:23 pm Sean Bruno wrote: > Ok, version 2 now in effect. I removed the return of BUS_PROBE_DEFAULT > and this seems to do the right thing in the cases I have. > > If there are no objections to this, I'll chuck it over into -head soonish. > > > http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt Is this for a BIOS that is reporting non-contiguous Cx states or is this a patch to let your custom Intel Cx driver work? If the latter, I would rather have the driver export a contiguous range of virtual Cx states (what ACPI actually does under the covers) rather than change our code to allow sparse ACPI Cx states. -- John Baldwin ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 20/06/2012 23:54 Sean Bruno said the following: > On Wed, 2012-06-20 at 13:18 -0700, Andriy Gapon wrote: >>> I also, disagree with the idea of "FreeBSD C-states" as that is not >> the >>> intention of the code. The code, from my read, is trying to >> interpret >>> C-states as though they are always defined sequentially and >> non-sparse. >> >> I seem to recall that this is an ACPI requirement. I could be >> mistaken, but no >> time to double-check at the moment. >> >> > > Just to check as I'm actively looking at this code I went and grabbed > the December 6, 2011 ACPI spec. http://www.acpi.info/spec.htm > > chap 8.1 pretty clearly states that C2 and C3 are optional states. So it > appears that you can have a C3 without a C2. So, I suspect that the > idea that the index the cx_states array is always going to be 1 less > that the ACPI Cstate value isn't by spec. Or something ... :-) I think that the chapter on _CST is more relevant here (8.4.2.1 in my copy of the spec). But anyway, there is no such requirement in the specification. I was misremembering the requirement that states should be ordered. So, would you like to produce a cleaned up version of your patch with only this change in it? -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 21/06/2012 16:50 John Baldwin said the following:
> On Wednesday, June 20, 2012 7:28:36 pm Sean Bruno wrote:
>> On Wed, 2012-06-20 at 09:44 -0700, Sean Bruno wrote:
>>> On Tue, 2012-06-19 at 09:02 -0700, Sean Bruno wrote:
http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt
>>>
>>> also, I wanted to point out that I'm returning BUS_PROBE_GENERIC here.
>>>
>>> I want to emulate the Intel acpi_idle code that exists in linux-land and
>>> I *thought* that I could setup an acpi_cpu_idle module that would come
>>> in at a higher priority on Intel cpus, however there's some SYSINIT()
>>> hackery going on that I don't know how to handle gracefully. I'm not
>>> sure how to proceed with a different idle module here. thoughts?
>>>
>>> e.g.
>>>
>>> static void
>>> acpi_cpu_postattach(void *unused __unused)
>>> {
>>> device_t *devices;
>>> int err;
>>> int i, n;
>>>
>>> err = devclass_get_devices(acpi_cpu_devclass, &devices, &n);
>>> if (err != 0) {
>>> printf("devclass_get_devices(acpi_cpu_devclass) failed\n");
>>> return;
>>> }
>>> for (i = 0; i < n; i++)
>>> bus_generic_probe(devices[i]);
>>> for (i = 0; i < n; i++)
>>> bus_generic_attach(devices[i]);
>>> free(devices, M_TEMP);
>>> }
>>>
>>> SYSINIT(acpi_cpu, SI_SUB_CONFIGURE, SI_ORDER_MIDDLE,
>>> acpi_cpu_postattach, NULL);
>>>
>>>
>>
>>
>> Ohh ... right. This entire idea is stupid and fully demonstrates my
>> lack of understanding. bus_probe/attach can't be used, there's no BUS
>> here. So, SYSINIT() to the rescue. Ok, that changes things around a
>> lot for me. This BUS_PROBE_GENERIC idea is a dud.
>
> No, every device in new-bus can be a bus (and acpi_cpuX is in fact a bus).
> The issue here is that this driver is deferring attaching child devices until
> later in the boot. This is a bit of a lame workaround that should be using
> new-bus multipass instead.
>
Yes. The only excuse is that the hack was introduced, if I remember correctly,
before the multipass support.
--
Andriy Gapon
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 20/06/2012 19:44 Sean Bruno said the following:
> On Tue, 2012-06-19 at 09:02 -0700, Sean Bruno wrote:
>> http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt
>
> also, I wanted to point out that I'm returning BUS_PROBE_GENERIC here.
>
> I want to emulate the Intel acpi_idle code that exists in linux-land and
> I *thought* that I could setup an acpi_cpu_idle module that would come
> in at a higher priority on Intel cpus, however there's some SYSINIT()
> hackery going on that I don't know how to handle gracefully. I'm not
> sure how to proceed with a different idle module here. thoughts?
>
> e.g.
>
> static void
> acpi_cpu_postattach(void *unused __unused)
> {
> device_t *devices;
> int err;
> int i, n;
>
> err = devclass_get_devices(acpi_cpu_devclass, &devices, &n);
> if (err != 0) {
> printf("devclass_get_devices(acpi_cpu_devclass) failed\n");
> return;
> }
> for (i = 0; i < n; i++)
> bus_generic_probe(devices[i]);
> for (i = 0; i < n; i++)
> bus_generic_attach(devices[i]);
> free(devices, M_TEMP);
> }
>
> SYSINIT(acpi_cpu, SI_SUB_CONFIGURE, SI_ORDER_MIDDLE,
> acpi_cpu_postattach, NULL);
>
>
Just a little bit of explanation for the code.
acpi_cpu driver is attached very early because it does some ACPI configuration
in its attach method (_OSC/_PDC calls) and correct behavior of other ACPI
drivers may depend on this. On the other hand, acpi_cpu acts as a bus and has
child devices. Some of those children can not attach that early because they
need more of the system to be initialized (e.g. PCI bus). Hence the SYSINIT
hack to attach them later.
--
Andriy Gapon
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Wednesday, June 20, 2012 7:28:36 pm Sean Bruno wrote:
> On Wed, 2012-06-20 at 09:44 -0700, Sean Bruno wrote:
> > On Tue, 2012-06-19 at 09:02 -0700, Sean Bruno wrote:
> > > http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt
> >
> > also, I wanted to point out that I'm returning BUS_PROBE_GENERIC here.
> >
> > I want to emulate the Intel acpi_idle code that exists in linux-land and
> > I *thought* that I could setup an acpi_cpu_idle module that would come
> > in at a higher priority on Intel cpus, however there's some SYSINIT()
> > hackery going on that I don't know how to handle gracefully. I'm not
> > sure how to proceed with a different idle module here. thoughts?
> >
> > e.g.
> >
> > static void
> > acpi_cpu_postattach(void *unused __unused)
> > {
> > device_t *devices;
> > int err;
> > int i, n;
> >
> > err = devclass_get_devices(acpi_cpu_devclass, &devices, &n);
> > if (err != 0) {
> > printf("devclass_get_devices(acpi_cpu_devclass) failed\n");
> > return;
> > }
> > for (i = 0; i < n; i++)
> > bus_generic_probe(devices[i]);
> > for (i = 0; i < n; i++)
> > bus_generic_attach(devices[i]);
> > free(devices, M_TEMP);
> > }
> >
> > SYSINIT(acpi_cpu, SI_SUB_CONFIGURE, SI_ORDER_MIDDLE,
> > acpi_cpu_postattach, NULL);
> >
> >
>
>
> Ohh ... right. This entire idea is stupid and fully demonstrates my
> lack of understanding. bus_probe/attach can't be used, there's no BUS
> here. So, SYSINIT() to the rescue. Ok, that changes things around a
> lot for me. This BUS_PROBE_GENERIC idea is a dud.
No, every device in new-bus can be a bus (and acpi_cpuX is in fact a bus).
The issue here is that this driver is deferring attaching child devices until
later in the boot. This is a bit of a lame workaround that should be using
new-bus multipass instead.
--
John Baldwin
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Wed, 2012-06-20 at 09:44 -0700, Sean Bruno wrote:
> On Tue, 2012-06-19 at 09:02 -0700, Sean Bruno wrote:
> > http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt
>
> also, I wanted to point out that I'm returning BUS_PROBE_GENERIC here.
>
> I want to emulate the Intel acpi_idle code that exists in linux-land and
> I *thought* that I could setup an acpi_cpu_idle module that would come
> in at a higher priority on Intel cpus, however there's some SYSINIT()
> hackery going on that I don't know how to handle gracefully. I'm not
> sure how to proceed with a different idle module here. thoughts?
>
> e.g.
>
> static void
> acpi_cpu_postattach(void *unused __unused)
> {
> device_t *devices;
> int err;
> int i, n;
>
> err = devclass_get_devices(acpi_cpu_devclass, &devices, &n);
> if (err != 0) {
> printf("devclass_get_devices(acpi_cpu_devclass) failed\n");
> return;
> }
> for (i = 0; i < n; i++)
> bus_generic_probe(devices[i]);
> for (i = 0; i < n; i++)
> bus_generic_attach(devices[i]);
> free(devices, M_TEMP);
> }
>
> SYSINIT(acpi_cpu, SI_SUB_CONFIGURE, SI_ORDER_MIDDLE,
> acpi_cpu_postattach, NULL);
>
>
Ohh ... right. This entire idea is stupid and fully demonstrates my
lack of understanding. bus_probe/attach can't be used, there's no BUS
here. So, SYSINIT() to the rescue. Ok, that changes things around a
lot for me. This BUS_PROBE_GENERIC idea is a dud.
/me goes back to reading first and typing second
Sean
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Wed, 2012-06-20 at 13:18 -0700, Andriy Gapon wrote: > > I also, disagree with the idea of "FreeBSD C-states" as that is not > the > > intention of the code. The code, from my read, is trying to > interpret > > C-states as though they are always defined sequentially and > non-sparse. > > I seem to recall that this is an ACPI requirement. I could be > mistaken, but no > time to double-check at the moment. > > Just to check as I'm actively looking at this code I went and grabbed the December 6, 2011 ACPI spec. http://www.acpi.info/spec.htm chap 8.1 pretty clearly states that C2 and C3 are optional states. So it appears that you can have a C3 without a C2. So, I suspect that the idea that the index the cx_states array is always going to be 1 less that the ACPI Cstate value isn't by spec. Or something ... :-) Sean "I have no idea how computers work" Bruno ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 20/06/2012 19:14 Sean Bruno said the following: > Since this patch changes the output of the sysctl format, I disagree > with it. And I am not proposing it for the tree. > I also, disagree with the idea of "FreeBSD C-states" as that is not the > intention of the code. The code, from my read, is trying to interpret > C-states as though they are always defined sequentially and non-sparse. I seem to recall that this is an ACPI requirement. I could be mistaken, but no time to double-check at the moment. > I am still of the opinion that my patch is correct at this point. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Tue, 2012-06-19 at 09:02 -0700, Sean Bruno wrote:
> http://people.freebsd.org/~sbruno/acpi_cpu_cstate_sparse.txt
also, I wanted to point out that I'm returning BUS_PROBE_GENERIC here.
I want to emulate the Intel acpi_idle code that exists in linux-land and
I *thought* that I could setup an acpi_cpu_idle module that would come
in at a higher priority on Intel cpus, however there's some SYSINIT()
hackery going on that I don't know how to handle gracefully. I'm not
sure how to proceed with a different idle module here. thoughts?
e.g.
static void
acpi_cpu_postattach(void *unused __unused)
{
device_t *devices;
int err;
int i, n;
err = devclass_get_devices(acpi_cpu_devclass, &devices, &n);
if (err != 0) {
printf("devclass_get_devices(acpi_cpu_devclass) failed\n");
return;
}
for (i = 0; i < n; i++)
bus_generic_probe(devices[i]);
for (i = 0; i < n; i++)
bus_generic_attach(devices[i]);
free(devices, M_TEMP);
}
SYSINIT(acpi_cpu, SI_SUB_CONFIGURE, SI_ORDER_MIDDLE,
acpi_cpu_postattach, NULL);
___
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Tue, 2012-06-19 at 22:00 -0700, Andriy Gapon wrote: > > I do not think that this is a real problem. A cosmetic one - most > likely. > >> > > Yes, most likely. Except that the code seems to think that the > index of > > the Cstates is good enough for a comparison to value. More over, > the > > sysctl's accept a value like "C3" and manipulate that into an index > into > > the Cstate array without checking for the Cstate value. > > > > The impact of this patch corrects this cosmetic display issue. > > If you accept that there are "FreeBSD C-states" and everything is done > in terms > of them, then there is no problem. > I once wrote this trivial patch to see more information about > FreeBSD-reported > C-states: > https://gitorious.org/~avg/freebsd/avgbsd/commit/043e9b0da5b46d389971e0166789fbee8a4e8622?format=patch > Since this patch changes the output of the sysctl format, I disagree with it. I also, disagree with the idea of "FreeBSD C-states" as that is not the intention of the code. The code, from my read, is trying to interpret C-states as though they are always defined sequentially and non-sparse. I am still of the opinion that my patch is correct at this point. Sean ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Tue, 2012-06-19 at 22:00 -0700, Andriy Gapon wrote: > > It seems that the rc.conf value of performance_cx_lowest="LOW" is > what I > > really want, not economy_cx_lowest. > > Yes. Could you please try this without using your patch? > > I get an impression that its effect was to actually request C2 when > cx_lowest is > set to C1. > > -- > Andriy Gapon Confirmed. If I set performance_cx_lowest="LOW" then the system sets the "C2" state and really implements C3 correctly. I see a 25 watt power saving without the patch on the Dell r410. sean ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 20/06/2012 00:42 Sean Bruno said the following: > On Tue, 2012-06-19 at 14:07 -0700, Andriy Gapon wrote: >> on 19/06/2012 19:02 Sean Bruno said the following: >>> The first impact of this behavior is to list C3 as C2 in the list of >>> Cstates when you retrieve the cx_supported sysctls for the cpus. >> >> I do not think that this is a real problem. A cosmetic one - most likely. >> > Yes, most likely. Except that the code seems to think that the index of > the Cstates is good enough for a comparison to value. More over, the > sysctl's accept a value like "C3" and manipulate that into an index into > the Cstate array without checking for the Cstate value. > > The impact of this patch corrects this cosmetic display issue. If you accept that there are "FreeBSD C-states" and everything is done in terms of them, then there is no problem. I once wrote this trivial patch to see more information about FreeBSD-reported C-states: https://gitorious.org/~avg/freebsd/avgbsd/commit/043e9b0da5b46d389971e0166789fbee8a4e8622?format=patch >>> The >>> second impact is that the power_profile script never drops to a valid >>> Cstate when you set the economy_lowest variable in rc.conf. >> >> Could you please explain if this somehow follows from your first observation >> and >> how? >> If not, could you please share your finding on what exactly causes this to >> happen? >> >> Also, are we talking about a laptop here? Namely, judging from the >> reference to >> 'economy_lowest', are AC state changes in play? >> > > No, what I was attempting to do was configure a server such that it > would attempt to use the C3 state at idle. Since the server gets > configured by the power_state scripts via devd, the server is never > configured with its global cx_lowest as anything other than C1. e.g: > > -bash-4.2$ sysctl -a|grep cx_lowest > hw.acpi.cpu.cx_lowest: C1 > dev.cpu.0.cx_lowest: C1 > dev.cpu.1.cx_lowest: C1 > > -bash-4.2$ sysctl -a|grep cx_supported > dev.cpu.0.cx_supported: C1/1 C2/96 > dev.cpu.1.cx_supported: C1/1 C2/96 > > > It seems that the rc.conf value of performance_cx_lowest="LOW" is what I > really want, not economy_cx_lowest. Yes. Could you please try this without using your patch? I get an impression that its effect was to actually request C2 when cx_lowest is set to C1. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
On Tue, 2012-06-19 at 14:07 -0700, Andriy Gapon wrote: > on 19/06/2012 19:02 Sean Bruno said the following: > > The first impact of this behavior is to list C3 as C2 in the list of > > Cstates when you retrieve the cx_supported sysctls for the cpus. > > I do not think that this is a real problem. A cosmetic one - most likely. > Yes, most likely. Except that the code seems to think that the index of the Cstates is good enough for a comparison to value. More over, the sysctl's accept a value like "C3" and manipulate that into an index into the Cstate array without checking for the Cstate value. The impact of this patch corrects this cosmetic display issue. > > The > > second impact is that the power_profile script never drops to a valid > > Cstate when you set the economy_lowest variable in rc.conf. > > Could you please explain if this somehow follows from your first observation > and > how? > If not, could you please share your finding on what exactly causes this to > happen? > > Also, are we talking about a laptop here? Namely, judging from the reference > to > 'economy_lowest', are AC state changes in play? > No, what I was attempting to do was configure a server such that it would attempt to use the C3 state at idle. Since the server gets configured by the power_state scripts via devd, the server is never configured with its global cx_lowest as anything other than C1. e.g: -bash-4.2$ sysctl -a|grep cx_lowest hw.acpi.cpu.cx_lowest: C1 dev.cpu.0.cx_lowest: C1 dev.cpu.1.cx_lowest: C1 -bash-4.2$ sysctl -a|grep cx_supported dev.cpu.0.cx_supported: C1/1 C2/96 dev.cpu.1.cx_supported: C1/1 C2/96 It seems that the rc.conf value of performance_cx_lowest="LOW" is what I really want, not economy_cx_lowest. Sean ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: [CFT] Sparse Cstate Support -- Its possible, that I don't know what I'm doing.
on 19/06/2012 19:02 Sean Bruno said the following: > The first impact of this behavior is to list C3 as C2 in the list of > Cstates when you retrieve the cx_supported sysctls for the cpus. I do not think that this is a real problem. A cosmetic one - most likely. > The > second impact is that the power_profile script never drops to a valid > Cstate when you set the economy_lowest variable in rc.conf. Could you please explain if this somehow follows from your first observation and how? If not, could you please share your finding on what exactly causes this to happen? Also, are we talking about a laptop here? Namely, judging from the reference to 'economy_lowest', are AC state changes in play? -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
