[PATCH 2/2] acpi: method: add _PSS test
Keng-Yu Lin
kengyu at canonical.com
Thu Apr 19 06:33:42 UTC 2012
On Thu, Apr 19, 2012 at 11:45 AM, Alex Hung <alex.hung at canonical.com> wrote:
> On 04/18/2012 07:41 PM, Colin King wrote:
>>
>> From: Colin Ian King<colin.king at canonical.com>
>>
>> Signed-off-by: Colin Ian King<colin.king at canonical.com>
>> ---
>> src/acpi/method/method.c | 106
>> +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 104 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
>> index 6797868..fae4b31 100644
>> --- a/src/acpi/method/method.c
>> +++ b/src/acpi/method/method.c
>> @@ -102,7 +102,7 @@
>> * _PSD 8.4.4.5 N
>> * _PSL 11.4.8 N
>> * _PSR 10.3.1 Y
>> - * _PSS 8.4.4.2 N
>> + * _PSS 8.4.4.2 Y
>> * _PSV 11.4.9 Y
>> * _PSW 7.2.12 Y
>> * _PTC 8.4.3.1 N
>> @@ -307,7 +307,7 @@ static int method_name_check(fwts_framework *fw)
>> int failed = 0;
>>
>> if ((methods = fwts_method_get_names()) != NULL) {
>> - fwts_log_info(fw, "Found %d Methods\n", methods->len);
>> + fwts_log_info(fw, "Found %d Objects\n", methods->len);
>>
>> fwts_list_foreach(item, methods) {
>> char *ptr;
>> @@ -1846,6 +1846,105 @@ static int method_test_UID(fwts_framework *fw)
>> }
>>
>>
>> +/* Section 8.4 */
>> +
>> +static void method_test_PSS_return(fwts_framework *fw, char *name,
>> ACPI_BUFFER *buf, ACPI_OBJECT *obj, void *private)
>> +{
>> + int i;
>> + bool failed = false;
>> + uint32_t max_freq = 0;
>> + uint32_t prev_power = 0;
>> +
>> + if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) !=
>> FWTS_OK)
>> + return;
>> +
>> + /* Something is really wrong if we don't have any elements in _PSS
>> */
>> + if (obj->Package.Count< 1) {
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSElementCount",
>> + "_PSS should return package of at least 1 element,
>> "
>> + "got %d elements instead.",
>> + obj->Package.Count);
>> + fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>> + return;
>> + }
>> +
>> + for (i=0; i< obj->Package.Count; i++) {
>> + ACPI_OBJECT *pstate;
>> +
>> + if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) {
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSElementType",
>> + "_PSS package element %d was not a package.", i);
>> + fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>> + failed = true;
>> + continue; /* Skip processing sub-package */
>> + }
>> +
>> + pstate =&obj->Package.Elements[i];
pstate points to the i-th sub-package.
>>
>> + if (pstate->Package.Count != 6) {
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSSubPackageElementCount",
>> + "_PSS P-State sub-package %d was expected
>> to have "
>> + "6 elements, got %d elements instead.",
>> + i, obj->Package.Count);
>> + fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>> + failed = true;
>> + continue; /* Skip processing sub-package */
>> + }
>> +
>> + /* Elements need to be all ACPI integer types */
>> + if ((pstate->Package.Elements[0].Type !=
>> ACPI_TYPE_INTEGER) ||
>> + (pstate->Package.Elements[1].Type !=
>> ACPI_TYPE_INTEGER) ||
>> + (pstate->Package.Elements[2].Type !=
>> ACPI_TYPE_INTEGER) ||
>> + (pstate->Package.Elements[3].Type !=
>> ACPI_TYPE_INTEGER) ||
>> + (pstate->Package.Elements[4].Type !=
>> ACPI_TYPE_INTEGER) ||
>> + (pstate->Package.Elements[5].Type !=
>> ACPI_TYPE_INTEGER)) {
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSSubPackageElementType",
>> + "_PSS P-State sub-package %d was expected
>> to have "
>> + "6 Integer elements but didn't", i);
>> + failed = true;
>> + continue;
>> + }
>> +
>> + fwts_log_info(fw, "PState %d: CPU %ld Mhz, %lu mW, latency
>> %lu us, bus master latency %lu us.",
>> + i,
>> + (unsigned
>> long)pstate->Package.Elements[0].Integer.Value,
>> + (unsigned
>> long)pstate->Package.Elements[1].Integer.Value,
>> + (unsigned
>> long)pstate->Package.Elements[2].Integer.Value,
>> + (unsigned
>> long)pstate->Package.Elements[3].Integer.Value);
>> +
>> + if (max_freq< pstate->Package.Elements[0].Integer.Value)
>> + max_freq =
>> pstate->Package.Elements[0].Integer.Value;
>
> max_freq is supposed to be in the first package, is this if-statement
> necessary?
>
I think pstate is modified in each iteration as a pointer to a
sub-package (see above).
pstate->Package.Elements[0] is CoreFrequency
> Do you intend to check max_freq with all frequencies in following packages
> in case the packages are not in descending order?
>
> i.e
> if (max_freq < pstate->Package.Elements[i].Integer.Value)
> max_freq = pstate->Package.Elements[i].Integer.Value;
>
suppose you mean obj->Package.Elements[i]->Package.Elements[0].Integer.Value.
>
>> +
>> + /* Sanity check descending power dissipation levels */
>> + if ((i> 0)&& (prev_power != 0)&&
>> + (pstate->Package.Elements[1].Integer.Value>=
>> prev_power)) {
>> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSSubPackagePowerNotDecending",
>> + "_PSS P-State sub-package %d has a larger
>> power dissipation "
>> + "setting than the previous sub-package.",
>> i);
>> + fwts_advice(fw, "_PSS P-States must be ordered in
>> decending order of "
>> + "power dissipation, so that the zero'th
>> entry has the highest "
>> + "power dissipation level and the Nth has
>> the lowest.");
>> + failed = true;
>> + }
>> + prev_power = pstate->Package.Elements[1].Integer.Value;
>> + }
>> +
>> + if (max_freq< 1000) {
>
> Will it be better if we use
> "if (!failed && (max_freq = Package.Elements[0].Integer.Value) < 1000)"
> and "max_freq" needs not be set in the for-loop?
>
> If the sanity check fails in for-loop, may it not be necessary to check
> max_freq?
>
>
>> + fwts_warning(fw,
>> + "Maximum CPU frequency is %dHz and this is low for
>> "
>> + "a modern processor. This may indicate the _PSS
>> PStates "
>> + "are incorrect\n", max_freq);
>> + failed = true;
>> + }
>> +
>> + if (!failed)
>> + fwts_passed(fw, "_PSS correctly returned sane looking
>> package.");
>> +}
>> +
>> +static int method_test_PSS(fwts_framework *fw)
>> +{
>> + return method_evaluate_method(fw, METHOD_OPTIONAL, "_PSS", NULL,
>> 0, method_test_PSS_return, NULL);
>> +}
>> +
>> /* Tests */
>>
>> static fwts_framework_minor_test method_tests[] = {
>> @@ -1973,6 +2072,9 @@ static fwts_framework_minor_test method_tests[] = {
>> { method_test_ON, "Check _ON (Set resource on)." },
>> { method_test_OFF, "Check _OFF (Set resource off)." },
>>
>> + /* Section 8.4 */
>> + { method_test_PSS, "Check _PSS (Performance Supported States)." },
>> +
>> /* Appendix B, ACPI Extensions for Display Adapters */
>>
>> { method_test_DOS, "Check _DOS (Enable/Disable Output Switching)."
>> },
>
More information about the fwts-devel
mailing list