Mark.Haywood wrote: > Li, Aubrey wrote: >> Mark.Haywood wrote: >> >> >>>>>> sigh, buggy BIOS. "0x1B" checking will load "SSDT" table. Maybe >>>>>> there is something else in the table need to be loaded to support >>>>>> deep c-state, although I saw _CST(C1ST, C2ST, C3ST, C4ST) are >>>>>> already there. >>>>>> >>>>>> This issue should be fixed in 110 and later build due to the deep >>>>>> c-states support, by accident, :D. So no more work need to be >>>>>> done. >>>>>> >>>>>> >>>>>> >>>>> Not necessarily. Only if cpu_deep_cstates_supported() returns >>>>> B_TRUE. >>>>> >>>>> >>>>> >>>> No, PDC setting doesn't need to do that check. >>>> >>>> >>> Probably doesn't matter much, but besides the comment being wrong: >>> >>> >>> /* >>> * If we support deep cstates on this processor, then set the >>> * correct cstate_ops for the processor and enable >>> appropriate >>> * _PDC >>> bits. >>> */ >>> mach_state->ms_cstate.cma_ops = &cpu_idle_ops; >>> cpupm_intel_pdccap |= CPUPM_INTEL_PDC_C1_HALT | >>> CPUPM_INTEL_PDC_C2C3_MP | CPUPM_INTEL_PDC_C1_FFH; >>> >>> I do wonder why we are setting the bits if we don't support it? And >>> unless I'm misreading the code, we even try to read the C-state data >>> even in the case where we don't support the processor? >>> >>> >> >> For development/observation purpose. We can see the cstate data in >> kstat. This helps to diagnose system. And it also helps us to make >> the cstate cache function robust. >> > > Not sure I understand your point. We want to observe cstate data on > systems for which we don't support C-states?
Cstate data including Cx power consumption, latency, which are interesting to the developer. And some systems have two C3, not duplicate, the two are valid, these info are useful. C-state is more complicated than P-state. It depends on HPET and only support on NHM now, and may not need HPET in future, or support pre-NHM platform by another time source, I'd like to separate the cstate data and c-state support. Is there a reason why we can't do that? > Also, not sure how this makes the cstate cache function any more robust. Currently, if caching cstate data only works when cpu_deep_cstates_supported() returns true, we'll lost the chance to test it on the different kinds of machine. For example, we can not find the panic issue reported several days ago on Toshiba M9/M10. Does this make sense? :p > We don't have to > execute cpu_deep_cstates_supported() as part of the caching function? > We're going to run it as part of the CPU PM initialization anyway. We just export the data for observation, like we export the turbo info, Is there a reason why we can't do that? > >> We also have "idle_cpu_no_deep_c" mechanism. If the user set it in >> kmdb during boot, he/she still can get deep cstate support without >> reboot the system. >> > Actually, it allows the user to disable deep cstate, not > enable it. So, I don't see its pertinence in this discussion. Actually I often use this way to enable deep cstate when I did the testing, to save the reboot time, :) > Besides which, I don't see how it makes any difference in deciding whether > or not to use cpu_deep_cstates_supported() to decide how to set the _PDC bits. No difference to set _PDC, but be different to install idle function. This is enough, I think. > >> And, it helps to fix bugs to enable P-state, hehe, :D >> > > Yes. The BIOS bugs that exist because the BIOS developers don't > understand the _PDC bits. > >> Anyway, I didn't see a reason we can't do this in the current way. >> > > So effectively we *are* enabling all the _PDC bits all the > time (except > in the case where we know Solaris won't support SpeedStep). Sigh. The > _PDC was a bad idea. As long as the OSPM has the ability/function to support all of what _PDC bits indicate, why we can't just enable all the bits? > > In any case ... Jason, SpeedStep should work for you starting > with build > 110. > hehe, :D -Aubrey
