Re: [igt-dev] [PATCH i-g-t 08/10] lib/psr: Add PSR2 support to the remaning psr functions

Dhinakaran Pandiyan
2019-01-17T02:27:55Z

On Wed, 2019-01-16 at 18:14 -0800, Souza, Jose wrote:
> On Tue, 2019-01-15 at 21:33 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > Add the mode parameter to psr_enable() and psr_sink_support() so
> > > PSR1
> > > and PSR2 can be tested separated.
> > > For now all PSR tests will run only with PSR1 and the tests for
> > > PSR2
> > > will come in the future.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > lib/igt_psr.c | 39 +++++++++++++++++++++++++-
> > > --
> > > --
> > > --
> > > lib/igt_psr.h | 4 ++--
> > > tests/kms_fbcon_fbt.c | 9 ++++++--
> > > tests/kms_frontbuffer_tracking.c | 4 ++--
> > > tests/kms_psr.c | 5 ++--
> > > 5 files changed, 45 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index 5cc0fbc2..d7028f6c 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -83,9 +83,10 @@ static void restore_psr_debugfs(int sig)
> > > psr_write(psr_restore_debugfs_fd, "0");
> > > }
> > >
> > > -static bool psr_set(int debugfs_fd, bool enable)
> > > +static bool psr_set(int debugfs_fd, enum psr_mode mode)
> > > {
> > > int ret;
> > > + const char *debug_val;
> > >
> > > ret = has_psr_debugfs(debugfs_fd);
> > > if (ret == -ENODEV) {
> > > @@ -96,7 +97,18 @@ static bool psr_set(int debugfs_fd, bool
> > > enable)
> > > return false;
> > > }
> > >
> > > - ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > + switch (mode) {
> > > + case PSR_MODE_1:
> > > + debug_val = "0x3";
> > > + break;
> > > + case PSR_MODE_2:
> > > + debug_val = "0x2";
> > > + break;
> > > + default:
> > > + debug_val = "0x1";
> > > + }
> > > +
> > > + ret = psr_write(debugfs_fd, debug_val);
> > > igt_assert(ret > 0);
> > >
> > > /* Restore original value on exit */
> > > @@ -109,23 +121,34 @@ static bool psr_set(int debugfs_fd, bool
> > > enable)
> > > return ret;
> > > }
> > >
> > > -bool psr_enable(int debugfs_fd)
> > > +bool psr_enable(int debugfs_fd, enum psr_mode mode)
> > > {
> > > - return psr_set(debugfs_fd, true);
> > > + return psr_set(debugfs_fd, mode);
> > > }
> > >
> > > bool psr_disable(int debugfs_fd)
> > > {
> > > - return psr_set(debugfs_fd, false);
> > > + /* Any mode different than PSR_MODE_1/2 will disable PSR */
> >
> > Please consider adding PSR_MODE_DISABLE to get rid of this comment.
>
> I have commented in the previous patch, my opinion is that is better
> have this comments and handle this way than let user call all other
> functions with PSR_MODE_DISABLE.
Let's go ahead as is and improve this later.

>
> >
> > > + return psr_set(debugfs_fd, PSR_MODE_2 + 1);
> > > }
> > >
> > > -bool psr_sink_support(int debugfs_fd)
> > > +bool psr_sink_support(int debugfs_fd, enum psr_mode mode)
> > > {
> > > char buf[PSR_STATUS_MAX_LEN];
> > > int ret;
> > >
> > > ret = igt_debugfs_simple_read(debugfs_fd,
> > > "i915_edp_psr_status", buf,
> > > sizeof(buf));
> > > - return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
> > > - strstr(buf, "Sink support: yes"));
> > > + if (ret < 1)
> > > + return false;
> > > +
> > > + if (mode == PSR_MODE_1)
> > > + return strstr(buf, "Sink_Support: yes\n") ||
> > > + strstr(buf, "Sink support: yes");
> > > + else
> > > + /*
> > > + * i915 requires PSR version 0x03 that is PSR2 + SU
> > > with
> > > + * Y-coordinate to support PSR2
> > > + */
> > > + return strstr(buf, "Sink support: yes [0x03]");
> >
> > For some reason, I thought we also print whether the sink supports
> > PSR1
> > or PSR2 in debugfs. Hope it's not too late, did we ever consider
> >
> > Sink support: {No, PSR1, PSR2} [<PSR dpcd version>]?
>
> Okay, I will change kernel and IGT to have this debugfs output.
> We should print it just based on the DPCD version? Like version 0x2
> is
> PSR2 but we don't support PSR2 in that version of panel, same as a
> version 0x3 that do not require the Y-coordinate.
Oh yeah, that can get confusing without the debug message. The version
info from the DPCD is clearer even if not descriptive. Feel free to
leave it as is.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>