Re: [igt-dev] [PATCH i-g-t] tests/kms_async_flips: Add CRC test to make sure the correct fb is being scanned out

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

On Wed, May 05, 2021 at 09:02:42AM +0530, Karthik B S wrote:
> On 3/4/2021 9:20 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Use CRC to confirm that the old fb is no longer being scanned
> > out after the async flip completes. We do this by immediately
> > rendering garbage into the old fb after receiving the flip
> > event. Should fail if someone is improperly using mailbox style
> > flips while claiming to do async flips.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   tests/kms_async_flips.c | 135 ++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 135 insertions(+)
> >
> > diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> > index e397a54b874f..bc0f95b9ebf6 100644
> > --- a/tests/kms_async_flips.c
> > +++ b/tests/kms_async_flips.c
> > @@ -52,6 +52,11 @@ typedef struct {
> >          drmModeConnectorPtr connector;
> >          unsigned long flip_timestamp_us;
> >          double flip_interval;
> > +       igt_pipe_crc_t *pipe_crc;
> > +       igt_crc_t ref_crc;
> > +       int flip_count;
> > +       int frame_count;
> > +       bool flip_pending;
> >   } data_t;
> >
> >   static drmModeConnectorPtr find_connector_for_modeset(data_t *data)
> > @@ -379,6 +384,132 @@ static void test_init(data_t *data)
> >          drmModeFreeResources(res);
> >   }
> >
> > +static void queue_vblank(data_t *data)
> > +{
> > +       drmVBlank wait_vbl = {
> > +               .request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT |
> > +                       kmstest_get_pipe_from_crtc_id(data->drm_fd, data->crtc_id),
> > +               .request.sequence = 1,
> > +               .request.signal = (long)data,
> > +       };
> > +
> > +       igt_assert(drmIoctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &wait_vbl) == 0);
> > +}
> > +
> > +static void vblank_handler_crc(int fd_, unsigned int sequence, unsigned int tv_sec,
> > +                              unsigned int tv_usec, void *_data)
> > +{
> > +       data_t *data = _data;
> > +       igt_crc_t crc;
> > +
> > +       data->frame_count++;
> > +
> > +       igt_pipe_crc_get_single(data->pipe_crc, &crc);
> > +       igt_assert_crc_equal(&data->ref_crc, &crc);
> > +
> > +       /* check again next vblank */
> > +       queue_vblank(data);
> > +}
> > +
> > +static void flip_handler_crc(int fd_, unsigned int sequence, unsigned int tv_sec,
> > +                            unsigned int tv_usec, void *_data)
> > +{
> > +       data_t *data = _data;
> > +
> > +       data->flip_pending = false;
> > +       data->flip_count++;
> > +}
> > +
> > +static void wait_events_crc(data_t *data)
> > +{
> > +       drmEventContext evctx = {
> > +               .version = 2,
> > +               .vblank_handler = vblank_handler_crc,
> > +               .page_flip_handler = flip_handler_crc,
> > +       };
> > +
> > +       while (data->flip_pending) {
> > +               struct pollfd pfd = {
> > +                       .fd = data->drm_fd,
> > +                       .events = POLLIN,
> > +               };
> > +               int ret;
> > +
> > +               ret = poll(&pfd, 1, 2000);
> > +
> > +               switch (ret) {
> > +               case 0:
> > +                       igt_assert_f(0, "Flip Timeout\n");
> > +                       break;
> > +               case 1:
> > +                       ret = drmHandleEvent(data->drm_fd, &evctx);
> > +                       igt_assert(ret == 0);
> > +                       break;
> > +               default:
> > +                       /* unexpected */
> > +                       igt_assert(0);
> > +               }
> > +       }
> > +}
> > +
> > +static unsigned int clock_ms(void)
> > +{
> > +       struct timespec ts;
> > +
> > +       clock_gettime(CLOCK_MONOTONIC, &ts);
> > +
> > +       return ts.tv_sec * 1000 + ts.tv_nsec / 1000000;
> > +}
> > +
> > +static void test_crc(data_t *data)
> > +{
> > +       unsigned int frame = 0;
> > +       unsigned int start;
> > +       int ret;
> > +
> > +       data->flip_count = 0;
> > +       data->frame_count = 0;
> > +       data->flip_pending = false;
> > +
> > +       igt_draw_fill_fb(data->drm_fd, &data->bufs[frame], 0xff0000ff);
> > +       ret = drmModeSetCrtc(data->drm_fd, data->crtc_id, data->bufs[frame].fb_id, 0, 0,
> > +                            &data->connector->connector_id, 1, &data->connector->modes[0]);
> > +       igt_assert_eq(ret, 0);
> > +
> > +       data->pipe_crc = igt_pipe_crc_new(data->drm_fd,
> > +                                         kmstest_get_pipe_from_crtc_id(data->drm_fd, data->crtc_id),
> > +                                         INTEL_PIPE_CRC_SOURCE_AUTO);
> > +
> > +       igt_pipe_crc_start(data->pipe_crc);
> > +       igt_pipe_crc_get_single(data->pipe_crc, &data->ref_crc);
> > +
> > +       queue_vblank(data);
> > +
> > +       start = clock_ms();
> > +
> > +       while (clock_ms() - start < 2000) {
> > +               /* fill the next fb with the expected color */
> > +               igt_draw_fill_fb(data->drm_fd, &data->bufs[frame], 0xff0000ff);
> > +
> > +               data->flip_pending = true;
> > +               ret = drmModePageFlip(data->drm_fd, data->crtc_id, data->bufs[frame].fb_id,
> > +                                     DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT, data);
> > +               igt_assert_eq(ret, 0);
> > +
> > +               wait_events_crc(data);
> > +
> > +               /* clobber the previous fb which should no longer be scanned out */
> > +               frame = !frame;
> 
> Hi,
> 
> As mentioned in the commit message, we are trying to render garbage to 
> the old fb after flip events are completed.
> But here the frame variable is already updated before filling it with 
> garbage.
> This means we're filling this garbage on new fb and then we again fill 
> the same fb with green in the start of the while loop before submitting 
> the next flip?
> Is my understanding above correct? If not, could you please let me know 
> what I'm missing here?

We're trying to see if writing garbage to the fb which is not
supposedly being scanned out results in a crc change. And
before we actually flip back to that fb we need to get rid of
the garbage again and replace it with the correct color.

This should fail if the hardware/driver lies about supporting
async flips and actually fakes it using mailbox flips because
we will end up writing garbage into the current scanout buffer.

> 
> I tried to run it on my local setup by moving the the frame variable 
> update after the fill_fb below, but we see a CRC mismatch in that case.
> So I think I'm missing something here. But not sure what I'm missing.
> 
> Thanks,
> 
> Karthik.B.S
> 
> > +               igt_draw_fill_fb(data->drm_fd, &data->bufs[frame], rand());
> > +       }
> > +
> > +       igt_pipe_crc_stop(data->pipe_crc);
> > +       igt_pipe_crc_free(data->pipe_crc);
> > +
> > +       /* make sure we got at a reasonable number of async flips done */
> > +       igt_assert_lt(data->frame_count * 2, data->flip_count);
> > +}
> > +
> >   igt_main
> >   {
> >          static data_t data;
> > @@ -419,6 +550,10 @@ igt_main
> >                  igt_subtest("invalid-async-flip")
> >                          test_invalid(&data);
> >
> > +               igt_describe("Use CRC to verify async flip scans out the correct framebuffer");
> > +               igt_subtest("crc")
> > +                       test_crc(&data);
> > +
> >                  igt_fixture {
> >                          for (i = 0; i < ARRAY_SIZE(data.bufs); i++)
> >                                  igt_remove_fb(data.drm_fd, &data.bufs[i]);
> > --
> > 2.26.2
> >
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 

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