[PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Jeevan B]

Add a new test to validate deep sleep states during extended vblank
scenarios, where two frames are committed simultaneously for a give
time with reduced refresh rate.
v2: dealy of one frame added to simulate extended vblank.
    remove vrr related debug checks.

Signed-off-by: Jeevan B <jeevan.b@intel.com>
---
 tests/intel/kms_pm_dc.c | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
index 7766d34d7..9f89e537d 100644
--- a/tests/intel/kms_pm_dc.c
+++ b/tests/intel/kms_pm_dc.c
@@ -584,6 +584,46 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
 	return get_dc_counter(str);
 }
 
+static void test_deep_pkgc_state(data_t *data)
+{
+	unsigned int pre_val = 0, cur_val = 0;
+	time_t start = time(NULL), duration = 2, delay;
+	enum pipe pipe;
+	bool pkgc_flag;
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+	igt_output_t *output;
+
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		igt_output_set_pipe(output, pipe);
+		data->output = output;
+		data->mode = igt_output_get_mode(output);
+		setup_videoplayback(data);
+		igt_require(igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE));
+		igt_require(igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE));
+
+		primary = igt_output_get_plane_type(data->output,
+						    DRM_PLANE_TYPE_PRIMARY);
+		pre_val = read_pkgc_counter(data->debugfs_root_fd);
+		delay = 1 * ((1000 * 1000) / data->mode->vrefresh);
+		while (time(NULL) - start < duration) {
+			igt_plane_set_fb(primary, &data->fb_rgb);
+			igt_display_commit(&data->display);
+			usleep(delay);
+
+			igt_plane_set_fb(primary, &data->fb_rgr);
+			igt_display_commit(&data->display);
+			cur_val = read_pkgc_counter(data->debugfs_root_fd);
+			if (cur_val > pre_val) {
+				pkgc_flag = true;
+				continue;
+			}
+		}
+	}
+	cleanup_dc3co_fbs(data);
+	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");
+}
+
 static void test_pkgc_state_dpms(data_t *data)
 {
 	unsigned int timeout_sec = 6;
@@ -687,6 +727,15 @@ igt_main
 			test_dc_state_psr(&data, CHECK_DC6);
 	}
 
+	igt_describe("This test validates display engine entry to DC8 state "
+		     "while extended vblank");
+	igt_subtest("deep-pkgc") {
+		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
+			      "PC8+ residencies not supported\n");
+		igt_require(intel_display_ver(data.devid) >= 20);
+		test_deep_pkgc_state(&data);
+	}
+
 	igt_describe("This test validates display engine entry to DC5 state "
 		     "while all connectors's DPMS property set to OFF");
 	igt_subtest("dc5-dpms") {
-- 
2.25.1

RE: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Kandpal, Suraj]

> Subject: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the
> deep sleep state during extended vblank
> 
> Add a new test to validate deep sleep states during extended vblank scenarios,
> where two frames are committed simultaneously for a give time with reduced

Typo here *given

