Re: [igt-dev] [PATCH i-g-t v12 4/6] tests/i915/i915_pm_dc: Added test for DC5 during DPMS

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

On 8/23/2019 8:00 PM, Imre Deak wrote:
> On Wed, Aug 14, 2019 at 11:16:21PM +0530, Anshuman Gupta wrote:
>> From: Jyoti Yadav <jyoti.r.yadav@intel.com>
>>
>> Added new subtest for DC5 entry during DPMS on/off cycle.
>> During DPMS on/off cycle DC5 counter is incremented.
>>
>> v2: Rename the subtest with meaningful name.
>> v3: Rebased.
>> v4: Addressed review comments by removing leftover code
>>      cleanup().
>> v5: Addressed the review comment by removing redundant
>>      read_dc_counter() suggested by Imre.
>>      Listing actual change in patch set changelog to make review easier.
>> v6: Three way patch applied, no functional change.
>> v7: Disabling runtime suspend for the platform which support, DC9.
>>      rebased due to test name pm_dc changed to i915_pm_dc, aligning to
>>      other PM tests.
>> v8: Introduced setup_dc_dpms() in order to disable runtime pm, restoring
>>      POWER_DIR values to its original and enabling runtime pm  for other
>>      followed sub-tests.
>> v9: Check DC5 counter value after DPMS off, broke the dpms_on_off
>>      function to dpms_on and dpms_off. [Imre]
>> v10:Added AT_LEAST_Gen11 condition instead of IS_ICELAKE in order to
>>      disable runtime suspend. [Imre]
>>
>> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>>   tests/i915/i915_pm_dc.c | 62 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 62 insertions(+)
>>
>> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
>> index f261ecbf..f03d30a8 100644
>> --- a/tests/i915/i915_pm_dc.c
>> +++ b/tests/i915/i915_pm_dc.c
>> @@ -46,6 +46,7 @@ typedef struct {
>>   	enum psr_mode op_psr_mode;
>>   	drmModeModeInfo *mode;
>>   	igt_output_t *output;
>> +	bool runtime_suspend_disabled;
>>   } data_t;
>>   
>>   bool dc_state_wait_entry(int drm_fd, int dc_flag, int prev_dc_count);
>> @@ -173,6 +174,62 @@ static void test_dc_state_psr(data_t *data, int dc_flag)
>>   	cleanup(data);
>>   }
>>   
>> +static void setup_dc_dpms(data_t *data)
>> +{
>> +	if (IS_BROXTON(data->devid) || IS_GEMINILAKE(data->devid) ||
>> +	    AT_LEAST_GEN(data->devid, 11)) {
>> +		data->runtime_suspend_disabled = igt_disable_runtime_pm();
>> +		igt_require_f(data->runtime_suspend_disabled,
>> +			      "unable to disable runtime pm for i915\n");
>> +	} else {
>> +		data->runtime_suspend_disabled = false;
>> +	}
>> +}
>> +
>> +static void dpms_off(data_t *data)
>> +{
>> +	for (int i = 0; i < data->display.n_outputs; i++) {
>> +		kmstest_set_connector_dpms(data->drm_fd,
>> +					   data->display.outputs[i].config.connector,
>> +					   DRM_MODE_DPMS_OFF);
>> +	}
>> +
>> +	if (!data->runtime_suspend_disabled)
>> +		igt_assert(igt_wait_for_pm_status
>> +			   (IGT_RUNTIME_PM_STATUS_SUSPENDED));
>> +}
>> +
>> +static void dpms_on(data_t *data)
>> +{
>> +	for (int i = 0; i < data->display.n_outputs; i++) {
>> +		kmstest_set_connector_dpms(data->drm_fd,
>> +					   data->display.outputs[i].config.connector,
>> +					   DRM_MODE_DPMS_ON);
>> +	}
>> +
>> +	if (!data->runtime_suspend_disabled)
>> +		igt_assert(igt_wait_for_pm_status
>> +			   (IGT_RUNTIME_PM_STATUS_ACTIVE));
>> +}
>> +
>> +static void test_dc_state_dpms(data_t *data, int dc_flag)
>> +{
>> +	uint32_t dc_counter;
>> +
>> +	dc_counter = read_dc_counter(data->drm_fd, dc_flag);
>> +	dpms_off(data);
>> +	check_dc_counter(data->drm_fd, dc_flag, dc_counter);
>> +	dpms_on(data);
>> +
>> +	/* if runtime PM is disabled for i915 restore it,
>> +	 * so any other sub-test can use runtime-PM.
>> +	 */
>> +	if (data->runtime_suspend_disabled) {
>> +		igt_restore_runtime_pm();
>> +		igt_setup_runtime_pm();
>> +	}
> 
> The above restores what setup_dc_dpms() did so could you move it a
> cleanup_dc_dpms() function for clarity?
> 
>> +}
>> +
>>   int main(int argc, char *argv[])
>>   {
>>   	bool has_runtime_pm;
>> @@ -210,6 +267,11 @@ int main(int argc, char *argv[])
>>   		test_dc_state_psr(&data, CHECK_DC6);
>>   	}
>>   
>> +	igt_subtest("dc5-dpms") {
>> +		setup_dc_dpms(&data);
> 
> Could you move the above call to test_dc_state_dpms() and check for
I will do this and cleanup_dc_dpms() changes.
> CHECK_DC5 withing setup_dc_dpms()?
but i did not understand why do we need to check for CHECK_DC5 (dc flag) 
in setup_dc_dpms(), it is agnostic to DC5 and DC6. dc flag will be 
require in read_dc_counter() and check_dc_counter() function.
May be it will be clear when i will send entire patch set.
> 
> I couldn't spot any other issues so with these changes on the patchset:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> Could you please resend the whole patchset?
> 
>> +		test_dc_state_dpms(&data, CHECK_DC5);
>> +	}
>> +
>>   	igt_fixture {
>>   		close(data.debugfs_fd);
>>   		display_fini(&data);
>> -- 
>> 2.21.0
>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev