[igt-dev] [v4 PATCH i-g-t 1/2] tests/i915/kms_flip_scaled_crc: Convert tests to dynamic
Petri Latvala
petri.latvala at intel.com
Wed Jun 22 09:56:20 UTC 2022
On Wed, Jun 22, 2022 at 01:13:47PM +0530, Swati Sharma wrote:
> Convert the existing subtests to dynamic subtests at pipe/output level.
>
> v2: Refactored code in such a manner that once test has been run
> successfully on chosen pipe this pipe will not be tested again
> as this test is testing pipe feature. No need to be run
> separately for each connector using all pipes.
> v3: Changed seq to avoid modetoset as a dangling ptr
> v4: On eDP 1080p with 144Hz vrefresh, test fails with cdclk
> lim, however it passes with 90Hz vrefresh. Added the check
> to handle this scenario.
>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Signed-off-by: Swati Sharma <swati2.sharma at intel.com>
> ---
> tests/i915/kms_flip_scaled_crc.c | 120 +++++++++++++------------------
> 1 file changed, 51 insertions(+), 69 deletions(-)
>
> diff --git a/tests/i915/kms_flip_scaled_crc.c b/tests/i915/kms_flip_scaled_crc.c
> index 9740715f..208b748a 100644
> --- a/tests/i915/kms_flip_scaled_crc.c
> +++ b/tests/i915/kms_flip_scaled_crc.c
> @@ -163,8 +163,6 @@ const struct {
> },
> };
>
> -enum subrval {CONNECTORFAIL, CONNECTORSUCCESS, TESTSKIP, NOREQUESTEDFORMATONPIPE};
> -
> static void setup_fb(data_t *data, struct igt_fb *newfb, uint32_t width,
> uint32_t height, uint64_t format, uint64_t modifier)
> {
> @@ -220,12 +218,12 @@ static void clear_lut(data_t *data, enum pipe pipe)
> igt_pipe_obj_set_prop_value(pipe_obj, IGT_CRTC_GAMMA_LUT, 0);
> }
>
> -static enum subrval test_flip_to_scaled(data_t *data, uint32_t index,
> - enum pipe pipe, igt_output_t *output)
> +static void test_flip_to_scaled(data_t *data, uint32_t index,
> + enum pipe pipe, igt_output_t *output,
> + drmModeModeInfoPtr modetoset)
> {
> igt_plane_t *primary;
> igt_crc_t small_crc, big_crc;
> - drmModeModeInfoPtr modetoset = NULL;
> struct drm_event_vblank ev;
> int ret;
>
> @@ -254,14 +252,15 @@ static enum subrval test_flip_to_scaled(data_t *data, uint32_t index,
> data->big_fb.height);
> }
>
> + if (modetoset)
> + igt_output_override_mode(output, modetoset);
> igt_output_set_pipe(output, pipe);
Double checked what override_mode is doing and whether it's valid to
call before set_pipe; it is, and does the right thing.
Add a comment here that explains that setting a pipe invalidates the
modetoset pointer so no one will use it later. Assign it to NULL also.
>
> primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> - if (!igt_plane_has_format_mod(primary, data->small_fb.drm_format,
> + igt_skip_on_f (!igt_plane_has_format_mod(primary, data->small_fb.drm_format,
^^^^^^
Remove this extra space.
> data->small_fb.modifier) ||
> - !igt_plane_has_format_mod(primary, data->big_fb.drm_format,
> - data->big_fb.modifier))
> - return NOREQUESTEDFORMATONPIPE;
> + !igt_plane_has_format_mod(primary, data->big_fb.drm_format,
> + data->big_fb.modifier), "No requested format/modifier on pipe %s\n", kmstest_pipe_name(pipe));
>
> set_lut(data, pipe);
> igt_display_commit_atomic(&data->display, DRM_MODE_ATOMIC_ALLOW_MODESET,
> @@ -273,42 +272,16 @@ static enum subrval test_flip_to_scaled(data_t *data, uint32_t index,
> data->pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
> INTEL_PIPE_CRC_SOURCE_AUTO);
>
> - for (int i = 0; i < output->config.connector->count_modes; i++) {
> - if (output->config.connector->modes[i].hdisplay == data->attemptmodewidth &&
> - output->config.connector->modes[i].vdisplay == data->attemptmodeheight) {
> - if (modetoset &&
> - modetoset->vrefresh < output->config.connector->modes[i].vrefresh)
> - continue;
> -
> - modetoset = &output->config.connector->modes[i];
> - }
> - }
> -
> - if (!modetoset)
> - igt_debug("%dp mode was not found from connector, will try with default. This may cause cdclk to fail this test on this connector.\n",
> - data->attemptmodeheight);
> - else
> - igt_output_override_mode(output, modetoset);
> -
> igt_plane_set_position(primary, 0, 0);
> igt_plane_set_fb(primary, &data->small_fb);
> igt_plane_set_size(primary, data->attemptmodewidth,
> data->attemptmodeheight);
> ret = igt_display_try_commit_atomic(&data->display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> - switch (ret) {
> - case -ERANGE:
> - igt_debug("Platform scaling limits exceeded, skipping.\n");
> - return TESTSKIP;
> - case -EINVAL:
> - if (!modetoset) {
> - igt_debug("No %dp and default mode too big, cdclk limits exceeded. Check next connector\n",
> - data->attemptmodeheight);
> - return CONNECTORFAIL;
> - }
> - /* fallthrough */
> - default:
> - igt_assert_eq(ret, 0);
> - }
> +
> + igt_skip_on_f(ret == -ERANGE, "Platform scaling limits exceeded, skipping.\n");
> + igt_skip_on_f((ret == -EINVAL) && !modetoset, "No %dp and default mode too big, cdclk limits exceeded. Check next connector\n",
> + data->attemptmodeheight);
Reword this skip line, since we're testing the attemptmode and default
mode separately now.
> + igt_assert_eq(ret, 0);
>
> igt_pipe_crc_start(data->pipe_crc);
> igt_pipe_crc_get_current(data->drm_fd, data->pipe_crc, &small_crc);
> @@ -320,20 +293,10 @@ static enum subrval test_flip_to_scaled(data_t *data, uint32_t index,
> DRM_MODE_ATOMIC_ALLOW_MODESET |
> DRM_MODE_PAGE_FLIP_EVENT, NULL);
>
> - switch (ret) {
> - case -ERANGE:
> - igt_debug("Platform scaling limits exceeded, skipping.\n");
> - return TESTSKIP;
> - case -EINVAL:
> - if (!modetoset) {
> - igt_debug("No %dp and default mode too big, cdclk limits exceeded. Check next connector\n",
> - data->attemptmodeheight);
> - return CONNECTORFAIL;
> - }
> - /* fallthrough */
> - default:
> - igt_assert_eq(ret, 0);
> - }
> + igt_skip_on_f(ret == -ERANGE, "Platform scaling limits exceeded, skipping.\n");
> + igt_skip_on_f((ret == -EINVAL) && (!modetoset || modetoset->vrefresh > 90), "No %dp and default mode too big, cdclk limits exceeded. Check next connector\n",
> + data->attemptmodeheight);
Same here.
> + igt_assert_eq(ret, 0);
>
> igt_assert(read(data->drm_fd, &ev, sizeof(ev)) == sizeof(ev));
>
> @@ -349,8 +312,24 @@ static enum subrval test_flip_to_scaled(data_t *data, uint32_t index,
> igt_output_set_pipe(output, PIPE_NONE);
> igt_plane_set_fb(primary, NULL);
> igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +}
> +
> +static drmModeModeInfoPtr find_mode(data_t *data, igt_output_t *output)
> +{
> + drmModeModeInfoPtr modetoset = NULL;
> +
> + for (int i = 0; i < output->config.connector->count_modes; i++) {
> + if (output->config.connector->modes[i].hdisplay == data->attemptmodewidth &&
> + output->config.connector->modes[i].vdisplay == data->attemptmodeheight) {
> + if (modetoset &&
> + modetoset->vrefresh < output->config.connector->modes[i].vrefresh)
> + continue;
>
> - return CONNECTORSUCCESS;
> + modetoset = &output->config.connector->modes[i];
> + }
> + }
> +
> + return modetoset;
> }
>
> igt_main
> @@ -358,6 +337,7 @@ igt_main
> enum pipe pipe;
> data_t data = {};
> igt_output_t *output;
> + drmModeModeInfoPtr modetoset = NULL;
>
> igt_fixture {
> data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> @@ -379,28 +359,30 @@ igt_main
>
> for (int index = 0; index < ARRAY_SIZE(flip_scenario_test); index++) {
> igt_describe(flip_scenario_test[index].describe);
> - igt_subtest(flip_scenario_test[index].name) {
> - int validtests = 0;
> + igt_subtest_with_dynamic(flip_scenario_test[index].name) {
> free_fbs(&data);
> - for_each_pipe_static(pipe) {
> - enum subrval rval = CONNECTORSUCCESS;
> + for_each_pipe(&data.display, pipe) {
> + bool found = false;
> for_each_valid_output_on_pipe(&data.display, pipe, output) {
> - rval = test_flip_to_scaled(&data, index, pipe, output);
> -
> - igt_require(rval != TESTSKIP);
> -
> - // break out to test next pipe
> - if (rval == CONNECTORSUCCESS) {
> - validtests++;
> + modetoset = find_mode(&data, output);
> + if (modetoset) {
> + found = true;
> + igt_dynamic_f("pipe-%s-valid-mode", kmstest_pipe_name(pipe))
> + test_flip_to_scaled(&data, index, pipe, output, modetoset);
> break;
> }
> }
> - if (rval == NOREQUESTEDFORMATONPIPE)
> - igt_debug("No requested format/modifier on pipe %s\n", kmstest_pipe_name(pipe));
> + if (!found) {
> + for_each_valid_output_on_pipe(&data.display, pipe, output) {
> + igt_dynamic_f("pipe-%s-default-mode", kmstest_pipe_name(pipe))
> + test_flip_to_scaled(&data, index, pipe, output, NULL);
> + }
> + }
> + break;
This flow looks insane but keeps the original spirit and intention of
the code flow. Also I suggested it so naturally I'm fine with it.
With the above comments addressed, this is
Acked-by: Petri Latvala <petri.latvala at intel.com>
A full review from someone for the test code changes would be nice,
it's possible that I missed some quirks that need handling.
--
Petri Latvala
More information about the igt-dev
mailing list