| Message ID | 1513158652-8912-5-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Just a quick comment at the bottom. On Wed, Dec 13, 2017 at 03:20:50PM +0530, Vidya Srinivas wrote: > @@ -312,23 +480,40 @@ static void test_plane_scaling(data_t *d, enum pipe pipe) > igt_require_f(valid_tests, "no valid crtc/connector combinations found\n"); > } > > -igt_simple_main > +igt_main > { > data_t data = {}; > enum pipe pipe; > > igt_skip_on_simulation(); > > - > - data.drm_fd = drm_open_driver(DRIVER_INTEL); > - igt_require_pipe_crc(data.drm_fd); > - igt_display_init(&data.display, data.drm_fd); > - data.devid = intel_get_drm_devid(data.drm_fd); > + igt_fixture { > + data.drm_fd = drm_open_driver(DRIVER_INTEL); > + kmstest_set_vt_graphics_mode(); > + igt_require_pipe_crc(data.drm_fd); > + igt_display_init(&data.display, data.drm_fd); > + data.devid = intel_get_drm_devid(data.drm_fd); > + igt_require_gem(data.drm_fd); > + } > > data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0; Hm, would be nice if we could get rid of this hard-coded platform knowledge and autodiscover. But atm the kernel doesn't expose a "can_scale" property unfortunately. Maybe add a FIXME comment? Real reason I'm commenting here: You probably need to put this into the igt_fixture too, since otherwise you're test won't run correctly when just enumerating tests. > > - for_each_pipe_static(pipe) > - test_plane_scaling(&data, pipe); > + for_each_pipe_static(pipe) { > + > + igt_subtest_f("scaler_basic_test") { > + test_plane_scaling(&data, pipe); > + } > + > + igt_subtest_f("scaler_with_pixel_format") { > + test_scaler_with_pixel_format(&data, pipe); > + } > > - igt_display_fini(&data.display); > + igt_subtest_f("scaler_with_rotation") { > + test_scaler_with_rotation(&data, pipe); > + } > + > + igt_fixture { > + igt_display_fini(&data.display); > + } > + } Just a quick drive-by: You probably want to convert to subtest-based testing as the very first patch (without any functional tests). In your current series you add more subtests while still using igt_simple_main, which will blow up. The goal of a split up patch series isn't just to make review easier, but also testing: Every single step in your series is supposed to be a fully functional codebase. When you're not much experienced with building up such a patch series, it's good practice to double-check that before sending. I tend to use git rebase -x $test-script to automate that if possible. That way you can make sure that every single step in your patch series builds (cleanly!) and results in a working testcases (which should only change results if your patch aims to do that, not as some side-effect). Cheers, Daniel
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Thursday, December 14, 2017 4:25 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel <daniel.vetter@intel.com> > Subject: Re: [Intel-gfx] [PATCH i-g-t 4/6] i-g-t kms_plane_scaling: test > scaling with tiling rotation and pixel formats > > Just a quick comment at the bottom. > > On Wed, Dec 13, 2017 at 03:20:50PM +0530, Vidya Srinivas wrote: > > @@ -312,23 +480,40 @@ static void test_plane_scaling(data_t *d, enum > pipe pipe) > > igt_require_f(valid_tests, "no valid crtc/connector combinations > > found\n"); } > > > > -igt_simple_main > > +igt_main > > { > > data_t data = {}; > > enum pipe pipe; > > > > igt_skip_on_simulation(); > > > > - > > - data.drm_fd = drm_open_driver(DRIVER_INTEL); > > - igt_require_pipe_crc(data.drm_fd); > > - igt_display_init(&data.display, data.drm_fd); > > - data.devid = intel_get_drm_devid(data.drm_fd); > > + igt_fixture { > > + data.drm_fd = drm_open_driver(DRIVER_INTEL); > > + kmstest_set_vt_graphics_mode(); > > + igt_require_pipe_crc(data.drm_fd); > > + igt_display_init(&data.display, data.drm_fd); > > + data.devid = intel_get_drm_devid(data.drm_fd); > > + igt_require_gem(data.drm_fd); > > + } > > > > data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0; > > Hm, would be nice if we could get rid of this hard-coded platform > knowledge and autodiscover. But atm the kernel doesn't expose a > "can_scale" property unfortunately. Maybe add a FIXME comment? > > Real reason I'm commenting here: You probably need to put this into the > igt_fixture too, since otherwise you're test won't run correctly when just > enumerating tests. > > > > > - for_each_pipe_static(pipe) > > - test_plane_scaling(&data, pipe); > > + for_each_pipe_static(pipe) { > > + > > + igt_subtest_f("scaler_basic_test") { > > + test_plane_scaling(&data, pipe); > > + } > > + > > + igt_subtest_f("scaler_with_pixel_format") { > > + test_scaler_with_pixel_format(&data, pipe); > > + } > > > > - igt_display_fini(&data.display); > > + igt_subtest_f("scaler_with_rotation") { > > + test_scaler_with_rotation(&data, pipe); > > + } > > + > > + igt_fixture { > > + igt_display_fini(&data.display); > > + } > > + } > > Just a quick drive-by: You probably want to convert to subtest-based testing > as the very first patch (without any functional tests). In your current series > you add more subtests while still using igt_simple_main, which will blow > up. > > The goal of a split up patch series isn't just to make review easier, but also > testing: Every single step in your series is supposed to be a fully functional > codebase. When you're not much experienced with building up such a patch > series, it's good practice to double-check that before sending. I tend to use > git rebase -x $test-script to automate that if possible. That way you can > make sure that every single step in your patch series builds (cleanly!) and > results in a working testcases (which should only change results if your patch > aims to do that, not as some side-effect). Thank you. Sorry if there is any mistake here. But when we have added subtests we have changed it to igt_main and not simple. Before that we have not added subtests. We followed whatever you suggested - as in, first patch would correct just whatever is Not working as of now. Then in the later tests we added new subtests with enhancements. We ran all the tests once (I am sorry, Not the way you mentioned) fully and it ran on our APL board. Regards Vidya > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Op 14-12-17 om 11:55 schreef Daniel Vetter: > Just a quick comment at the bottom. > > On Wed, Dec 13, 2017 at 03:20:50PM +0530, Vidya Srinivas wrote: >> @@ -312,23 +480,40 @@ static void test_plane_scaling(data_t *d, enum pipe pipe) >> igt_require_f(valid_tests, "no valid crtc/connector combinations found\n"); >> } >> >> -igt_simple_main >> +igt_main >> { >> data_t data = {}; >> enum pipe pipe; >> >> igt_skip_on_simulation(); >> >> - >> - data.drm_fd = drm_open_driver(DRIVER_INTEL); >> - igt_require_pipe_crc(data.drm_fd); >> - igt_display_init(&data.display, data.drm_fd); >> - data.devid = intel_get_drm_devid(data.drm_fd); >> + igt_fixture { >> + data.drm_fd = drm_open_driver(DRIVER_INTEL); >> + kmstest_set_vt_graphics_mode(); >> + igt_require_pipe_crc(data.drm_fd); >> + igt_display_init(&data.display, data.drm_fd); >> + data.devid = intel_get_drm_devid(data.drm_fd); >> + igt_require_gem(data.drm_fd); >> + } >> >> data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0; > Hm, would be nice if we could get rid of this hard-coded platform > knowledge and autodiscover. But atm the kernel doesn't expose a > "can_scale" property unfortunately. Maybe add a FIXME comment? Isn't that what igt_display_try_commit_atomic + TEST_ONLY is for? num scalers might be less if a scaler is used by the crtc as well, so not something you can skip.. > Real reason I'm commenting here: You probably need to put this into the > igt_fixture too, since otherwise you're test won't run correctly when just > enumerating tests. > >> >> - for_each_pipe_static(pipe) >> - test_plane_scaling(&data, pipe); >> + for_each_pipe_static(pipe) { >> + >> + igt_subtest_f("scaler_basic_test") { >> + test_plane_scaling(&data, pipe); >> + } >> + >> + igt_subtest_f("scaler_with_pixel_format") { >> + test_scaler_with_pixel_format(&data, pipe); >> + } >> >> - igt_display_fini(&data.display); >> + igt_subtest_f("scaler_with_rotation") { >> + test_scaler_with_rotation(&data, pipe); >> + } >> + >> + igt_fixture { >> + igt_display_fini(&data.display); >> + } >> + } > Just a quick drive-by: You probably want to convert to subtest-based > testing as the very first patch (without any functional tests). In your > current series you add more subtests while still using igt_simple_main, > which will blow up. > > The goal of a split up patch series isn't just to make review easier, but > also testing: Every single step in your series is supposed to be a fully > functional codebase. When you're not much experienced with building up > such a patch series, it's good practice to double-check that before > sending. I tend to use git rebase -x $test-script to automate that if > possible. That way you can make sure that every single step in your patch > series builds (cleanly!) and results in a working testcases (which should > only change results if your patch aims to do that, not as some > side-effect). > > Cheers, Daniel
diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c index a827cb3..3725d8e 100644 --- a/tests/kms_plane_scaling.c +++ b/tests/kms_plane_scaling.c @@ -54,6 +54,52 @@ typedef struct { } data_t; #define FILE_NAME "1080p-left.png" +#define MIN_SRC_WIDTH_HEIGHT 9 +#define MAX_SRC_WIDTH 4096 + +#define MAX_ROTATION 4 +static igt_rotation_t get_rotation_angle(int rot) +{ + switch (rot) { + case 0: + return IGT_ROTATION_0; + break; + case 1: + return IGT_ROTATION_90; + break; + case 2: + return IGT_ROTATION_180; + break; + case 3: + return IGT_ROTATION_270; + break; + default: + igt_info("Unknown/Unsupported Rotation %d\n", rot); + return IGT_ROTATION_0; + } +} + +#define MAX_TILING 4 +static uint64_t get_tiling(int tiling) +{ + switch (tiling) { + case 0: + return LOCAL_DRM_FORMAT_MOD_NONE; + break; + case 1: + return LOCAL_I915_FORMAT_MOD_X_TILED; + break; + case 2: + return LOCAL_I915_FORMAT_MOD_Y_TILED; + break; + case 3: + return LOCAL_I915_FORMAT_MOD_Yf_TILED; + break; + default: + igt_info("Unknown/Unsupported Tiling %d\n", tiling); + return LOCAL_DRM_FORMAT_MOD_NONE; + } +} static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe, igt_plane_t *plane, drmModeModeInfo *mode, enum igt_commit_style s) @@ -129,6 +175,128 @@ static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane) igt_display_commit2(display, COMMIT_UNIVERSAL); } +static void paint_fb(data_t *d, struct igt_fb *fb) +{ + cairo_t *cr; + + cr = igt_get_cairo_ctx(d->drm_fd, fb); + igt_paint_color(cr, 0, 0, fb->width, fb->height, 0.0, 1.0, 0.0); + igt_assert(cairo_status(cr) == 0); + cairo_destroy(cr); +} + +static void check_scaling_pipe_plane_rot(data_t *d, igt_plane_t *plane, uint32_t pixel_format, + uint64_t tiling, enum pipe pipe, igt_output_t *output, + igt_rotation_t rot) +{ + igt_display_t *display = &d->display; + int width, height; + drmModeModeInfo *mode; + igt_output_set_pipe(output, pipe); + mode = igt_output_get_mode(output); + + /* create buffer in the range of min and max source side limit.*/ + width = height = MIN_SRC_WIDTH_HEIGHT; + d->fb_id1 = igt_create_fb(display->drm_fd, width, height, + pixel_format, tiling, &d->fb1); + paint_fb(d, &d->fb1); + igt_assert(d->fb_id1); + igt_plane_set_fb(plane, &d->fb1); + + /* Check min to full resolution upscaling */ + igt_fb_set_position(&d->fb1, plane, 0, 0); + igt_fb_set_size(&d->fb1, plane, width, height); + igt_plane_set_position(plane, 0, 0); + igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay); + igt_plane_set_rotation(plane, rot); + igt_display_commit2(display, COMMIT_ATOMIC); + + igt_plane_set_fb(plane, NULL); + igt_plane_set_position(plane, 0, 0); + if (d->fb_id1) { + igt_remove_fb(d->drm_fd, &d->fb1); + d->fb_id1 = 0; + } +} + +static void test_scaler_with_rotation_pipe(data_t *d, igt_output_t *output, + enum pipe pipe) +{ + igt_display_t *display = &d->display; + igt_plane_t *plane; + + igt_output_set_pipe(output, pipe); + for_each_plane_on_pipe(display, pipe, plane) { + + if (plane->type == DRM_PLANE_TYPE_CURSOR) + continue; + + for (int i = 0; i < MAX_ROTATION; i++) { + igt_rotation_t rot = get_rotation_angle(i); + check_scaling_pipe_plane_rot(d, plane, DRM_FORMAT_XRGB8888, + LOCAL_I915_FORMAT_MOD_Y_TILED, + pipe, output, rot); + } + } +} + +static void test_scaler_with_rotation(data_t *d, enum pipe pipe) +{ + igt_output_t *output; + + for_each_valid_output_on_pipe(&d->display, pipe, output) { + igt_output_set_pipe(output, pipe); + test_scaler_with_rotation_pipe(d, output, pipe); + igt_output_set_pipe(output, PIPE_ANY); + } +} + +static void +test_scaler_with_pixel_format_pipe_plane(data_t *d, igt_output_t *output, + enum pipe pipe, igt_plane_t *plane, + uint64_t tiling) +{ + uint32_t format; + + for_each_format_in_plane(plane, format) { + if (igt_is_cairo_supported_format(format)) { + check_scaling_pipe_plane_rot(d, plane, format, tiling, + pipe, output, IGT_ROTATION_0); + } + } +} + +static void test_scaler_with_pixel_format_pipe(data_t *d, igt_output_t *output, + enum pipe pipe) +{ + igt_display_t *display = &d->display; + igt_plane_t *plane; + + igt_output_set_pipe(output, pipe); + for_each_plane_on_pipe(display, pipe, plane) { + + if (plane->type == DRM_PLANE_TYPE_CURSOR) + continue; + + for (int i = 0; i < MAX_TILING; i++) { + uint64_t tiling = get_tiling(i); + test_scaler_with_pixel_format_pipe_plane(d, output, pipe, plane, + tiling); + } + } +} + +static void test_scaler_with_pixel_format(data_t *d, enum pipe pipe) +{ + igt_output_t *output; + + for_each_valid_output_on_pipe(&d->display, pipe, output) { + igt_output_set_pipe(output, pipe); + test_scaler_with_pixel_format_pipe(d, output, pipe); + igt_output_set_pipe(output, PIPE_ANY); + } +} + /* does iterative scaling on plane2 */ static void iterate_plane_scaling(data_t *d, drmModeModeInfo *mode) { @@ -312,23 +480,40 @@ static void test_plane_scaling(data_t *d, enum pipe pipe) igt_require_f(valid_tests, "no valid crtc/connector combinations found\n"); } -igt_simple_main +igt_main { data_t data = {}; enum pipe pipe; igt_skip_on_simulation(); - - data.drm_fd = drm_open_driver(DRIVER_INTEL); - igt_require_pipe_crc(data.drm_fd); - igt_display_init(&data.display, data.drm_fd); - data.devid = intel_get_drm_devid(data.drm_fd); + igt_fixture { + data.drm_fd = drm_open_driver(DRIVER_INTEL); + kmstest_set_vt_graphics_mode(); + igt_require_pipe_crc(data.drm_fd); + igt_display_init(&data.display, data.drm_fd); + data.devid = intel_get_drm_devid(data.drm_fd); + igt_require_gem(data.drm_fd); + } data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0; - for_each_pipe_static(pipe) - test_plane_scaling(&data, pipe); + for_each_pipe_static(pipe) { + + igt_subtest_f("scaler_basic_test") { + test_plane_scaling(&data, pipe); + } + + igt_subtest_f("scaler_with_pixel_format") { + test_scaler_with_pixel_format(&data, pipe); + } - igt_display_fini(&data.display); + igt_subtest_f("scaler_with_rotation") { + test_scaler_with_rotation(&data, pipe); + } + + igt_fixture { + igt_display_fini(&data.display); + } + } }