[igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

Souza, Jose jose.souza at intel.com
Thu Jan 17 23:41:57 UTC 2019


On Thu, 2019-01-17 at 15:09 -0800, Pandiyan, Dhinakaran wrote:
> On Thu, 2019-01-17 at 15:05 -0800, Souza, Jose wrote:
> > On Thu, 2019-01-17 at 13:51 -0800, Dhinakaran Pandiyan wrote:
> > > On Thu, 2019-01-17 at 13:43 -0800, Souza, Jose wrote:
> > > > On Tue, 2019-01-15 at 22:44 -0800, Dhinakaran Pandiyan wrote:
> > > > > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza
> > > > > wrote:
> > > > > > The main tests for PSR1 check if hardware tracking is
> > > > > > detecting
> > > > > > changes in planes when modifing it in different ways and
> > > > > > now
> > > > > > those tests will also run for PSR2 if supported by source
> > > > > > and
> > > > > > sink.
> > > > > > 
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > > > ---
> > > > > >  tests/kms_psr.c | 119
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > ----
> > > > > >  1 file changed, 110 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > > > > index 8f2ef89c..b2202cd3 100644
> > > > > > --- a/tests/kms_psr.c
> > > > > > +++ b/tests/kms_psr.c
> > > > > > @@ -61,6 +61,7 @@ typedef struct {
> > > > > >  	int debugfs_fd;
> > > > > >  	enum operations op;
> > > > > >  	int test_plane_id;
> > > > > > +	enum psr_mode op_psr_mode;
> > > > > >  	uint32_t devid;
> > > > > >  	uint32_t crtc_id;
> > > > > >  	igt_display_t display;
> > > > > > @@ -72,6 +73,7 @@ typedef struct {
> > > > > >  	drmModeModeInfo *mode;
> > > > > >  	igt_output_t *output;
> > > > > >  	bool with_psr_disabled;
> > > > > > +	bool supports_psr2;
> > > > > >  } data_t;
> > > > > >  
> > > > > >  static void create_cursor_fb(data_t *data)
> > > > > > @@ -190,10 +192,10 @@ static void fill_render(data_t *data,
> > > > > > uint32_t
> > > > > > handle, unsigned char color)
> > > > > >  	gem_bo_busy(data->drm_fd, handle);
> > > > > >  }
> > > > > >  
> > > > > > -static bool sink_support(data_t *data)
> > > > > > +static bool sink_support(data_t *data, enum psr_mode mode)
> > > > > >  {
> > > > > >  	return data->with_psr_disabled ||
> > > > > > -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> > > > > > +	       psr_sink_support(data->debugfs_fd, mode);
> > > > > >  }
> > > > > >  
> > > > > >  static bool psr_wait_entry_if_enabled(data_t *data)
> > > > > > @@ -201,7 +203,23 @@ static bool
> > > > > > psr_wait_entry_if_enabled(data_t
> > > > > > *data)
> > > > > >  	if (data->with_psr_disabled)
> > > > > >  		return true;
> > > > > >  
> > > > > > -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> > > > > > +	return psr_wait_entry(data->debugfs_fd, data-
> > > > > > > op_psr_mode);
> > > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +static bool psr_wait_update_if_enabled(data_t *data)
> > > > > > +{
> > > > > > +	if (data->with_psr_disabled)
> > > > > > +		return true;
> > > > > > +
> > > > > > +	return psr_wait_update(data->debugfs_fd, data-
> > > > > > > op_psr_mode);
> > > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +static bool psr_enable_if_enabled(data_t *data)
> > > > > 
> > > > > This function name is quite confusing. What do you think of
> > > > > using
> > > > > macros like this
> > > > > 
> > > > > #define PSR_WAIT_ENTRY(data, mode)
> > > > > 	return data->with_psr_disabled || psr_wait_entry(data-
> > > > > > debugfs_fd, mode);
> > > > > 
> > > > > ...
> > > > 
> > > > I'm happy with both approaches but I guess we can change that
> > > > in
> > > > a
> > > > patch on top of this one.
> > > > 
> > > > > > +{
> > > > > > +	if (data->with_psr_disabled)
> > > > > > +		return true;
> > > > > > +
> > > > > > +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
> > > > > >  }
> > > > > >  
> > > > > >  static inline void manual(const char *expected)
> > > > > > @@ -291,7 +309,7 @@ static void run_test(data_t *data)
> > > > > >  		expected = "screen GREEN";
> > > > > >  		break;
> > > > > >  	}
> > > > > > -	igt_assert(psr_wait_update(data->debugfs_fd,
> > > > > > PSR_MODE_1));
> > > > > > +	igt_assert(psr_wait_update_if_enabled(data));
> > > > > >  	manual(expected);
> > > > > >  }
> > > > > >  
> > > > > > @@ -369,6 +387,7 @@ static void setup_test_plane(data_t
> > > > > > *data,
> > > > > > int
> > > > > > test_plane)
> > > > > >  
> > > > > >  static void test_setup(data_t *data)
> > > > > >  {
> > > > > > +	psr_enable_if_enabled(data);
> > > > > >  	setup_test_plane(data, data->test_plane_id);
> > > > > >  	igt_assert(psr_wait_entry_if_enabled(data));
> > > > > >  }
> > > > > > @@ -399,7 +418,7 @@ static int opt_handler(int opt, int
> > > > > > opt_index,
> > > > > > void *_data)
> > > > > >  int main(int argc, char *argv[])
> > > > > >  {
> > > > > >  	const char *help_str =
> > > > > > -	       "  --no-psr\tRun test without PSR.";
> > > > > > +	       "  --no-psr\tRun test without PSR/PSR2.";
> > > > > >  	static struct option long_options[] = {
> > > > > >  		{"no-psr", 0, 0, 'n'},
> > > > > >  		{ 0, 0, 0, 0 }
> > > > > > @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
> > > > > >  		kmstest_set_vt_graphics_mode();
> > > > > >  		data.devid = intel_get_drm_devid(data.drm_fd);
> > > > > >  
> > > > > > -		if (!data.with_psr_disabled)
> > > > > > -			psr_enable(data.debugfs_fd,
> > > > > > PSR_MODE_1);
> > > > > > -
> > > > > > -		igt_require_f(sink_support(&data),
> > > > > > +		igt_require_f(sink_support(&data, PSR_MODE_1),
> > > > > >  			      "Sink does not support PSR\n");
> > > > > >  
> > > > > > +		if (sink_support(&data, PSR_MODE_2)) {
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > > +			psr_enable_if_enabled(&data);
> > > > > 
> > > > > There is another enable in test_setup() that the PSR2
> > > > > subtests
> > > > > call.
> > > > > Why do we need two?
> > > > 
> > > > Here I'm enabling by hand to check if source and sink support
> > > > PSR2
> > > > so
> > > > data.supports_psr2 is set with the right value and we skip the
> > > > PSR2
> > > > tests when not supported.
> > > 
> > > How do you distinguish that from a true failure where PSR2 should
> > > have
> > > got enabled but it doesn't?
> > 
> > We can't distinguish but after the first CI reports if a machine
> > that
> > had PSR2 tests running suddenly start to skip PSR2 tests it will be
> > reported by CI.
> > 
> Remove this code and let the test fail instead, easier to notice.

Okay, I will change to:

if (intel_gen(intel_get_drm_devid(data.drm_fd)) >= 9)
	data.supports_psr2 = sink_support(&data, PSR_MODE_2);

Or would be better add this gen check inside of sink_support()?
This is for a case where a PSR2 panels is placed in a machine with with
a gen that do not support PSR2.

> 
> > > > > > +			data.supports_psr2 =
> > > > > > psr_wait_entry_if_enabled(&data);
> > > > > > +		}
> > > > > > +
> > > > > >  		data.bufmgr =
> > > > > > drm_intel_bufmgr_gem_init(data.drm_fd,
> > > > > > 4096);
> > > > > >  		igt_assert(data.bufmgr);
> > > > > >  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > > > > > @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest("basic") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > > +		test_setup(&data);
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > 
> > > > > Run this in a loop (PSR_MODE_1...PSR_MODE_2) instead of
> > > > > repeating
> > > > > the
> > > > > same calls? 
> > > > 
> > > > I think have separated tests is better because CI reports will
> > > > show
> > > > what version of PSR has a regression, we can run specific tests
> > > > while
> > > > debuging...
> > > 
> > > Calling igt_subtest() in a loop should generate separate subtests
> > > iiuc.
> > 
> > Ohh now I got what you want to do, looks good I will change to
> > this.
> > 
> > But we will need to generate PSR2 tests for every machine, as the
> > igt_fixture() do not run when getting the list of subtests and CI
> > probably uses that to split the tests through the shards.
> > 
> > 
> > > > > > +
> > > > > > +	igt_subtest("psr2_basic") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > 
> > > > > We don't have to generate this subtest if the sink does not
> > > > > support
> > > > > PSR2. kms_frontbuffer_tracking generates subtests
> > > > > dynamically,
> > > > > we
> > > > > can
> > > > > use the same approach here.
> > > > > 
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > >  		test_setup(&data);
> > > > > >  		test_cleanup(&data);
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest("no_drrs") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > > +		test_setup(&data);
> > > > > > +		igt_assert(drrs_disabled(&data));
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > > +
> > > > > > +	igt_subtest("psr2_no_drrs") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > >  		test_setup(&data);
> > > > > >  		igt_assert(drrs_disabled(&data));
> > > > > > @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
> > > > > >  
> > > > > >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> > > > > >  		igt_subtest_f("primary_%s", op_str(op)) {
> > > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > > +			data.op = op;
> > > > > > +			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_PRIMARY;
> > > > > > +			test_setup(&data);
> > > > > > +			run_test(&data);
> > > > > > +			test_cleanup(&data);
> > > > > > +		}
> > > > > > +
> > > > > > +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > >  			data.op = op;
> > > > > >  			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_PRIMARY;
> > > > > >  			test_setup(&data);
> > > > > > @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
> > > > > >  
> > > > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > > > >  		igt_subtest_f("sprite_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > > +			data.op = op;
> > > > > > +			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_OVERLAY;
> > > > > > +			test_setup(&data);
> > > > > > +			run_test(&data);
> > > > > > +			test_cleanup(&data);
> > > > > > +		}
> > > > > > +
> > > > > > +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > >  			data.op = op;
> > > > > >  			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_OVERLAY;
> > > > > >  			test_setup(&data);
> > > > > > @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
> > > > > >  
> > > > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > > > >  		igt_subtest_f("cursor_%s", op_str(op)) {
> > > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > > +			data.op = op;
> > > > > > +			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_CURSOR;
> > > > > > +			test_setup(&data);
> > > > > > +			run_test(&data);
> > > > > > +			test_cleanup(&data);
> > > > > > +		}
> > > > > > +
> > > > > > +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > >  			data.op = op;
> > > > > >  			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_CURSOR;
> > > > > >  			test_setup(&data);
> > > > > > @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest_f("dpms") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.op = RENDER;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > > +		test_setup(&data);
> > > > > > +		dpms_off_on(&data);
> > > > > > +		run_test(&data);
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > > +
> > > > > > +	igt_subtest_f("psr2_dpms") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.op = RENDER;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > >  		test_setup(&data);
> > > > > > @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest_f("suspend") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.op = PLANE_ONOFF;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > > > > +		test_setup(&data);
> > > > > > +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM
> > > > > > ,
> > > > > > +					      SUSPEND_TEST_NONE
> > > > > > );
> > > > > > +		igt_assert(psr_wait_entry_if_enabled(&data));
> > > > > > +		run_test(&data);
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > > +
> > > > > > +	igt_subtest_f("psr2_suspend") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.op = PLANE_ONOFF;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > > > >  		test_setup(&data);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20190117/36f0fcac/attachment.sig>


More information about the igt-dev mailing list