[igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: Fix user space read too slow error
Chris Wilson
chris at chris-wilson.co.uk
Wed Dec 4 17:57:14 UTC 2019
Quoting Ville Syrjälä (2019-12-04 17:50:30)
> 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 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.
>
> Hmm. Looks like set_domain(GTT, GTT) + set_domain(GTT, 0)
> won't even flush the gtt write domain anymore?
Not anymore, it should never have flushed iirc.
The logic was only every about flushing incompatible domains, and would
not have flagged the second as an incompatible request with the current
write_domain.
> 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...
clflush on scanout itself is unserialised with the HW, so whether it is
done immediately or in a worker does not seem material. You can request
a wait again with set-domain, or gem_wait to block on the clflush and
all other rendering. I suppose.
-Chris
More information about the igt-dev
mailing list