Re: [igt-dev] [PATCH i-g-t v3 3/6] tests/fbcon_fbt: Add and user psr_long_wait_update()

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

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

On Thu, 2019-04-11 at 21:25 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2019-04-10 at 15:07 -0700, José Roberto de Souza wrote:
> > When fbcon is enabled, PSR will be active between cursor blinks so
> > what it should really use to test PSR is psr_wait_entry(), so a new
> > feature callback was added.
> We could add a PSR only blinking subtest that verifies both entry and
> exit
> happen as the next step.
> 
> > But the fbcon cursor blinks at 5hz what give us 200ms between each
> > screen update what make psr_wait_update() prone to fail the test
> > because it timed out before a blink could happen, so here adding
> > and
> > using psr_long_wait_update() that have a longer timeout.
> 
> Patch 4 that sets up the test conditions should come before this
> patch that
> validates the kernel's response.

Okay reordering.

> 
> Also, is there a way to read the blink rate?

There is no way, I found a old patch adding it but it was not merged.

> 
> > v3:
> > - 3 previous patches squashed in this one (Maarten)
> > - Back to !feature->wait_until_enabled() to test feature state when
> > all CRTCS are disabled(Dhinakaran)
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c         |  5 +++++
> >  lib/igt_psr.h         |  1 +
> >  tests/kms_fbcon_fbt.c | 17 +++++++++++++++--
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index b5847bfd..b92ea73f 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -54,6 +54,11 @@ bool psr_wait_update(int debugfs_fd, enum
> > psr_mode mode)
> >  	return igt_wait(!psr_active_check(debugfs_fd, mode), 40, 10);
> >  }
> >  
> > +bool psr_long_wait_update(int debugfs_fd, enum psr_mode mode)
> > +{
> > +	return igt_wait(!psr_active_check(debugfs_fd, mode), 500, 10);
> nit: We could reduce the debugfs read frequency as the timeout is
> also larger,
> haven't really given enough though into what's a sensible upper
> limit. But,
> something like 50 should be okay.
> 
> > +}
> > +
> >  static ssize_t psr_write(int debugfs_fd, const char *buf)
> >  {
> >  	return igt_sysfs_write(debugfs_fd, "i915_edp_psr_debug", buf,
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > index 49599cf8..ca385736 100644
> > --- a/lib/igt_psr.h
> > +++ b/lib/igt_psr.h
> > @@ -37,6 +37,7 @@ enum psr_mode {
> >  
> >  bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
> >  bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
> > +bool psr_long_wait_update(int debugfs_fd, enum psr_mode mode);
> >  bool psr_enable(int debugfs_fd, enum psr_mode);
> >  bool psr_disable(int debugfs_fd);
> >  bool psr_sink_support(int debugfs_fd, enum psr_mode);
> > diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> > index a9d91839..a5340d85 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)
> > +{
> This warrants an explanation as to why fbc_wait_until_update ==
> !fbc_wait_until_enabled.
> 

This was just moved from what it was doing before.

> My understanding is that we do not expect fbc to become active at all
> with
> fbcon. Can you confirm that?

It is not expected because fbcon do not uses a tiled framebuffer so a
fence can not be setup on the framebuffer and FBC code requires a fence
to accurate track frontbuffer modifications(what maybe is not necessary
anymore as we now have intel_fbc_invalidate()/flush()).

If one day fbcon starts to use a tiled framebuffer we would need to
check the 'Compressing' status as in each blink it would disable the
compressing but FBC would still be enabled.

Adding it.

> 
> With an explanation included and patches re-ordered, 
> 
> Reviewed-by: Dhinkaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> > +	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_long_wait_update(debugfs_fd, PSR_MODE_1);
> > +}
> > +
> >  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,
> >  };
> > @@ -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));
> >  
> >  	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