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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 29, 2019 at 10:44:35PM +0200, Ville Syrjälä wrote:
> 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@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@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.

Hmm. Looks like set_domain(GTT, GTT) + set_domain(GTT, 0)
won't even flush the gtt write domain anymore? A change introduced
in commit ef74921bc679 ("drm/i915: Combine write_domain flushes
to a single function") if I'm reading the logs correctly.

So I guess we now need the dirtyfb (or sw_finish) ioctl. That
should end up doing the wc/chipset flushes we want.

Though now I'm left wondering how we would do this had we
used the cpu write domain. AFAICS the flush_if_display() thing
will always do the clflush asynchronously, but here we
want a sync flush since we need the display engine to see
the new data asap. I wonder if we have any frontbuffer
rendering tests that can actually hit this...

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev