<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-15"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi Juha-Pekka, The new test added do cover the gap of having multiple-plane rotation together, along with the top and bottom cropping scenarios. The test work well for square figures i.e. the width = height =512, but for rectangular figure, the test needs some fix in get_multiplane_crc(), as this function does not assume that the width and height are same. Please find suggestions inline. Other than that, I could only nitpick some of the styling issues given inline. On 10/26/2018 6:28 PM, Juha-Pekka Heikkila wrote: <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap="">Add three new tests which try primary and sprite planes next to each other with different plane formats, rotations and tiling modes. multiplane-rotation subtest run test through with both planes fully visible. multiplane-rotation-cropping-top will crop primary plane to left/top corner and sprite plane to right/top corner while running rotation tests. multiplane-rotation-cropping-bottom will crop primary plane to left/bottom corner and sprite plane to right/bottom corner while running rotation tests. Signed-off-by: Juha-Pekka Heikkila <a class="moz-txt-link-rfc2396E" href="mailto:juhapekka.heikkila@gmail.com"><juhapekka.heikkila@gmail.com></a> --- tests/kms_rotation_crc.c | 249 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 249 insertions(+) diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c index 366c254..df9ac91 100644 --- a/tests/kms_rotation_crc.c +++ b/tests/kms_rotation_crc.c @@ -26,6 +26,25 @@ #include <math.h> #define MAX_FENCES 32 +#define MAXMULTIPLANESAMOUNT 2 + +struct p_struct { + igt_plane_t *plane; + struct igt_fb fb; +}; + +enum p_pointorigo { + p_top = 1 << 0, + p_bottom = 1 << 1, + p_left = 1 << 2, + p_right = 1 << 3 +}; + +struct p_point{ + enum p_pointorigo origo; + int32_t x; + int32_t y; +}; typedef struct { int gfx_fd; @@ -43,6 +62,9 @@ typedef struct { uint32_t override_fmt; uint64_t override_tiling; int devid; + + struct p_struct *multiplaneoldview; + struct p_point planepos[MAXMULTIPLANESAMOUNT]; } data_t; typedef struct { @@ -395,6 +417,197 @@ static void test_plane_rotation(data_t *data, int plane_type, bool test_bad_form } } +typedef struct { + int32_t x1, y1; + uint64_t width, height, tiling, planetype, format; + igt_rotation_t rotation_sw, rotation_hw; +} planeinfos; + +static void get_multiplane_crc(data_t *data, igt_output_t *output, + igt_crc_t *crc_output, planeinfos* planeinfo, + int numplanes) +{ + uint32_t w, h; + igt_display_t *display = &data->display; + struct p_struct *planes, *oldplanes; + int c, ret; + + oldplanes = data->multiplaneoldview; + planes = malloc(sizeof(*planes)*numplanes); + + for (c = 0; c < numplanes; c++) { + planes[c].plane = igt_output_get_plane_type(output, + planeinfo[c].planetype); + + w = planeinfo[c].width; + h = planeinfo[c].height; + + if ((planeinfo[c].rotation_hw|planeinfo[c].rotation_sw) + &(IGT_ROTATION_90|IGT_ROTATION_270)) + igt_swap(w, h); +</pre> </blockquote> In case of an actual hardware rotation 90 and 270, the width and height should not be swapped. The 'if condition' should have only check for rotation_sw, and not for rotation_hw. <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap=""> + igt_plane_set_size(planes[c].plane, w, h);</pre> </blockquote> For 90 and 270 degree rotation (i.e. rotation_hw) , igt_plane_set_size() should have height in place of width and vice-versa. if (rotation_hw & (IGT_ROTATION_90 | IGT_ROTATION_270) igt_plane_set_size(plane[c].plane, h, w); Further more this function igt_plane_set_size should be called after igt_plane_set_fb(), because igt_plane_set_fb sets the plane size with fb.width and fb.height, thereby overwriting our values. <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap=""> + + igt_create_fb(data->gfx_fd, w, h, planeinfo[c].format, + planeinfo[c].tiling, &planes[c].fb); + + paint_squares(data, planeinfo[c].rotation_sw, &planes[c].fb, 1.0f); + igt_plane_set_fb(planes[c].plane, &planes[c].fb); + igt_plane_set_position(planes[c].plane, planeinfo[c].x1, planeinfo[c].y1); + igt_plane_set_rotation(planes[c].plane, planeinfo[c].rotation_hw); + } + + ret = igt_display_try_commit2(display, COMMIT_ATOMIC); + igt_assert_eq(ret, 0); + + igt_pipe_crc_get_current(data->gfx_fd, data->pipe_crc, crc_output); + + for (c = 0; c < numplanes && oldplanes; c++) + igt_remove_fb(data->gfx_fd, &oldplanes[c].fb); + + free(oldplanes); + data->multiplaneoldview = (void*)planes; +} + +static void pointlocation(data_t *data, planeinfos* p, drmModeModeInfo *mode, + int c) +{ + p[c].x1 = data->planepos[c].x + +((data->planepos[c].origo&p_right)?mode->hdisplay:0); + p[c].y1 = data->planepos[c].y + +((data->planepos[c].origo&p_bottom)?mode->vdisplay:0); +}</pre> </blockquote> Space around '&' and '?' <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap="">+ +/* + * Here is pipe parameter which is now used only for first pipe. + * It is left here if this test ever was wanted to be run on + * different pipes. + */ +static void test_multi_plane_rotation(data_t *data, enum pipe pipe) +{ + igt_display_t *display = &data->display; + igt_output_t *output; + igt_crc_t retcrc_sw, retcrc_hw; + planeinfos p[2]; + int c; + struct p_struct *oldplanes; + drmModeModeInfo *mode; + + const static struct { + igt_rotation_t rotation; + uint64_t width; + uint64_t height; + uint64_t tiling; + } planeconfigs[] = { + {IGT_ROTATION_0, 512, 512, LOCAL_DRM_FORMAT_MOD_NONE },</pre> </blockquote> In my humble opinion, we can use a rectangle content, instead of a square content. The function get_muliplane_crc already takes care of different width and height. Only change will be required in function point_location. <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap="">+ {IGT_ROTATION_0, 512, 512, LOCAL_I915_FORMAT_MOD_X_TILED }, + {IGT_ROTATION_0, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED }, + {IGT_ROTATION_0, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED }, + {IGT_ROTATION_90, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED }, + {IGT_ROTATION_90, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED }, + {IGT_ROTATION_180, 512, 512, LOCAL_DRM_FORMAT_MOD_NONE }, + {IGT_ROTATION_180, 512, 512, LOCAL_I915_FORMAT_MOD_X_TILED }, + {IGT_ROTATION_180, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED }, + {IGT_ROTATION_180, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED }, + {IGT_ROTATION_270, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED }, + {IGT_ROTATION_270, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED }, + }; + + /* + * These are those modes which are tested. For testing feel interesting + * case with tiling are 2 byte wide and 4 byte wide. + * + * TODO: + * Built support for NV12 here. + */ + const static uint32_t formatlist[] = {DRM_FORMAT_RGB565, + DRM_FORMAT_XRGB8888}; + + + for_each_valid_output_on_pipe(display, pipe, output) { + int i, j, k, l; + igt_output_set_pipe(output, pipe); + + mode = igt_output_get_mode(output); + + igt_display_require_output(display); + igt_display_commit2(display, COMMIT_ATOMIC); + + data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, + INTEL_PIPE_CRC_SOURCE_AUTO); + igt_pipe_crc_start(data->pipe_crc); + + for (i = 0; i < ARRAY_SIZE(planeconfigs); i++) { + p[0].planetype = DRM_PLANE_TYPE_PRIMARY; + p[0].width = planeconfigs[i].width; + p[0].height = planeconfigs[i].height; + p[0].tiling = planeconfigs[i].tiling; + pointlocation(data, (planeinfos*)&p, mode, 0); + + for (k = 0; k < ARRAY_SIZE(formatlist); k++) { + p[0].format = formatlist[k]; + + for (j = 0; j < ARRAY_SIZE(planeconfigs); j++) { + p[1].planetype = DRM_PLANE_TYPE_OVERLAY; + p[1].width = planeconfigs[j].width; + p[1].height = planeconfigs[j].height; + p[1].tiling = planeconfigs[j].tiling; + pointlocation(data, (planeinfos*)&p, + mode, 1); + + for (l = 0; l < ARRAY_SIZE(formatlist); l++) { + p[1].format = formatlist[l]; + + /* + * RGB565 90/270 degrees rotation is supported + * from gen11 onwards. + */ + if (p[0].format == DRM_FORMAT_RGB565 && + (planeconfigs[i].rotation&(IGT_ROTATION_90|IGT_ROTATION_270)) + && intel_gen(data->devid) < 11) + continue; + + if (p[1].format == DRM_FORMAT_RGB565 && + (planeconfigs[j].rotation&(IGT_ROTATION_90|IGT_ROTATION_270))</pre> </blockquote> Space around '&' and '|' <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap="">+ && intel_gen(data->devid) < 11) + continue; + + + p[0].rotation_sw = planeconfigs[i].rotation; + p[0].rotation_hw = IGT_ROTATION_0; + p[1].rotation_sw = planeconfigs[j].rotation; + p[1].rotation_hw = IGT_ROTATION_0; + get_multiplane_crc(data, output, &retcrc_sw, + (planeinfos*)&p, MAXMULTIPLANESAMOUNT); + + igt_swap(p[0].rotation_sw, p[0].rotation_hw); + igt_swap(p[1].rotation_sw, p[1].rotation_hw); + get_multiplane_crc(data, output, &retcrc_hw, + (planeinfos*)&p, MAXMULTIPLANESAMOUNT); + + igt_assert_crc_equal(&retcrc_sw, &retcrc_hw); + } + } + } + } + igt_pipe_crc_stop(data->pipe_crc); + igt_pipe_crc_free(data->pipe_crc); + igt_output_set_pipe(output, PIPE_ANY); + } + + /* + * Old fbs are deleted only after new ones are set on planes. + * This is done to speed up the test + */ + oldplanes = data->multiplaneoldview; + for (c = 0; c < MAXMULTIPLANESAMOUNT && oldplanes; c++) + igt_remove_fb(data->gfx_fd, &oldplanes[c].fb); + + free(oldplanes); + data->multiplaneoldview = NULL; + data->pipe_crc = NULL; +} + static void test_plane_rotation_exhaust_fences(data_t *data, enum pipe pipe, igt_output_t *output, @@ -595,6 +808,42 @@ igt_main } } + igt_subtest_f("multiplane-rotation") { + igt_require(gen >= 9); + cleanup_crtc(&data); + data.planepos[0].origo = p_top|p_left; + data.planepos[0].x = 128; + data.planepos[0].y = 128; + data.planepos[1].origo = p_top|p_right; + data.planepos[1].x = -128-512;</pre> </blockquote> Space around '|' and '-' operators. Valid for few more lines below. Not sure if we will ever hit a case where there is a lower resolution e.g. 640x480, the figure will be cropped, which is not desired, the test however would still pass as crc would match. Perhaps we can skip if resolution is less than some threshold. <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap=""> + data.planepos[1].y = 128; + test_multi_plane_rotation(&data, 0); + } + + igt_subtest_f("multiplane-rotation-cropping-top") { + igt_require(gen >= 9); + cleanup_crtc(&data); + data.planepos[0].origo = p_top|p_left; + data.planepos[0].x = -128; + data.planepos[0].y = -128; + data.planepos[1].origo = p_top|p_right;</pre> </blockquote> <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap="">+ data.planepos[1].x = -512+128; + data.planepos[1].y = -128; + test_multi_plane_rotation(&data, 0); + } + + igt_subtest_f("multiplane-rotation-cropping-bottom") { + igt_require(gen >= 9); + cleanup_crtc(&data); + data.planepos[0].origo = p_bottom|p_left; + data.planepos[0].x = -128; + data.planepos[0].y = -512+128; + data.planepos[1].origo = p_bottom|p_right;</pre> </blockquote> <blockquote cite="mid:1540558693-28252-1-git-send-email-juhapekka.heikkila@gmail.com" type="cite"> <pre wrap="">+ data.planepos[1].x = -512+128; + data.planepos[1].y = -512+128; + test_multi_plane_rotation(&data, 0); + } + /* * exhaust-fences should be last test, if it fails we may OOM in * the following subtests otherwise. </pre> </blockquote> </body> </html>