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

[Ville Syrjälä]

On Fri, Nov 29, 2019 at 10:38:16PM +0200, Juha-Pekka Heikkila wrote:
> 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.

We already have the gem_sync() in igt_fb for the cases where we
convert via render/blitter engine. What we seem to be missing is
the set_domain(.write_domain=0) for the cpu gtt path. The dirtyfb
is only done for dumb fbs for whatever reason.

-- 
Ville Syrjälä
Intel