[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 at intel.com
Mon Apr 8 19:06:37 UTC 2019
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 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?
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
> >
More information about the igt-dev
mailing list