> refresh rate.
> v2: dealy of one frame added to simulate extended vblank.
>     remove vrr related debug checks.
> 
> Signed-off-by: Jeevan B <jeevan.b@intel.com>
> ---
>  tests/intel/kms_pm_dc.c | 49
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c index
> 7766d34d7..9f89e537d 100644
> --- a/tests/intel/kms_pm_dc.c
> +++ b/tests/intel/kms_pm_dc.c
> @@ -584,6 +584,46 @@ static unsigned int read_pkgc_counter(int
> debugfs_root_fd)
>  	return get_dc_counter(str);
>  }
> 
> +static void test_deep_pkgc_state(data_t *data) {
> +	unsigned int pre_val = 0, cur_val = 0;
> +	time_t start = time(NULL), duration = 2, delay;
> +	enum pipe pipe;
> +	bool pkgc_flag;
> +	igt_display_t *display = &data->display;
> +	igt_plane_t *primary;
> +	igt_output_t *output;
> +
> +	for_each_pipe_with_valid_output(display, pipe, output) {
> +		igt_output_set_pipe(output, pipe);
> +		data->output = output;
> +		data->mode = igt_output_get_mode(output);
> +		setup_videoplayback(data);
> +		igt_require(igt_output_has_prop(output,
> IGT_CONNECTOR_VRR_CAPABLE));
> +		igt_require(igt_output_get_prop(output,
> IGT_CONNECTOR_VRR_CAPABLE));

Why are there two igt_require checking the same thing here?

> +
> +		primary = igt_output_get_plane_type(data->output,
> +
> DRM_PLANE_TYPE_PRIMARY);
> +		pre_val = read_pkgc_counter(data->debugfs_root_fd);
> +		delay = 1 * ((1000 * 1000) / data->mode->vrefresh);

If 1000*1000 is constant why not use a #define would look cleaner 

> +		while (time(NULL) - start < duration) {
> +			igt_plane_set_fb(primary, &data->fb_rgb);
> +			igt_display_commit(&data->display);
> +			usleep(delay);
> +
> +			igt_plane_set_fb(primary, &data->fb_rgr);
> +			igt_display_commit(&data->display);
> +			cur_val = read_pkgc_counter(data->debugfs_root_fd);
> +			if (cur_val > pre_val) {
> +				pkgc_flag = true;
> +				continue;
> +			}
> +		}
> +	}
> +	cleanup_dc3co_fbs(data);
> +	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n"); }
> +
>  static void test_pkgc_state_dpms(data_t *data)  {
>  	unsigned int timeout_sec = 6;
> @@ -687,6 +727,15 @@ igt_main
>  			test_dc_state_psr(&data, CHECK_DC6);
>  	}
> 
> +	igt_describe("This test validates display engine entry to DC8 state "
> +		     "while extended vblank");

*during extended vblank

Regards,
Suraj Kandpal
> +	igt_subtest("deep-pkgc") {
> +
> 	igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
> +			      "PC8+ residencies not supported\n");
> +		igt_require(intel_display_ver(data.devid) >= 20);
> +		test_deep_pkgc_state(&data);
> +	}
> +
>  	igt_describe("This test validates display engine entry to DC5 state "
>  		     "while all connectors's DPMS property set to OFF");
>  	igt_subtest("dc5-dpms") {
> --
> 2.25.1

RE: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[B, Jeevan]

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> Sent: Monday, May 27, 2024 10:33 AM
> To: B, Jeevan <jeevan.b@intel.com>; igt-dev@lists.freedesktop.org
> Cc: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> Subject: RE: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate
> the deep sleep state during extended vblank
> 
> > Subject: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to
> > validate the deep sleep state during extended vblank
> >
> > Add a new test to validate deep sleep states during extended vblank
> > scenarios, where two frames are committed simultaneously for a give
> > time with reduced
> 
> Typo here *given
> 
> > refresh rate.
> > v2: dealy of one frame added to simulate extended vblank.
> >     remove vrr related debug checks.
> >
> > Signed-off-by: Jeevan B <jeevan.b@intel.com>
> > ---
> >  tests/intel/kms_pm_dc.c | 49
> > +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c index
> > 7766d34d7..9f89e537d 100644
> > --- a/tests/intel/kms_pm_dc.c
> > +++ b/tests/intel/kms_pm_dc.c
> > @@ -584,6 +584,46 @@ static unsigned int read_pkgc_counter(int
> > debugfs_root_fd)
> >  	return get_dc_counter(str);
> >  }
> >
> > +static void test_deep_pkgc_state(data_t *data) {
> > +	unsigned int pre_val = 0, cur_val = 0;
> > +	time_t start = time(NULL), duration = 2, delay;
> > +	enum pipe pipe;
> > +	bool pkgc_flag;
> > +	igt_display_t *display = &data->display;
> > +	igt_plane_t *primary;
> > +	igt_output_t *output;
> > +
> > +	for_each_pipe_with_valid_output(display, pipe, output) {
> > +		igt_output_set_pipe(output, pipe);
> > +		data->output = output;
> > +		data->mode = igt_output_get_mode(output);
> > +		setup_videoplayback(data);
> > +		igt_require(igt_output_has_prop(output,
> > IGT_CONNECTOR_VRR_CAPABLE));
> > +		igt_require(igt_output_get_prop(output,
> > IGT_CONNECTOR_VRR_CAPABLE));
> 
> Why are there two igt_require checking the same thing here?

To check VRR is supported and available. 

BR, Jeevan B 
> 
> > +
> > +		primary = igt_output_get_plane_type(data->output,
> > +
> > DRM_PLANE_TYPE_PRIMARY);
> > +		pre_val = read_pkgc_counter(data->debugfs_root_fd);
> > +		delay = 1 * ((1000 * 1000) / data->mode->vrefresh);
> 
> If 1000*1000 is constant why not use a #define would look cleaner
> 
> > +		while (time(NULL) - start < duration) {
> > +			igt_plane_set_fb(primary, &data->fb_rgb);
> > +			igt_display_commit(&data->display);
> > +			usleep(delay);
> > +
> > +			igt_plane_set_fb(primary, &data->fb_rgr);
> > +			igt_display_commit(&data->display);
> > +			cur_val = read_pkgc_counter(data->debugfs_root_fd);
> > +			if (cur_val > pre_val) {
> > +				pkgc_flag = true;
> > +				continue;
> > +			}
> > +		}
> > +	}
> > +	cleanup_dc3co_fbs(data);
> > +	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n"); }
> > +
> >  static void test_pkgc_state_dpms(data_t *data)  {
> >  	unsigned int timeout_sec = 6;
> > @@ -687,6 +727,15 @@ igt_main
> >  			test_dc_state_psr(&data, CHECK_DC6);
> >  	}
> >
> > +	igt_describe("This test validates display engine entry to DC8 state "
> > +		     "while extended vblank");
> 
> *during extended vblank
> 
> Regards,
> Suraj Kandpal
> > +	igt_subtest("deep-pkgc") {
> > +
> > 	igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
> > +			      "PC8+ residencies not supported\n");
> > +		igt_require(intel_display_ver(data.devid) >= 20);
> > +		test_deep_pkgc_state(&data);
> > +	}
> > +
> >  	igt_describe("This test validates display engine entry to DC5 state "
> >  		     "while all connectors's DPMS property set to OFF");
> >  	igt_subtest("dc5-dpms") {
> > --
> > 2.25.1

[PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Jeevan B]

Add a new test to validate deep sleep states during extended vblank
scenarios, where two frames are committed simultaneously for a given
time with reduced refresh rate.
v2: dealy of one frame added to simulate extended vblank.
    remove vrr related debug checks.
v3: fix typo and add define. (Suraj)

Signed-off-by: Jeevan B <jeevan.b@intel.com>
---
 tests/intel/kms_pm_dc.c | 54 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
index 7766d34d7..f7989ed47 100644
--- a/tests/intel/kms_pm_dc.c
+++ b/tests/intel/kms_pm_dc.c
@@ -76,6 +76,10 @@
  *
  * SUBTEST: dc9-dpms
  * Description: This test validates display engine entry to DC9 state
+ *
+ * SUBTEST: deep-pkgc
+ * Description: This test validates display engine entry to PKGC10 state for extended vblank
+ * Functionality: pm_dc
  */
 
 /* DC State Flags */
@@ -89,6 +93,7 @@
 #define PACKAGE_CSTATE_PATH  "pmc_core/package_cstate_show"
 #define KMS_POLL_DISABLE 0
 #define DC9_RESETS_DC_COUNTERS(devid) (!(IS_DG1(devid) || IS_DG2(devid) || AT_LEAST_DISPLAY(devid, 14)))
+#define MSECS 1000000
 
 IGT_TEST_DESCRIPTION("Tests to validate display power DC states.");
 
@@ -584,6 +589,46 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
 	return get_dc_counter(str);
 }
 
