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 18:28 +0000, Hogander, Jouni wrote:
> On Wed, 2021-11-24 at 13:56 +0000, Souza, Jose wrote:
> > 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.
> 
> Ok, makes sense. Why do you choose to switch to PSR1 instead of just
> disabling PSR?

Switching to PSR1 allows us to execute kms_cursor_legacy in the same matter as without PSR and saves power, also it validates so common code between
PSR1 and PSR2.

> 
> > 
> > > > 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?
> 
> I don't have better suggestions.

will rename to it and add documentation to the functions and re-send.

Thanks for the comments.


> 
> > 
> > > > +
> > > > +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.d
> > > > rm_f
> > > > d);
> > > >               igt_display_fini(&display);
> > > >       }
> > > >  }
>