Re: [igt-dev] [PATCH i-g-t v6 2/4] tests/i915/i915_pm_rpm: modeset-pc8-residency-stress

From: "Gupta, Anshuman" <anshuman.gupta@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t v6 2/4] tests/i915/i915_pm_rpm: modeset-pc8-residency-stress
Date: Sat, 27 Apr 2019 12:05:41 +0530	[thread overview]
Message-ID: <1174fc3f-fbdd-d420-ac91-fc22f4edc34b@intel.com> (raw)
In-Reply-To: <20190426175943.GT24299@intel.com>

On 4/26/2019 11:29 PM, Ville Syrjälä wrote:
> On Fri, Apr 26, 2019 at 11:15:24PM +0530, Anshuman Gupta wrote:
>> Introduced pc8_needs_screen_off flag in order to differentiate
>> between HASWELL/BROADWELL and AT_LEAST_GEN9. GEN9 onwards
>> PC8+ residency does't require display to be turned on.
> 
> Why are we so fixated on pc8? Shouldn't we just check that we reach
> the max package c-state with displays off?
pc8_plus_residency_changed() do check pc8 || pc9 || pc10.
though subtest is not failing if max PC9 or PC10 is not reached.

As of now most of CI gen9 (with PCH) platforms are failing to reach pc8 
itself with display off.

IMHO we should add this coverage of max package C state or even s0ix,
once pc8 passed on these failing platforms, what is your opinion about it.

I have added a DEBUG patch with this series to setup powertop and to 
collect PMC sysfs entries logs in order get root cause of pc8 failures.

Thanks ,
Anshuman Gupta.
> 
>>
>> v3:Removed pc8_needs_screen_off from mode_set_data structure,
>>     made it global, aligning to has_pc8 and has_runtime_pm globals. [Ram]
>>     Made modeset_subtest() to tests PC8+ residency after enabling a screen,
>>     earlier it expects PC8+ residency to stop on HSW/BDW.
>>
>> v4:Fixed conditional code for pc8_needs_screen_off. [Ram]
>>     Used macros for timeout values, given to PC8+ residency check function. [Ram]
>>     Changed the screen on timeout to check pc8+ residency to 10 seconds.
>>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>>   tests/i915/i915_pm_rpm.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
>> index 9315dbd..bd13d21 100644
>> --- a/tests/i915/i915_pm_rpm.c
>> +++ b/tests/i915/i915_pm_rpm.c
>> @@ -907,7 +907,7 @@ static void modeset_subtest(enum screen_type type, int rounds, int wait_flags)
>>   		if (wait_flags & WAIT_STATUS)
>>   			igt_assert(wait_for_suspended());
>>   		if (wait_flags & WAIT_PC8_RES)
>> -			igt_assert(pc8_plus_residency_changed(30));
>> +			igt_assert(pc8_plus_residency_changed(TIME_OUT_SEC_30));
>>   		if (wait_flags & WAIT_EXTRA)
>>   			sleep(5);
>>   
>> @@ -917,7 +917,13 @@ static void modeset_subtest(enum screen_type type, int rounds, int wait_flags)
>>   		if (wait_flags & WAIT_STATUS)
>>   			igt_assert(wait_for_active());
>>   		if (wait_flags & WAIT_PC8_RES)
>> -			igt_assert(!pc8_plus_residency_changed(5));
>> +			if (pc8_needs_screen_off)
>> +				igt_assert(!pc8_plus_residency_changed
>> +					   (TIME_OUT_SEC_5));
>> +			else
>> +				igt_assert(pc8_plus_residency_changed
>> +					   (TIME_OUT_SEC_10));
>> +
>>   		if (wait_flags & WAIT_EXTRA)
>>   			sleep(5);
>>   	}
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev