[igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: Fix user space read too slow error

[Juha-Pekka Heikkila]

On 29.11.2019 22.08, Chris Wilson wrote:
> Quoting Juha-Pekka Heikkilä (2019-11-29 19:57:48)
>> On Fri, Nov 29, 2019 at 6:04 PM Chris Wilson <chris at chris-wilson.co.uk> wrote:
>>>
>>> Quoting Juha-Pekka Heikkila (2019-11-29 15:52:45)
>>>> Having crc running continuously cause this test sometime
>>>> fill crc buffer, fix this problem as well as do some generic
>>>> cleanups.
>>>>
>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>>> ---
>>>>   tests/kms_cursor_crc.c | 106 +++++++++++++++++++++++++------------------------
>>>>   1 file changed, 54 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
>>>> index 6c4c457..6982437 100644
>>>> --- a/tests/kms_cursor_crc.c
>>>> +++ b/tests/kms_cursor_crc.c
>>>> @@ -52,7 +52,6 @@ typedef struct {
>>>>          struct igt_fb fb;
>>>>          igt_output_t *output;
>>>>          enum pipe pipe;
>>>> -       igt_crc_t ref_crc;
>>>>          int left, right, top, bottom;
>>>>          int screenw, screenh;
>>>>          int refresh;
>>>> @@ -60,6 +59,9 @@ typedef struct {
>>>>          int cursor_max_w, cursor_max_h;
>>>>          igt_pipe_crc_t *pipe_crc;
>>>>          unsigned flags;
>>>> +       igt_plane_t *primary;
>>>> +       igt_plane_t *cursor;
>>>> +       cairo_surface_t *surface;
>>>>   } data_t;
>>>>
>>>>   #define TEST_DPMS (1<<0)
>>>> @@ -89,23 +91,15 @@ static void draw_cursor(cairo_t *cr, int x, int y, int cw, int ch, double a)
>>>>
>>>>   static void cursor_enable(data_t *data)
>>>>   {
>>>> -       igt_output_t *output = data->output;
>>>> -       igt_plane_t *cursor =
>>>> -               igt_output_get_plane_type(output, DRM_PLANE_TYPE_CURSOR);
>>>> -
>>>> -       igt_plane_set_fb(cursor, &data->fb);
>>>> -       igt_plane_set_size(cursor, data->curw, data->curh);
>>>> -       igt_fb_set_size(&data->fb, cursor, data->curw, data->curh);
>>>> +       igt_plane_set_fb(data->cursor, &data->fb);
>>>> +       igt_plane_set_size(data->cursor, data->curw, data->curh);
>>>> +       igt_fb_set_size(&data->fb, data->cursor, data->curw, data->curh);
>>>>   }
>>>>
>>>>   static void cursor_disable(data_t *data)
>>>>   {
>>>> -       igt_output_t *output = data->output;
>>>> -       igt_plane_t *cursor =
>>>> -               igt_output_get_plane_type(output, DRM_PLANE_TYPE_CURSOR);
>>>> -
>>>> -       igt_plane_set_fb(cursor, NULL);
>>>> -       igt_plane_set_position(cursor, 0, 0);
>>>> +       igt_plane_set_fb(data->cursor, NULL);
>>>> +       igt_plane_set_position(data->cursor, 0, 0);
>>>>   }
>>>>
>>>>   static bool chv_cursor_broken(data_t *data, int x)
>>>> @@ -144,30 +138,40 @@ static bool cursor_visible(data_t *data, int x, int y)
>>>>          return true;
>>>>   }
>>>>
>>>> +static void restore_image(data_t *data)
>>>> +{
>>>> +       cairo_t *cr;
>>>> +
>>>> +       cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb);
>>>> +       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(data->drm_fd, &data->primary_fb, cr);
>>>> +       gem_sync(data->drm_fd, data->primary_fb.gem_handle);
>>>
>>> This is a generic KMS test, gem_sync() is not supported.
>>
>> Idea here was to make certain memory copied to FB is really there. I
>> did ask from Mika for ideas and he suggested changing domain to cause
>> synchronization, in ioctl_wrappers.c at gem_set_domain() comments
>> suggested to use gem_sync() instead.
> 
> Then you are trying to paper over real bugs. Userspace calls dirtyfb to
> declare it has updated the fb, the kernel then has to take care of
> making sure it is coherent prior to the next flip, or worry more if it
> is already on the scanout.

I did talk with Ville about this and Ville said there's no guarantee in 
IGT for this synchronization. I do set FB as dirty in my code because I 
use only one FB, I set this for case when FBC is in use. I'm not 
familiar with workings of gem here but looking at tearing on my screen I 
got the idea when igt_put_cairo_ctx() finishes memory being copied is 
not necessarily yet all copied.

/Juha-Pekka