[igt-dev] [PATCH i-g-t] tests/i915/i915_pm_dc : Check DC5 state with externel active panel

[Imre Deak]

On Thu, Jun 30, 2022 at 11:27:34AM +0530, Mohammed Thasleem wrote:
> This test validates negative scenario of DC5 state by keeping
> all connectors's DPMS property set to ON.
> 
> v2: Added function to check externel panel and skip test if not found.
> v3: Moved check_external_panel inside igt_require_f and removed
>     igt_skip_on_f.
> 
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem at intel.com>
> ---
>  tests/i915/i915_pm_dc.c | 45 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
> index ba49d014..e29f9eb8 100644
> --- a/tests/i915/i915_pm_dc.c
> +++ b/tests/i915/i915_pm_dc.c
> @@ -246,6 +246,13 @@ static void check_dc_counter(data_t *data, int dc_flag, uint32_t prev_dc_count)
>  		     data->debugfs_dump = igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
>  }
>  
> +static void check_dc_counter_negative(data_t *data, int dc_flag, uint32_t prev_dc_count)
> +{
> +	igt_assert_f(!dc_state_wait_entry(data->debugfs_fd, dc_flag, prev_dc_count),
> +			"%s state is achieved\n%s:\n%s\n", dc_state_name(dc_flag), PWR_DOMAIN_INFO,
> +			data->debugfs_dump = igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
> +}
> +
>  static void setup_videoplayback(data_t *data)
>  {
>  	color_t red_green_blue[] = {
> @@ -411,6 +418,18 @@ static void test_dc_state_dpms(data_t *data, int dc_flag)
>  	cleanup_dc_dpms(data);
>  }
>  
> +static void test_dc_state_dpms_negative(data_t *data, int dc_flag)
> +{
> +	uint32_t dc_counter;
> +
> +	require_dc_counter(data->debugfs_fd, dc_flag);
> +	setup_dc_dpms(data);
> +	dc_counter = read_dc_counter(data->debugfs_fd, dc_flag);
> +	dpms_on(data);
> +	check_dc_counter_negative(data, dc_flag, dc_counter);
> +	cleanup_dc_dpms(data);
> +}
> +
>  static bool support_dc6(int debugfs_fd)
>  {
>  	char buf[4096];
> @@ -485,6 +504,23 @@ static void test_dc9_dpms(data_t *data)
>  	setup_dc9_dpms(data, dc_target);
>  }
>  
> +static int check_external_panel(igt_display_t *display)

The function will also count outputs like DSI, LVDS, so would be better
to call it something like has_panels_without_dc_support().

> +{
> +	igt_output_t *output = NULL;

No need to init the above.

> +	int external_panel = 0;
> +
> +	for_each_connected_output(display, output) {
> +		drmModeConnectorPtr c = output->config.connector;
> +
> +		if (c->connector_type != DRM_MODE_CONNECTOR_eDP &&
> +		    c->connection == DRM_MODE_CONNECTED) {

The above will iterate through connectors that are in the connected
state, so no need to check that here. This single line branch doesn't
need to be enclosed in {}.

With the above nits fixed, looks ok to me:
Reviewed-by: Imre Deak <imre.deak at intel.com>

> +			external_panel++;
> +		}
> +	}
> +
> +	return external_panel;
> +}
> +
>  static void kms_poll_state_restore(int sig)
>  {
>  	int sysfs_fd;
> @@ -552,6 +588,15 @@ igt_main
>  		test_dc_state_dpms(&data, CHECK_DC5);
>  	}
>  
> +	igt_describe("This test validates negative scenario of DC5 display "
> +		     "engine entry to DC5 state while all connectors's DPMS "
> +		     "property set to ON");
> +	igt_subtest("dc5-dpms-negative") {
> +		igt_require_f(check_external_panel(&data.display),
> +			      "External panel not detected, skip execution\n");
> +		test_dc_state_dpms_negative(&data, CHECK_DC5);
> +	}
> +
>  	igt_describe("This test validates display engine entry to DC5 state "
>  		     "while all connectors's DPMS property set to OFF");
>  	igt_subtest("dc6-dpms") {
> -- 
> 2.25.1
>