Re: [igt-dev] [PATCH] [intel-gfx] [PATCH i-g-t] tests/pm_backlight.c : Brightness test with DPMS and System suspend.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Aug 23, 2018 at 08:16:28AM -0400, Jyoti Yadav wrote:
> From: Jyoti <jyoti.r.yadav@intel.com>
> 
> BIOS programs few of PWM related registers during initial boot.
> But during System suspend those registers are cleared.
> This test aim to check whether display programs those registers properly after
> system resume.
> Also checks brightness programming during DPMS ON/OFF cycle to check backlight
> programming is done properly from display side.
> 
> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
> ---
>  tests/pm_backlight.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c
> index a695f90..6f6d727 100644
> --- a/tests/pm_backlight.c
> +++ b/tests/pm_backlight.c
> @@ -47,6 +47,7 @@ struct context {
>  #define FADESPEED 100 /* milliseconds between steps */
>  
>  IGT_TEST_DESCRIPTION("Basic backlight sysfs test");
> +static int8_t *pm_data = NULL;
>  
>  static int backlight_read(int *result, const char *fname)
>  {
> @@ -150,19 +151,60 @@ static void test_fade(struct context *context)
>  		nanosleep(&ts, NULL);
>  	}
>  }
> +static void test_fade_with_dpms(struct context *context, igt_output_t *output)

nip: what about test_fade_after_dpms_on_off() ?

but with is not wrong or bad, just my poor brain first thought you
would try fade test between off and on, what doesn't make any sense I know...

> +{
> +	int i;
> +	static const struct timespec ts = { .tv_sec = 0, .tv_nsec = FADESPEED*1000000 };
> +	bool has_runtime_pm;
> +	has_runtime_pm = igt_setup_runtime_pm();
> +	igt_info("Runtime PM support: %d\n", has_runtime_pm);
> +	igt_assert(has_runtime_pm);
> +	kmstest_set_connector_dpms(output->display->drm_fd, output->config.connector, DRM_MODE_DPMS_OFF);
> +	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
> +	kmstest_set_connector_dpms(output->display->drm_fd, output->config.connector, DRM_MODE_DPMS_ON);
> +	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIVE));
> +
> +	/* Fade out, then in */
> +	for (i = context->max; i > 0; i -= context->max / FADESTEPS) {
> +		test_and_verify(context, i);
> +		nanosleep(&ts, NULL);
> +	}
> +	for (i = 0; i <= context->max; i += context->max / FADESTEPS) {
> +		test_and_verify(context, i);
> +		nanosleep(&ts, NULL);
> +	}

I think this function should only include the dpms part and then we call

test_fade(context) directly instead of duplicating the code...

> +}
> +static void test_fade_with_suspend(struct context *context, igt_output_t *output)
> +{
> +	int i;
> +	static const struct timespec ts = { .tv_sec = 0, .tv_nsec = FADESPEED*1000000 };
> +	kmstest_set_connector_dpms(output->display->drm_fd, output->config.connector, DRM_MODE_DPMS_OFF);
> +	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
> +	igt_system_suspend_autoresume(SUSPEND_STATE_MEM, SUSPEND_TEST_NONE);
> +
> +	/* Fade out, then in */
> +	for (i = context->max; i > 0; i -= context->max / FADESTEPS) {
> +		test_and_verify(context, i);
> +		nanosleep(&ts, NULL);
> +	}
> +	for (i = 0; i <= context->max; i += context->max / FADESTEPS) {
> +		test_and_verify(context, i);
> +		nanosleep(&ts, NULL);
> +	}

same here...

besides the code duplication everything else lgtm

Thanks,
Rodrigo.

> +}
>  
>  igt_main
>  {
>  	struct context context = {0};
>  	int old;
>  	igt_display_t display;
> +	igt_output_t *output;
>  	struct igt_fb fb;
>  
>  	igt_skip_on_simulation();
>  
>  	igt_fixture {
>  		enum pipe pipe;
> -		igt_output_t *output;
>  		bool found = false;
>  		char full_name[32] = {};
>  		char *name;
> @@ -187,7 +229,6 @@ igt_main
>  		for_each_pipe_with_valid_output(&display, pipe, output) {
>  			if (strcmp(name + 6, output->name))
>  				continue;
> -
>  			found = true;
>  			break;
>  		}
> @@ -205,6 +246,7 @@ igt_main
>  		igt_plane_set_fb(primary, &fb);
>  
>  		igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +		pm_data = igt_pm_enable_sata_link_power_management();
>  	}
>  
>  	igt_subtest("basic-brightness")
> @@ -213,6 +255,10 @@ igt_main
>  		test_bad_brightness(&context);
>  	igt_subtest("fade")
>  		test_fade(&context);
> +	igt_subtest("fade_with_dpms")
> +		test_fade_with_dpms(&context, output);
> +	igt_subtest("fade_with_suspend")
> +		test_fade_with_suspend(&context, output);
>  
>  	igt_fixture {
>  		/* Restore old brightness */
> @@ -220,6 +266,8 @@ igt_main
>  
>  		igt_display_fini(&display);
>  		igt_remove_fb(display.drm_fd, &fb);
> +		igt_pm_restore_sata_link_power_management(pm_data);
> +		free(pm_data);
>  		close(display.drm_fd);
>  	}
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> 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