[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 22:17:40 UTC 2019
Thanks for your feedback Imre, I will make the necessary changes
and split these into two separate patches.
Regards
Manasi
On Mon, Apr 08, 2019 at 11:39:27PM +0300, Imre Deak wrote:
> On Mon, Apr 08, 2019 at 12:06:37PM -0700, Manasi Navare wrote:
> > 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.
>
> I'd still avoid the churn (making the review easier) by adding a new
> global
>
> int force_dsc_restore_fd = -1;
>
> using it for restoring the global force_dsc_en_orig. By doing that you
> won't need conn_name in the exit handler, see below.
>
> > > > 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);
>
> Instead of the above:
>
> static void restore_force_dsc_en(void)
> {
> if (force_dsc_restore_fd < 0)
> return;
>
> igt_debug("Restoring DSC enable\n");
> igt_assert(write(force_dsc_restore_fd, force_dsc_en_orig ? "1" : "0", 1) == 1);
>
> close(force_dsc_restore_fd);
> force_dsc_restore_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?
>
> Yes it's needed, we want to do the restore for each connector.
>
> >
> > > > 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);
>
> Here call a new save_force_dsc_en() function containing:
>
> force_dsc_en_orig = is_force_dsc_enabled(data);
> strcpy(file_name, data->conn_name);
> strcat(file_name, "/i915_dsc_fec_support");
> force_dsc_restore_fd = openat(igt_debugfs_dir(data->drm_fd),
> file_name, O_WRONLY);
> igt_assert(force_dsc_restore_fd >= 0);
>
>
> > > > + 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?
>
> It can stay here based on the above comments.
>
> > >
> > > > 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()
>
> Based on the above you'll still need to close here debugfs_fd, as we'll
> now use a separate fd for restoring.
>
> >
> > Manasi
> > >
> > > --Imre
> > >
> > > > --
> > > > 2.19.1
> > > >
More information about the igt-dev
mailing list