+static void test_deep_pkgc_state(data_t *data)
+{
+	unsigned int pre_val = 0, cur_val = 0;
+	time_t start = time(NULL), duration = 2, delay;
+	enum pipe pipe;
+	bool pkgc_flag;
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+	igt_output_t *output;
+
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		igt_output_set_pipe(output, pipe);
+		data->output = output;
+		data->mode = igt_output_get_mode(output);
+		setup_videoplayback(data);
+		igt_require(igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE));
+		igt_require(igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE));
+
+		primary = igt_output_get_plane_type(data->output,
+						    DRM_PLANE_TYPE_PRIMARY);
+		pre_val = read_pkgc_counter(data->debugfs_root_fd);
+		delay = 1 * (MSECS / data->mode->vrefresh);
+		while (time(NULL) - start < duration) {
+			igt_plane_set_fb(primary, &data->fb_rgb);
+			igt_display_commit(&data->display);
+			usleep(delay);
+
+			igt_plane_set_fb(primary, &data->fb_rgr);
+			igt_display_commit(&data->display);
+			cur_val = read_pkgc_counter(data->debugfs_root_fd);
+			if (cur_val > pre_val) {
+				pkgc_flag = true;
+				continue;
+			}
+		}
+	}
+	cleanup_dc3co_fbs(data);
+	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");
+}
+
 static void test_pkgc_state_dpms(data_t *data)
 {
 	unsigned int timeout_sec = 6;
@@ -687,6 +732,15 @@ igt_main
 			test_dc_state_psr(&data, CHECK_DC6);
 	}
 
+	igt_describe("This test validates display engine entry to PKGC10 state "
+		     "during extended vblank");
+	igt_subtest("deep-pkgc") {
+		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
+			      "PC8+ residencies not supported\n");
+		igt_require(intel_display_ver(data.devid) >= 20);
+		test_deep_pkgc_state(&data);
+	}
+
 	igt_describe("This test validates display engine entry to DC5 state "
 		     "while all connectors's DPMS property set to OFF");
 	igt_subtest("dc5-dpms") {
-- 
2.25.1

Re: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Modem, Bhanuprakash]

Hi Jeevan,

On 27-05-2024 03:51 pm, Jeevan B wrote:
> Add a new test to validate deep sleep states during extended vblank
> scenarios, where two frames are committed simultaneously for a given
> time with reduced refresh rate.
> v2: dealy of one frame added to simulate extended vblank.
>      remove vrr related debug checks.
> v3: fix typo and add define. (Suraj)
> 
> Signed-off-by: Jeevan B <jeevan.b@intel.com>
> ---
>   tests/intel/kms_pm_dc.c | 54 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 54 insertions(+)
> 
> diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
> index 7766d34d7..f7989ed47 100644
> --- a/tests/intel/kms_pm_dc.c
> +++ b/tests/intel/kms_pm_dc.c
> @@ -76,6 +76,10 @@
>    *
>    * SUBTEST: dc9-dpms
>    * Description: This test validates display engine entry to DC9 state
> + *
> + * SUBTEST: deep-pkgc
> + * Description: This test validates display engine entry to PKGC10 state for extended vblank
> + * Functionality: pm_dc
>    */
>   
>   /* DC State Flags */
> @@ -89,6 +93,7 @@
>   #define PACKAGE_CSTATE_PATH  "pmc_core/package_cstate_show"
>   #define KMS_POLL_DISABLE 0
>   #define DC9_RESETS_DC_COUNTERS(devid) (!(IS_DG1(devid) || IS_DG2(devid) || AT_LEAST_DISPLAY(devid, 14)))
> +#define MSECS 1000000
>   
>   IGT_TEST_DESCRIPTION("Tests to validate display power DC states.");
>   
> @@ -584,6 +589,46 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
>   	return get_dc_counter(str);
>   }
>   
> +static void test_deep_pkgc_state(data_t *data)
> +{
> +	unsigned int pre_val = 0, cur_val = 0;
> +	time_t start = time(NULL), duration = 2, delay;
> +	enum pipe pipe;
> +	bool pkgc_flag;

Please initialize this flag with "false".

> +	igt_display_t *display = &data->display;
> +	igt_plane_t *primary;
> +	igt_output_t *output;
> +
> +	for_each_pipe_with_valid_output(display, pipe, output) {
> +		igt_output_set_pipe(output, pipe);

Please do igt_display_reset()/igt_output_set_pipe(NULL) before setting 
the output to pipe.

> +		data->output = output;
> +		data->mode = igt_output_get_mode(output);
> +		setup_videoplayback(data);
> +		igt_require(igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE));
> +		igt_require(igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE));

Check for the config capabilities before setting up anything.
(Move these checks to just after initializing the loops)

if (!has_vrr_capable || !get_vrr_capable)
	continue;

> +
> +		primary = igt_output_get_plane_type(data->output,
> +						    DRM_PLANE_TYPE_PRIMARY);
> +		pre_val = read_pkgc_counter(data->debugfs_root_fd);
> +		delay = 1 * (MSECS / data->mode->vrefresh);
> +		while (time(NULL) - start < duration) {
> +			igt_plane_set_fb(primary, &data->fb_rgb);
> +			igt_display_commit(&data->display);
> +			usleep(delay);
> +
> +			igt_plane_set_fb(primary, &data->fb_rgr);
> +			igt_display_commit(&data->display);

Why do we need 2 commits, lets simplify this as below:

                 pre_val = read_pkgc_counter(data->debugfs_root_fd);
                 delay = 1 * (MSECS / data->mode->vrefresh);
+
+               igt_plane_set_fb(primary, &data->fb_rgb);
+               igt_display_commit(&data->display);
+
+               flip = true;
+
                 while (time(NULL) - start < duration) {
-                       igt_plane_set_fb(primary, &data->fb_rgb);
+                       flip = !flip;
+                       igt_plane_set_fb(primary, flip ? &data->fb_rgb : 
&data->fb_rgr);
                         igt_display_commit(&data->display);
-                       usleep(delay);

-                       igt_plane_set_fb(primary, &data->fb_rgr);
-                       igt_display_commit(&data->display);
                         cur_val = read_pkgc_counter(data->debugfs_root_fd);
                         if (cur_val > pre_val) {
                                 pkgc_flag = true;
-                               continue;
+                               break;
                         }
+                       usleep(delay);
                 }
         }

> +			cur_val = read_pkgc_counter(data->debugfs_root_fd);
> +			if (cur_val > pre_val) {
> +				pkgc_flag = true;
> +				continue;

If PKGC states achieved, we must do the "break" instead of "continue". 
Right?

> +			}
> +		}
> +	}
> +	cleanup_dc3co_fbs(data);
> +	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");

It'll throw an error if there is no valid output found, instead we must 
skip.

- Bhanu

> +}
> +
>   static void test_pkgc_state_dpms(data_t *data)
>   {
>   	unsigned int timeout_sec = 6;
> @@ -687,6 +732,15 @@ igt_main
>   			test_dc_state_psr(&data, CHECK_DC6);
>   	}
>   
> +	igt_describe("This test validates display engine entry to PKGC10 state "
> +		     "during extended vblank");
> +	igt_subtest("deep-pkgc") {
> +		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
> +			      "PC8+ residencies not supported\n");
> +		igt_require(intel_display_ver(data.devid) >= 20);
> +		test_deep_pkgc_state(&data);
> +	}
> +
>   	igt_describe("This test validates display engine entry to DC5 state "
>   		     "while all connectors's DPMS property set to OFF");
>   	igt_subtest("dc5-dpms") {

[PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Jeevan B]

Add a new test to validate deep sleep states during extended vblank
scenarios, where two frames are committed simultaneously for a given
time with reduced refresh rate.
v2: dealy of one frame added to simulate extended vblank.
    remove vrr related debug checks.
v3: fix typo and add define. (Suraj)
v4: fix structure and add skip if no vrr monitor found. (Bhanu)

Signed-off-by: Jeevan B <jeevan.b@intel.com>
---
 tests/intel/kms_pm_dc.c | 78 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
index 7766d34d7..51416c2f0 100644
--- a/tests/intel/kms_pm_dc.c
+++ b/tests/intel/kms_pm_dc.c
@@ -76,6 +76,10 @@
  *
  * SUBTEST: dc9-dpms
  * Description: This test validates display engine entry to DC9 state
+ *
+ * SUBTEST: deep-pkgc
+ * Description: This test validates display engine entry to PKGC10 state for extended vblank
+ * Functionality: pm_dc
  */
 
 /* DC State Flags */
@@ -89,6 +93,7 @@
 #define PACKAGE_CSTATE_PATH  "pmc_core/package_cstate_show"
 #define KMS_POLL_DISABLE 0
 #define DC9_RESETS_DC_COUNTERS(devid) (!(IS_DG1(devid) || IS_DG2(devid) || AT_LEAST_DISPLAY(devid, 14)))
+#define MSECS 1000000
 
 IGT_TEST_DESCRIPTION("Tests to validate display power DC states.");
 
@@ -584,6 +589,70 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
 	return get_dc_counter(str);
 }
 
+static void test_deep_pkgc_state(data_t *data)
+{
+	unsigned int pre_val = 0, cur_val = 0;
+	time_t start = time(NULL), duration = 2, delay;
+	enum pipe pipe;
+	bool pkgc_flag = false;
+	bool vrr_supported = false, flip = true;
+
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+	igt_output_t *output;
+
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		// Check VRR capabilities before setting up
+		if (igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&
+		    igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
+			vrr_supported = true;
+			break;
+		}
+	}
+
+	// Skip the test if no VRR capable output is found
+	if (!vrr_supported)
+		igt_skip("No VRR capable output found, skipping the test.\n");
+
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		igt_display_reset(display);
+		igt_output_set_pipe(output, PIPE_NONE);
+
+		if (!igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) ||
+		   !igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
+			continue;
+		}
+
+		igt_output_set_pipe(output, pipe);
+
+		data->output = output;
+		data->mode = igt_output_get_mode(output);
+		setup_videoplayback(data);
+
+		primary = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
+		pre_val = read_pkgc_counter(data->debugfs_root_fd);
+		delay = 1 * (MSECS / data->mode->vrefresh);
+
+		igt_plane_set_fb(primary, &data->fb_rgb);
+		igt_display_commit(&data->display);
+
+		while (time(NULL) - start < duration) {
+			flip = !flip;
+			igt_plane_set_fb(primary, flip ? &data->fb_rgb : &data->fb_rgr);
+			igt_display_commit(&data->display);
+
+			cur_val = read_pkgc_counter(data->debugfs_root_fd);
+			if (cur_val > pre_val) {
+				pkgc_flag = true;
+				break;
+			}
+			usleep(delay);
+		}
+	}
+	cleanup_dc3co_fbs(data);
+	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");
+}
+
 static void test_pkgc_state_dpms(data_t *data)
 {
 	unsigned int timeout_sec = 6;
@@ -687,6 +756,15 @@ igt_main
 			test_dc_state_psr(&data, CHECK_DC6);
 	}
 
+	igt_describe("This test validates display engine entry to PKGC10 state "
+		     "during extended vblank");
+	igt_subtest("deep-pkgc") {
+		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
+			      "PC8+ residencies not supported\n");
+		igt_require(intel_display_ver(data.devid) >= 20);
+		test_deep_pkgc_state(&data);
+	}
+
 	igt_describe("This test validates display engine entry to DC5 state "
 		     "while all connectors's DPMS property set to OFF");
 	igt_subtest("dc5-dpms") {
-- 
2.25.1

Re: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Modem, Bhanuprakash]

Hi Jeevan,

On 27-05-2024 11:09 pm, Jeevan B wrote:
> Add a new test to validate deep sleep states during extended vblank
> scenarios, where two frames are committed simultaneously for a given
> time with reduced refresh rate.
> v2: dealy of one frame added to simulate extended vblank.
>      remove vrr related debug checks.
> v3: fix typo and add define. (Suraj)
> v4: fix structure and add skip if no vrr monitor found. (Bhanu)
> 
> Signed-off-by: Jeevan B <jeevan.b@intel.com>
> ---
>   tests/intel/kms_pm_dc.c | 78 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 78 insertions(+)
> 
> diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
> index 7766d34d7..51416c2f0 100644
> --- a/tests/intel/kms_pm_dc.c
> +++ b/tests/intel/kms_pm_dc.c
> @@ -76,6 +76,10 @@
>    *
>    * SUBTEST: dc9-dpms
>    * Description: This test validates display engine entry to DC9 state
> + *
> + * SUBTEST: deep-pkgc
> + * Description: This test validates display engine entry to PKGC10 state for extended vblank
> + * Functionality: pm_dc
>    */
>   
>   /* DC State Flags */
> @@ -89,6 +93,7 @@
>   #define PACKAGE_CSTATE_PATH  "pmc_core/package_cstate_show"
>   #define KMS_POLL_DISABLE 0
>   #define DC9_RESETS_DC_COUNTERS(devid) (!(IS_DG1(devid) || IS_DG2(devid) || AT_LEAST_DISPLAY(devid, 14)))
> +#define MSECS 1000000

Please make this arch/compiler safe

#define MSECS (1000000ul)

>   
>   IGT_TEST_DESCRIPTION("Tests to validate display power DC states.");
>   
> @@ -584,6 +589,70 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
>   	return get_dc_counter(str);
>   }
>   
> +static void test_deep_pkgc_state(data_t *data)
> +{
> +	unsigned int pre_val = 0, cur_val = 0;
> +	time_t start = time(NULL), duration = 2, delay;
> +	enum pipe pipe;
> +	bool pkgc_flag = false;
> +	bool vrr_supported = false, flip = true;
> +
> +	igt_display_t *display = &data->display;
> +	igt_plane_t *primary;
> +	igt_output_t *output;
> +
> +	for_each_pipe_with_valid_output(display, pipe, output) {
> +		// Check VRR capabilities before setting up
> +		if (igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&
> +		    igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
> +			vrr_supported = true;
> +			break;
> +		}
> +	}
> +
> +	// Skip the test if no VRR capable output is found

Please fix the comment style as

/* This is single line comment. */

/*
  * This is multi-line
  * comment.
  */

> +	if (!vrr_supported)
> +		igt_skip("No VRR capable output found, skipping the test.\n");
> +
> +	for_each_pipe_with_valid_output(display, pipe, output) {

This loop is redundant, just capture the output in above loop & use it 
here directly. No need to iterate all outputs again.

> +		igt_display_reset(display);
> +		igt_output_set_pipe(output, PIPE_NONE);

This is redundant, as igt_display_reset() is doing the same, please drop it.

> +
> +		if (!igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) ||
> +		   !igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
> +			continue;
> +		}

This check is no more required.

> +
> +		igt_output_set_pipe(output, pipe);
> +
> +		data->output = output;
> +		data->mode = igt_output_get_mode(output);
> +		setup_videoplayback(data);
> +
> +		primary = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
> +		pre_val = read_pkgc_counter(data->debugfs_root_fd);
> +		delay = 1 * (MSECS / data->mode->vrefresh);

To make sure to achieve the extended vblank, let's try with some value 
which is less than the vrefresh.

Ex: 1 * (MSECS / (vrefresh - 10))

- Bhanu

> +
> +		igt_plane_set_fb(primary, &data->fb_rgb);
> +		igt_display_commit(&data->display);
> +
> +		while (time(NULL) - start < duration) {
> +			flip = !flip;
> +			igt_plane_set_fb(primary, flip ? &data->fb_rgb : &data->fb_rgr);
> +			igt_display_commit(&data->display);
> +
> +			cur_val = read_pkgc_counter(data->debugfs_root_fd);
> +			if (cur_val > pre_val) {
> +				pkgc_flag = true;
> +				break;
> +			}
> +			usleep(delay);
> +		}
> +	}
> +	cleanup_dc3co_fbs(data);
> +	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");
> +}
> +
>   static void test_pkgc_state_dpms(data_t *data)
>   {
>   	unsigned int timeout_sec = 6;
> @@ -687,6 +756,15 @@ igt_main
>   			test_dc_state_psr(&data, CHECK_DC6);
>   	}
>   
> +	igt_describe("This test validates display engine entry to PKGC10 state "
> +		     "during extended vblank");
> +	igt_subtest("deep-pkgc") {
> +		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
> +			      "PC8+ residencies not supported\n");
> +		igt_require(intel_display_ver(data.devid) >= 20);
> +		test_deep_pkgc_state(&data);
> +	}
> +
>   	igt_describe("This test validates display engine entry to DC5 state "
>   		     "while all connectors's DPMS property set to OFF");
>   	igt_subtest("dc5-dpms") {

[PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Jeevan B]

Add a new test to validate deep sleep states during extended vblank
scenarios, where two frames are committed simultaneously for a given
time with reduced refresh rate.
v2: dealy of one frame added to simulate extended vblank.
    remove vrr related debug checks.
v3: fix typo and add define. (Suraj)
v4: fix structure and add skip if no vrr monitor found. (Bhanu)
v5: remove redundant code and correct delay logic. (Bhanu)

Signed-off-by: Jeevan B <jeevan.b@intel.com>
---
 tests/intel/kms_pm_dc.c | 72 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
index 7766d34d7..7ce1b7051 100644
--- a/tests/intel/kms_pm_dc.c
+++ b/tests/intel/kms_pm_dc.c
@@ -76,6 +76,10 @@
  *
  * SUBTEST: dc9-dpms
  * Description: This test validates display engine entry to DC9 state
+ *
+ * SUBTEST: deep-pkgc
+ * Description: This test validates display engine entry to PKGC10 state for extended vblank
+ * Functionality: pm_dc
  */
 
 /* DC State Flags */
@@ -89,6 +93,7 @@
 #define PACKAGE_CSTATE_PATH  "pmc_core/package_cstate_show"
 #define KMS_POLL_DISABLE 0
 #define DC9_RESETS_DC_COUNTERS(devid) (!(IS_DG1(devid) || IS_DG2(devid) || AT_LEAST_DISPLAY(devid, 14)))
+#define MSECS (1000000ul)
 
 IGT_TEST_DESCRIPTION("Tests to validate display power DC states.");
 
@@ -584,6 +589,64 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
 	return get_dc_counter(str);
 }
 
+static void test_deep_pkgc_state(data_t *data)
+{
+	unsigned int pre_val = 0, cur_val = 0;
+	time_t start = time(NULL), duration = 2, delay;
+	enum pipe pipe;
+	bool pkgc_flag = false;
+	bool vrr_supported = false, flip = true;
+
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+	igt_output_t *output = NULL;
+
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		/* Check VRR capabilities before setting up */
+		if (igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&
+		    igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
+			vrr_supported = true;
+			break;
+		}
+	}
+
+	/* Skip the test if no VRR capable output is found */
+	if (!vrr_supported)
+		igt_skip("No VRR capable output found, skipping the test.\n");
+
+	igt_display_reset(display);
+
+	if (output) {
+		igt_output_set_pipe(output, pipe);
+
+		data->output = output;
+		data->mode = igt_output_get_mode(output);
+		setup_videoplayback(data);
+
+		primary = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
+		pre_val = read_pkgc_counter(data->debugfs_root_fd);
+		delay = 1 * (MSECS / (data->mode->vrefresh - 10));
+
+		igt_plane_set_fb(primary, &data->fb_rgb);
+		igt_display_commit(&data->display);
+
+		while (time(NULL) - start < duration) {
+			flip = !flip;
+			igt_plane_set_fb(primary, flip ? &data->fb_rgb : &data->fb_rgr);
+			igt_display_commit(&data->display);
+
+			cur_val = read_pkgc_counter(data->debugfs_root_fd);
+			if (cur_val > pre_val) {
+				pkgc_flag = true;
+				break;
+			}
+			usleep(delay);
+		}
+	}
+	cleanup_dc3co_fbs(data);
+	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");
+}
+
 static void test_pkgc_state_dpms(data_t *data)
 {
 	unsigned int timeout_sec = 6;
@@ -687,6 +750,15 @@ igt_main
 			test_dc_state_psr(&data, CHECK_DC6);
 	}
 
+	igt_describe("This test validates display engine entry to PKGC10 state "
+		     "during extended vblank");
+	igt_subtest("deep-pkgc") {
+		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
+			      "PC8+ residencies not supported\n");
+		igt_require(intel_display_ver(data.devid) >= 20);
+		test_deep_pkgc_state(&data);
+	}
+
 	igt_describe("This test validates display engine entry to DC5 state "
 		     "while all connectors's DPMS property set to OFF");
 	igt_subtest("dc5-dpms") {
-- 
2.25.1

Re: [PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Modem, Bhanuprakash]

Hi Jeevan,

On 28-05-2024 03:39 pm, Jeevan B wrote:
> Add a new test to validate deep sleep states during extended vblank
> scenarios, where two frames are committed simultaneously for a given
> time with reduced refresh rate.
> v2: dealy of one frame added to simulate extended vblank.
>      remove vrr related debug checks.
> v3: fix typo and add define. (Suraj)
> v4: fix structure and add skip if no vrr monitor found. (Bhanu)
> v5: remove redundant code and correct delay logic. (Bhanu)
> 
> Signed-off-by: Jeevan B <jeevan.b@intel.com>
> ---
>   tests/intel/kms_pm_dc.c | 72 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
> 
> diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
> index 7766d34d7..7ce1b7051 100644
> --- a/tests/intel/kms_pm_dc.c
> +++ b/tests/intel/kms_pm_dc.c
> @@ -76,6 +76,10 @@
>    *
>    * SUBTEST: dc9-dpms
>    * Description: This test validates display engine entry to DC9 state
> + *
> + * SUBTEST: deep-pkgc
> + * Description: This test validates display engine entry to PKGC10 state for extended vblank
> + * Functionality: pm_dc
>    */
>   
>   /* DC State Flags */
> @@ -89,6 +93,7 @@
>   #define PACKAGE_CSTATE_PATH  "pmc_core/package_cstate_show"
>   #define KMS_POLL_DISABLE 0
>   #define DC9_RESETS_DC_COUNTERS(devid) (!(IS_DG1(devid) || IS_DG2(devid) || AT_LEAST_DISPLAY(devid, 14)))
> +#define MSECS (1000000ul)
>   
>   IGT_TEST_DESCRIPTION("Tests to validate display power DC states.");
>   
> @@ -584,6 +589,64 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
>   	return get_dc_counter(str);
>   }
>   
> +static void test_deep_pkgc_state(data_t *data)
> +{
> +	unsigned int pre_val = 0, cur_val = 0;
> +	time_t start = time(NULL), duration = 2, delay;
> +	enum pipe pipe;
> +	bool pkgc_flag = false;
> +	bool vrr_supported = false, flip = true;
> +
> +	igt_display_t *display = &data->display;
> +	igt_plane_t *primary;
> +	igt_output_t *output = NULL;
> +
> +	for_each_pipe_with_valid_output(display, pipe, output) {
> +		/* Check VRR capabilities before setting up */
> +		if (igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&
> +		    igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
> +			vrr_supported = true;
> +			break;
> +		}
> +	}
> +
> +	/* Skip the test if no VRR capable output is found */
> +	if (!vrr_supported)
> +		igt_skip("No VRR capable output found, skipping the test.\n");
> +
> +	igt_display_reset(display);
> +
> +	if (output) {

Do we really need this check? if vrr_supported check is pass means there 
is a valid output available.

Apart from that, overall patch looks good to me. You can use my R-b 
after addressing above comment.

Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>

- Bhanu

> +		igt_output_set_pipe(output, pipe);
> +
> +		data->output = output;
> +		data->mode = igt_output_get_mode(output);
> +		setup_videoplayback(data);
> +
> +		primary = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
> +		pre_val = read_pkgc_counter(data->debugfs_root_fd);
> +		delay = 1 * (MSECS / (data->mode->vrefresh - 10));
> +
> +		igt_plane_set_fb(primary, &data->fb_rgb);
> +		igt_display_commit(&data->display);
> +
> +		while (time(NULL) - start < duration) {
> +			flip = !flip;
> +			igt_plane_set_fb(primary, flip ? &data->fb_rgb : &data->fb_rgr);
> +			igt_display_commit(&data->display);
> +
> +			cur_val = read_pkgc_counter(data->debugfs_root_fd);
> +			if (cur_val > pre_val) {
> +				pkgc_flag = true;
> +				break;
> +			}
> +			usleep(delay);
> +		}
> +	}
> +	cleanup_dc3co_fbs(data);
> +	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");
> +}
> +
>   static void test_pkgc_state_dpms(data_t *data)
>   {
>   	unsigned int timeout_sec = 6;
> @@ -687,6 +750,15 @@ igt_main
>   			test_dc_state_psr(&data, CHECK_DC6);
>   	}
>   
> +	igt_describe("This test validates display engine entry to PKGC10 state "
> +		     "during extended vblank");
> +	igt_subtest("deep-pkgc") {
> +		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
> +			      "PC8+ residencies not supported\n");
> +		igt_require(intel_display_ver(data.devid) >= 20);
> +		test_deep_pkgc_state(&data);
> +	}
> +
>   	igt_describe("This test validates display engine entry to DC5 state "
>   		     "while all connectors's DPMS property set to OFF");
>   	igt_subtest("dc5-dpms") {

[PATCH i-g-t] tests/intel/kms_pm_dc: Add a new test to validate the deep sleep state during extended vblank

[Jeevan B]

Add a new test to validate deep sleep states during extended vblank
scenarios, where two frames are committed simultaneously for a given
time with reduced refresh rate.
v2: dealy of one frame added to simulate extended vblank.
    remove vrr related debug checks.
v3: fix typo and add define. (Suraj)
v4: fix structure and add skip if no vrr monitor found. (Bhanu)
v5: remove redundant code and correct delay logic. (Bhanu)

Signed-off-by: Jeevan B <jeevan.b@intel.com>
Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
---
 tests/intel/kms_pm_dc.c | 71 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c
index 7766d34d7..e1318bfa6 100644
--- a/tests/intel/kms_pm_dc.c
+++ b/tests/intel/kms_pm_dc.c
@@ -76,6 +76,10 @@
  *
  * SUBTEST: dc9-dpms
  * Description: This test validates display engine entry to DC9 state
+ *
+ * SUBTEST: deep-pkgc
+ * Description: This test validates display engine entry to PKGC10 state for extended vblank
+ * Functionality: pm_dc
  */
 
 /* DC State Flags */
@@ -89,6 +93,7 @@
 #define PACKAGE_CSTATE_PATH  "pmc_core/package_cstate_show"
 #define KMS_POLL_DISABLE 0
 #define DC9_RESETS_DC_COUNTERS(devid) (!(IS_DG1(devid) || IS_DG2(devid) || AT_LEAST_DISPLAY(devid, 14)))
+#define MSECS (1000000ul)
 
 IGT_TEST_DESCRIPTION("Tests to validate display power DC states.");
 
@@ -584,6 +589,63 @@ static unsigned int read_pkgc_counter(int debugfs_root_fd)
 	return get_dc_counter(str);
 }
 
+static void test_deep_pkgc_state(data_t *data)
+{
+	unsigned int pre_val = 0, cur_val = 0;
+	time_t start = time(NULL), duration = 2, delay;
+	enum pipe pipe;
+	bool pkgc_flag = false;
+	bool vrr_supported = false, flip = true;
+
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+	igt_output_t *output = NULL;
+
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		/* Check VRR capabilities before setting up */
+		if (igt_output_has_prop(output, IGT_CONNECTOR_VRR_CAPABLE) &&
+		    igt_output_get_prop(output, IGT_CONNECTOR_VRR_CAPABLE)) {
+			vrr_supported = true;
+			break;
+		}
+	}
+
+	/* Skip the test if no VRR capable output is found */
+	if (!vrr_supported)
+		igt_skip("No VRR capable output found, skipping the test.\n");
+
+	igt_display_reset(display);
+
+	igt_output_set_pipe(output, pipe);
+
+	data->output = output;
+	data->mode = igt_output_get_mode(output);
+	setup_videoplayback(data);
+
+	primary = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
+	pre_val = read_pkgc_counter(data->debugfs_root_fd);
+	delay = 1 * (MSECS / (data->mode->vrefresh - 10));
+
+	igt_plane_set_fb(primary, &data->fb_rgb);
+	igt_display_commit(&data->display);
+
+	while (time(NULL) - start < duration) {
+		flip = !flip;
+		igt_plane_set_fb(primary, flip ? &data->fb_rgb : &data->fb_rgr);
+		igt_display_commit(&data->display);
+
+		cur_val = read_pkgc_counter(data->debugfs_root_fd);
+		if (cur_val > pre_val) {
+			pkgc_flag = true;
+			break;
+		}
+		usleep(delay);
+	}
+
+	cleanup_dc3co_fbs(data);
+	igt_assert_f(pkgc_flag, "PKGC10 is not achieved.\n");
+}
+
 static void test_pkgc_state_dpms(data_t *data)
 {
 	unsigned int timeout_sec = 6;
@@ -687,6 +749,15 @@ igt_main
 			test_dc_state_psr(&data, CHECK_DC6);
 	}
 
+	igt_describe("This test validates display engine entry to PKGC10 state "
+		     "during extended vblank");
+	igt_subtest("deep-pkgc") {
+		igt_require_f(igt_pm_pc8_plus_residencies_enabled(data.msr_fd),
+			      "PC8+ residencies not supported\n");
+		igt_require(intel_display_ver(data.devid) >= 20);
+		test_deep_pkgc_state(&data);
+	}
+
 	igt_describe("This test validates display engine entry to DC5 state "
 		     "while all connectors's DPMS property set to OFF");
 	igt_subtest("dc5-dpms") {
-- 
2.25.1