Re: [PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> v2: * Minor functional tweaks and bug fixes
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>

This patch is not just a refactor. It changes a how the code behaves.
Please split it into many patches: one patch replaces code with
equivalent code from the PSR library, and the other patches should each
do one of the actual code changes that this patch does.

> ---
>  tests/kms_frontbuffer_tracking.c | 119 +++++++--------------------
> ------------
>  1 file changed, 19 insertions(+), 100 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index c24e4a8..3a8b754 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -183,7 +183,7 @@ struct {
>  };
>  
>  
> -#define SINK_CRC_SIZE 12
> +#define SINK_CRC_SIZE 6

As Rodrigo mentioned, this is going to break.


>  typedef struct {
>  	char data[SINK_CRC_SIZE];
>  } sink_crc_t;
> @@ -327,28 +327,6 @@ drmModeModeInfo std_1024_mode = {
>  	.name = "Custom 1024x768",
>  };
>  
> -static drmModeModeInfoPtr
> get_connector_smallest_mode(drmModeConnectorPtr c)
> -{
> -	int i;
> -	drmModeModeInfoPtr smallest = NULL;
> -
> -	for (i = 0; i < c->count_modes; i++) {
> -		drmModeModeInfoPtr mode = &c->modes[i];
> -
> -		if (!smallest)
> -			smallest = mode;
> -
> -		if (mode->hdisplay * mode->vdisplay <
> -		    smallest->hdisplay * smallest->vdisplay)
> -			smallest = mode;
> -	}
> -
> -	if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -		smallest = &std_1024_mode;
> -
> -	return smallest;
> -}
> -
>  static drmModeConnectorPtr get_connector(uint32_t id)
>  {
>  	int i;
> @@ -421,30 +399,6 @@ static void init_mode_params(struct
> modeset_params *params, uint32_t crtc_id,
>  	params->sprite.h = 64;
>  }
>  
> -static bool connector_get_mode(drmModeConnectorPtr c,
> drmModeModeInfoPtr *mode)
> -{
> -	*mode = NULL;
> -
> -	if (c->connection != DRM_MODE_CONNECTED || !c->count_modes)
> -		return false;
> -
> -	if (c->connector_type == DRM_MODE_CONNECTOR_eDP &&
> opt.no_edp)
> -		return false;
> -
> -	if (opt.small_modes)
> -		*mode = get_connector_smallest_mode(c);
> -	else
> -		*mode = &c->modes[0];
> -
> -	 /* On HSW the CRC WA is so awful that it makes you think
> everything is
> -	  * bugged. */
> -	if (IS_HASWELL(intel_get_drm_devid(drm.fd)) &&
> -	    c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -		*mode = &std_1024_mode;
> -
> -	return true;
> -}
> -
>  static bool connector_supports_pipe_a(drmModeConnectorPtr connector)
>  {
>  	int i;
> @@ -473,7 +427,7 @@ static bool find_connector(bool edp_only, bool
> pipe_a, uint32_t forbidden_id,
>  			continue;
>  		if (c->connector_id == forbidden_id)
>  			continue;
> -		if (!connector_get_mode(c, &mode))
> +		if (!igt_psr_find_good_mode(c, &mode))

igt_psr_find_good_mode() doesn't seem to do what connector_get_mode()
does, we can't use it. This is going to introduce bugs.


>  			continue;
>  
>  		*ret_connector = c;
> @@ -804,23 +758,6 @@ static void fbc_print_status(void)
>  	igt_info("FBC status:\n%s\n", buf);
>  }
>  
> -static bool psr_is_enabled(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	return strstr(buf, "\nActive: yes\n") &&
> -	       strstr(buf, "\nHW Enabled & Active bit: yes\n");
> -}
> -
> -static void psr_print_status(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	igt_info("PSR status:\n%s\n", buf);
> -}
> -
>  static struct timespec fbc_get_last_action(void)
>  {
>  	struct timespec ret = { 0, 0 };
> @@ -926,44 +863,31 @@ static bool fbc_wait_until_enabled(void)
>  	return igt_wait(fbc_is_enabled(), 2000, 1);
>  }
>  
> -static bool psr_wait_until_enabled(void)
> -{
> -	return igt_wait(psr_is_enabled(), 5000, 1);
> -}
> -
>  #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)
>  
>  static void get_sink_crc(sink_crc_t *crc, bool mandatory)
>  {
> -	int rc, errno_;
> +	int rc;
>  
>  	if (!sink_crc.supported) {
>  		memcpy(crc, "unsupported!", SINK_CRC_SIZE);
>  		return;
>  	}
>  
> -	lseek(sink_crc.fd, 0, SEEK_SET);
> -
> -	rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> -	errno_ = errno;
> -
> -	if (rc == -1 && errno_ == ENOTTY) {
> +	rc = igt_psr_get_sink_crc(sink_crc.fd, crc->data);
> +	if (rc == ENOTTY) {
>  		igt_info("Sink CRC not supported: panel doesn't
> support it\n");

We'll print this twice now.


>  		sink_crc.supported = false;
> -	} else if (rc == -1 && errno_ == ETIMEDOUT) {
> -		if (sink_crc.reliable) {
> -			igt_info("Sink CRC is unreliable on this
> machine.\n");
> +	} else if (rc == ETIMEDOUT) {
> +		if (sink_crc.reliable) 
>  			sink_crc.reliable = false;
> -		}
> +		
>  

There are some problems with white spaces in the lines above: a double
blank line and trailing spaces at the end of lines.


>  		if (mandatory)
>  			igt_skip("Sink CRC is unreliable on this
> machine.\n");
>  	} else {
> -		igt_assert_f(rc != -1, "Unexpected error: %d\n",
> errno_);
> -		igt_assert(rc == SINK_CRC_SIZE);
> +		igt_assert_f(rc == 0, "Unexpected error: %d\n", rc);
>  	}
>  }
>  
> @@ -1180,7 +1104,7 @@ static void disable_features(const struct
> test_mode *t)
>  		return;
>  
>  	fbc_disable();
> -	psr_disable();
> +	igt_psr_disable();
>  }
>  
>  static void *busy_thread_func(void *data)
> @@ -1432,7 +1356,7 @@ static void setup_sink_crc(void)
>  	fill_fb_region(&prim_mode_params.fb, COLOR_PRIM_BG);
>  	set_mode_for_params(&prim_mode_params);
>  
> -	sink_crc.fd = openat(drm.debugfs, "i915_sink_crc_eDP1",
> O_RDONLY);
> +	sink_crc.fd = igt_psr_open_sink_crc(drm.debugfs);
>  	igt_assert_lte(0, sink_crc.fd);
>  
>  	/* Do a first read to try to detect if it's supported. */
> @@ -1547,14 +1471,6 @@ static void teardown_fbc(void)
>  {
>  }
>  
> -static bool psr_sink_has_support(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	return strstr(buf, "Sink_Support: yes\n");
> -}
> -
>  static void setup_psr(void)
>  {
>  	if (get_connector(prim_mode_params.connector_id)-
> >connector_type !=
> @@ -1563,7 +1479,7 @@ static void setup_psr(void)
>  		return;
>  	}
>  
> -	if (!psr_sink_has_support()) {
> +	if (!igt_psr_sink_support(drm.fd)) {
>  		igt_info("Can't test PSR: not supported by
> sink.\n");
>  		return;
>  	}
> @@ -1717,12 +1633,15 @@ static int adjust_assertion_flags(const
> struct test_mode *t, int flags)
>  	}								
> \
>  									
> \
>  	if (flags_ & ASSERT_PSR_ENABLED) {				
> \
> -		if (!psr_wait_until_enabled()) {			
> \
> -			psr_print_status();				
> \
> +		if (!igt_psr_await_status(drm.fd, true)) {		
> \
> +			igt_psr_print_status(drm.fd);		
> 	\
>  			igt_assert_f(false, "PSR disabled\n");	
> 	\
>  		}							
> \
>  	} else if (flags_ & ASSERT_PSR_DISABLED) {			
> \
> -		igt_assert(!psr_wait_until_enabled());		
> 	\
> +		if (!igt_psr_await_status(drm.fd, false)) {		
> \
> +			igt_psr_print_status(drm.fd);               
>     \
> +			igt_assert_f(false, "PSR enabled\n");	
> 	\
> +		}							
> \
>  	}								
> \
>  } while (0)
>  
> @@ -1822,7 +1741,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();
> +		igt_psr_enable();
>  }
>  
>  static void check_test_requirements(const struct test_mode *t)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx