Re: [PATCH i-g-t] tests/kms_plane: Ensure planes recover from DPMS

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Mar 05, 2015 at 07:01:09AM -0800, Matt Roper wrote:
> On Thu, Mar 05, 2015 at 01:32:19PM +0100, Daniel Vetter wrote:
> > On Wed, Mar 04, 2015 at 10:50:53AM -0800, Matt Roper wrote:
> > > i915 was using the main atomic 'disable plane' to turn off sprite planes
> > > during a CRTC disable.  This was problematic because it modified the
> > > plane state, preventing us from recovering the original state later.
> > > One such case was that during a DPMS OFF followed by a DPMS ON, any
> > > sprite planes would not be restored properly.
> > > 
> > > Let's add a test that toggles DPMS off and on and ensures that the CRC
> > > remains the same (i.e., planes are successfully restored unchanged).
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  tests/kms_plane.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> > > index 8a08f20..b66ab1d 100644
> > > --- a/tests/kms_plane.c
> > > +++ b/tests/kms_plane.c
> > > @@ -145,6 +145,7 @@ create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
> > >  
> > >  enum {
> > >  	TEST_POSITION_FULLY_COVERED = 1 << 0,
> > > +	TEST_DPMS = 1 << 1,
> > >  };
> > >  
> > >  static void
> > > @@ -158,7 +159,7 @@ test_plane_position_with_output(data_t *data,
> > >  	igt_plane_t *primary, *sprite;
> > >  	struct igt_fb primary_fb, sprite_fb;
> > >  	drmModeModeInfo *mode;
> > > -	igt_crc_t crc;
> > > +	igt_crc_t crc, crc2;
> > >  
> > >  	igt_info("Testing connector %s using pipe %s plane %d\n",
> > >  		 igt_output_name(output), kmstest_pipe_name(pipe), plane);
> > > @@ -194,11 +195,24 @@ test_plane_position_with_output(data_t *data,
> > >  
> > >  	igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
> > >  
> > > +	if (flags & TEST_DPMS) {
> > > +		kmstest_set_connector_dpms(data->drm_fd,
> > > +					   output->config.connector,
> > > +					   DRM_MODE_DPMS_OFF);
> > > +		kmstest_set_connector_dpms(data->drm_fd,
> > > +					   output->config.connector,
> > > +					   DRM_MODE_DPMS_ON);
> > > +	}
> > 
> > I think yet one more subtest to test against system suspend/resume in this
> > spot would be good.
> 
> We actually already do have a subtest for suspend/resume
> ("plane-panning-bottom-right-suspend-pipe-%s-plane-%d") which was
> probably failing before my kernel patch (although when I tried running
> it my system had bigger problems and simply wasn't ever coming back from
> suspend).  I just tried it again and it does seem to be running properly
> now.

Hm I guess just yet another regression that failed to get through our
QA/bug team maze :( A quick check in bugzilla at least didn't turn up
anything. Oh well.

> > > +
> > > +	igt_pipe_crc_collect_crc(data->pipe_crc, &crc2);
> > > +
> > >  	if (flags & TEST_POSITION_FULLY_COVERED)
> > >  		igt_assert(igt_crc_equal(&test.reference_crc, &crc));
> > >  	else
> > >  		igt_assert(!igt_crc_equal(&test.reference_crc, &crc));
> > >  
> > > +	igt_assert(igt_crc_equal(&crc, &crc2));
> > > +
> > >  	igt_plane_set_fb(primary, NULL);
> > >  	igt_plane_set_fb(sprite, NULL);
> > >  
> > > @@ -358,6 +372,11 @@ run_tests_for_pipe_plane(data_t *data, enum pipe pipe, enum igt_plane plane)
> > >  		      kmstest_pipe_name(pipe), plane)
> > >  		test_plane_position(data, pipe, plane, 0);
> > >  
> > > +	igt_subtest_f("plane-position-hole-dpms-pipe-%s-plane-%d",
> > 
> > Usually we call these -vs-dpms (and -vs-suspend for the suspend/resume
> > one).
> 
> Yeah, that's what I usually see, but the existing suspend test for
> kms_plane lacked the "vs" so I left it off dpms as well for consistency.
> Should I update the name of the existing test as well or leave it as is?
> I'm not sure if renaming an existing test would cause problems for PRTS
> or QA tracking.

Naming convention just says that it should have suspend/dpms in the name
somewhere. So if this is more consistent then we're good I think. I'll
apply your patch, thanks.
-Daniel

> 
> 
> Matt
> 
> > -Daniel
> > 
> > > +		      kmstest_pipe_name(pipe), plane)
> > > +		test_plane_position(data, pipe, plane,
> > > +				    TEST_DPMS);
> > > +
> > >  	igt_subtest_f("plane-panning-top-left-pipe-%s-plane-%d",
> > >  		      kmstest_pipe_name(pipe), plane)
> > >  		test_plane_panning(data, pipe, plane, TEST_PANNING_TOP_LEFT);
> > > -- 
> > > 1.8.5.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx