[igt-dev] [RFC PATCH 1/2] tests/kms_vrr: Use atomic API for page flip

Wed Aug 5 07:59:50 UTC 2020

Hi

> -----Original Message-----
> From: Kazlauskas, Nicholas <nicholas.kazlauskas at amd.com>
> Sent: Tuesday, August 4, 2020 9:41 PM
> To: Navare, Manasi D <manasi.d.navare at intel.com>; Modem, Bhanuprakash
> <bhanuprakash.modem at intel.com>
> Cc: harry.wentland at amd.com; igt-dev at lists.freedesktop.org; Kiran, Pichika
> Uday <pichika.uday.kiran at intel.com>
> Subject: Re: [RFC PATCH 1/2] tests/kms_vrr: Use atomic API for page flip
> 
> Sorry for the delay on checking this.
> 
> I've given it a spin and on Raven/Navi I see that before applying the
> patches I get SUCCESS for all 3 subtests but after applying them every
> test fails.
> 
> Patch 1 is what breaks it, but it's hard to say whether this is a test
> bug or AMDGPU driver bug from initial investigation.
> 
> I'm seeing that the vblank_ns = last_vblank_ns so I think we're still on
> the same frame when we get the event back.
> 
> I would think this would affect userspace behavior but adaptive sync
> works fine on this kernel for games and benchmarks.
> 
> Not sure if what I was doing with wait_for_vblank works is actually
> waiting for the page flip event from the driver or just the vblank
> event. We want to be comparing the delta between the page-flip events.
[Bhanu] That’s true, wait_for_vblank() is waiting for vblank event and I'll fix this in next revision.
I am not sure this can cause the failure, "delta b/w the page-flip events" vs "delta b/w the vblank events"
> 
> Regards,
> Nicholas Kazlauskas
> 
> On 2020-08-03 6:17 p.m., Navare, Manasi wrote:
> >
> > @Harry and @Nicholas, could you run kms_vrr IGT with this patch on AMD
> > to see that with teh atomic API, it doesnt break VRR on AMD?
> >
> > Regards
> > Manasi
> >
> > On Wed, Jul 01, 2020 at 09:16:59PM -0700, Modem, Bhanuprakash wrote:
> >>> -----Original Message-----
> >>> From: Navare, Manasi D <manasi.d.navare at intel.com>
> >>> Sent: Wednesday, June 3, 2020 12:41 AM
> >>> To: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>
> >>> Cc: igt-dev at lists.freedesktop.org; harry.wentland at amd.com;
> >>> nicholas.kazlauskas at amd.com; Kiran, Pichika Uday
> >>> <pichika.uday.kiran at intel.com>
> >>> Subject: Re: [RFC PATCH 1/2] tests/kms_vrr: Use atomic API for page
> flip
> >>>
> >>> On Mon, May 11, 2020 at 11:56:46AM +0530, bhanuprakash.modem at intel.com
> >>> wrote:
> >>>> From: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> >>>>
> >>>> We should avoid using drmModePageFlip as it'll only be used for
> >>>> legacy drivers, instead, use igt_display_commit_atomic() API to
> >>>> page flip for atomic display code path.
> >>>>
> >>>> Cc: Harry Wentland <harry.wentland at amd.com>
> >>>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> >>>> Cc: Manasi Navare <manasi.d.navare at intel.com>
> >>>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> >>>
> >>> I have tested this on i915 and the page flips are captured correctly
> with
> >>> non blocking page flip requests using atomic_commit call.
> >>>
> >>> @Harry, @Nicholas could you test this on AMD driver to make
> >>> sure its not breaking anything?
> >> [Bhanu]
> >> @Harry, @Nicholas did you get a chance to check this patch?
> >>>
> >>> Manasi
> >>>
> >>>> ---
> >>>>   tests/kms_vrr.c | 39 ++++++++++-----------------------------
> >>>>   1 file changed, 10 insertions(+), 29 deletions(-)
> >>>>
> >>>> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
> >>>> index 73115fef..0fe28931 100644
> >>>> --- a/tests/kms_vrr.c
> >>>> +++ b/tests/kms_vrr.c
> >>>> @@ -126,11 +126,11 @@ static range_t get_vrr_range(data_t *data,
> >>> igt_output_t *output)
> >>>>   }
> >>>>
> >>>>   /* Returns a suitable vrr test frequency. */
> >>>> -static uint32_t get_test_rate_ns(data_t *data, igt_output_t *output)
> >>>> +static uint64_t get_test_rate_ns(data_t *data, igt_output_t *output)
> >>>>   {
> >>>>   drmModeModeInfo *mode = igt_output_get_mode(output);
> >>>>   range_t range;
> >>>> -uint32_t vtest;
> >>>> +uint64_t vtest;
> >>>>
> >>>>   /*
> >>>>    * The frequency with the fastest convergence speed should be
> >>>> @@ -210,32 +210,18 @@ wait_for_vblank(data_t *data, enum pipe pipe)
> >>>>   return get_vblank_event_ns(data);
> >>>>   }
> >>>>
> >>>> -/* Performs an asynchronous non-blocking page-flip on a pipe. */
> >>>> -static int
> >>>> +/* Performs an atomic non-blocking page-flip on a pipe. */
> >>>> +static void
> >>>>   do_flip(data_t *data, enum pipe pipe_id, igt_fb_t *fb)
> >>>>   {
> >>>> -igt_pipe_t *pipe = &data->display.pipes[pipe_id];
> >>>> -int ret;
> >>>> -
> >>>>   igt_set_timeout(1, "Scheduling page flip\n");
> >>>>
> >>>> -/*
> >>>> - * Only the legacy flip ioctl supports async flips.
> >>>> - * It's also non-blocking, but returns -EBUSY if flipping too fast.
> >>>> - * 2x monitor tests will need async flips in the atomic API.
> >>>> - */
> >>>> -do {
> >>>> -ret = drmModePageFlip(data->drm_fd, pipe->crtc_id,
> >>>> -      fb->fb_id,
> >>>> -      DRM_MODE_PAGE_FLIP_EVENT |
> >>>> -      DRM_MODE_PAGE_FLIP_ASYNC,
> >>>> -      data);
> >>>> -} while (ret == -EBUSY);
> >>>> -
> >>>> -igt_assert_eq(ret, 0);
> >>>> -igt_reset_timeout();
> >>>> +igt_display_commit_atomic(&data->display,
> >>>> +  DRM_MODE_ATOMIC_NONBLOCK |
> >>>> +  DRM_MODE_PAGE_FLIP_EVENT,
> >>>> +  NULL);
> >>>>
> >>>> -return 0;
> >>>> +igt_reset_timeout();
> >>>>   }
> >>>>
> >>>>   /*
> >>>> @@ -246,11 +232,6 @@ do_flip(data_t *data, enum pipe pipe_id,
> igt_fb_t
> >>> *fb)
> >>>>    * can arbitrarily restrict the bounds further than the absolute
> >>>>    * min and max range. But VRR is really about extending the flip
> >>>>    * to prevent stuttering or to match a source content rate.
> >>>> - *
> >>>> - * The only way to "present" at a fixed rate like userspace in a
> vendor
> >>>> - * neutral manner is to do it with async flips. This avoids the need
> >>>> - * to wait for next vblank and it should eventually converge at the
> >>>> - * desired rate.
> >>>>    */
> >>>>   static uint32_t
> >>>>   flip_and_measure(data_t *data, igt_output_t *output, enum pipe
> pipe,
> >>>> @@ -271,7 +252,7 @@ flip_and_measure(data_t *data, igt_output_t
> *output,
> >>> enum pipe pipe,
> >>>>   front = !front;
> >>>>   do_flip(data, pipe, front ? &data->fb1 : &data->fb0);
> >>>>
> >>>> -vblank_ns = get_vblank_event_ns(data);
> >>>> +vblank_ns = wait_for_vblank(data, pipe);
> >>>>   diff_ns = rate_ns - (vblank_ns - last_vblank_ns);
> >>>>   last_vblank_ns = vblank_ns;
> >>>>
> >>>> --
> >>>> 2.24.1.1.gb6d4d82bd5
> >>>>