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

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4337 bytes --]

On Thu, 2019-04-11 at 22:01 -0700, Dhinakaran Pandiyan wrote:
> 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.

Okay

> 
> > +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.

Assert during test exit?

> 
> Missing
> 	close(fbcon_blink_restore_debugfs_fd);

Done

> 
> > +}
> > +
> > +/**
> > + * 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.

Done

> 
> 
> > +
> > +	/* 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,
> > +								    NUL
> > L, 10);
> What is the point of this conversion if we are going to blindly write
> it back?
> Make buffer static instead.

saving one byte?!
but okay changing fbcon_blink_restore_value to char[2]

> 
> > +			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?

Done

> 
> > +	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)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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