Re: [igt-dev] [PATCH i-g-t 07/20] tests/psr: Kill MMAP_GTT_WAITING

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Apr 16, 2018 at 02:44:02PM -0700, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Thu, 2018-04-12 at 13:11 +0200, Katarzyna Dec wrote:
> > On Tue, Apr 10, 2018 at 07:37:19PM -0700, Dhinakaran Pandiyan wrote:
> > > No PSR event should take 10s, don't see value in this test.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  tests/kms_psr_sink_crc.c | 25 -------------------------
> > >  1 file changed, 25 deletions(-)
> > > 
> > > diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> > > index 94134b7d..d9cce5ea 100644
> > > --- a/tests/kms_psr_sink_crc.c
> > > +++ b/tests/kms_psr_sink_crc.c
> > > @@ -39,7 +39,6 @@ bool running_with_psr_disabled;
> > >  enum operations {
> > >  	PAGE_FLIP,
> > >  	MMAP_GTT,
> > > -	MMAP_GTT_WAITING,
> > >  	MMAP_CPU,
> > >  	BLT,
> > >  	RENDER,
> > > @@ -52,7 +51,6 @@ static const char *op_str(enum operations op)
> > >  	static const char * const name[] = {
> > >  		[PAGE_FLIP] = "page_flip",
> > >  		[MMAP_GTT] = "mmap_gtt",
> > > -		[MMAP_GTT_WAITING] = "mmap_gtt_waiting",
> > >  		[MMAP_CPU] = "mmap_cpu",
> > >  		[BLT] = "blt",
> > >  		[RENDER] = "render",
> > > @@ -319,29 +317,6 @@ static void run_test(data_t *data)
> > >  		munmap(ptr, data->mod_size);
> > >  		expected = "BLACK or TRANSPARENT mark on top of plane in test";
> > >  		break;
> > > -	case MMAP_GTT_WAITING:
> > > -		ptr = gem_mmap__gtt(data->drm_fd, handle, data->mod_size,
> > > -				    PROT_WRITE);
> > > -		gem_set_domain(data->drm_fd, handle,
> > > -			       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > -
> > > -		/* Printing white on white so the screen shouldn't change */
> > > -		memset(ptr, 0xff, data->mod_size);
> > > -		get_sink_crc(data, crc);
> > > -		if (data->test_plane == DRM_PLANE_TYPE_PRIMARY)
> > > -			assert_or_manual(strncmp(ref_crc, crc, CRC_LEN) == 0, "screen WHITE");
> > > -		else
> > > -			assert_or_manual(strncmp(ref_crc, crc, CRC_LEN) == 0,
> > > -			       "GREEN background with WHITE box");
> > > -
> > > -		igt_info("Waiting 10s...\n");
> > > -		sleep(10);
> > Is this time too long? Couldn't we decrease that? Or maybe that is not a point.
> > Why this subtest is not relevant any more?
> 
> 
> I believe the test was relevant in the past when the feature was being
> developed, perhaps there was a driver bug back then.
> 
> Like I wrote in the commit message, no PSR event takes 10s to happen, so
> there is no point in having this test.
> 
> Cc: Rodrigo, who wrote these tests.

This test got written when we didn't have the frontbuffer tracking
and the psr exit was spread all over the code and there was this case that
Chris raised where after a while if another gtt write was happening my old
hammers wouldn't detect.

With the framebuffer tracking that got solved and this never caught any
error. So I'm in favor of removing it:


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> 
> > Kasia
> > > -
> > > -		/* Now lets print black to change the screen */
> > > -		memset(ptr, 0, data->mod_size);
> > > -		munmap(ptr, data->mod_size);
> > > -		expected = "BLACK or TRANSPARENT mark on top of plane in test";
> > > -		break;
> > >  	case MMAP_CPU:
> > >  		ptr = gem_mmap__cpu(data->drm_fd, handle, 0, data->mod_size,
> > >  				    PROT_WRITE);
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev