Re: [igt-dev] [PATCH i-g-t v3 4/6] tests/fbcon_fbt: Enable cursor blinking

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Wed, 2019-04-10 at 15:07 -0700, José Roberto de Souza wrote:
> If cursor blinking is disabled no screen updates will happen and
> fbcon_fbt subtests will fail, so lets enable cursor blink while
> running this test and restore to the previous value when exiting it.

I'd also prefer the test doing a cursor update instead, but don't want to block
this change as it is an improvement.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_sysfs.c       | 46 +++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_sysfs.h       |  1 +
>  tests/kms_fbcon_fbt.c |  1 +
>  3 files changed, 48 insertions(+)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index f806f4fc..904fbd17 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -610,3 +610,49 @@ void kick_snd_hda_intel(void)
>  out:
>  	close(fd);
>  }
> +
> +static int fbcon_blink_restore_debugfs_fd = -1;
fbcon_cursor_blink_fd? A bit shorter and matches the file name.

> +static uint8_t fbcon_blink_restore_value;
> +
> +static void fbcon_blink_restore(int sig)
> +{
> +	char buffer[4];
> +	int r;
> +
> +	r = snprintf(buffer, sizeof(buffer), "%u", fbcon_blink_restore_value);
> +	write(fbcon_blink_restore_debugfs_fd, buffer, r + 1);
Add an assert here too.

Missing
	close(fbcon_blink_restore_debugfs_fd);

> +}
> +
> +/**
> + * fbcon_blink_enable:
> + * @enable: if true enables the fbcon cursor blinking otherwise disables it
> + *
> + * Enables or disables the cursor blinking in fbcon, it also restores the
> + * previous blinking state when exiting test.
> + *
> + */
> +void fbcon_blink_enable(bool enable)
Having the enable parameter is neat, we can add subtest disabling cursor and
check if PSR remains active.

What'd be really cool though is to verify the blink rate against PSR exit rate.

> +{
> +	const char *cur_blink_path = "/sys/class/graphics/fbcon/cursor_blink";
> +	char buffer[4];
> +	int fd, r;
> +
> +	fd = open(cur_blink_path, O_RDWR);
> +	igt_assert(fd >= 0);
igt_require()
This is a test requirement than a failure of the test itself.


> +
> +	/* Restore original value on exit */
> +	if (fbcon_blink_restore_debugfs_fd == -1) {
> +		r = read(fd, buffer, sizeof(buffer));
> +		if (r > 0) {
> +			fbcon_blink_restore_value = (uint8_t)strtol(buffer,
> +								    NULL, 10);
What is the point of this conversion if we are going to blindly write it back?
Make buffer static instead.

> +			fbcon_blink_restore_debugfs_fd = dup(fd);
> +			igt_assert(fbcon_blink_restore_debugfs_fd >= 0);
> +			igt_install_exit_handler(fbcon_blink_restore);
> +		}
> +	}
> +
> +	r = snprintf(buffer, sizeof(buffer), enable ? "1" : "0");
> +	write(fd, buffer, r + 1);
Assert the return here?

> +	close(fd);
> +}
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index c12e36d1..b646df30 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -57,5 +57,6 @@ bool igt_sysfs_set_boolean(int dir, const char *attr, bool
> value);
>  
>  void bind_fbcon(bool enable);
>  void kick_snd_hda_intel(void);
> +void fbcon_blink_enable(bool enable);
>  
>  #endif /* __IGT_SYSFS_H__ */
> diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> index a5340d85..5e510db0 100644
> --- a/tests/kms_fbcon_fbt.c
> +++ b/tests/kms_fbcon_fbt.c
> @@ -299,6 +299,7 @@ static void setup_environment(void)
>  	 * is necessary enable it again
>  	 */
>  	bind_fbcon(true);
> +	fbcon_blink_enable(true);
>  }
>  
>  static void teardown_environment(void)

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev