From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3B0686EA2D for ; Fri, 27 Mar 2020 13:25:45 +0000 (UTC) References: <20200317071906.28879-1-swati2.sharma@intel.com> From: "Sharma, Swati2" Message-ID: Date: Fri, 27 Mar 2020 18:55:40 +0530 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Subject: Re: [igt-dev] [PATCH i-g-t v4] tests/kms_atomic: add test to validate immutable zpos List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: "Peres, Martin" , "igt-dev@lists.freedesktop.org" Cc: "Latvala, Petri" List-ID: On 25-Mar-20 1:15 PM, Peres, Martin wrote: > On 2020-03-17 09:19, Swati Sharma wrote: >> i915 implements immutable zpos property whereas the existing test >> case is written to validate mutable zpos. >> >> Added a new sub-test to validate immutable zpos. Test validates >> the reported zpos property of plane by making sure only higher >> zpos plane covers the lower zpos one (lower plane covers full screen, >> upper plane covers half screen). >> >> Only two planes at a time are tested in increasing fashion to avoid >> combinatorial explosion. By transitive property, >> if (p1, z1) < (p2, z2) and (p2, z2) < (p3, z3) then (p1, z1) < (p3, z3) >> >> where p and z denotes planes and zpos >> >> v2: -Removed intel only checks [Martin] >> -Used XRGB8888 pixel format as used in other IGTs >> -Added documentation [Martin] >> -Removed skip, instead continue if plane doesn't support zpos [Martin] >> -Sorted planes in increasing order of zpos [Martin] >> v3: -Fix description [Martin] >> -Avoid sorting [Ankit] >> v4: -Change in commit message [Ankit] >> -Few minor changes [Ankit] >> >> Issue: https://gitlab.freedesktop.org/drm/intel/issues/404 >> Signed-off-by: Swati Sharma >> Reviewed-by: Martin Peres >> Reviewed-by: Ankit Nautiyal >> --- >> tests/kms_atomic.c | 160 ++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 150 insertions(+), 10 deletions(-) >> >> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c >> index 8462d128..4cb34d6d 100644 >> --- a/tests/kms_atomic.c >> +++ b/tests/kms_atomic.c >> @@ -121,7 +121,7 @@ static void plane_check_current_state(igt_plane_t *plane, const uint64_t *values >> } >> >> static void plane_commit(igt_plane_t *plane, enum igt_commit_style s, >> - enum kms_atomic_check_relax relax) >> + enum kms_atomic_check_relax relax) >> { >> igt_display_commit2(plane->pipe->display, s); >> plane_check_current_state(plane, plane->values, relax); >> @@ -277,9 +277,9 @@ static uint32_t plane_get_igt_format(igt_plane_t *plane) >> } >> >> static void >> -plane_primary_overlay_zpos(igt_pipe_t *pipe, igt_output_t *output, >> - igt_plane_t *primary, igt_plane_t *overlay, >> - uint32_t format_primary, uint32_t format_overlay) >> +plane_primary_overlay_mutable_zpos(igt_pipe_t *pipe, igt_output_t *output, >> + igt_plane_t *primary, igt_plane_t *overlay, >> + uint32_t format_primary, uint32_t format_overlay) >> { >> struct igt_fb fb_primary, fb_overlay; >> drmModeModeInfo *mode = igt_output_get_mode(output); >> @@ -320,7 +320,7 @@ plane_primary_overlay_zpos(igt_pipe_t *pipe, igt_output_t *output, >> igt_plane_set_prop_value(overlay, IGT_PLANE_ZPOS, 1); >> >> igt_info("Committing with overlay on top, it has a hole "\ >> - "through which the primary should be seen\n"); >> + "through which the primary should be seen\n"); >> plane_commit(primary, COMMIT_ATOMIC, ATOMIC_RELAX_NONE); >> >> igt_assert_eq_u64(igt_plane_get_prop(primary, IGT_PLANE_ZPOS), 0); >> @@ -346,7 +346,7 @@ plane_primary_overlay_zpos(igt_pipe_t *pipe, igt_output_t *output, >> igt_put_cairo_ctx(pipe->display->drm_fd, &fb_primary, cr); >> >> igt_info("Committing with a hole in the primary through "\ >> - "which the underlay should be seen\n"); >> + "which the underlay should be seen\n"); >> plane_commit(primary, COMMIT_ATOMIC, ATOMIC_RELAX_NONE); >> >> /* reset it back to initial state */ >> @@ -358,6 +358,136 @@ plane_primary_overlay_zpos(igt_pipe_t *pipe, igt_output_t *output, >> igt_assert_eq_u64(igt_plane_get_prop(overlay, IGT_PLANE_ZPOS), 1); >> } >> >> +static void >> +plane_immutable_zpos(igt_display_t *display, igt_pipe_t *pipe, >> + igt_output_t *output) >> +{ >> + cairo_t *cr; >> + struct igt_fb fb_ref; >> + igt_plane_t *primary; >> + drmModeModeInfo *mode; >> + igt_pipe_crc_t *pipe_crc; >> + igt_crc_t ref_crc, new_crc; >> + int n_planes = pipe->n_planes; >> + igt_plane_t *plane_ptr[n_planes]; >> + uint32_t w_lower, h_lower, w_upper, h_upper; >> + >> + igt_require(n_planes >= 2); >> + >> + mode = igt_output_get_mode(output); >> + primary = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY); >> + >> + /* for lower plane */ >> + w_lower = mode->hdisplay; >> + h_lower = mode->vdisplay; >> + >> + /* for upper plane */ >> + w_upper = mode->hdisplay / 2; >> + h_upper = mode->vdisplay / 2; >> + >> + igt_create_color_fb(display->drm_fd, >> + w_lower, h_lower, >> + DRM_FORMAT_XRGB8888, >> + I915_TILING_NONE, >> + 0.0, 0.0, 0.0, &fb_ref); >> + >> + /* create reference image */ >> + cr = igt_get_cairo_ctx(display->drm_fd, &fb_ref); >> + igt_assert(cairo_status(cr) == 0); >> + igt_paint_color(cr, 0, 0, w_lower, h_lower, 0.0, 0.0, 1.0); >> + igt_paint_color(cr, w_upper / 2, h_upper / 2, w_upper, h_upper, 1.0, 1.0, 0.0); >> + igt_put_cairo_ctx(display->drm_fd, &fb_ref, cr); >> + igt_plane_set_fb(primary, &fb_ref); >> + igt_display_commit2(display, COMMIT_ATOMIC); >> + >> + /* create the pipe_crc object for this pipe */ >> + pipe_crc = igt_pipe_crc_new(pipe->display->drm_fd, pipe->pipe, >> + INTEL_PIPE_CRC_SOURCE_AUTO); >> + >> + /* get reference crc */ >> + igt_pipe_crc_start(pipe_crc); >> + igt_pipe_crc_get_current(display->drm_fd, pipe_crc, &ref_crc); >> + >> + igt_plane_set_fb(primary, NULL); >> + >> + for (int k = 0; k < n_planes; k++) { >> + int zpos; >> + igt_plane_t *temp; >> + >> + temp = &display->pipes[pipe->pipe].planes[k]; >> + >> + if (!igt_plane_has_prop(temp, IGT_PLANE_ZPOS)) >> + continue; >> + >> + assert(zpos < n_planes); > > I'll assume you get this fixed, as suggested by Petri (unspecified zpos > and igt_assert_lt). > Done in v5. >> + >> + zpos = igt_plane_get_prop(temp, IGT_PLANE_ZPOS); >> + plane_ptr[zpos] = temp; >> + } >> + >> + /* >> + * checking only pairs of plane in increasing fashion >> + * to avoid combinatorial explosion >> + */ >> + for (int i = 0; i < n_planes - 1; i++) { >> + int fb_id_lower, fb_id_upper; >> + struct igt_fb fb_lower, fb_upper; >> + igt_plane_t *plane_lower, *plane_upper; >> + >> + if (plane_ptr[i] != NULL) >> + plane_lower = plane_ptr[i]; >> + else >> + continue; >> + >> + while (i < (n_planes - 1)) { >> + if (plane_ptr[i + 1] != NULL) { >> + plane_upper = plane_ptr[i + 1]; >> + break; >> + } else { >> + i++; >> + continue; >> + } >> + } >> + >> + if (i == (n_planes - 1)) >> + break; > > This should be impossible to reach given that the for loop is i < > n_planes - 1 > > You can drop this. Done in v5. >> + >> + if ((plane_lower->type == DRM_PLANE_TYPE_CURSOR) || >> + (plane_upper->type == DRM_PLANE_TYPE_CURSOR)) >> + continue; >> + >> + fb_id_lower = igt_create_color_fb(display->drm_fd, >> + w_lower, h_lower, >> + DRM_FORMAT_XRGB8888, >> + I915_TILING_NONE, >> + 0.0, 0.0, 1.0, &fb_lower); >> + igt_assert(fb_id_lower); >> + >> + fb_id_upper = igt_create_color_fb(display->drm_fd, >> + w_upper, h_upper, >> + DRM_FORMAT_XRGB8888, >> + I915_TILING_NONE, >> + 1.0, 1.0, 0.0, &fb_upper); >> + igt_assert(fb_id_upper); >> + >> + igt_plane_set_position(plane_lower, 0, 0); >> + igt_plane_set_fb(plane_lower, &fb_lower); >> + >> + igt_plane_set_position(plane_upper, w_upper / 2, h_upper / 2); >> + igt_plane_set_fb(plane_upper, &fb_upper); >> + >> + igt_info("Committing with the plane[%d] underneath "\ >> + "plane[%d]\n", i, (i + 1)); >> + igt_display_commit2(display, COMMIT_ATOMIC); >> + igt_pipe_crc_get_current(pipe->display->drm_fd, pipe_crc, &new_crc); >> + >> + igt_assert_crc_equal(&ref_crc, &new_crc); >> + >> + igt_plane_set_fb(plane_lower, NULL); >> + igt_plane_set_fb(plane_upper, NULL); >> + } >> +} >> + >> static void plane_overlay(igt_pipe_t *pipe, igt_output_t *output, igt_plane_t *plane) >> { >> drmModeModeInfo *mode = igt_output_get_mode(output); >> @@ -987,14 +1117,16 @@ igt_main >> plane_primary(pipe_obj, primary, &fb); >> } >> >> - igt_subtest("plane_primary_overlay_zpos") { >> + igt_describe("Verify that the overlay plane can cover the primary one (and "\ >> + "vice versa) by changing their zpos property."); >> + igt_subtest("plane_primary_overlay_mutable_zpos") { >> uint32_t format_primary = DRM_FORMAT_ARGB8888; >> uint32_t format_overlay = DRM_FORMAT_ARGB1555; >> >> igt_plane_t *overlay = >> igt_pipe_get_plane_type(pipe_obj, DRM_PLANE_TYPE_OVERLAY); >> - >> igt_require(overlay); >> + >> igt_require(igt_plane_has_prop(primary, IGT_PLANE_ZPOS)); >> igt_require(igt_plane_has_prop(overlay, IGT_PLANE_ZPOS)); >> >> @@ -1002,8 +1134,15 @@ igt_main >> igt_require(igt_plane_has_format_mod(overlay, format_overlay, 0x0)); >> >> igt_output_set_pipe(output, pipe); >> - plane_primary_overlay_zpos(pipe_obj, output, primary, overlay, >> - format_primary, format_overlay); >> + plane_primary_overlay_mutable_zpos(pipe_obj, output, primary, overlay, >> + format_primary, format_overlay); >> + } >> + >> + igt_describe("Verify the reported zpos property of planes by making sure "\ >> + "only higher zpos planes cover the lower zpos ones."); > > With what you have right now, you are checking planes only 2 by 2, as > opposed to keeping the planes under the "main" plane active, but with a > different color, in order to check for non-linearity between planes' > reported zpos. This would require really broken HW, but this is what the > test is about :s > > To save memory bandwidth, we could have a 2x2 red fb that would be used > by all planes ;) > > I am however OK with merging this now, as it is already a pretty good > test! With the changes suggested by Petri, this patch is: > > Reviewed-by: Martin Peres Thanks! > >> + igt_subtest("plane_immutable_zpos") { >> + igt_output_set_pipe(output, pipe); >> + plane_immutable_zpos(&display, pipe_obj, output); >> } >> >> igt_subtest("test_only") { >> @@ -1011,6 +1150,7 @@ igt_main >> >> test_only(pipe_obj, primary, output); >> } >> + >> igt_subtest("plane_cursor_legacy") { >> igt_plane_t *cursor = >> igt_pipe_get_plane_type(pipe_obj, DRM_PLANE_TYPE_CURSOR); >> > -- ~Swati Sharma _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev