Re: [igt-dev] [PATCH i-g-t v2] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare <manasi.d.navare@intel.com>]

On Mon, Apr 08, 2019 at 03:08:05PM +0300, Imre Deak wrote:
> On Fri, Apr 05, 2019 at 03:55:49PM -0700, Manasi Navare wrote:
> > DSC enable gets configured during compute_config and needs
> > a full modeset to force DSC.
> > Sometimes in between the tests, if the initial output is same as the
> > mode being set for DSC then it will not do a full modeset.
> > So we disable the output before forcing a mode with DSC enable.
> > 
> > You need the kernel patch:
> > https://patchwork.freedesktop.org/series/59084/
> > 
> > v2:
> > * Restore the value of force dsc enable after the test and in igt_exit_handler
> > * Add igt_install_exit_handler to restore values on fail/exit
> > 
> > Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> > Cc: Petri Latvala <petri.latvala@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  tests/kms_dp_dsc.c | 133 +++++++++++++++++++++++++++------------------
> >  1 file changed, 81 insertions(+), 52 deletions(-)
> > 
> > diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> > index da93cd74..acaee890 100644
> > --- a/tests/kms_dp_dsc.c
> > +++ b/tests/kms_dp_dsc.c
> > @@ -63,119 +63,147 @@ typedef struct {
> >  	int crtc;
> >  	enum pipe pipe;
> >  	char conn_name[128];
> > +	bool force_dsc_en;
> 
> force_dsc_en_orig?

Sure, I will rename this

> 
> >  } data_t;
> >  
> > +data_t data = {};
> > +
> 
> Maybe make only debugfs_fd and force_dsc_en_orig global to reduce the
> churn? Also init debugfs_fd to -1, so the exit handler knows if it's
> been closed already.

But then for restore I also need data.conn_name. So i thought I would make
the whole thing global.

