[igt-dev] [PATCH i-g-t] tests/kms_atomic: add test to validate immutable zpos
Martin Peres
martin.peres at linux.intel.com
Thu Feb 27 07:03:10 UTC 2020
On 2020-02-26 15:45, Swati Sharma wrote:
> i915 implements immutable zpos property whereas the existing test
> case is written to validate mutable zpos.
>
> Added new test case to validate immutable zpos and skip existing
> test case if i915 driver is not detected.
>
> Issue: https://gitlab.freedesktop.org/drm/intel/issues/404
> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> ---
> tests/kms_atomic.c | 120 ++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 112 insertions(+), 8 deletions(-)
>
> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> index 8462d128..7a5edc8e 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);
> @@ -358,6 +358,97 @@ 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;
> + int n_planes;
> + uint32_t format;
> + struct igt_fb fb_ref;
> + igt_plane_t *primary;
> + drmModeModeInfo *mode;
> + igt_pipe_crc_t *pipe_crc;
> + igt_crc_t ref_crc, new_crc;
> + igt_plane_t *plane_lower, *plane_upper;
> + uint32_t w_lower, h_lower, w_upper, h_upper;
> +
> + n_planes = pipe->n_planes;
> + 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;
> +
> + if (intel_gen(display->drm_fd) == 3)
> + format = DRM_FORMAT_RGB565;
> + else
> + format = DRM_FORMAT_ARGB8888;
Let's not make this test intel-only. There should be a function in IGT
to pick an available format supported by IGT (no matter which one).
> +
> + igt_create_color_fb(display->drm_fd,
> + w_lower, h_lower,
> + format, I915_TILING_NONE,
> + 0.0, 0.0, 0.0, &fb_ref);
> +
> + 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);
Isn't that something the other zpos function do too? Maybe extracting it
into a function would be good? Or make the plane_zpos function take a
parameter (immutable / mutable)?
> +
> + /* 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);
Space / tab issue here?
> +
> + igt_plane_set_fb(primary, NULL);
> +
A comment here explaining that we want to avoid combinatorial explosion
and thus only check pairs of planes in an increasing fashion?
> + for (int i = 0; i < n_planes - 1; i++) {
> + struct igt_fb fb[2];
> + plane_lower = &display->pipes[pipe->pipe].planes[i];
> + plane_upper = &display->pipes[pipe->pipe].planes[i + 1];
> +
> + igt_require(igt_plane_has_prop(plane_lower, IGT_PLANE_ZPOS));
> + igt_require(igt_plane_has_prop(plane_upper, IGT_PLANE_ZPOS));
Isn't require leading to a skip if one plane is not supporting the zpos?
Shouldn't we just continue instead?
> +
> + if ((plane_lower->type == DRM_PLANE_TYPE_CURSOR) ||
> + (plane_upper->type == DRM_PLANE_TYPE_CURSOR))
> + continue;
Why special case the cursor plane? Because of its size?
> +
> + igt_create_color_fb(display->drm_fd, w_lower, h_lower,
> + format, I915_TILING_NONE,
> + 0.0, 0.0, 1.0, &fb[0]);
> +
> + igt_create_color_fb(display->drm_fd, w_upper, h_upper,
> + format, I915_TILING_NONE,
> + 1.0, 1.0, 0.0, &fb[1]);
> +
> + igt_plane_set_position(plane_lower, 0, 0);
> + igt_plane_set_fb(plane_lower, &fb[0]);
> +
> + igt_plane_set_position(plane_upper, w_upper / 2, h_upper / 2);
> + igt_plane_set_fb(plane_upper, &fb[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);
Just a though, since the lower plane is supposed to always cover
anything under, we could set all the planes under the lower plane to a
solid red color. This would make sure that lower planes have no
influence over upper planes.
What do you think?
> +
> + 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 +1078,20 @@ igt_main
> plane_primary(pipe_obj, primary, &fb);
> }
>
> - igt_subtest("plane_primary_overlay_zpos") {
> + igt_subtest("plane_primary_overlay_mutable_zpos") {
> + /*
> + * Since i915 driver doesn't support mutable zpos;
> + * skipping.
> + */
> + igt_require(!is_i915_device(display.drm_fd));
> +
Let it fail / skip, no need to encode in IGT the capabilities of i915.
> 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 +1099,14 @@ 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_subtest("plane_immutable_zpos") {
> + igt_require(is_i915_device(display.drm_fd));
> + igt_output_set_pipe(output, pipe);
> + plane_immutable_zpos(&display, pipe_obj, output);
> }
Documentation missing for both the plane_immutable_zpos and
plane_primary_overlay_mutable_zpos. Since you are now the expert on
this, it would be nice for you to explain what the tests do as explained
in
https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Core.html#igt-describe
. Something like "Test that the reported zpos of a plane is correct by
making sure a full-screen plane covers all other planes with a lower
zpos, and the plane with the next available zpos is indeed partially
covering the full-screen plane".
Otherwise, it looks pretty good. Looks more minor improvements needed
rather than anything big. Well done!
Martin
>
> igt_subtest("test_only") {
> @@ -1011,6 +1114,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);
>
More information about the igt-dev
mailing list