[igt-dev] [PATCH i-g-t v2] tests/kms_dp_dsc: Force a full modeset when we force dsc enable
Imre Deak
imre.deak at intel.com
Mon Apr 8 12:08:05 UTC 2019
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 at intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare at 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?
> } 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.
> 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()?
> {
> 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;
> - 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;
> 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.
> 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.
> 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.
--Imre
> --
> 2.19.1
>
More information about the igt-dev
mailing list