> 
> >  static inline void manual(const char *expected)
> >  {
> >  	igt_debug_manual_check("all", expected);
> >  }
> >  
> > -static bool is_dp_dsc_supported(data_t *data)
> > +static bool is_dp_dsc_supported(void)
> >  {
> >  	char file_name[128] = {0};
> >  	char buf[512];
> >  
> > -	strcpy(file_name, data->conn_name);
> > +	strcpy(file_name, data.conn_name);
> >  	strcat(file_name, "/i915_dsc_fec_support");
> > -	igt_require(igt_debugfs_simple_read(data->debugfs_fd, file_name, buf,
> > +	igt_require(igt_debugfs_simple_read(data.debugfs_fd, file_name, buf,
> >  					    sizeof(buf)) > 0);
> > -	igt_debugfs_read(data->drm_fd, file_name, buf);
> > +	igt_debugfs_read(data.drm_fd, file_name, buf);
> >  
> >  	return strstr(buf, "DSC_Sink_Support: yes");
> >  }
> >  
> > -static bool is_dp_fec_supported(data_t *data)
> > +static bool is_dp_fec_supported(void)
> >  {
> >  	char file_name[128] = {0};
> >  	char buf[512];
> >  
> > -	strcpy(file_name, data->conn_name);
> > +	strcpy(file_name, data.conn_name);
> >  	strcat(file_name, "/i915_dsc_fec_support");
> > -	igt_debugfs_read(data->drm_fd, file_name, buf);
> > +	igt_debugfs_read(data.drm_fd, file_name, buf);
> >  
> >  	return strstr(buf, "FEC_Sink_Support: yes");
> >  }
> >  
> > -static bool is_dp_dsc_enabled(data_t *data)
> > +static bool is_dp_dsc_enabled(void)
> >  {
> >  	char file_name[128] = {0};
> >  	char buf[512];
> >  
> > -	strcpy(file_name, data->conn_name);
> > +	strcpy(file_name, data.conn_name);
> >  	strcat(file_name, "/i915_dsc_fec_support");
> > -	igt_debugfs_read(data->drm_fd, file_name, buf);
> > +	igt_debugfs_read(data.drm_fd, file_name, buf);
> >  
> >  	return strstr(buf, "DSC_Enabled: yes");
> >  }
> >  
> > -static void force_dp_dsc_enable(data_t *data)
> > +static bool is_force_dsc_enabled(void)
> > +{
> > +	char file_name[128] = {0};
> > +	char buf[512];
> > +
> > +	strcpy(file_name, data.conn_name);
> > +	strcat(file_name, "/i915_dsc_fec_support");
> > +	igt_debugfs_read(data.drm_fd, file_name, buf);
> > +
> > +	return strstr(buf, "Force_DSC_Enable: yes");
> > +}
> > +
> > +static void force_dp_dsc_enable(void)
> >  {
> >  	char file_name[128] = {0};
> >  	int ret;
> >  
> > -	strcpy(file_name, data->conn_name);
> > +	strcpy(file_name, data.conn_name);
> >  	strcat(file_name, "/i915_dsc_fec_support");
> > -	igt_debug ("Forcing DSC enable on %s\n", data->conn_name);
> > -	ret = igt_sysfs_write(data->debugfs_fd, file_name, "1", 1);
> > +	igt_debug ("Forcing DSC enable on %s\n", data.conn_name);
> > +	ret = igt_sysfs_write(data.debugfs_fd, file_name, "1", 1);
> >  	igt_assert_f(ret > 0, "debugfs_write failed");
> >  }
> >  
> > -static void clear_dp_dsc_enable(data_t *data)
> > +static void restore_dp_dsc_enable(void)
> 
> Better named as restore_force_dsc_en()?

Ok

> 
> >  {
> >  	char file_name[128] = {0};
> >  	int ret;
> >  
> 
> You need to check if the sig handler was called multiple times for some
> reason:
> 
> 	if (debugfs_fd < 0)
> 		return;

Ok will add this check here

> 
> > -	strcpy(file_name, data->conn_name);
> > +	strcpy(file_name, data.conn_name);
> >  	strcat(file_name, "/i915_dsc_fec_support");
> > -	igt_debug ("Clearing DSC enable on %s\n", data->conn_name);
> > -	ret = igt_sysfs_write(data->debugfs_fd, file_name, "0", 1);
> > +	igt_debug ("Restoring DSC enable on %s\n", data.conn_name);
> > +	ret = igt_sysfs_write(data.debugfs_fd, file_name,
> > +			      data.force_dsc_en ? "1" : "0", 1);
> 
> 	close(debugfs_fd);
> 	debugfs_fd = -1;

But if I close it here, I cant call restore function in update_display() after the test is completed.
Probably i dont need it there?

> 	
> >  	igt_assert_f(ret > 0, "debugfs_write failed");
> >  }
> >  
> > -static void test_cleanup(data_t *data) {
> > +static void test_cleanup(void) {
> >  	igt_plane_t *primary;
> >  
> > -	if (data->output) {
> > -		primary = igt_output_get_plane_type(data->output,
> > +	if (data.output) {
> > +		primary = igt_output_get_plane_type(data.output,
> >  						    DRM_PLANE_TYPE_PRIMARY);
> >  		igt_plane_set_fb(primary, NULL);
> > -		igt_display_commit(&data->display);
> > -		igt_remove_fb(data->drm_fd, &data->fb_test_pattern);
> > +		igt_display_commit(&data.display);
> > +		igt_remove_fb(data.drm_fd, &data.fb_test_pattern);
> >  	}
> >  }
> >  
> > +static void kms_dp_dsc_exit_handler(int sig) {
> > +
> > +	restore_dp_dsc_enable();
> > +}
> > +
> >  
> >  /*
> >   * Re-probe connectors and do a modeset with DSC
> >   *
> >   */
> > -static void update_display(data_t *data, enum dsc_test_type test_type)
> > +static void update_display(enum dsc_test_type test_type)
> >  {
> >  	igt_plane_t *primary;
> > -	data->mode = igt_output_get_mode(data->output);
> > -	data->connector = data->output->config.connector;
> > +	data.mode = igt_output_get_mode(data.output);
> > +	data.connector = data.output->config.connector;
> >  
> > -	if (data->connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> > -	    data->pipe == PIPE_A) {
> > +	if (data.connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> > +	    data.pipe == PIPE_A) {
> >  		igt_debug("DSC not supported on Pipe A on external DP\n");
> >  		return;
> >  	}
> >  
> > +	/* Disable the output first */
> > +	igt_output_set_pipe(data.output, PIPE_NONE);
> > +	igt_display_commit(&data.display);
> > +
> >  	if (test_type == test_basic_dsc_enable) {
> >  		bool enabled;
> >  
> > -		igt_debug("DSC is supported on %s\n", data->conn_name);
> > -		force_dp_dsc_enable(data);
> > +		igt_debug("DSC is supported on %s\n", data.conn_name);
> > +		force_dp_dsc_enable();
> >  
> > -		igt_create_pattern_fb(data->drm_fd, data->mode->hdisplay,
> > -				      data->mode->vdisplay,
> > +		igt_output_set_pipe(data.output, data.pipe);
> > +		igt_create_pattern_fb(data.drm_fd, data.mode->hdisplay,
> > +				      data.mode->vdisplay,
> >  				      DRM_FORMAT_XRGB8888,
> >  				      LOCAL_DRM_FORMAT_MOD_NONE,
> > -				      &data->fb_test_pattern);
> > -		primary = igt_output_get_plane_type(data->output,
> > +				      &data.fb_test_pattern);
> > +		primary = igt_output_get_plane_type(data.output,
> >  						    DRM_PLANE_TYPE_PRIMARY);
> > -		igt_plane_set_fb(primary, &data->fb_test_pattern);
> > -		igt_display_commit(&data->display);
> > +
> > +		/* Now set the output to the desired mode */
> > +		igt_plane_set_fb(primary, &data.fb_test_pattern);
> > +		igt_display_commit(&data.display);
> >  
> >  		/*
> >  		 * Until we have CRC check support, manually check if RGB test
> > @@ -183,38 +211,37 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> >  		 */
> >  		manual("RGB test pattern without corruption");
> >  
> > -		enabled = is_dp_dsc_enabled(data);
> > -		clear_dp_dsc_enable(data);
> > +		enabled = is_dp_dsc_enabled();
> > +		restore_dp_dsc_enable();
> >  
> >  		igt_assert_f(enabled,
> > -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> > -			     data->conn_name,
> > -			     kmstest_pipe_name(data->pipe));
> > +			     "Default DSC enable failed on Connector: %s Pipe: %s\n",
> > +			     data.conn_name,
> > +			     kmstest_pipe_name(data.pipe));
> >  	} else {
> >  		igt_assert(!"Unknown test type\n");
> >  	}
> >  }
> >  
> > -static void run_test(data_t *data, igt_output_t *output,
> > +static void run_test(igt_output_t *output,
> >  		     enum dsc_test_type test_type)
> >  {
> >  	enum pipe pipe;
> >  
> > -	for_each_pipe(&data->display, pipe) {
> > +	for_each_pipe(&data.display, pipe) {
> >  
> >  		if (igt_pipe_connector_valid(pipe, output)) {
> > -			igt_output_set_pipe(output, pipe);
> > -			data->pipe = pipe;
> > -			data->output = output;
> > -			update_display(data, test_type);
> > -			test_cleanup(data);
> > +			data.pipe = pipe;
> > +			data.output = output;
> > +			update_display(test_type);
> > +			test_cleanup();
> >  		}
> >  	}
> >  }
> >  
> >  igt_main
> >  {
> > -	data_t data = {};
> > +	//data_t data = {};
> 
> Leftover line.

Yes, sorry about that, will clean it up

> 
> >  	igt_output_t *output;
> >  	drmModeRes *res;
> >  	drmModeConnector *connector;
> > @@ -225,6 +252,7 @@ igt_main
> >  		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> >  		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
> >  		kmstest_set_vt_graphics_mode();
> > +		igt_install_exit_handler(kms_dp_dsc_exit_handler);
> 
> Before this you need to actually save the original value, otherwise the
> exit handler may restore an uninitialized value in case it's called
> before you save it.

Since the value is per connector, I cant save it until I have gone through the whole connector type checks.
I will move the igt_install_exit_handler(kms_dp_dsc_exit_handler); to before run_test call then?


> 
> >  		igt_display_require(&data.display, data.drm_fd);
> >  		igt_require(res = drmModeGetResources(data.drm_fd));
> >  	}
> > @@ -245,19 +273,20 @@ igt_main
> >  				sprintf(data.conn_name, "%s-%d",
> >  					kmstest_connector_type_str(connector->connector_type),
> >  					connector->connector_type_id);
> > -				if(!is_dp_dsc_supported(&data)) {
> > +				if(!is_dp_dsc_supported()) {
> >  					igt_debug("DSC not supported on connector %s \n",
> >  						  data.conn_name);
> >  					continue;
> >  				}
> >  				if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort &&
> > -				    !is_dp_fec_supported(&data)) {
> > +				    !is_dp_fec_supported()) {
> >  					igt_debug("DSC cannot be enabled without FEC on %s\n",
> >  						  data.conn_name);
> >  					continue;
> >  				}
> >  				test_conn_cnt++;
> > -				run_test(&data, output, test_basic_dsc_enable);
> > +				data.force_dsc_en = is_force_dsc_enabled();
> > +				run_test(output, test_basic_dsc_enable);
> >  			}
> >  			igt_skip_on(test_conn_cnt == 0);
> >  		}
> 
> Under igt_fixture you close debugfs_fd, but that's not good, since the
> exit handler will be called even during normal process exit, and so the
> exit handler will find unexpectedly that the fd it needs is closed.
> 
> Just leave the closing up to the exit handler as I commented above,
> leaving a comment about this fact under igt_fixture.

Ok will remove the debugfs fd from the igt_fixture()

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