[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
Fri Aug 17 03:40:42 UTC 2018
On Thu, 2018-08-16 at 13:31 -0700, Dhinakaran Pandiyan wrote:
> 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)
The meaning of this bool return is non-obvious within this library. Add
some documentation perhaps?
> > > > +{
> > > > + 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.
I supposed this will be changed in the next version as there is no use
for interrupts in IGTs
> > > > + */
> > > > + 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");
OCD nitpick, feel free to ignore, it'd be nice to consistently use hex
everywhere.
With these minor changes dealt with, this patch is good.
-DK
> > >
> > > "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