Re: [igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: use flipping instead of frontbuffer

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 31.3.2021 11.25, Juha-Pekka Heikkila wrote:
> On 30.3.2021 17.40, Ville Syrjälä wrote:
>> On Wed, Mar 24, 2021 at 08:30:08PM +0200, Juha-Pekka Heikkila wrote:
>>> take out frontbuffer usage from this test as it may fail this test
>>> on frontbuffer related issues instead of cursor issues
>>>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>>   tests/kms_cursor_crc.c | 98 +++++++++++++++++++++++-------------------
>>>   1 file changed, 54 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
>>> index 13d6156c9..d172d703e 100644
>>> --- a/tests/kms_cursor_crc.c
>>> +++ b/tests/kms_cursor_crc.c
>>> @@ -70,8 +70,8 @@ typedef struct {
>>>   #define TEST_DPMS (1<<0)
>>>   #define TEST_SUSPEND (1<<1)
>>> -#define FRONTBUFFER 0
>>> -#define RESTOREBUFFER 1
>>> +#define HWCURSORBUFFER 0
>>> +#define SWCOMPARISONBUFFER 1
>>>   static void draw_cursor(cairo_t *cr, int x, int y, int cw, int ch, 
>>> double a)
>>>   {
>>> @@ -144,22 +144,16 @@ static bool cursor_visible(data_t *data, int x, 
>>> int y)
>>>       return true;
>>>   }
>>> -static void restore_image(data_t *data)
>>> +static void restore_image(data_t *data, uint32_t buffer)
>>>   {
>>>       cairo_t *cr;
>>> -    igt_display_t *display = &data->display;
>>> -    /* rendercopy stripped in igt using cairo */
>>> -    cr = igt_get_cairo_ctx(data->drm_fd,
>>> -                   &data->primary_fb[FRONTBUFFER]);
>>> +    cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb[buffer]);
>>>       cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
>>>       cairo_set_source_surface(cr, data->surface, 0, 0);
>>>       cairo_rectangle(cr, 0, 0, data->screenw, data->screenh);
>>>       cairo_fill(cr);
>>>       igt_put_cairo_ctx(cr);
>>> -    igt_dirty_fb(data->drm_fd, &data->primary_fb[FRONTBUFFER]);
>>> -    igt_wait_for_vblank(data->drm_fd,
>>> -                display->pipes[data->pipe].crtc_offset);
>>>   }
>>>   static void do_single_test(data_t *data, int x, int y)
>>> @@ -168,13 +162,11 @@ static void do_single_test(data_t *data, int x, 
>>> int y)
>>>       igt_pipe_crc_t *pipe_crc = data->pipe_crc;
>>>       igt_crc_t crc, ref_crc;
>>>       cairo_t *cr;
>>> -    int ret = 0;
>>> +    int ret = 0, hwcursorframe;
>>>       igt_print_activity();
>>>       /* Hardware test */
>>> -    restore_image(data);
>>> -
>>>       igt_plane_set_position(data->cursor, x, y);
>>>       cursor_enable(data);
>>> @@ -182,19 +174,21 @@ static void do_single_test(data_t *data, int x, 
>>> int y)
>>>           ret = igt_display_try_commit2(display, COMMIT_LEGACY);
>>>           igt_assert_eq(ret, -EINVAL);
>>>           igt_plane_set_position(data->cursor, 0, y);
>>> -
>>>           return;
>>>       }
>>>       igt_display_commit(display);
>>> -    /* Extra vblank wait is because nonblocking cursor ioctl */
>>> -    igt_wait_for_vblank(data->drm_fd,
>>> -            display->pipes[data->pipe].crtc_offset);
>>> -    igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
>>> +    igt_plane_set_fb(data->primary, &data->primary_fb[HWCURSORBUFFER]);
>>> +    igt_display_commit(display);
>>> +
>>> +    hwcursorframe = kmstest_get_vblank(data->drm_fd, data->pipe, 0) 
>>> + 1;
>>> +    restore_image(data, SWCOMPARISONBUFFER);
>>>       if (data->flags & (TEST_DPMS | TEST_SUSPEND)) {
>>>           igt_crc_t crc_after;
>>> +        /* Extra vblank wait to build full crc before dpms/suspend */
>>> +        igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
>>>           /*
>>>            * stop/start crc to avoid dmesg notifications about userspace
>>>            * reading too slow.
>>> @@ -220,18 +214,22 @@ static void do_single_test(data_t *data, int x, 
>>> int y)
>>>           igt_assert_crc_equal(&crc, &crc_after);
>>>       }
>>> -    cursor_disable(data);
>>> -
>>>       /* Now render the same in software and collect crc */
>>> -    cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[FRONTBUFFER]);
>>> +    cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[SWCOMPARISONBUFFER]);
>>>       draw_cursor(cr, x, y, data->curw, data->curh, 1.0);
>>>       igt_put_cairo_ctx(cr);
>>> +    igt_plane_set_fb(data->primary, 
>>> &data->primary_fb[SWCOMPARISONBUFFER]);
>>>       igt_display_commit(display);
>>> -    igt_dirty_fb(data->drm_fd, &data->primary_fb[FRONTBUFFER]);
>>> -    /* Extra vblank wait is because nonblocking cursor ioctl */
>>> +
>>> +    cursor_disable(data);
>>> +    igt_display_commit(display);
>>
>> Why the double commit?
> 
> I guess that's not needed. I was just aiming to get frames to settle 
> correctly with flipping on above commit.
> 

