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

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7530 bytes --]

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.

> 
> > +	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.

> 
> >  }
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > index 4fff77ec..7e7017bf 100644
> > --- a/lib/igt_psr.h
> > +++ b/lib/igt_psr.h
> > @@ -37,8 +37,8 @@ enum psr_mode {
> >  
> >  bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
> >  bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
> > -bool psr_enable(int debugfs_fd);
> > +bool psr_enable(int debugfs_fd, enum psr_mode);
> >  bool psr_disable(int debugfs_fd);
> > -bool psr_sink_support(int debugfs_fd);
> > +bool psr_sink_support(int debugfs_fd, enum psr_mode);
> >  
> >  #endif
> > diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> > index 2823b47a..9d0d5a36 100644
> > --- a/tests/kms_fbcon_fbt.c
> > +++ b/tests/kms_fbcon_fbt.c
> > @@ -199,6 +199,11 @@ static bool psr_wait_until_enabled(int
> > debugfs_fd)
> >  	return r;
> >  }
> >  
> > +static bool psr_supported_on_chipset(int debugfs_fd)
> > +{
> > +	return psr_sink_support(debugfs_fd, PSR_MODE_1);
> > +}
> > +
> >  static void disable_features(int debugfs_fd)
> >  {
> >  	igt_set_module_param_int("enable_fbc", 0);
> > @@ -212,7 +217,7 @@ static inline void fbc_modparam_enable(int
> > debugfs_fd)
> >  
> >  static inline void psr_debugfs_enable(int debugfs_fd)
> >  {
> > -	psr_enable(debugfs_fd);
> > +	psr_enable(debugfs_fd, PSR_MODE_1);
> >  }
> >  
> >  struct feature {
> > @@ -226,7 +231,7 @@ struct feature {
> >  	.connector_possible_fn = connector_can_fbc,
> >  	.enable = fbc_modparam_enable,
> >  }, psr = {
> > -	.supported_on_chipset = psr_sink_support,
> > +	.supported_on_chipset = psr_supported_on_chipset,
> >  	.wait_until_enabled = psr_wait_until_enabled,
> >  	.connector_possible_fn = connector_can_psr,
> >  	.enable = psr_debugfs_enable,
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index ed9a039a..609f7b41 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1425,7 +1425,7 @@ static void setup_psr(void)
> >  		return;
> >  	}
> >  
> > -	if (!psr_sink_support(drm.debugfs)) {
> > +	if (!psr_sink_support(drm.debugfs, PSR_MODE_1)) {
> >  		igt_info("Can't test PSR: not supported by sink.\n");
> >  		return;
> >  	}
> > @@ -1722,7 +1722,7 @@ static bool enable_features_for_test(const
> > struct test_mode *t)
> >  	if (t->feature & FEATURE_FBC)
> >  		fbc_enable();
> >  	if (t->feature & FEATURE_PSR)
> > -		ret = psr_enable(drm.debugfs);
> > +		ret = psr_enable(drm.debugfs, PSR_MODE_1);
> >  	if (t->feature & FEATURE_DRRS)
> >  		drrs_enable();
> >  
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index 23d000bd..c31dc317 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -191,7 +191,8 @@ static void fill_render(data_t *data, uint32_t
> > handle, unsigned char color)
> >  
> >  static bool sink_support(data_t *data)
> >  {
> > -	return data->with_psr_disabled || psr_sink_support(data-
> > > debugfs_fd);
> > +	return data->with_psr_disabled ||
> > +	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> >  }
> >  
> >  static bool psr_wait_entry_if_enabled(data_t *data)
> > @@ -410,7 +411,7 @@ int main(int argc, char *argv[])
> >  		data.devid = intel_get_drm_devid(data.drm_fd);
> >  
> Store it in struct data so that the value stays consistent throughout
> the test case?
> >  		if (!data.with_psr_disabled)
> > -			psr_enable(data.debugfs_fd);
> > +			psr_enable(data.debugfs_fd, PSR_MODE_1);
> >  
> >  		igt_require_f(sink_support(&data),
> >  			      "Sink does not support PSR\n");

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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