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

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

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

On Wed, 2019-04-10 at 13:42 -0700, Pandiyan, Dhinakaran wrote:
> > -----Original Message-----
> > From: Souza, Jose
> > Sent: Wednesday, April 10, 2019 1:32 PM
> > To: igt-dev@lists.freedesktop.org; Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com>
> > Cc: Vivi, Rodrigo <rodrigo.vivi@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@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@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()?

Origin is = 4(ORIGIN_DIRTYFB) and it is only intel_psr_flush() calls so
we just poke cursor registers to force a PSR exit.

> 
> -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));
> > > >  	}
> > > >  }
> > > > 

[-- 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