On second thought I think this pattern is correct. What happen here is 
there's hwcursor on screen and it need to be there for full frame, that 
first commit blocks until flip is done and after that cursor is removed 
from screen. Then wait for vblank to make certain we got rid of the 
cursor so it doesn't affect crc which then will be collected bit below 
with igt_pipe_crc_get_current(..)

I'm making new version of this patch but I think this part will need to 
stay as is?

>>
>>> +
>>>       igt_wait_for_vblank(data->drm_fd,
>>>               display->pipes[data->pipe].crtc_offset);
>>
>> Why is this still here? Ah, legacy commits means cursor doesn't
>> block. Still a single commit should be fine AFAICS, even if
>> we need to keep the extra vblank waits.
> 
> I think this is needed. Though, I can try it out by failing 
> intentionally and see it failed in expected way. With these changes this 
> test is no more blinking on screen between hw/sw cursor image changes 
> hence I wanted to be certain.
> 
>>
>>> +    if (!(data->flags & (TEST_DPMS | TEST_SUSPEND)))
>>> +        igt_pipe_crc_get_for_frame(data->drm_fd, pipe_crc, 
>>> hwcursorframe, &crc);
>>
>> Why are we not just grabbing the crc earlier like we used to?
> 
> Doing it this way while waiting for crc to be ready that time is spent 
> on restoring sw cursor image. On slowest machines this yields to ~1s won 
> time on subtest execution.
> 
>>
>>> +
>>>       igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
>>>       igt_assert_crc_equal(&crc, &ref_crc);
>>>   }
>>> @@ -244,8 +242,6 @@ static void do_fail_test(data_t *data, int x, int 
>>> y, int expect)
>>>       igt_print_activity();
>>>       /* Hardware test */
>>> -    restore_image(data);
>>> -
>>>       cursor_enable(data);
>>>       igt_plane_set_position(data->cursor, x, y);
>>>       ret = igt_display_try_commit2(display, COMMIT_LEGACY);
>>> @@ -367,8 +363,8 @@ static void cleanup_crtc(data_t *data)
>>>       igt_plane_set_fb(data->primary, NULL);
>>>       igt_display_commit(display);
>>> -    igt_remove_fb(data->drm_fd, &data->primary_fb[FRONTBUFFER]);
>>> -    igt_remove_fb(data->drm_fd, &data->primary_fb[RESTOREBUFFER]);
>>> +    igt_remove_fb(data->drm_fd, &data->primary_fb[HWCURSORBUFFER]);
>>> +    igt_remove_fb(data->drm_fd, &data->primary_fb[SWCOMPARISONBUFFER]);
>>>       igt_display_reset(display);
>>>   }
>>> @@ -389,18 +385,18 @@ static void prepare_crtc(data_t *data, 
>>> igt_output_t *output,
>>>                   DRM_FORMAT_XRGB8888,
>>>                   LOCAL_DRM_FORMAT_MOD_NONE,
>>>                   0.0, 0.0, 0.0,
>>> -                &data->primary_fb[FRONTBUFFER]);
>>> +                &data->primary_fb[HWCURSORBUFFER]);
>>>       igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
>>>                   DRM_FORMAT_XRGB8888,
>>>                   LOCAL_DRM_FORMAT_MOD_NONE,
>>>                   0.0, 0.0, 0.0,
>>> -                &data->primary_fb[RESTOREBUFFER]);
>>> +                &data->primary_fb[SWCOMPARISONBUFFER]);
>>>       data->primary = igt_output_get_plane_type(output, 
>>> DRM_PLANE_TYPE_PRIMARY);
>>>       data->cursor = igt_output_get_plane_type(output, 
>>> DRM_PLANE_TYPE_CURSOR);
>>> -    igt_plane_set_fb(data->primary, &data->primary_fb[FRONTBUFFER]);
>>> +    igt_plane_set_fb(data->primary, 
>>> &data->primary_fb[SWCOMPARISONBUFFER]);
>>>       igt_display_commit(display);
>>> @@ -428,6 +424,10 @@ static void prepare_crtc(data_t *data, 
>>> igt_output_t *output,
>>>       cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
>>>       igt_paint_test_pattern(cr, data->screenw, data->screenh);
>>>       cairo_destroy(cr);
>>> +
>>> +    /* Set HW cusor buffer in place */
>>> +    restore_image(data, HWCURSORBUFFER);
>>> +
>>>       igt_pipe_crc_start(data->pipe_crc);
>>>   }
>>> @@ -447,6 +447,15 @@ static void test_cursor_alpha(data_t *data, 
>>> double a)
>>>                       LOCAL_DRM_FORMAT_MOD_NONE,
>>>                       &data->fb);
>>>       igt_assert(fb_id);
>>> +
>>> +    /* empty buffer */
>>> +    cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[HWCURSORBUFFER]);
>>> +    igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
>>> +            0.0, 0.0, 0.0);
>>> +    igt_put_cairo_ctx(cr);
>>> +    igt_plane_set_fb(data->primary, &data->primary_fb[HWCURSORBUFFER]);
>>> +    igt_display_commit(display);
>>
>> Why are we rendering this again? Shouldn't this fb remaing unchanged
>> throughout the test?
> 
> This is bit of kludge. On arriving here after prepare_crtc(..) 
> HWCURSORBUFFER has testimage which is used in those other tests while 
> SWCURSORBUFFER is here black. This is because HWCURSORBUFFER never need 
> to be redrawn during those other tests.
> 
> Other alternative is to set SWCURSORBUFFER also to testimage in 
> prepare_crtc(..) and keep on restoring it here. I did prefer blacking 
> HWCURSORBUFFER here so appearance of this test stay the same after these 
> changes. It's same story with those other re-renderings below.
> 
>>
>>> +
>>>       cr = igt_get_cairo_ctx(data->drm_fd, &data->fb);
>>>       igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
>>>       igt_put_cairo_ctx(cr);
>>> @@ -462,23 +471,17 @@ static void test_cursor_alpha(data_t *data, 
>>> double a)
>>>       igt_remove_fb(data->drm_fd, &data->fb);
>>>       /* Software Test - render cursor in software, drawn it directly 
>>> on PF */
>>> -    cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[FRONTBUFFER]);
>>> +    cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[SWCOMPARISONBUFFER]);
>>>       igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
>>>       igt_put_cairo_ctx(cr);
>>> -
>>> +    igt_plane_set_fb(data->primary, 
>>> &data->primary_fb[SWCOMPARISONBUFFER]);
>>>       igt_display_commit(display);
>>>       igt_wait_for_vblank(data->drm_fd,
>>> -            display->pipes[data->pipe].crtc_offset);
>>> +                display->pipes[data->pipe].crtc_offset);
>>>       igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
>>>       /* Compare CRC from Hardware/Software tests */
>>>       igt_assert_crc_equal(&crc, &ref_crc);
>>> -
>>> -    /*Clear Screen*/
>>> -    cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[FRONTBUFFER]);
>>> -    igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
>>> -            0.0, 0.0, 0.0);
>>> -    igt_put_cairo_ctx(cr);
>>>   }
>>>   static void test_cursor_transparent(data_t *data)
>>> @@ -582,6 +585,11 @@ static void test_cursor_size(data_t *data)
>>>                     &data->fb);
>>>       igt_assert(fb_id);
>>> +    /* empty buffer */
>>> +    cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[HWCURSORBUFFER]);
>>> +    igt_paint_color(cr, 0, 0, data->screenw, data->screenh, 0.0, 
>>> 0.0, 0.0);
>>> +    igt_put_cairo_ctx(cr);
>>
>> Another one.
>>
>>> +
>>>       /* Use a solid white rectangle as the cursor */
>>>       cr = igt_get_cairo_ctx(data->drm_fd, &data->fb);
>>>       igt_paint_color_alpha(cr, 0, 0, cursor_max_size, 
>>> cursor_max_size, 1.0, 1.0, 1.0, 1.0);
>>> @@ -604,16 +612,18 @@ static void test_cursor_size(data_t *data)
>>>       /* Software test loop */
>>>       for (i = 0, size = cursor_max_size; size >= 64; size /= 2, i++) {
>>>           /* Now render the same in software and collect crc */
>>> -        cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[FRONTBUFFER]);
>>> +        cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[HWCURSORBUFFER]);
>>>           igt_paint_color_alpha(cr, 0, 0, size, size, 1.0, 1.0, 1.0, 
>>> 1.0);
>>>           igt_put_cairo_ctx(cr);
>>
>> and here.
>>
>>> -
>>> +        igt_plane_set_fb(data->primary, 
>>> &data->primary_fb[HWCURSORBUFFER]);
>>>           igt_display_commit(display);
>>> -        igt_wait_for_vblank(data->drm_fd,
>>> -                display->pipes[data->pipe].crtc_offset);
>>>           igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
>>> +
>>>           /* Clear screen afterwards */
>>> -        cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[FRONTBUFFER]);
>>> +        igt_plane_set_fb(data->primary, 
>>> &data->primary_fb[SWCOMPARISONBUFFER]);
>>> +        igt_display_commit(display);
>>> +
>>> +        cr = igt_get_cairo_ctx(data->drm_fd, 
>>> &data->primary_fb[HWCURSORBUFFER]);
>>>           igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
>>>                   0.0, 0.0, 0.0);
>>>           igt_put_cairo_ctx(cr);
>>
>> Hmm. This loop is now a bit confusing. We're flipping back
>> and forth between the two fbs on the primary plane but we
>> only actually care what happens during the sw side of it.
>> Might make sense to combine the two loops, grab the crc for
>> the hw and sw cursor back-to-back, and thus just get
>> rid of the crc[] array entirely.
>>
> 
> probably good idea, I'll see what comes out of that.
> 
> /Juha-Pekka
> 

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev