[igt-dev] [PATCH i-g-t] tests/kms_atomic: add test to validate immutable zpos
Sharma, Swati2
swati2.sharma at intel.com
Thu Mar 12 08:00:19 UTC 2020
On 27-Feb-20 1:01 PM, Martin Peres wrote:
> On 2020-02-27 09:03, Martin Peres wrote:
>> 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?
>
> Oops, forgot to say here that you are relying on the planes to be
> exposed with the same order as the zpos, and never checking it directly.
>
> I propose you create a new plane array here that you will iterate
> through that would order the planes by zpos. Warn about planes without a
> zpos and planes with the same zpos as a current one. Should should skip
> if there are no planes with a zpos.
Done in v2.
>
> Ignore the n^2 loop, even on ARM there aren't that many planes that this
> would become a problem.
>
>>
>>> + 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);
>>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>>
--
~Swati Sharma
More information about the igt-dev
mailing list