[igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
Dhinakaran Pandiyan
dhinakaran.pandiyan at gmail.com
Thu Aug 16 20:31:34 UTC 2018
On Thu, 2018-08-16 at 13:41 +0200, Maarten Lankhorst wrote:
> Op 16-08-18 om 02:26 schreef Dhinakaran Pandiyan:
> > On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
> > > It's harmful to write to enable_psr at runtime, and the patch
> > > that
> > > allows
> > > us to change i915_edp_psr_debug with the panel running will
> > > require
> > > us
> > > to abandon the module parameter. Hence the userspace change needs
> > > to
> > > be
> > > put in IGT first before we can change it at kernel time.
> > >
> > > Toggling it to debugfs will mean we can skip a modeset when
> > > changing
> > > our
> > > feature set.
> > >
> > > Changes since v1:
> > > - Rebase with the previous patches dropped.
> > > Changes since v2:
> > > - Rebase on top of new api in i915_edp_psr_debug.
> > > Changes since v3:
> > > - Enable IRQ debugging for extra logging.
> > > - Force PSR1 mode. (dhnkrn)
> > > - Move PSR enable/disable functions to lib/igt_psr. (dhnkrn)
> > >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.c
> > > om>
> > > ---
> > > lib/igt_psr.c | 90
> > > +++++++++++++++++++++++++++++++-
> > > lib/igt_psr.h | 2 +
> > > tests/kms_frontbuffer_tracking.c | 6 +--
> > > 3 files changed, 93 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index c979b0b59936..393702037861 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -21,7 +21,9 @@
> > > * IN THE SOFTWARE.
> > > */
> > >
> > > -#include "igt_psr.h"
> > > +#include "igt_psr.h"
> > > +#include "igt_sysfs.h"
> > > +#include <errno.h>
> > >
> > > bool psr_active(int fd, bool check_active)
> > > {
> > > @@ -38,3 +40,89 @@ bool psr_wait_entry(int fd)
> > > {
> > > return igt_wait(psr_active(fd, true), 500, 1);
> > > }
> > > +
> > > +static ssize_t psr_write(int fd, const char *buf)
> > > +{
> > > + return igt_sysfs_write(fd, "i915_edp_psr_debug", buf,
> > > strlen(buf));
> > > +}
> > > +
> > > +static int has_psr_debugfs(int fd)
> > > +{
> > > + int ret;
> > > +
> > > + /*
> > > + * Check if new PSR debugfs api is usable by writing an
> > > invalid value.
> > > + * Legacy mode will return OK here, debugfs api will
> > > return
> > > -EINVAL.
> > > + * -ENODEV is returned.
> > > + */
> > > + ret = psr_write(fd, "0xf");
> > > + if (ret == -EINVAL)
> > > + return 0;
> > > + else if (ret >= 0)
> > > + return -EINVAL;
> >
> > We should disable debug here, PSR interrupts aren't used in IGTs.
> > Looks
> > like you sent this patch before I responded to your question about
> > interrupts.
> >
> >
> > > + else
> > > + return ret;
> > > +}
> > > +
> > > +static bool psr_modparam_set(int val)
> > > +{
> > > + static int oldval = -1;
> > > +
> > > + igt_set_module_param_int("enable_psr", val);
> > > +
> > > + if (val == oldval)
> > > + return false;
> > > +
> > > + oldval = val;
> > > + return true;
> > > +}
> > > +
> > > +static int psr_restore_debugfs_fd = -1;
> > > +
> > > +static void restore_psr_debugfs(int sig)
> > > +{
> > > + psr_write(psr_restore_debugfs_fd, "0");
> > > +}
> > > +
> > > +static bool psr_set(int fd, bool enable)
> > > +{
> > > + int ret;
> > > +
> > > + igt_skip_on_f(fd < 0, "Could not open debugfs dir\n");
> > > +
> > > + ret = has_psr_debugfs(fd);
> > > + if (ret == -ENODEV) {
> > > + igt_skip_on_f(enable, "PSR not available\n");
> > > + return false;
> > > + }
> > > +
> > > + /*
> > > + * Restore original debugfs value on exit, even for
> > > + * legacy case, because writing 0xf will set the debug
> > > flag
> > > + * regardless.
> > > + */
> > > + if (psr_restore_debugfs_fd == -1) {
> > > + psr_restore_debugfs_fd = dup(fd);
> > > + igt_assert(psr_restore_debugfs_fd >= 0);
> > > + igt_install_exit_handler(restore_psr_debugfs);
> > > + }
> > > +
> > > + if (ret == -EINVAL) {
> > > + ret = psr_modparam_set(enable);
> > > + } else {
> > > + ret = psr_write(fd, enable ? "0x13" : "1");
> >
> > "0x3" to avoid enabling interrupts.
> >
> > And am not quite sure how this return translates to need_modeset in
> > the
> > next patch. Why is a modeset needed if psr_write() successfully
> > wrote
> > all the bytes?
>
> Oops, should reset to false in that case.
>
> Any other comments on the series or is it good with those 2 fixes?
I need a bit more time to review patch 2/2, not very familiar with the
flow in frontbuffer_tracking.c
>
> ~Maarten
> > > + igt_assert(ret > 0);
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +bool psr_enable(int fd)
> > > +{
> > > + return psr_set(fd, true);
> > > +}
> > > +
> > > +bool psr_disable(int fd)
> > > +{
> > > + return psr_set(fd, false);
> > > +}
> > > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > > index 980f85e04b3e..0ef22c3d1258 100644
> > > --- a/lib/igt_psr.h
> > > +++ b/lib/igt_psr.h
> > > @@ -30,5 +30,7 @@
> > >
> > > bool psr_wait_entry(int fd);
> > > bool psr_active(int fd, bool check_active);
> > > +bool psr_enable(int fd);
> > > +bool psr_disable(int fd);
> > >
> > > #endif
> > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > b/tests/kms_frontbuffer_tracking.c
> > > index 1dfd7c1cee8d..7ea2f697184d 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -941,8 +941,6 @@ static bool
> > > drrs_wait_until_rr_switch_to_low(void)
> > >
> > > #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
> > > #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> > > -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> > > -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
> > > #define drrs_enable() drrs_set(1)
> > > #define drrs_disable() drrs_set(0)
> > >
> > > @@ -1137,7 +1135,7 @@ static void disable_features(const struct
> > > test_mode *t)
> > > return;
> > >
> > > fbc_disable();
> > > - psr_disable();
> > > + psr_disable(drm.debugfs);
> > > drrs_disable();
> > > }
> > >
> > > @@ -1719,7 +1717,7 @@ static void enable_features_for_test(const
> > > struct test_mode *t)
> > > if (t->feature & FEATURE_FBC)
> > > fbc_enable();
> > > if (t->feature & FEATURE_PSR)
> > > - psr_enable();
> > > + psr_enable(drm.debugfs);
> > > if (t->feature & FEATURE_DRRS)
> > > drrs_enable();
> > > }
>
>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
-DK
More information about the igt-dev
mailing list