[PATCH i-g-t] kms_cursor_crc: Move igt_pipe_crc_{new, free} to init

[PATCH i-g-t] kms_cursor_crc: Move igt_pipe_crc_{new, free} to init

[Matt Roper]

If a subtest fails, cleanup_crtc() never gets called.  Currently that
also causes igt_pipe_crc_free() to never be called, leading all
subsequent subtests to also fail with -EBUSY at igt_pipe_crc_new().
Move the calls to igt_pipe_crc_{new,free} into igt_main so that
we don't need to worry about closing and reopening the CRC
filedescriptor for each subtest.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tests/kms_cursor_crc.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 06625ee..1b90baa 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -39,11 +39,14 @@
 #define DRM_CAP_CURSOR_HEIGHT 0x9
 #endif
 
+#define MAX_PIPES 3
+
 typedef struct {
 	int drm_fd;
 	igt_display_t display;
 	struct igt_fb primary_fb;
 	struct igt_fb fb;
+	igt_pipe_crc_t *pipe_crcs[MAX_PIPES];
 } data_t;
 
 typedef struct {
@@ -251,12 +254,6 @@ static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
 
 	igt_display_commit(display);
 
-	/* create the pipe_crc object for this pipe */
-	if (test_data->pipe_crc)
-		igt_pipe_crc_free(test_data->pipe_crc);
-
-	test_data->pipe_crc = igt_pipe_crc_new(test_data->pipe,
-					       INTEL_PIPE_CRC_SOURCE_AUTO);
 	if (!test_data->pipe_crc) {
 		igt_info("auto crc not supported on this connector with pipe %i\n",
 			 test_data->pipe);
@@ -289,7 +286,6 @@ static void cleanup_crtc(test_data_t *test_data, igt_output_t *output)
 	igt_display_t *display = &data->display;
 	igt_plane_t *primary;
 
-	igt_pipe_crc_free(test_data->pipe_crc);
 	test_data->pipe_crc = NULL;
 
 	igt_remove_fb(data->drm_fd, &data->primary_fb);
@@ -315,6 +311,7 @@ static void run_test(data_t *data, void (*testfunc)(test_data_t *), int cursor_w
 		test_data.output = output;
 		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
 			test_data.pipe = p;
+			test_data.pipe_crc = data->pipe_crcs[p];
 
 			if (!prepare_crtc(&test_data, output, cursor_w, cursor_h))
 				continue;
@@ -385,6 +382,7 @@ igt_main
 	uint64_t cursor_width = 64, cursor_height = 64;
 	data_t data = {};
 	int ret;
+	int p;
 
 	igt_skip_on_simulation();
 
@@ -405,11 +403,20 @@ igt_main
 		igt_require_pipe_crc();
 
 		igt_display_init(&data.display, data.drm_fd);
+
+		for (p = 0; p < igt_display_get_n_pipes(&data.display); p++)
+			data.pipe_crcs[p] = igt_pipe_crc_new(p,
+							     INTEL_PIPE_CRC_SOURCE_AUTO);
 	}
 
 	run_test_generic(&data, cursor_width);
 
 	igt_fixture {
+		for (p = 0; p < igt_display_get_n_pipes(&data.display); p++) {
+			if (data.pipe_crcs[p])
+				igt_pipe_crc_free(data.pipe_crcs[p]);
+		}
+
 		igt_display_fini(&data.display);
 	}
 }
-- 
1.8.5.1

Re: [PATCH i-g-t] kms_cursor_crc: Move igt_pipe_crc_{new, free} to init

[Ville Syrjälä]

On Thu, May 22, 2014 at 09:47:39AM -0700, Matt Roper wrote:
> If a subtest fails, cleanup_crtc() never gets called.  Currently that
> also causes igt_pipe_crc_free() to never be called, leading all
> subsequent subtests to also fail with -EBUSY at igt_pipe_crc_new().
> Move the calls to igt_pipe_crc_{new,free} into igt_main so that
> we don't need to worry about closing and reopening the CRC
> filedescriptor for each subtest.

IIRC we can't call them at init because when you call igt_pipe_crc_new()
the pipe->port mapping has to be properly set up so that the auto crc
source will know what to do.

The subtest failure case is the reason why at the start of every subtest
we call igt_pipe_crc_free() before calling igt_pipe_crc_new(). Are you
saying that's not working as intended?

> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  tests/kms_cursor_crc.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> index 06625ee..1b90baa 100644
> --- a/tests/kms_cursor_crc.c
> +++ b/tests/kms_cursor_crc.c
> @@ -39,11 +39,14 @@
>  #define DRM_CAP_CURSOR_HEIGHT 0x9
>  #endif
>  
> +#define MAX_PIPES 3
> +
>  typedef struct {
>  	int drm_fd;
>  	igt_display_t display;
>  	struct igt_fb primary_fb;
>  	struct igt_fb fb;
> +	igt_pipe_crc_t *pipe_crcs[MAX_PIPES];
>  } data_t;
>  
>  typedef struct {
> @@ -251,12 +254,6 @@ static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
>  
>  	igt_display_commit(display);
>  
> -	/* create the pipe_crc object for this pipe */
> -	if (test_data->pipe_crc)
> -		igt_pipe_crc_free(test_data->pipe_crc);
> -
> -	test_data->pipe_crc = igt_pipe_crc_new(test_data->pipe,
> -					       INTEL_PIPE_CRC_SOURCE_AUTO);
>  	if (!test_data->pipe_crc) {
>  		igt_info("auto crc not supported on this connector with pipe %i\n",
>  			 test_data->pipe);
> @@ -289,7 +286,6 @@ static void cleanup_crtc(test_data_t *test_data, igt_output_t *output)
>  	igt_display_t *display = &data->display;
>  	igt_plane_t *primary;
>  
> -	igt_pipe_crc_free(test_data->pipe_crc);
>  	test_data->pipe_crc = NULL;
>  
>  	igt_remove_fb(data->drm_fd, &data->primary_fb);
> @@ -315,6 +311,7 @@ static void run_test(data_t *data, void (*testfunc)(test_data_t *), int cursor_w
>  		test_data.output = output;
>  		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
>  			test_data.pipe = p;
> +			test_data.pipe_crc = data->pipe_crcs[p];
>  
>  			if (!prepare_crtc(&test_data, output, cursor_w, cursor_h))
>  				continue;
> @@ -385,6 +382,7 @@ igt_main
>  	uint64_t cursor_width = 64, cursor_height = 64;
>  	data_t data = {};
>  	int ret;
> +	int p;
>  
>  	igt_skip_on_simulation();
>  
> @@ -405,11 +403,20 @@ igt_main
>  		igt_require_pipe_crc();
>  
>  		igt_display_init(&data.display, data.drm_fd);
> +
> +		for (p = 0; p < igt_display_get_n_pipes(&data.display); p++)
> +			data.pipe_crcs[p] = igt_pipe_crc_new(p,
> +							     INTEL_PIPE_CRC_SOURCE_AUTO);
>  	}
>  
>  	run_test_generic(&data, cursor_width);
>  
>  	igt_fixture {
> +		for (p = 0; p < igt_display_get_n_pipes(&data.display); p++) {
> +			if (data.pipe_crcs[p])
> +				igt_pipe_crc_free(data.pipe_crcs[p]);
> +		}
> +
>  		igt_display_fini(&data.display);
>  	}
>  }
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH i-g-t] kms_cursor_crc: Move igt_pipe_crc_{new, free} to init

[Matt Roper]

On Thu, May 22, 2014 at 08:01:09PM +0300, Ville Syrjälä wrote:
> On Thu, May 22, 2014 at 09:47:39AM -0700, Matt Roper wrote:
> > If a subtest fails, cleanup_crtc() never gets called.  Currently that
> > also causes igt_pipe_crc_free() to never be called, leading all
> > subsequent subtests to also fail with -EBUSY at igt_pipe_crc_new().
> > Move the calls to igt_pipe_crc_{new,free} into igt_main so that
> > we don't need to worry about closing and reopening the CRC
> > filedescriptor for each subtest.
> 
> IIRC we can't call them at init because when you call igt_pipe_crc_new()
> the pipe->port mapping has to be properly set up so that the auto crc
> source will know what to do.
> 
> The subtest failure case is the reason why at the start of every subtest
> we call igt_pipe_crc_free() before calling igt_pipe_crc_new(). Are you
> saying that's not working as intended?

Right, you're working on a fresh test_data structure for each call of
run_test(), so you've already lost the test_data->pipe_crc pointer that
you're trying to check for NULL there.

I guess the right fix is to move the pipe_crc pointer up into data_t
rather than test_data_t?


Matt

> 
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  tests/kms_cursor_crc.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> > index 06625ee..1b90baa 100644
> > --- a/tests/kms_cursor_crc.c
> > +++ b/tests/kms_cursor_crc.c
> > @@ -39,11 +39,14 @@
> >  #define DRM_CAP_CURSOR_HEIGHT 0x9
> >  #endif
> >  
> > +#define MAX_PIPES 3
> > +
> >  typedef struct {
> >  	int drm_fd;
> >  	igt_display_t display;
> >  	struct igt_fb primary_fb;
> >  	struct igt_fb fb;
> > +	igt_pipe_crc_t *pipe_crcs[MAX_PIPES];
> >  } data_t;
> >  
> >  typedef struct {
> > @@ -251,12 +254,6 @@ static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
> >  
> >  	igt_display_commit(display);
> >  
> > -	/* create the pipe_crc object for this pipe */
> > -	if (test_data->pipe_crc)
> > -		igt_pipe_crc_free(test_data->pipe_crc);
> > -
> > -	test_data->pipe_crc = igt_pipe_crc_new(test_data->pipe,
> > -					       INTEL_PIPE_CRC_SOURCE_AUTO);
> >  	if (!test_data->pipe_crc) {
> >  		igt_info("auto crc not supported on this connector with pipe %i\n",
> >  			 test_data->pipe);
> > @@ -289,7 +286,6 @@ static void cleanup_crtc(test_data_t *test_data, igt_output_t *output)
> >  	igt_display_t *display = &data->display;
> >  	igt_plane_t *primary;
> >  
> > -	igt_pipe_crc_free(test_data->pipe_crc);
> >  	test_data->pipe_crc = NULL;
> >  
> >  	igt_remove_fb(data->drm_fd, &data->primary_fb);
> > @@ -315,6 +311,7 @@ static void run_test(data_t *data, void (*testfunc)(test_data_t *), int cursor_w
> >  		test_data.output = output;
> >  		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
> >  			test_data.pipe = p;
> > +			test_data.pipe_crc = data->pipe_crcs[p];
> >  
> >  			if (!prepare_crtc(&test_data, output, cursor_w, cursor_h))
> >  				continue;
> > @@ -385,6 +382,7 @@ igt_main
> >  	uint64_t cursor_width = 64, cursor_height = 64;
> >  	data_t data = {};
> >  	int ret;
> > +	int p;
> >  
> >  	igt_skip_on_simulation();
> >  
> > @@ -405,11 +403,20 @@ igt_main
> >  		igt_require_pipe_crc();
> >  
> >  		igt_display_init(&data.display, data.drm_fd);
> > +
> > +		for (p = 0; p < igt_display_get_n_pipes(&data.display); p++)
> > +			data.pipe_crcs[p] = igt_pipe_crc_new(p,
> > +							     INTEL_PIPE_CRC_SOURCE_AUTO);
> >  	}
> >  
> >  	run_test_generic(&data, cursor_width);
> >  
> >  	igt_fixture {
> > +		for (p = 0; p < igt_display_get_n_pipes(&data.display); p++) {
> > +			if (data.pipe_crcs[p])
> > +				igt_pipe_crc_free(data.pipe_crcs[p]);
> > +		}
> > +
> >  		igt_display_fini(&data.display);
> >  	}
> >  }
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH i-g-t] kms_cursor_crc: Move igt_pipe_crc_{new, free} to init

[Ville Syrjälä]

On Thu, May 22, 2014 at 10:06:37AM -0700, Matt Roper wrote:
> On Thu, May 22, 2014 at 08:01:09PM +0300, Ville Syrjälä wrote:
> > On Thu, May 22, 2014 at 09:47:39AM -0700, Matt Roper wrote:
> > > If a subtest fails, cleanup_crtc() never gets called.  Currently that
> > > also causes igt_pipe_crc_free() to never be called, leading all
> > > subsequent subtests to also fail with -EBUSY at igt_pipe_crc_new().
> > > Move the calls to igt_pipe_crc_{new,free} into igt_main so that
> > > we don't need to worry about closing and reopening the CRC
> > > filedescriptor for each subtest.
> > 
> > IIRC we can't call them at init because when you call igt_pipe_crc_new()
> > the pipe->port mapping has to be properly set up so that the auto crc
> > source will know what to do.
> > 
> > The subtest failure case is the reason why at the start of every subtest
> > we call igt_pipe_crc_free() before calling igt_pipe_crc_new(). Are you
> > saying that's not working as intended?
> 
> Right, you're working on a fresh test_data structure for each call of
> run_test(), so you've already lost the test_data->pipe_crc pointer that
> you're trying to check for NULL there.
> 
> I guess the right fix is to move the pipe_crc pointer up into data_t
> rather than test_data_t?

I guess just kill test_data_t and shovel everything into data_t. I don't
remember why the test_data_t came to be there, but I don't think it
serves any purpose anymore.

-- 
Ville Syrjälä
Intel OTC

[PATCH i-g-t] kms_cursor_crc: Combine data_t and test_data_t

[Matt Roper]

If a subtest fails, cleanup_crtc() never gets called and then the
test_data_t structure for the test is lost, including the CRC file
descriptor that we never got a chance to release; this causes all
subsequent tests to fail with -EBUSY at igt_pipe_crc_new().

The split between permanent data_t and temporary test_data_t doesn't
seem to serve a purpose, so just combine the fields from both into
data_t.  This will prevent us from losing the CRC filedescriptor so that
we can properly close and reopen it after a failed test.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tests/kms_cursor_crc.c | 206 +++++++++++++++++++++++--------------------------
 1 file changed, 97 insertions(+), 109 deletions(-)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 06625ee..92d9a3c 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -44,10 +44,6 @@ typedef struct {
 	igt_display_t display;
 	struct igt_fb primary_fb;
 	struct igt_fb fb;
-} data_t;
-
-typedef struct {
-	data_t *data;
 	igt_output_t *output;
 	enum pipe pipe;
 	igt_crc_t ref_crc;
@@ -55,7 +51,7 @@ typedef struct {
 	int screenw, screenh;
 	int curw, curh; /* cursor size */
 	igt_pipe_crc_t *pipe_crc;
-} test_data_t;
+} data_t;
 
 static void draw_cursor(cairo_t *cr, int x, int y, int w)
 {
@@ -71,11 +67,10 @@ static void draw_cursor(cairo_t *cr, int x, int y, int w)
 	igt_paint_color_alpha(cr, x + w, y + w, w, w, 0.5, 0.5, 0.5, 1.0);
 }
 
-static void cursor_enable(test_data_t *test_data)
+static void cursor_enable(data_t *data)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
-	igt_output_t *output = test_data->output;
+	igt_output_t *output = data->output;
 	igt_plane_t *cursor;
 
 	cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR);
@@ -83,11 +78,10 @@ static void cursor_enable(test_data_t *test_data)
 	igt_display_commit(display);
 }
 
-static void cursor_disable(test_data_t *test_data)
+static void cursor_disable(data_t *data)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
-	igt_output_t *output = test_data->output;
+	igt_output_t *output = data->output;
 	igt_plane_t *cursor;
 
 	cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR);
@@ -96,11 +90,10 @@ static void cursor_disable(test_data_t *test_data)
 }
 
 
-static void do_single_test(test_data_t *test_data, int x, int y)
+static void do_single_test(data_t *data, int x, int y)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
-	igt_pipe_crc_t *pipe_crc = test_data->pipe_crc;
+	igt_pipe_crc_t *pipe_crc = data->pipe_crc;
 	igt_crc_t crc, ref_crc;
 	igt_plane_t *cursor;
 	cairo_t *cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb);
@@ -108,93 +101,93 @@ static void do_single_test(test_data_t *test_data, int x, int y)
 	igt_info("."); fflush(stdout);
 
 	/* Hardware test */
-	igt_paint_test_pattern(cr, test_data->screenw, test_data->screenh);
-	cursor_enable(test_data);
-	cursor = igt_output_get_plane(test_data->output, IGT_PLANE_CURSOR);
+	igt_paint_test_pattern(cr, data->screenw, data->screenh);
+	cursor_enable(data);
+	cursor = igt_output_get_plane(data->output, IGT_PLANE_CURSOR);
 	igt_plane_set_position(cursor, x, y);
 	igt_display_commit(display);
-	igt_wait_for_vblank(data->drm_fd, test_data->pipe);
+	igt_wait_for_vblank(data->drm_fd, data->pipe);
 	igt_pipe_crc_collect_crc(pipe_crc, &crc);
-	cursor_disable(test_data);
+	cursor_disable(data);
 
 	/* Now render the same in software and collect crc */
-	draw_cursor(cr, x, y, test_data->curw);
+	draw_cursor(cr, x, y, data->curw);
 	igt_display_commit(display);
-	igt_wait_for_vblank(data->drm_fd, test_data->pipe);
+	igt_wait_for_vblank(data->drm_fd, data->pipe);
 	igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
 	/* Clear screen afterwards */
-	igt_paint_color(cr, 0, 0, test_data->screenw, test_data->screenh,
-			    0.0, 0.0, 0.0);
+	igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
+			0.0, 0.0, 0.0);
 
 	igt_assert(igt_crc_equal(&crc, &ref_crc));
 }
 
-static void do_test(test_data_t *test_data,
+static void do_test(data_t *data,
 		    int left, int right, int top, int bottom)
 {
-	do_single_test(test_data, left, top);
-	do_single_test(test_data, right, top);
-	do_single_test(test_data, right, bottom);
-	do_single_test(test_data, left, bottom);
+	do_single_test(data, left, top);
+	do_single_test(data, right, top);
+	do_single_test(data, right, bottom);
+	do_single_test(data, left, bottom);
 }
 
-static void test_crc_onscreen(test_data_t *test_data)
+static void test_crc_onscreen(data_t *data)
 {
-	int left = test_data->left;
-	int right = test_data->right;
-	int top = test_data->top;
-	int bottom = test_data->bottom;
-	int cursor_w = test_data->curw;
-	int cursor_h = test_data->curh;
+	int left = data->left;
+	int right = data->right;
+	int top = data->top;
+	int bottom = data->bottom;
+	int cursor_w = data->curw;
+	int cursor_h = data->curh;
 
 	/* fully inside  */
-	do_test(test_data, left, right, top, bottom);
+	do_test(data, left, right, top, bottom);
 
 	/* 2 pixels inside */
-	do_test(test_data, left - (cursor_w-2), right + (cursor_w-2), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h-2), bottom + (cursor_h-2));
-	do_test(test_data, left - (cursor_w-2), right + (cursor_w-2), top - (cursor_h-2), bottom + (cursor_h-2));
+	do_test(data, left - (cursor_w-2), right + (cursor_w-2), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h-2), bottom + (cursor_h-2));
+	do_test(data, left - (cursor_w-2), right + (cursor_w-2), top - (cursor_h-2), bottom + (cursor_h-2));
 
 	/* 1 pixel inside */
-	do_test(test_data, left - (cursor_w-1), right + (cursor_w-1), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h-1), bottom + (cursor_h-1));
-	do_test(test_data, left - (cursor_w-1), right + (cursor_w-1), top - (cursor_h-1), bottom + (cursor_h-1));
+	do_test(data, left - (cursor_w-1), right + (cursor_w-1), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h-1), bottom + (cursor_h-1));
+	do_test(data, left - (cursor_w-1), right + (cursor_w-1), top - (cursor_h-1), bottom + (cursor_h-1));
 }
 
-static void test_crc_offscreen(test_data_t *test_data)
+static void test_crc_offscreen(data_t *data)
 {
-	int left = test_data->left;
-	int right = test_data->right;
-	int top = test_data->top;
-	int bottom = test_data->bottom;
-	int cursor_w = test_data->curw;
-	int cursor_h = test_data->curh;
+	int left = data->left;
+	int right = data->right;
+	int top = data->top;
+	int bottom = data->bottom;
+	int cursor_w = data->curw;
+	int cursor_h = data->curh;
 
 	/* fully outside */
-	do_test(test_data, left - (cursor_w), right + (cursor_w), top             , bottom             );
-	do_test(test_data, left             , right             , top - (cursor_h), bottom + (cursor_h));
-	do_test(test_data, left - (cursor_w), right + (cursor_w), top - (cursor_h), bottom + (cursor_h));
+	do_test(data, left - (cursor_w), right + (cursor_w), top             , bottom             );
+	do_test(data, left             , right             , top - (cursor_h), bottom + (cursor_h));
+	do_test(data, left - (cursor_w), right + (cursor_w), top - (cursor_h), bottom + (cursor_h));
 
 	/* fully outside by 1 extra pixels */
-	do_test(test_data, left - (cursor_w+1), right + (cursor_w+1), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h+1), bottom + (cursor_h+1));
-	do_test(test_data, left - (cursor_w+1), right + (cursor_w+1), top - (cursor_h+1), bottom + (cursor_h+1));
+	do_test(data, left - (cursor_w+1), right + (cursor_w+1), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h+1), bottom + (cursor_h+1));
+	do_test(data, left - (cursor_w+1), right + (cursor_w+1), top - (cursor_h+1), bottom + (cursor_h+1));
 
 	/* fully outside by 2 extra pixels */
-	do_test(test_data, left - (cursor_w+2), right + (cursor_w+2), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h+2), bottom + (cursor_h+2));
-	do_test(test_data, left - (cursor_w+2), right + (cursor_w+2), top - (cursor_h+2), bottom + (cursor_h+2));
+	do_test(data, left - (cursor_w+2), right + (cursor_w+2), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h+2), bottom + (cursor_h+2));
+	do_test(data, left - (cursor_w+2), right + (cursor_w+2), top - (cursor_h+2), bottom + (cursor_h+2));
 
 	/* fully outside by a lot of extra pixels */
-	do_test(test_data, left - (cursor_w+512), right + (cursor_w+512), top                 , bottom                 );
-	do_test(test_data, left                 , right                 , top - (cursor_h+512), bottom + (cursor_h+512));
-	do_test(test_data, left - (cursor_w+512), right + (cursor_w+512), top - (cursor_h+512), bottom + (cursor_h+512));
+	do_test(data, left - (cursor_w+512), right + (cursor_w+512), top                 , bottom                 );
+	do_test(data, left                 , right                 , top - (cursor_h+512), bottom + (cursor_h+512));
+	do_test(data, left - (cursor_w+512), right + (cursor_w+512), top - (cursor_h+512), bottom + (cursor_h+512));
 
 	/* go nuts */
-	do_test(test_data, INT_MIN, INT_MAX, INT_MIN, INT_MAX);
+	do_test(data, INT_MIN, INT_MAX, INT_MIN, INT_MAX);
 }
 
-static void test_crc_sliding(test_data_t *test_data)
+static void test_crc_sliding(data_t *data)
 {
 	int i;
 
@@ -202,34 +195,33 @@ static void test_crc_sliding(test_data_t *test_data)
 	 * no alignment issues. Horizontal, vertical and diagonal test.
 	 */
 	for (i = 0; i < 16; i++) {
-		do_single_test(test_data, i, 0);
-		do_single_test(test_data, 0, i);
-		do_single_test(test_data, i, i);
+		do_single_test(data, i, 0);
+		do_single_test(data, 0, i);
+		do_single_test(data, i, i);
 	}
 }
 
-static void test_crc_random(test_data_t *test_data)
+static void test_crc_random(data_t *data)
 {
 	int i;
 
 	/* Random cursor placement */
 	for (i = 0; i < 50; i++) {
-		int x = rand() % (test_data->screenw + test_data->curw * 2) - test_data->curw;
-		int y = rand() % (test_data->screenh + test_data->curh * 2) - test_data->curh;
-		do_single_test(test_data, x, y);
+		int x = rand() % (data->screenw + data->curw * 2) - data->curw;
+		int y = rand() % (data->screenh + data->curh * 2) - data->curh;
+		do_single_test(data, x, y);
 	}
 }
 
-static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
+static bool prepare_crtc(data_t *data, igt_output_t *output,
 			 int cursor_w, int cursor_h)
 {
 	drmModeModeInfo *mode;
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
 	igt_plane_t *primary;
 
 	/* select the pipe we want to use */
-	igt_output_set_pipe(output, test_data->pipe);
+	igt_output_set_pipe(output, data->pipe);
 	igt_display_commit(display);
 
 	if (!output->valid) {
@@ -241,10 +233,10 @@ static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
 	/* create and set the primary plane fb */
 	mode = igt_output_get_mode(output);
 	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
-				DRM_FORMAT_XRGB8888,
-				false, /* tiled */
-				0.0, 0.0, 0.0,
-				&data->primary_fb);
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 0.0,
+			    &data->primary_fb);
 
 	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
 	igt_plane_set_fb(primary, &data->primary_fb);
@@ -252,45 +244,44 @@ static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
 	igt_display_commit(display);
 
 	/* create the pipe_crc object for this pipe */
-	if (test_data->pipe_crc)
-		igt_pipe_crc_free(test_data->pipe_crc);
+	if (data->pipe_crc)
+		igt_pipe_crc_free(data->pipe_crc);
 
-	test_data->pipe_crc = igt_pipe_crc_new(test_data->pipe,
-					       INTEL_PIPE_CRC_SOURCE_AUTO);
-	if (!test_data->pipe_crc) {
+	data->pipe_crc = igt_pipe_crc_new(data->pipe,
+					  INTEL_PIPE_CRC_SOURCE_AUTO);
+	if (!data->pipe_crc) {
 		igt_info("auto crc not supported on this connector with pipe %i\n",
-			 test_data->pipe);
+			 data->pipe);
 		return false;
 	}
 
 	/* x/y position where the cursor is still fully visible */
-	test_data->left = 0;
-	test_data->right = mode->hdisplay - cursor_w;
-	test_data->top = 0;
-	test_data->bottom = mode->vdisplay - cursor_h;
-	test_data->screenw = mode->hdisplay;
-	test_data->screenh = mode->vdisplay;
-	test_data->curw = cursor_w;
-	test_data->curh = cursor_h;
+	data->left = 0;
+	data->right = mode->hdisplay - cursor_w;
+	data->top = 0;
+	data->bottom = mode->vdisplay - cursor_h;
+	data->screenw = mode->hdisplay;
+	data->screenh = mode->vdisplay;
+	data->curw = cursor_w;
+	data->curh = cursor_h;
 
 	/* make sure cursor is disabled */
-	cursor_disable(test_data);
-	igt_wait_for_vblank(data->drm_fd, test_data->pipe);
+	cursor_disable(data);
+	igt_wait_for_vblank(data->drm_fd, data->pipe);
 
 	/* get reference crc w/o cursor */
-	igt_pipe_crc_collect_crc(test_data->pipe_crc, &test_data->ref_crc);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &data->ref_crc);
 
 	return true;
 }
 
-static void cleanup_crtc(test_data_t *test_data, igt_output_t *output)
+static void cleanup_crtc(data_t *data, igt_output_t *output)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
 	igt_plane_t *primary;
 
-	igt_pipe_crc_free(test_data->pipe_crc);
-	test_data->pipe_crc = NULL;
+	igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = NULL;
 
 	igt_remove_fb(data->drm_fd, &data->primary_fb);
 
@@ -301,38 +292,35 @@ static void cleanup_crtc(test_data_t *test_data, igt_output_t *output)
 	igt_display_commit(display);
 }
 
-static void run_test(data_t *data, void (*testfunc)(test_data_t *), int cursor_w, int cursor_h)
+static void run_test(data_t *data, void (*testfunc)(data_t *), int cursor_w, int cursor_h)
 {
 	igt_display_t *display = &data->display;
 	igt_output_t *output;
 	enum pipe p;
-	test_data_t test_data = {
-		.data = data,
-	};
 	int valid_tests = 0;
 
 	for_each_connected_output(display, output) {
-		test_data.output = output;
+		data->output = output;
 		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
-			test_data.pipe = p;
+			data->pipe = p;
 
-			if (!prepare_crtc(&test_data, output, cursor_w, cursor_h))
+			if (!prepare_crtc(data, output, cursor_w, cursor_h))
 				continue;
 
 			valid_tests++;
 
 			igt_info("Beginning %s on pipe %c, connector %s\n",
-				 igt_subtest_name(), pipe_name(test_data.pipe),
+				 igt_subtest_name(), pipe_name(data->pipe),
 				 igt_output_name(output));
 
-			testfunc(&test_data);
+			testfunc(data);
 
 			igt_info("\n%s on pipe %c, connector %s: PASSED\n\n",
-				 igt_subtest_name(), pipe_name(test_data.pipe),
+				 igt_subtest_name(), pipe_name(data->pipe),
 				 igt_output_name(output));
 
 			/* cleanup what prepare_crtc() has done */
-			cleanup_crtc(&test_data, output);
+			cleanup_crtc(data, output);
 		}
 	}
 
-- 
1.8.5.1

Re: [PATCH i-g-t] kms_cursor_crc: Move igt_pipe_crc_{new, free} to init

[Daniel Vetter]

On Thu, May 22, 2014 at 08:12:58PM +0300, Ville Syrjälä wrote:
> On Thu, May 22, 2014 at 10:06:37AM -0700, Matt Roper wrote:
> > On Thu, May 22, 2014 at 08:01:09PM +0300, Ville Syrjälä wrote:
> > > On Thu, May 22, 2014 at 09:47:39AM -0700, Matt Roper wrote:
> > > > If a subtest fails, cleanup_crtc() never gets called.  Currently that
> > > > also causes igt_pipe_crc_free() to never be called, leading all
> > > > subsequent subtests to also fail with -EBUSY at igt_pipe_crc_new().
> > > > Move the calls to igt_pipe_crc_{new,free} into igt_main so that
> > > > we don't need to worry about closing and reopening the CRC
> > > > filedescriptor for each subtest.
> > > 
> > > IIRC we can't call them at init because when you call igt_pipe_crc_new()
> > > the pipe->port mapping has to be properly set up so that the auto crc
> > > source will know what to do.
> > > 
> > > The subtest failure case is the reason why at the start of every subtest
> > > we call igt_pipe_crc_free() before calling igt_pipe_crc_new(). Are you
> > > saying that's not working as intended?
> > 
> > Right, you're working on a fresh test_data structure for each call of
> > run_test(), so you've already lost the test_data->pipe_crc pointer that
> > you're trying to check for NULL there.
> > 
> > I guess the right fix is to move the pipe_crc pointer up into data_t
> > rather than test_data_t?
> 
> I guess just kill test_data_t and shovel everything into data_t. I don't
> remember why the test_data_t came to be there, but I don't think it
> serves any purpose anymore.

Also just run igts with piglit. That runs every subtest separately which
helps a lot for such issues. Running all the tests is kinda just the
quick&hacky versions if you're wreaking havoc with a single tests.

Otoh for that I just run one subtest through the cmdline switch. So not
sure how much we should bother here really ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch