[igt-dev] [PATCH i-g-t v2 3/8] tests/fbcon_fbt: Add wait_until_update() callback to features

[Pandiyan, Dhinakaran]

> -----Original Message-----
> From: Souza, Jose
> Sent: Wednesday, April 10, 2019 1:32 PM
> To: igt-dev at lists.freedesktop.org; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan at intel.com>
> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [igt-dev] [PATCH i-g-t v2 3/8] tests/fbcon_fbt: Add
> wait_until_update() callback to features
> 
> On Tue, 2019-04-09 at 14:41 -0700, Dhinakaran Pandiyan wrote:
> > On Wed, 2019-03-27 at 13:03 -0700, José Roberto de Souza wrote:
> > > Lets be more explicit and add and implement a callback to check if
> > > feature had a state update, that is what some points of the test
> > > want to test.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > ---
> > >  tests/kms_fbcon_fbt.c | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c index
> > > a9d91839..9ab1d630 100644
> > > --- a/tests/kms_fbcon_fbt.c
> > > +++ b/tests/kms_fbcon_fbt.c
> > > @@ -130,6 +130,11 @@ static bool fbc_wait_until_enabled(int
> > > debugfs_fd)
> > >  	return r;
> > >  }
> > >
> > > +static bool fbc_wait_until_update(int debugfs) {
> > > +	return !fbc_wait_until_enabled(debugfs); }
> > > +
> > >  typedef bool (*connector_possible_fn)(drmModeConnectorPtr
> > > connector);
> > >
> > >  static void set_mode_for_one_screen(struct drm_info *drm, struct
> > > igt_fb *fb, @@ -196,6 +201,11 @@ static bool
> > > psr_supported_on_chipset(int
> > > debugfs_fd)
> > >  	return psr_sink_support(debugfs_fd, PSR_MODE_1);  }
> > >
> > > +static bool psr_wait_until_update(int debugfs_fd) {
> > > +	return !psr_wait_until_enabled(debugfs_fd);
> > > +}
> > > +
> > >  static void disable_features(int debugfs_fd)  {
> > >  	igt_set_module_param_int("enable_fbc", 0); @@ -215,16 +225,19
> @@
> > > static inline void psr_debugfs_enable(int
> > > debugfs_fd)
> > >  struct feature {
> > >  	bool (*supported_on_chipset)(int debugfs_fd);
> > >  	bool (*wait_until_enabled)(int debugfs_fd);
> > > +	bool (*wait_until_update)(int debugfs_fd);
> > >  	bool (*connector_possible_fn)(drmModeConnectorPtr connector);
> > >  	void (*enable)(int debugfs_fd);
> > >  } fbc = {
> > >  	.supported_on_chipset = fbc_supported_on_chipset,
> > >  	.wait_until_enabled = fbc_wait_until_enabled,
> > > +	.wait_until_update = fbc_wait_until_update,
> > >  	.connector_possible_fn = connector_can_fbc,
> > >  	.enable = fbc_modparam_enable,
> > >  }, psr = {
> > >  	.supported_on_chipset = psr_supported_on_chipset,
> > >  	.wait_until_enabled = psr_wait_until_enabled,
> > > +	.wait_until_update = psr_wait_until_update,
> > >  	.connector_possible_fn = connector_can_psr,
> > >  	.enable = psr_debugfs_enable,
> > >  };
> > > @@ -243,7 +256,7 @@ static void subtest(struct feature *feature,
> > > bool suspend)
> > >
> > >  	kmstest_unset_all_crtcs(drm.fd, drm.res);
> > >  	wait_user("Modes unset.");
> > > -	igt_assert(!feature->wait_until_enabled(drm.debugfs_fd));
> > > +	igt_assert(feature->wait_until_update(drm.debugfs_fd));
> > I believe the intention here is to test if PSR and FBC got disabled
> > when pipes are disabled.
> 
> Yeah, here pipes will be disable so all the features, so here should we
> keep: igt_assert(!feature->wait_until_enabled(drm.debugfs_fd));
Yup.

> 
> >
> > >
> > >  	set_mode_for_one_screen(&drm, &fb, feature-
> > > >connector_possible_fn);
> > >  	wait_user("Screen set.");
> > > @@ -263,13 +276,13 @@ static void subtest(struct feature *feature,
> > > bool
> > > suspend)
> > >  	sleep(3);
> > >
> > >  	wait_user("Back to fbcon.");
> > > -	igt_assert(!feature->wait_until_enabled(drm.debugfs_fd));
> > > +	igt_assert(feature->wait_until_update(drm.debugfs_fd));
> > >
> > And here, I believe the expectation is that fbcon leads to frontbuffer
> > invalidate and thus disabling PSR and FBC.
> 
> It will disable PSR while doing the full modeset but in fbcon PSR will be
> enabled between cursor blinks, that is why we need to check for update
> here.
> 
I understand, I have seen that myself. Not sure at what point it changed from
fbcon completely disabling PSR and FBC with frontbuffer_invalidate() to the current
behavior. iirc we just get frontbuffer_flush() now. TBH, I don't understand fbcon that
well.

What's the origin value in frontbuffer_invalidate/flush()? Do we get flips for 
cursor blinks? Or is it a fronbuffer_invalidate() followed by a frontbuffer_flush()?

-DK

> >
> > >  	if (suspend) {
> > >  		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
> > >  					      SUSPEND_TEST_NONE);
> > >  		sleep(5);
> > > -		igt_assert(!feature-
> > > >wait_until_enabled(drm.debugfs_fd));
> > > +		igt_assert(feature->wait_until_update(drm.debugfs_fd));
> > >  	}
> > >  }
> > >