[PATCH] drm/i915: Don't clobber plane state on internal disables

[PATCH] drm/i915: Don't clobber plane state on internal disables

[Matt Roper]

We need to disable all sprite planes when disabling the CRTC.  We had
been using the top-level atomic 'disable' entrypoint to accomplish this,
which was wrong.  Not only can this lead to various locking issues, it
also modifies the actual plane state, making it impossible to restore
the plane properly later.  For example, a DPMS off followed by a DPMS on
will result in any sprite planes in use not being restored properly.

The proper solution here is to call directly into our 'commit plane'
hook with a copy of the plane's current state that has 'visible' set to
false.  Committing this dummy state will turn off the plane, but will
not touch the actual plane->state pointer, allowing us to properly
restore the plane state later.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
This was a pretty big problem so I suspect this is probably tied to some of our
existing bugzilla's but I'm not sure exactly which ones.  It seems that we lack
an i-g-t test for plane status across DPMS, so I'll send along a new i-g-t test
for that shortly.

 drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92cb2ff..6277701 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4201,6 +4201,24 @@ static void intel_enable_sprite_planes(struct drm_crtc *crtc)
 	}
 }
 
+/*
+ * Disable a plane internally without actually modifying the plane's state.
+ * This will allow us to easily restore the plane later by just reprogramming
+ * its state.
+ */
+static void disable_plane_internal(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_plane_state *state =
+		plane->funcs->atomic_duplicate_state(plane);
+	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+
+	intel_state->visible = false;
+	intel_plane->commit_plane(plane, intel_state);
+
+	intel_plane_destroy_state(plane, state);
+}
+
 static void intel_disable_sprite_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -4210,8 +4228,8 @@ static void intel_disable_sprite_planes(struct drm_crtc *crtc)
 
 	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
 		intel_plane = to_intel_plane(plane);
-		if (intel_plane->pipe == pipe)
-			plane->funcs->disable_plane(plane);
+		if (plane->fb && intel_plane->pipe == pipe)
+			disable_plane_internal(plane);
 	}
 }
 
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

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);
+	}
+
+	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",
+		      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

Re: [PATCH] drm/i915: Don't clobber plane state on internal disables

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5887
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  280/280              280/280
ILK                                  308/308              308/308
SNB                 -1              328/328              327/328
IVB                 -1              379/379              378/379
BYT                                  294/294              294/294
HSW                                  387/387              387/387
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt_kms_rotation_crc_sprite-rotation      DMESG_WARN(1)PASS(4)      DMESG_WARN(2)
*IVB  igt_kms_rotation_crc_sprite-rotation      PASS(2)      DMESG_WARN(2)
*BDW  igt_gem_gtt_hog      PASS(3)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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.

> +
> +	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).
-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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't clobber plane state on internal disables

[Daniel Vetter]

On Wed, Mar 04, 2015 at 10:49:04AM -0800, Matt Roper wrote:
> We need to disable all sprite planes when disabling the CRTC.  We had
> been using the top-level atomic 'disable' entrypoint to accomplish this,
> which was wrong.  Not only can this lead to various locking issues, it
> also modifies the actual plane state, making it impossible to restore
> the plane properly later.  For example, a DPMS off followed by a DPMS on
> will result in any sprite planes in use not being restored properly.
> 
> The proper solution here is to call directly into our 'commit plane'
> hook with a copy of the plane's current state that has 'visible' set to
> false.  Committing this dummy state will turn off the plane, but will
> not touch the actual plane->state pointer, allowing us to properly
> restore the plane state later.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> This was a pretty big problem so I suspect this is probably tied to some of our
> existing bugzilla's but I'm not sure exactly which ones.  It seems that we lack
> an i-g-t test for plane status across DPMS, so I'll send along a new i-g-t test
> for that shortly.
> 
>  drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92cb2ff..6277701 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4201,6 +4201,24 @@ static void intel_enable_sprite_planes(struct drm_crtc *crtc)
>  	}
>  }
>  
> +/*
> + * Disable a plane internally without actually modifying the plane's state.
> + * This will allow us to easily restore the plane later by just reprogramming
> + * its state.
> + */
> +static void disable_plane_internal(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct drm_plane_state *state =
> +		plane->funcs->atomic_duplicate_state(plane);
> +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +
> +	intel_state->visible = false;
> +	intel_plane->commit_plane(plane, intel_state);
> +
> +	intel_plane_destroy_state(plane, state);

I wonder whether long-term we should do a full atomic commit including the
begin/end dance here. But this is more than good enough for now, so merged
to dinq.

Thanks, Daniel

> +}
> +
>  static void intel_disable_sprite_planes(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -4210,8 +4228,8 @@ static void intel_disable_sprite_planes(struct drm_crtc *crtc)
>  
>  	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>  		intel_plane = to_intel_plane(plane);
> -		if (intel_plane->pipe == pipe)
> -			plane->funcs->disable_plane(plane);
> +		if (plane->fb && intel_plane->pipe == pipe)
> +			disable_plane_internal(plane);
>  	}
>  }
>  
> -- 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

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.

> 
> > +
> > +	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.


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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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