| Message ID | 1400777259-20091-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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); } }
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(-)