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

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

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>


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev