Re: [igt-dev] [PATCH i-g-t] tests/kms_cursor_legacy: Disable Intel's PSR2 selective fetch in this test

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2021-11-24 at 13:45 +0000, Hogander, Jouni wrote:
> Hello Jose,
> 
> Couple of comments below.
> 
> On Tue, 2021-11-23 at 09:10 -0800, José Roberto de Souza wrote:
> > When doing a primary or sprite plane flip, PSR2 selective fetch code
> > also adds all the planes including cursor that overlaps with the
> > area being updated, so this causes legacy cursor API calls to
> > wait for a pending atomic commit to finish causing tests that do
> > checks with vblank counters.
> > 
> > So here when running in an Intel platform that has PSR2 selective
> > fetch enabled, it will switch to PSR1 before executing the subtests.
> > Because what this whole test mostly wants to do, is check if
> > userspace
> > can do asynchronous cursors updates.
> 
> If the testcase can't be run with PSR2 selective fetch I would expect
> PSR2 selective fetch is either disabled or this testcase is skipped
> rather than switching to PSR1. Do you have some specific reason for
> choosing switching to PSR1?

To have test coverage, what if a platform in CI has all boards with eDP panels that supports PSR2, kms_cursor_legacy will skip for that whole
platform.

> 
> > 
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2346
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Jouni Hogander <jouni.hogander@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c             | 31 +++++++++++++++++++++++++++++++
> >  lib/igt_psr.h             |  2 ++
> >  tests/kms_cursor_legacy.c | 10 ++++++++++
> >  3 files changed, 43 insertions(+)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index 8fa8b192f..554fe243c 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -280,3 +280,34 @@ bool i915_psr2_selective_fetch_check(int drm_fd)
> > 
> >       return ret;
> >  }
> > +
> > +/*
> > + * Check if PSR2 selective fetch is enabled, if yes switch to PSR1
> > and return
> > + * true otherwise return false.
> > + */
> > +bool i915_psr2_selective_fetch_disable(int drm_fd)
> > +{
> > +     int debugfs_fd;
> > +     bool ret = false;
> > +
> > +     if (!is_i915_device(drm_fd))
> > +             return ret;
> > +
> > +     debugfs_fd = igt_debugfs_dir(drm_fd);
> > +     if (psr2_selective_fetch_check(debugfs_fd)) {
> > +             psr_set(drm_fd, debugfs_fd, PSR_MODE_1);
> > +             ret = true;
> > +     }
> > +
> > +     close(debugfs_fd);
> > +     return ret;
> > +}
> 
> This function is named as i915_psr2_selective_fetch_disable. I would
> expect it disables psr2 selective fetch rather than switches to psr1.

Huum true.
maybe i915_psr2_sel_fetch_to_psr1(), do you have any other suggestions?

> 
> > +
> > +void i915_psr2_selective_fetch_restore(int drm_fd)
> > +{
> > +     int debugfs_fd;
> > +
> > +     debugfs_fd = igt_debugfs_dir(drm_fd);
> > +     psr_set(drm_fd, debugfs_fd, PSR_MODE_2_SEL_FETCH);
> > +     close(debugfs_fd);
> > +}
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > index a075f336d..0f60a1011 100644
> > --- a/lib/igt_psr.h
> > +++ b/lib/igt_psr.h
> > @@ -48,5 +48,7 @@ bool psr2_wait_su(int debugfs_fd, uint16_t
> > *num_su_blocks);
> >  void psr_print_debugfs(int debugfs_fd);
> > 
> >  bool i915_psr2_selective_fetch_check(int drm_fd);
> > +bool i915_psr2_selective_fetch_disable(int drm_fd);
> > +void i915_psr2_selective_fetch_restore(int drm_fd);
> > 
> >  #endif
> > diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c
> > index cd2f84984..e2490902e 100644
> > --- a/tests/kms_cursor_legacy.c
> > +++ b/tests/kms_cursor_legacy.c
> > @@ -27,6 +27,7 @@
> > 
> >  #include "i915/gem.h"
> >  #include "igt.h"
> > +#include "igt_psr.h"
> >  #include "igt_rand.h"
> >  #include "igt_stats.h"
> > 
> > @@ -1421,6 +1422,7 @@ igt_main
> >  {
> >       const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> >       igt_display_t display = { .drm_fd = -1 };
> > +     bool intel_psr2_restore = false;
> >       int i;
> > 
> >       igt_fixture {
> > @@ -1428,6 +1430,12 @@ igt_main
> >               kmstest_set_vt_graphics_mode();
> > 
> >               igt_display_require(&display, display.drm_fd);
> > +             /*
> > +              * Not possible to evade vblank after a primary or
> > sprite plane
> > +              * page flip with cursor legacy APIS when Intel's PSR2
> > selective
> > +              * fetch is enabled, so switching PSR1 for this whole
> > test.
> > +              */
> > +             intel_psr2_restore =
> > i915_psr2_selective_fetch_disable(display.drm_fd);
> >       }
> > 
> >       /*Test description for pipe-* and all-pipe-* tests*/
> > @@ -1609,6 +1617,8 @@ igt_main
> >               }
> > 
> >       igt_fixture {
> > +             if (intel_psr2_restore)
> > +                     i915_psr2_selective_fetch_restore(display.drm_f
> > d);
> >               igt_display_fini(&display);
> >       }
> >  }
>