Li, Aubrey wrote: > 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? >
I just think it would have been better to have had this stuff disabled by default and then provide a boolean to enable it if the user wanted it. Otherwise, I think we are providing data that is basically useless to most users. *And* if the BIOS developers ever got smart and made some real decisions based on whether or not the OSPM supported certain features, we'd be lying to them. >> 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 > Again, I'd prefer we had the option disabled by default if we don't really support C-states for the system. > >> 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? > In the case of turbo info, we actually support Turbo Mode. No, I see no reason to provide deep C-state data (by default) when we aren't supporting it on a particular system. > >>> 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, :) > Ah, I misread the code. Regardless, it has no pertinence on this discussion since we are talking about which bits to set in the _PDC. > > >> 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. > I have no issue with whether or not the idle function is installed. I think that is working correctly. > >>> 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? > I don't think the _PDC was intended to be used that way, "Yeah, we have the ability, but hehe we're not really supporting it for this processor. Got you!!!" But then again, I think the mechanism is totally broken. It hardly matters whether the OSPM supports the stuff it says it supports. Just tell the BIOS you can do it all. > >> In any case ... Jason, SpeedStep should work for you starting >> with build >> 110. >> >> > > hehe, :D > Let's just replace the _PDC with a boolean that tells the BIOS whether or not is should load the CPU PM related tables or not. Unfortunately, we can't really do that since some BIOS developers return different tables depending on whether the OSPM can support MP. This was probably the only useful functionality provided by the _PDC. I've made a bigger deal out of this than it deserves. But the _PDC has caused more trouble than anything else. I sympathize for anyone who is running an older OS that does not support deep C-states, because they no longer have SpeedStep support on a system like the one mentioned in this thread. Mark > -Aubrey >
