| Message ID | 296f249138cb4f55466294b07a7aba25801ff146.1560374714.git.rodrigosiqueiramelo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | igt: Add support for testing writeback connectors | expand |
Hi, Thanks for the patch! Here are a few comments. For bonus points, it would be nice to add igt_describe descriptions of each sub-test. On Wed, 2019-06-12 at 23:16 -0300, Brian Starkey wrote: > Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and > WRITEBACK_FB_ID properties on writeback connectors, ensuring their > behaviour is correct. > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > [rebased and updated do_writeback_test() function to address feedback] > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > --- > tests/Makefile.sources | 1 + > tests/kms_writeback.c | 314 +++++++++++++++++++++++++++++++++++++++++ > tests/meson.build | 1 + > 3 files changed, 316 insertions(+) > create mode 100644 tests/kms_writeback.c > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index 027ed82f..03cc8efa 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -77,6 +77,7 @@ TESTS_progs = \ > kms_universal_plane \ > kms_vblank \ > kms_vrr \ > + kms_writeback \ > meta_test \ > perf \ > perf_pmu \ > diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c > new file mode 100644 > index 00000000..66ef48a6 > --- /dev/null > +++ b/tests/kms_writeback.c > @@ -0,0 +1,314 @@ > +/* > + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#include <errno.h> > +#include <stdbool.h> > +#include <stdio.h> > +#include <string.h> > + > +#include "igt.h" > +#include "igt_core.h" > +#include "igt_fb.h" > + > +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output) > +{ > + drmModePropertyBlobRes *blob = NULL; > + uint64_t blob_id; > + int ret; > + > + ret = kmstest_get_property(output->display->drm_fd, > + output->config.connector->connector_id, > + DRM_MODE_OBJECT_CONNECTOR, > + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS], > + NULL, &blob_id, NULL); > + if (ret) > + blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id); > + > + igt_assert(blob); > + > + return blob; > +} > + > +static bool check_writeback_config(igt_display_t *display, igt_output_t *output) > +{ > + igt_fb_t input_fb, output_fb; > + igt_plane_t *plane; > + uint32_t writeback_format = DRM_FORMAT_XRGB8888; > + uint64_t tiling = igt_fb_mod_to_tiling(0); > + int width, height, ret; > + drmModeModeInfo override_mode = { > + .clock = 25175, > + .hdisplay = 640, > + .hsync_start = 656, > + .hsync_end = 752, > + .htotal = 800, > + .hskew = 0, > + .vdisplay = 480, > + .vsync_start = 490, > + .vsync_end = 492, > + .vtotal = 525, > + .vscan = 0, > + .vrefresh = 60, > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > + .name = {"640x480-60"}, > + }; > + igt_output_override_mode(output, &override_mode); > + > + width = override_mode.hdisplay; > + height = override_mode.vdisplay; > + > + ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb); > + igt_assert(ret >= 0); > + > + ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb); > + igt_assert(ret >= 0); > + > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > + igt_plane_set_fb(plane, &input_fb); > + igt_output_set_writeback_fb(output, &output_fb); > + > + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | > + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); Okay, we're using atomic test-only mode to know if we can perform tests with the writeback output. It's probably fine, but we don't use WRITEBACK_PIXEL_FORMATS, which makes me a little bit sad. > + igt_plane_set_fb(plane, NULL); > + igt_remove_fb(display->drm_fd, &input_fb); > + igt_remove_fb(display->drm_fd, &output_fb); > + > + return !ret; > +} > + > +static igt_output_t *kms_writeback_get_output(igt_display_t *display) > +{ > + int i; > + > + for (i = 0; i < display->n_outputs; i++) { > + igt_output_t *output = &display->outputs[i]; > + int j; > + > + if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > + continue; > + > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON); Hmm. Is this really necessary? "Real" userspace won't do this, so I don't think we need to either. > + for (j = 0; j < igt_display_get_n_pipes(display); j++) { > + igt_output_set_pipe(output, j); > + > + if (check_writeback_config(display, output)) { > + igt_debug("Using connector %u:%s on pipe %d\n", > + output->config.connector->connector_id, > + output->name, j); > + return output; > + } We could probably add an igt_debug statement in case we don't use this writeback output. > + } > + > + /* Restore any connectors we don't use, so we don't trip on them later */ > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED); > + } > + > + return NULL; > +} > + > +static void check_writeback_fb_id(igt_output_t *output) > +{ > + uint64_t check_fb_id; > + > + check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID); > + igt_assert(check_fb_id == 0); > +} > + > +static int do_writeback_test(igt_output_t *output, uint32_t flags, > + uint32_t fb_id, int32_t *out_fence_ptr, > + bool ptr_valid) flags seems to always be set to DRM_MODE_ATOMIC_ALLOW_MODESET. We can probably remove the parameter from this function. > +{ > + int ret; > + igt_display_t *display = output->display; > + struct kmstest_connector_config *config = &output->config; > + > + igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id); > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id); > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr); > + > + if (ptr_valid) > + *out_fence_ptr = 0; > + > + ret = igt_display_try_commit_atomic(display, flags, NULL); > + > + if (ptr_valid) > + igt_assert(*out_fence_ptr == -1); I'm confused. Why should this be -1 even if we igt_display_try_commit_atomic succeeds? > + /* WRITEBACK_FB_ID must always read as zero */ > + check_writeback_fb_id(output); > + > + return ret; > +} > + > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > +{ > + int i, ret; > + int32_t out_fence; > + struct { > + uint32_t fb_id; > + bool ptr_valid; > + int32_t *out_fence_ptr; > + } invalid_tests[] = { > + { > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > + .fb_id = 0, > + .ptr_valid = true, > + .out_fence_ptr = &out_fence, > + }, > + { > + /* Invalid output buffer. */ > + .fb_id = invalid_fb->fb_id, > + .ptr_valid = true, > + .out_fence_ptr = &out_fence, > + }, This doesn't belong in this function (invalid_out_fence), since this checks an invalid framebuffer, not an invalid fence. We should probably move it to writeback_fb_id (and rename that function to test_fb?). > + { > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > + .fb_id = valid_fb->fb_id, > + .ptr_valid = false, > + .out_fence_ptr = (int32_t *)0x8, > + }, > + }; > + > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > + invalid_tests[i].fb_id, > + invalid_tests[i].out_fence_ptr, > + invalid_tests[i].ptr_valid); > + igt_assert(ret != 0); Maybe we can check for -ret == EINVAL? > + } > +} > + > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) invalid_fb doesn't seem to be used here. valid_fb seems to be set to the input framebuffer. It's probably not a good idea to use the same FB for input and writeback output. > +{ > + > + int ret; > + > + /* Valid output buffer */ > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > + valid_fb->fb_id, NULL, false); > + igt_assert(ret == 0); > + > + /* Invalid object for WRITEBACK_FB_ID */ > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > + output->id, NULL, false); > + igt_assert(ret == -EINVAL); > + > + /* Zero WRITEBACK_FB_ID */ > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > + 0, NULL, false); > + igt_assert(ret == 0); > +} > + > +igt_main > +{ > + igt_display_t display; > + igt_output_t *output; > + igt_plane_t *plane; > + igt_fb_t input_fb; > + drmModeModeInfo mode; > + int ret; > + > + memset(&display, 0, sizeof(display)); > + > + igt_fixture { > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > + igt_display_require(&display, display.drm_fd); > + > + kmstest_set_vt_graphics_mode(); > + > + igt_display_require(&display, display.drm_fd); > + > + igt_require(display.is_atomic); > + > + output = kms_writeback_get_output(&display); > + igt_require(output); > + > + if (output->use_override_mode) > + memcpy(&mode, &output->override_mode, sizeof(mode)); > + else > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > + > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > + igt_require(plane); Maybe we can assert on this? > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > + mode.vdisplay, > + DRM_FORMAT_XRGB8888, > + igt_fb_mod_to_tiling(0), This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be better to make this clear. (Applies to other lines of this patch) > + &input_fb); > + igt_assert(ret >= 0); > + igt_plane_set_fb(plane, &input_fb); > + } > + > + igt_subtest("writeback-pixel-formats") { > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); Need to drmModeFreePropertyBlob this. > + const char *valid_chars = "0123456 ABCGNRUVXY"; > + unsigned int i; > + char *c; > + > + /* > + * We don't have a comprehensive list of formats, so just check > + * that the blob length is sensible and that it doesn't contain > + * any outlandish characters > + */ > + igt_assert(!(formats_blob->length % 4)); > + c = formats_blob->data; > + for (i = 0; i < formats_blob->length; i++) > + igt_assert_f(strchr(valid_chars, c[i]), > + "Unexpected character %c\n", c[i]); Honestly, I'm not a fan of this check. There's no guarantee that fourcc codes will be made from ASCII characters, in fact some formats already have non-printable chars in them. I don't want to have to update this test when a new fourcc format is added. We currently don't have a test for IN_FORMATS. If we really want to check formats, we could have a big array of known formats and add a bool is_valid_format(uint32_t fmt) function. > + } > + > + igt_subtest("writeback-invalid-out-fence") { > + igt_fb_t invalid_fb; invalid_output_fb would be a better name IMHO. > + ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2, > + mode.vdisplay / 2, > + DRM_FORMAT_XRGB8888, > + igt_fb_mod_to_tiling(0), > + &invalid_fb); > + igt_require(ret > 0); We should probably use a different variable: ret is signed, igt_create_fb isn't. Also ret doesn't make it clear that the return value is the KMS framebuffer ID. (Applies to other subtests) > + invalid_out_fence(output, &input_fb, &invalid_fb); (Still not sure why we provide the input FB to this function.) > + igt_remove_fb(display.drm_fd, &invalid_fb); > + } > + > + igt_subtest("writeback-fb-id") { > + igt_fb_t output_fb; > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay, > + DRM_FORMAT_XRGB8888, > + igt_fb_mod_to_tiling(0), > + &output_fb); > + igt_require(ret > 0); > + > + writeback_fb_id(output, &input_fb, &output_fb); > + > + igt_remove_fb(display.drm_fd, &output_fb); > + } > + > + igt_fixture { > + igt_remove_fb(display.drm_fd, &input_fb); > + igt_display_fini(&display); > + } > +} > diff --git a/tests/meson.build b/tests/meson.build > index f168fbba..9c77cfcd 100644 > --- a/tests/meson.build > +++ b/tests/meson.build > @@ -63,6 +63,7 @@ test_progs = [ > 'kms_universal_plane', > 'kms_vblank', > 'kms_vrr', > + 'kms_writeback', > 'meta_test', > 'panfrost_get_param', > 'panfrost_gem_new', > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev
On 07/10, Ser, Simon wrote: > Hi, > > Thanks for the patch! Here are a few comments. > > For bonus points, it would be nice to add igt_describe descriptions of > each sub-test. Hi Simon, First of all, thanks for your feedback; I already applied most of your suggestions. I just have some inline comments/questions. > On Wed, 2019-06-12 at 23:16 -0300, Brian Starkey wrote: > > Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and > > WRITEBACK_FB_ID properties on writeback connectors, ensuring their > > behaviour is correct. > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > [rebased and updated do_writeback_test() function to address feedback] > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > > --- > > tests/Makefile.sources | 1 + > > tests/kms_writeback.c | 314 +++++++++++++++++++++++++++++++++++++++++ > > tests/meson.build | 1 + > > 3 files changed, 316 insertions(+) > > create mode 100644 tests/kms_writeback.c > > > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > index 027ed82f..03cc8efa 100644 > > --- a/tests/Makefile.sources > > +++ b/tests/Makefile.sources > > @@ -77,6 +77,7 @@ TESTS_progs = \ > > kms_universal_plane \ > > kms_vblank \ > > kms_vrr \ > > + kms_writeback \ > > meta_test \ > > perf \ > > perf_pmu \ > > diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c > > new file mode 100644 > > index 00000000..66ef48a6 > > --- /dev/null > > +++ b/tests/kms_writeback.c > > @@ -0,0 +1,314 @@ > > +/* > > + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the next > > + * paragraph) shall be included in all copies or substantial portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > + * IN THE SOFTWARE. > > + * > > + */ > > + > > +#include <errno.h> > > +#include <stdbool.h> > > +#include <stdio.h> > > +#include <string.h> > > + > > +#include "igt.h" > > +#include "igt_core.h" > > +#include "igt_fb.h" > > + > > +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output) > > +{ > > + drmModePropertyBlobRes *blob = NULL; > > + uint64_t blob_id; > > + int ret; > > + > > + ret = kmstest_get_property(output->display->drm_fd, > > + output->config.connector->connector_id, > > + DRM_MODE_OBJECT_CONNECTOR, > > + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS], > > + NULL, &blob_id, NULL); > > + if (ret) > > + blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id); > > + > > + igt_assert(blob); > > + > > + return blob; > > +} > > + > > +static bool check_writeback_config(igt_display_t *display, igt_output_t *output) > > +{ > > + igt_fb_t input_fb, output_fb; > > + igt_plane_t *plane; > > + uint32_t writeback_format = DRM_FORMAT_XRGB8888; > > + uint64_t tiling = igt_fb_mod_to_tiling(0); > > + int width, height, ret; > > + drmModeModeInfo override_mode = { > > + .clock = 25175, > > + .hdisplay = 640, > > + .hsync_start = 656, > > + .hsync_end = 752, > > + .htotal = 800, > > + .hskew = 0, > > + .vdisplay = 480, > > + .vsync_start = 490, > > + .vsync_end = 492, > > + .vtotal = 525, > > + .vscan = 0, > > + .vrefresh = 60, > > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > > + .name = {"640x480-60"}, > > + }; > > + igt_output_override_mode(output, &override_mode); > > + > > + width = override_mode.hdisplay; > > + height = override_mode.vdisplay; > > + > > + ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb); > > + igt_assert(ret >= 0); > > + > > + ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb); > > + igt_assert(ret >= 0); > > + > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > + igt_plane_set_fb(plane, &input_fb); > > + igt_output_set_writeback_fb(output, &output_fb); > > + > > + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | > > + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > Okay, we're using atomic test-only mode to know if we can perform tests > with the writeback output. It's probably fine, but we don't use > WRITEBACK_PIXEL_FORMATS, which makes me a little bit sad. Sorry, I did not fully understand part. Did you expect to see something like the below code before igt_display_try_commit_atomic()? igt_output_set_prop_value(output, WRITEBACK_PIXEL_FORMATS, DRM_FORMAT_XRGB8888); > > + igt_plane_set_fb(plane, NULL); > > + igt_remove_fb(display->drm_fd, &input_fb); > > + igt_remove_fb(display->drm_fd, &output_fb); > > + > > + return !ret; > > +} > > + > > +static igt_output_t *kms_writeback_get_output(igt_display_t *display) > > +{ > > + int i; > > + > > + for (i = 0; i < display->n_outputs; i++) { > > + igt_output_t *output = &display->outputs[i]; > > + int j; > > + > > + if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > > + continue; > > + > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON); > > Hmm. Is this really necessary? "Real" userspace won't do this, so I > don't think we need to either. Hmmm, I have a little experience with userspace; however, I tested kms_writeback on top of vkms without this line, and everything worked well. If I remove this line, should I also drop the line that force connector to FORCE_CONNECTOR_UNSPECIFIED? Another question, if FORCE_CONNECTOR_ON is something that userspace won't want to do, why do we have it? > > + for (j = 0; j < igt_display_get_n_pipes(display); j++) { > > + igt_output_set_pipe(output, j); > > + > > + if (check_writeback_config(display, output)) { > > + igt_debug("Using connector %u:%s on pipe %d\n", > > + output->config.connector->connector_id, > > + output->name, j); > > + return output; > > + } > > We could probably add an igt_debug statement in case we don't use this > writeback output. > > > + } > > + > > + /* Restore any connectors we don't use, so we don't trip on them later */ > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED); > > + } > > + > > + return NULL; > > +} > > + > > +static void check_writeback_fb_id(igt_output_t *output) > > +{ > > + uint64_t check_fb_id; > > + > > + check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID); > > + igt_assert(check_fb_id == 0); > > +} > > + > > +static int do_writeback_test(igt_output_t *output, uint32_t flags, > > + uint32_t fb_id, int32_t *out_fence_ptr, > > + bool ptr_valid) > > flags seems to always be set to DRM_MODE_ATOMIC_ALLOW_MODESET. We can > probably remove the parameter from this function. > > > +{ > > + int ret; > > + igt_display_t *display = output->display; > > + struct kmstest_connector_config *config = &output->config; > > + > > + igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id); > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id); > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr); > > + > > + if (ptr_valid) > > + *out_fence_ptr = 0; > > + > > + ret = igt_display_try_commit_atomic(display, flags, NULL); > > + > > + if (ptr_valid) > > + igt_assert(*out_fence_ptr == -1); > > I'm confused. Why should this be -1 even if we > igt_display_try_commit_atomic succeeds? I inspected the drm_mode_atomic_ioctl() function and I noticed that It calls complete_signaling() in its turn this function assign -1 to out_fence_ptr. Since all of this happens at the end of atomic_commit, I believe that we don’t need it. I already removed it. > > + /* WRITEBACK_FB_ID must always read as zero */ > > + check_writeback_fb_id(output); > > + > > + return ret; > > +} > > + > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > +{ > > + int i, ret; > > + int32_t out_fence; > > + struct { > > + uint32_t fb_id; > > + bool ptr_valid; > > + int32_t *out_fence_ptr; > > + } invalid_tests[] = { > > + { > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > + .fb_id = 0, > > + .ptr_valid = true, > > + .out_fence_ptr = &out_fence, > > + }, > > + { > > + /* Invalid output buffer. */ > > + .fb_id = invalid_fb->fb_id, > > + .ptr_valid = true, > > + .out_fence_ptr = &out_fence, > > + }, > > This doesn't belong in this function (invalid_out_fence), since this > checks an invalid framebuffer, not an invalid fence. We should probably > move it to writeback_fb_id (and rename that function to test_fb?). > > > + { > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > + .fb_id = valid_fb->fb_id, > > + .ptr_valid = false, > > + .out_fence_ptr = (int32_t *)0x8, > > + }, > > + }; > > + > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > + invalid_tests[i].fb_id, > > + invalid_tests[i].out_fence_ptr, > > + invalid_tests[i].ptr_valid); > > + igt_assert(ret != 0); > > Maybe we can check for -ret == EINVAL? > > > + } > > +} > > + > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > the input framebuffer. It's probably not a good idea to use the same FB > for input and writeback output. > > > +{ > > + > > + int ret; > > + > > + /* Valid output buffer */ > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > + valid_fb->fb_id, NULL, false); > > + igt_assert(ret == 0); > > + > > + /* Invalid object for WRITEBACK_FB_ID */ > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > + output->id, NULL, false); > > + igt_assert(ret == -EINVAL); > > + > > + /* Zero WRITEBACK_FB_ID */ > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > + 0, NULL, false); > > + igt_assert(ret == 0); > > +} > > + > > +igt_main > > +{ > > + igt_display_t display; > > + igt_output_t *output; > > + igt_plane_t *plane; > > + igt_fb_t input_fb; > > + drmModeModeInfo mode; > > + int ret; > > + > > + memset(&display, 0, sizeof(display)); > > + > > + igt_fixture { > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > + igt_display_require(&display, display.drm_fd); > > + > > + kmstest_set_vt_graphics_mode(); > > + > > + igt_display_require(&display, display.drm_fd); > > + > > + igt_require(display.is_atomic); > > + > > + output = kms_writeback_get_output(&display); > > + igt_require(output); > > + > > + if (output->use_override_mode) > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > + else > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > + > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > + igt_require(plane); > > Maybe we can assert on this? > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > + mode.vdisplay, > > + DRM_FORMAT_XRGB8888, > > + igt_fb_mod_to_tiling(0), > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > better to make this clear. > > (Applies to other lines of this patch) > > > + &input_fb); > > + igt_assert(ret >= 0); > > + igt_plane_set_fb(plane, &input_fb); > > + } > > + > > + igt_subtest("writeback-pixel-formats") { > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > Need to drmModeFreePropertyBlob this. > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > + unsigned int i; > > + char *c; > > + > > + /* > > + * We don't have a comprehensive list of formats, so just check > > + * that the blob length is sensible and that it doesn't contain > > + * any outlandish characters > > + */ > > + igt_assert(!(formats_blob->length % 4)); > > + c = formats_blob->data; > > + for (i = 0; i < formats_blob->length; i++) > > + igt_assert_f(strchr(valid_chars, c[i]), > > + "Unexpected character %c\n", c[i]); > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > codes will be made from ASCII characters, in fact some formats already > have non-printable chars in them. I don't want to have to update this > test when a new fourcc format is added. > > We currently don't have a test for IN_FORMATS. If we really want to > check formats, we could have a big array of known formats and add a > bool is_valid_format(uint32_t fmt) function. Agree with you. How about remove this test? Thanks Best Regards > > + } > > + > > + igt_subtest("writeback-invalid-out-fence") { > > + igt_fb_t invalid_fb; > > invalid_output_fb would be a better name IMHO. > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2, > > + mode.vdisplay / 2, > > + DRM_FORMAT_XRGB8888, > > + igt_fb_mod_to_tiling(0), > > + &invalid_fb); > > + igt_require(ret > 0); > > We should probably use a different variable: ret is signed, > igt_create_fb isn't. Also ret doesn't make it clear that the return > value is the KMS framebuffer ID. > > (Applies to other subtests) > > > + invalid_out_fence(output, &input_fb, &invalid_fb); > > (Still not sure why we provide the input FB to this function.) > > > + igt_remove_fb(display.drm_fd, &invalid_fb); > > + } > > + > > + igt_subtest("writeback-fb-id") { > > + igt_fb_t output_fb; > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay, > > + DRM_FORMAT_XRGB8888, > > + igt_fb_mod_to_tiling(0), > > + &output_fb); > > + igt_require(ret > 0); > > + > > + writeback_fb_id(output, &input_fb, &output_fb); > > + > > + igt_remove_fb(display.drm_fd, &output_fb); > > + } > > + > > + igt_fixture { > > + igt_remove_fb(display.drm_fd, &input_fb); > > + igt_display_fini(&display); > > + } > > +} > > diff --git a/tests/meson.build b/tests/meson.build > > index f168fbba..9c77cfcd 100644 > > --- a/tests/meson.build > > +++ b/tests/meson.build > > @@ -63,6 +63,7 @@ test_progs = [ > > 'kms_universal_plane', > > 'kms_vblank', > > 'kms_vrr', > > + 'kms_writeback', > > 'meta_test', > > 'panfrost_get_param', > > 'panfrost_gem_new', > > _______________________________________________ > > igt-dev mailing list > > igt-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
On Thu, 2019-07-11 at 23:44 -0300, Rodrigo Siqueira wrote: > On 07/10, Ser, Simon wrote: > > Hi, > > > > Thanks for the patch! Here are a few comments. > > > > For bonus points, it would be nice to add igt_describe descriptions of > > each sub-test. > > Hi Simon, > > First of all, thanks for your feedback; I already applied most of your > suggestions. I just have some inline comments/questions. > > > On Wed, 2019-06-12 at 23:16 -0300, Brian Starkey wrote: > > > Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and > > > WRITEBACK_FB_ID properties on writeback connectors, ensuring their > > > behaviour is correct. > > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > [rebased and updated do_writeback_test() function to address feedback] > > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > > > --- > > > tests/Makefile.sources | 1 + > > > tests/kms_writeback.c | 314 +++++++++++++++++++++++++++++++++++++++++ > > > tests/meson.build | 1 + > > > 3 files changed, 316 insertions(+) > > > create mode 100644 tests/kms_writeback.c > > > > > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > > index 027ed82f..03cc8efa 100644 > > > --- a/tests/Makefile.sources > > > +++ b/tests/Makefile.sources > > > @@ -77,6 +77,7 @@ TESTS_progs = \ > > > kms_universal_plane \ > > > kms_vblank \ > > > kms_vrr \ > > > + kms_writeback \ > > > meta_test \ > > > perf \ > > > perf_pmu \ > > > diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c > > > new file mode 100644 > > > index 00000000..66ef48a6 > > > --- /dev/null > > > +++ b/tests/kms_writeback.c > > > @@ -0,0 +1,314 @@ > > > +/* > > > + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. > > > + * > > > + * Permission is hereby granted, free of charge, to any person obtaining a > > > + * copy of this software and associated documentation files (the "Software"), > > > + * to deal in the Software without restriction, including without limitation > > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > > + * and/or sell copies of the Software, and to permit persons to whom the > > > + * Software is furnished to do so, subject to the following conditions: > > > + * > > > + * The above copyright notice and this permission notice (including the next > > > + * paragraph) shall be included in all copies or substantial portions of the > > > + * Software. > > > + * > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > > + * IN THE SOFTWARE. > > > + * > > > + */ > > > + > > > +#include <errno.h> > > > +#include <stdbool.h> > > > +#include <stdio.h> > > > +#include <string.h> > > > + > > > +#include "igt.h" > > > +#include "igt_core.h" > > > +#include "igt_fb.h" > > > + > > > +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output) > > > +{ > > > + drmModePropertyBlobRes *blob = NULL; > > > + uint64_t blob_id; > > > + int ret; > > > + > > > + ret = kmstest_get_property(output->display->drm_fd, > > > + output->config.connector->connector_id, > > > + DRM_MODE_OBJECT_CONNECTOR, > > > + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS], > > > + NULL, &blob_id, NULL); > > > + if (ret) > > > + blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id); > > > + > > > + igt_assert(blob); > > > + > > > + return blob; > > > +} > > > + > > > +static bool check_writeback_config(igt_display_t *display, igt_output_t *output) > > > +{ > > > + igt_fb_t input_fb, output_fb; > > > + igt_plane_t *plane; > > > + uint32_t writeback_format = DRM_FORMAT_XRGB8888; > > > + uint64_t tiling = igt_fb_mod_to_tiling(0); > > > + int width, height, ret; > > > + drmModeModeInfo override_mode = { > > > + .clock = 25175, > > > + .hdisplay = 640, > > > + .hsync_start = 656, > > > + .hsync_end = 752, > > > + .htotal = 800, > > > + .hskew = 0, > > > + .vdisplay = 480, > > > + .vsync_start = 490, > > > + .vsync_end = 492, > > > + .vtotal = 525, > > > + .vscan = 0, > > > + .vrefresh = 60, > > > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > > > + .name = {"640x480-60"}, > > > + }; > > > + igt_output_override_mode(output, &override_mode); > > > + > > > + width = override_mode.hdisplay; > > > + height = override_mode.vdisplay; > > > + > > > + ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb); > > > + igt_assert(ret >= 0); > > > + > > > + ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb); > > > + igt_assert(ret >= 0); > > > + > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > + igt_plane_set_fb(plane, &input_fb); > > > + igt_output_set_writeback_fb(output, &output_fb); > > > + > > > + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | > > > + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > > Okay, we're using atomic test-only mode to know if we can perform tests > > with the writeback output. It's probably fine, but we don't use > > WRITEBACK_PIXEL_FORMATS, which makes me a little bit sad. > > Sorry, I did not fully understand part. Did you expect to see something > like the below code before igt_display_try_commit_atomic()? > > igt_output_set_prop_value(output, WRITEBACK_PIXEL_FORMATS, > DRM_FORMAT_XRGB8888); Hmm, no, we cannot change the list of writeback formats (it's immutable). This comment doesn't require any action, it's just me thinking aloud :P I'm just thinking that it would be nice to choose our format depending on the WRITEBACK_PIXEL_FORMATS property. And probably run tests with each supported format (or, alternatively, choose a random one). That way we can make sure WRITEBACK_PIXEL_FORMATS isn't broken. However, this can be left for a later patch. > > > + igt_plane_set_fb(plane, NULL); > > > + igt_remove_fb(display->drm_fd, &input_fb); > > > + igt_remove_fb(display->drm_fd, &output_fb); > > > + > > > + return !ret; > > > +} > > > + > > > +static igt_output_t *kms_writeback_get_output(igt_display_t *display) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < display->n_outputs; i++) { > > > + igt_output_t *output = &display->outputs[i]; > > > + int j; > > > + > > > + if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > > > + continue; > > > + > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON); > > > > Hmm. Is this really necessary? "Real" userspace won't do this, so I > > don't think we need to either. > > Hmmm, I have a little experience with userspace; however, I tested > kms_writeback on top of vkms without this line, and everything worked > well. If I remove this line, should I also drop the line that force > connector to FORCE_CONNECTOR_UNSPECIFIED? I believe so. > Another question, if FORCE_CONNECTOR_ON is something that userspace > won't want to do, why do we have it? We use kmstest_force_connector in injection tests: those pick a disconnected HDMI connector and trick the kernel into thinking it's connected. We generally also force an EDID and this is used to emulate a screen (e.g. a screen that supports audio, 4K or 3D). This is only meant to be used for testing though, hence "real userspace shouldn't use it". > > > + for (j = 0; j < igt_display_get_n_pipes(display); j++) { > > > + igt_output_set_pipe(output, j); > > > + > > > + if (check_writeback_config(display, output)) { > > > + igt_debug("Using connector %u:%s on pipe %d\n", > > > + output->config.connector->connector_id, > > > + output->name, j); > > > + return output; > > > + } > > > > We could probably add an igt_debug statement in case we don't use this > > writeback output. > > > > > + } > > > + > > > + /* Restore any connectors we don't use, so we don't trip on them later */ > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED); > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > +static void check_writeback_fb_id(igt_output_t *output) > > > +{ > > > + uint64_t check_fb_id; > > > + > > > + check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID); > > > + igt_assert(check_fb_id == 0); > > > +} > > > + > > > +static int do_writeback_test(igt_output_t *output, uint32_t flags, > > > + uint32_t fb_id, int32_t *out_fence_ptr, > > > + bool ptr_valid) > > > > flags seems to always be set to DRM_MODE_ATOMIC_ALLOW_MODESET. We can > > probably remove the parameter from this function. > > > > > +{ > > > + int ret; > > > + igt_display_t *display = output->display; > > > + struct kmstest_connector_config *config = &output->config; > > > + > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id); > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id); > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr); > > > + > > > + if (ptr_valid) > > > + *out_fence_ptr = 0; > > > + > > > + ret = igt_display_try_commit_atomic(display, flags, NULL); > > > + > > > + if (ptr_valid) > > > + igt_assert(*out_fence_ptr == -1); > > > > I'm confused. Why should this be -1 even if we > > igt_display_try_commit_atomic succeeds? > > I inspected the drm_mode_atomic_ioctl() function and I noticed that It > calls complete_signaling() in its turn this function assign -1 to > out_fence_ptr. Since all of this happens at the end of atomic_commit, I > believe that we don’t need it. I already removed it. I think I'm still confused :) I'm probably missing something. As far as I understand, we are doing an atomic commit, sometimes with a valid WRITEBACK_FB_ID and WRITEBACK_OUT_FENCE_PTR. In this case it should start the writeback process in the kernel and we should get a valid out_fence_ptr? Why do we always get -1? > > > + /* WRITEBACK_FB_ID must always read as zero */ > > > + check_writeback_fb_id(output); > > > + > > > + return ret; > > > +} > > > + > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > +{ > > > + int i, ret; > > > + int32_t out_fence; > > > + struct { > > > + uint32_t fb_id; > > > + bool ptr_valid; > > > + int32_t *out_fence_ptr; > > > + } invalid_tests[] = { > > > + { > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > + .fb_id = 0, > > > + .ptr_valid = true, > > > + .out_fence_ptr = &out_fence, > > > + }, > > > + { > > > + /* Invalid output buffer. */ > > > + .fb_id = invalid_fb->fb_id, > > > + .ptr_valid = true, > > > + .out_fence_ptr = &out_fence, > > > + }, > > > > This doesn't belong in this function (invalid_out_fence), since this > > checks an invalid framebuffer, not an invalid fence. We should probably > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > > + { > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > + .fb_id = valid_fb->fb_id, > > > + .ptr_valid = false, > > > + .out_fence_ptr = (int32_t *)0x8, > > > + }, > > > + }; > > > + > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + invalid_tests[i].fb_id, > > > + invalid_tests[i].out_fence_ptr, > > > + invalid_tests[i].ptr_valid); > > > + igt_assert(ret != 0); > > > > Maybe we can check for -ret == EINVAL? > > > > > + } > > > +} > > > + > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > the input framebuffer. It's probably not a good idea to use the same FB > > for input and writeback output. > > > > > +{ > > > + > > > + int ret; > > > + > > > + /* Valid output buffer */ > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + valid_fb->fb_id, NULL, false); > > > + igt_assert(ret == 0); > > > + > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + output->id, NULL, false); > > > + igt_assert(ret == -EINVAL); > > > + > > > + /* Zero WRITEBACK_FB_ID */ > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + 0, NULL, false); > > > + igt_assert(ret == 0); > > > +} > > > + > > > +igt_main > > > +{ > > > + igt_display_t display; > > > + igt_output_t *output; > > > + igt_plane_t *plane; > > > + igt_fb_t input_fb; > > > + drmModeModeInfo mode; > > > + int ret; > > > + > > > + memset(&display, 0, sizeof(display)); > > > + > > > + igt_fixture { > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > + igt_display_require(&display, display.drm_fd); > > > + > > > + kmstest_set_vt_graphics_mode(); > > > + > > > + igt_display_require(&display, display.drm_fd); > > > + > > > + igt_require(display.is_atomic); > > > + > > > + output = kms_writeback_get_output(&display); > > > + igt_require(output); > > > + > > > + if (output->use_override_mode) > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > + else > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > + > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > + igt_require(plane); > > > > Maybe we can assert on this? > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > + mode.vdisplay, > > > + DRM_FORMAT_XRGB8888, > > > + igt_fb_mod_to_tiling(0), > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > better to make this clear. > > > > (Applies to other lines of this patch) > > > > > + &input_fb); > > > + igt_assert(ret >= 0); > > > + igt_plane_set_fb(plane, &input_fb); > > > + } > > > + > > > + igt_subtest("writeback-pixel-formats") { > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > Need to drmModeFreePropertyBlob this. > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > + unsigned int i; > > > + char *c; > > > + > > > + /* > > > + * We don't have a comprehensive list of formats, so just check > > > + * that the blob length is sensible and that it doesn't contain > > > + * any outlandish characters > > > + */ > > > + igt_assert(!(formats_blob->length % 4)); > > > + c = formats_blob->data; > > > + for (i = 0; i < formats_blob->length; i++) > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > + "Unexpected character %c\n", c[i]); > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > codes will be made from ASCII characters, in fact some formats already > > have non-printable chars in them. I don't want to have to update this > > test when a new fourcc format is added. > > > > We currently don't have a test for IN_FORMATS. If we really want to > > check formats, we could have a big array of known formats and add a > > bool is_valid_format(uint32_t fmt) function. > > Agree with you. How about remove this test? I guess we could always keep the length % 4 test, even if it's just a sanity test. At least this one won't ever need to be changed. I don't feel strongly about it. > Thanks > Best Regards > > > > + } > > > + > > > + igt_subtest("writeback-invalid-out-fence") { > > > + igt_fb_t invalid_fb; > > > > invalid_output_fb would be a better name IMHO. > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2, > > > + mode.vdisplay / 2, > > > + DRM_FORMAT_XRGB8888, > > > + igt_fb_mod_to_tiling(0), > > > + &invalid_fb); > > > + igt_require(ret > 0); > > > > We should probably use a different variable: ret is signed, > > igt_create_fb isn't. Also ret doesn't make it clear that the return > > value is the KMS framebuffer ID. > > > > (Applies to other subtests) > > > > > + invalid_out_fence(output, &input_fb, &invalid_fb); > > > > (Still not sure why we provide the input FB to this function.) > > > > > + igt_remove_fb(display.drm_fd, &invalid_fb); > > > + } > > > + > > > + igt_subtest("writeback-fb-id") { > > > + igt_fb_t output_fb; > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay, > > > + DRM_FORMAT_XRGB8888, > > > + igt_fb_mod_to_tiling(0), > > > + &output_fb); > > > + igt_require(ret > 0); > > > + > > > + writeback_fb_id(output, &input_fb, &output_fb); > > > + > > > + igt_remove_fb(display.drm_fd, &output_fb); > > > + } > > > + > > > + igt_fixture { > > > + igt_remove_fb(display.drm_fd, &input_fb); > > > + igt_display_fini(&display); > > > + } > > > +} > > > diff --git a/tests/meson.build b/tests/meson.build > > > index f168fbba..9c77cfcd 100644 > > > --- a/tests/meson.build > > > +++ b/tests/meson.build > > > @@ -63,6 +63,7 @@ test_progs = [ > > > 'kms_universal_plane', > > > 'kms_vblank', > > > 'kms_vrr', > > > + 'kms_writeback', > > > 'meta_test', > > > 'panfrost_get_param', > > > 'panfrost_gem_new', > > > _______________________________________________ > > > igt-dev mailing list > > > igt-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
Hi Rodrigo, On Thu, Jul 11, 2019 at 11:44:35PM -0300, Rodrigo Siqueira wrote: > On 07/10, Ser, Simon wrote: > > Hi, > > > > Thanks for the patch! Here are a few comments. > > > > For bonus points, it would be nice to add igt_describe descriptions of > > each sub-test. > > Hi Simon, > > First of all, thanks for your feedback; I already applied most of your > suggestions. I just have some inline comments/questions. > > > On Wed, 2019-06-12 at 23:16 -0300, Brian Starkey wrote: > > > Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and > > > WRITEBACK_FB_ID properties on writeback connectors, ensuring their > > > behaviour is correct. > > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > [rebased and updated do_writeback_test() function to address feedback] > > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > > > --- > > > tests/Makefile.sources | 1 + > > > tests/kms_writeback.c | 314 +++++++++++++++++++++++++++++++++++++++++ > > > tests/meson.build | 1 + > > > 3 files changed, 316 insertions(+) > > > create mode 100644 tests/kms_writeback.c > > > > > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > > index 027ed82f..03cc8efa 100644 > > > --- a/tests/Makefile.sources > > > +++ b/tests/Makefile.sources > > > @@ -77,6 +77,7 @@ TESTS_progs = \ > > > kms_universal_plane \ > > > kms_vblank \ > > > kms_vrr \ > > > + kms_writeback \ > > > meta_test \ > > > perf \ > > > perf_pmu \ > > > diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c > > > new file mode 100644 > > > index 00000000..66ef48a6 > > > --- /dev/null > > > +++ b/tests/kms_writeback.c > > > @@ -0,0 +1,314 @@ > > > +/* > > > + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. > > > + * > > > + * Permission is hereby granted, free of charge, to any person obtaining a > > > + * copy of this software and associated documentation files (the "Software"), > > > + * to deal in the Software without restriction, including without limitation > > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > > + * and/or sell copies of the Software, and to permit persons to whom the > > > + * Software is furnished to do so, subject to the following conditions: > > > + * > > > + * The above copyright notice and this permission notice (including the next > > > + * paragraph) shall be included in all copies or substantial portions of the > > > + * Software. > > > + * > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > > + * IN THE SOFTWARE. > > > + * > > > + */ > > > + > > > +#include <errno.h> > > > +#include <stdbool.h> > > > +#include <stdio.h> > > > +#include <string.h> > > > + > > > +#include "igt.h" > > > +#include "igt_core.h" > > > +#include "igt_fb.h" > > > + > > > +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output) > > > +{ > > > + drmModePropertyBlobRes *blob = NULL; > > > + uint64_t blob_id; > > > + int ret; > > > + > > > + ret = kmstest_get_property(output->display->drm_fd, > > > + output->config.connector->connector_id, > > > + DRM_MODE_OBJECT_CONNECTOR, > > > + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS], > > > + NULL, &blob_id, NULL); > > > + if (ret) > > > + blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id); > > > + > > > + igt_assert(blob); > > > + > > > + return blob; > > > +} > > > + > > > +static bool check_writeback_config(igt_display_t *display, igt_output_t *output) > > > +{ > > > + igt_fb_t input_fb, output_fb; > > > + igt_plane_t *plane; > > > + uint32_t writeback_format = DRM_FORMAT_XRGB8888; > > > + uint64_t tiling = igt_fb_mod_to_tiling(0); > > > + int width, height, ret; > > > + drmModeModeInfo override_mode = { > > > + .clock = 25175, > > > + .hdisplay = 640, > > > + .hsync_start = 656, > > > + .hsync_end = 752, > > > + .htotal = 800, > > > + .hskew = 0, > > > + .vdisplay = 480, > > > + .vsync_start = 490, > > > + .vsync_end = 492, > > > + .vtotal = 525, > > > + .vscan = 0, > > > + .vrefresh = 60, > > > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > > > + .name = {"640x480-60"}, > > > + }; > > > + igt_output_override_mode(output, &override_mode); > > > + > > > + width = override_mode.hdisplay; > > > + height = override_mode.vdisplay; > > > + > > > + ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb); > > > + igt_assert(ret >= 0); > > > + > > > + ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb); > > > + igt_assert(ret >= 0); > > > + > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > + igt_plane_set_fb(plane, &input_fb); > > > + igt_output_set_writeback_fb(output, &output_fb); > > > + > > > + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | > > > + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > > Okay, we're using atomic test-only mode to know if we can perform tests > > with the writeback output. It's probably fine, but we don't use > > WRITEBACK_PIXEL_FORMATS, which makes me a little bit sad. > > Sorry, I did not fully understand part. Did you expect to see something > like the below code before igt_display_try_commit_atomic()? > > igt_output_set_prop_value(output, WRITEBACK_PIXEL_FORMATS, > DRM_FORMAT_XRGB8888); > > > > + igt_plane_set_fb(plane, NULL); > > > + igt_remove_fb(display->drm_fd, &input_fb); > > > + igt_remove_fb(display->drm_fd, &output_fb); > > > + > > > + return !ret; > > > +} > > > + > > > +static igt_output_t *kms_writeback_get_output(igt_display_t *display) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < display->n_outputs; i++) { > > > + igt_output_t *output = &display->outputs[i]; > > > + int j; > > > + > > > + if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > > > + continue; > > > + > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON); > > > > Hmm. Is this really necessary? "Real" userspace won't do this, so I > > don't think we need to either. > > Hmmm, I have a little experience with userspace; however, I tested > kms_writeback on top of vkms without this line, and everything worked > well. If I remove this line, should I also drop the line that force > connector to FORCE_CONNECTOR_UNSPECIFIED? > > Another question, if FORCE_CONNECTOR_ON is something that userspace > won't want to do, why do we have it? This is probably a left-over from the times when the design of the writeback connectors called for them to be always disconnected, in order not to trip old userspace. Last iteration before upstreaming the writeback into DRM we switched to an earlier design where DRM_CLIENT_CAP_WRITEBACK_CONNECTORS is needed in order to expose the writeback connector, and then it is reported as connected. You can drop this from the patch. > > > > + for (j = 0; j < igt_display_get_n_pipes(display); j++) { > > > + igt_output_set_pipe(output, j); > > > + > > > + if (check_writeback_config(display, output)) { > > > + igt_debug("Using connector %u:%s on pipe %d\n", > > > + output->config.connector->connector_id, > > > + output->name, j); > > > + return output; > > > + } > > > > We could probably add an igt_debug statement in case we don't use this > > writeback output. > > > > > + } > > > + > > > + /* Restore any connectors we don't use, so we don't trip on them later */ > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED); > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > +static void check_writeback_fb_id(igt_output_t *output) > > > +{ > > > + uint64_t check_fb_id; > > > + > > > + check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID); > > > + igt_assert(check_fb_id == 0); > > > +} > > > + > > > +static int do_writeback_test(igt_output_t *output, uint32_t flags, > > > + uint32_t fb_id, int32_t *out_fence_ptr, > > > + bool ptr_valid) > > > > flags seems to always be set to DRM_MODE_ATOMIC_ALLOW_MODESET. We can > > probably remove the parameter from this function. > > > > > +{ > > > + int ret; > > > + igt_display_t *display = output->display; > > > + struct kmstest_connector_config *config = &output->config; > > > + > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id); > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id); > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr); > > > + > > > + if (ptr_valid) > > > + *out_fence_ptr = 0; > > > + > > > + ret = igt_display_try_commit_atomic(display, flags, NULL); > > > + > > > + if (ptr_valid) > > > + igt_assert(*out_fence_ptr == -1); > > > > I'm confused. Why should this be -1 even if we > > igt_display_try_commit_atomic succeeds? > > I inspected the drm_mode_atomic_ioctl() function and I noticed that It > calls complete_signaling() in its turn this function assign -1 to > out_fence_ptr. Since all of this happens at the end of atomic_commit, I > believe that we don’t need it. I already removed it. Yeah, I think the reasons for that have been lost into the mist of time. > > > > + /* WRITEBACK_FB_ID must always read as zero */ > > > + check_writeback_fb_id(output); > > > + > > > + return ret; > > > +} > > > + > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > +{ > > > + int i, ret; > > > + int32_t out_fence; > > > + struct { > > > + uint32_t fb_id; > > > + bool ptr_valid; > > > + int32_t *out_fence_ptr; > > > + } invalid_tests[] = { > > > + { > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > + .fb_id = 0, > > > + .ptr_valid = true, > > > + .out_fence_ptr = &out_fence, > > > + }, > > > + { > > > + /* Invalid output buffer. */ > > > + .fb_id = invalid_fb->fb_id, > > > + .ptr_valid = true, > > > + .out_fence_ptr = &out_fence, > > > + }, > > > > This doesn't belong in this function (invalid_out_fence), since this > > checks an invalid framebuffer, not an invalid fence. We should probably > > move it to writeback_fb_id (and rename that function to test_fb?). It tries to test that you can't trick the driver to do any work on a fence if the framebuffer is invalid, so the set of tests tries: no fb with valid fence, invalid fb with valid fence, valid fb but invalid fence and assumes that no fb with invalid fence is a NOP anyway. > > > > > + { > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > + .fb_id = valid_fb->fb_id, > > > + .ptr_valid = false, > > > + .out_fence_ptr = (int32_t *)0x8, > > > + }, > > > + }; > > > + > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + invalid_tests[i].fb_id, > > > + invalid_tests[i].out_fence_ptr, > > > + invalid_tests[i].ptr_valid); > > > + igt_assert(ret != 0); > > > > Maybe we can check for -ret == EINVAL? > > > > > + } > > > +} > > > + > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > the input framebuffer. It's probably not a good idea to use the same FB > > for input and writeback output. > > > > > +{ > > > + > > > + int ret; > > > + > > > + /* Valid output buffer */ > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + valid_fb->fb_id, NULL, false); > > > + igt_assert(ret == 0); > > > + > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + output->id, NULL, false); > > > + igt_assert(ret == -EINVAL); > > > + > > > + /* Zero WRITEBACK_FB_ID */ > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > + 0, NULL, false); > > > + igt_assert(ret == 0); > > > +} > > > + > > > +igt_main > > > +{ > > > + igt_display_t display; > > > + igt_output_t *output; > > > + igt_plane_t *plane; > > > + igt_fb_t input_fb; > > > + drmModeModeInfo mode; > > > + int ret; > > > + > > > + memset(&display, 0, sizeof(display)); > > > + > > > + igt_fixture { > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > + igt_display_require(&display, display.drm_fd); > > > + > > > + kmstest_set_vt_graphics_mode(); > > > + > > > + igt_display_require(&display, display.drm_fd); > > > + > > > + igt_require(display.is_atomic); > > > + > > > + output = kms_writeback_get_output(&display); > > > + igt_require(output); > > > + > > > + if (output->use_override_mode) > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > + else > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > + > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > + igt_require(plane); > > > > Maybe we can assert on this? > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > + mode.vdisplay, > > > + DRM_FORMAT_XRGB8888, > > > + igt_fb_mod_to_tiling(0), > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > better to make this clear. Agree. The patchset pre-dates the modifiers support (or has the same age, I forgot) > > > > (Applies to other lines of this patch) > > > > > + &input_fb); > > > + igt_assert(ret >= 0); > > > + igt_plane_set_fb(plane, &input_fb); > > > + } > > > + > > > + igt_subtest("writeback-pixel-formats") { > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > Need to drmModeFreePropertyBlob this. > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > + unsigned int i; > > > + char *c; > > > + > > > + /* > > > + * We don't have a comprehensive list of formats, so just check > > > + * that the blob length is sensible and that it doesn't contain > > > + * any outlandish characters > > > + */ > > > + igt_assert(!(formats_blob->length % 4)); > > > + c = formats_blob->data; > > > + for (i = 0; i < formats_blob->length; i++) > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > + "Unexpected character %c\n", c[i]); > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > codes will be made from ASCII characters, in fact some formats already > > have non-printable chars in them. I don't want to have to update this > > test when a new fourcc format is added. Like the comments says, we don't have a full list of formats to check against. Suggestions on how to improve are welcome, but I don't think we should delay (any longer) the patchset due to this. Best regards, Liviu > > > > We currently don't have a test for IN_FORMATS. If we really want to > > check formats, we could have a big array of known formats and add a > > bool is_valid_format(uint32_t fmt) function. > > Agree with you. How about remove this test? > > Thanks > Best Regards > > > > + } > > > + > > > + igt_subtest("writeback-invalid-out-fence") { > > > + igt_fb_t invalid_fb; > > > > invalid_output_fb would be a better name IMHO. > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2, > > > + mode.vdisplay / 2, > > > + DRM_FORMAT_XRGB8888, > > > + igt_fb_mod_to_tiling(0), > > > + &invalid_fb); > > > + igt_require(ret > 0); > > > > We should probably use a different variable: ret is signed, > > igt_create_fb isn't. Also ret doesn't make it clear that the return > > value is the KMS framebuffer ID. > > > > (Applies to other subtests) > > > > > + invalid_out_fence(output, &input_fb, &invalid_fb); > > > > (Still not sure why we provide the input FB to this function.) > > > > > + igt_remove_fb(display.drm_fd, &invalid_fb); > > > + } > > > + > > > + igt_subtest("writeback-fb-id") { > > > + igt_fb_t output_fb; > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay, > > > + DRM_FORMAT_XRGB8888, > > > + igt_fb_mod_to_tiling(0), > > > + &output_fb); > > > + igt_require(ret > 0); > > > + > > > + writeback_fb_id(output, &input_fb, &output_fb); > > > + > > > + igt_remove_fb(display.drm_fd, &output_fb); > > > + } > > > + > > > + igt_fixture { > > > + igt_remove_fb(display.drm_fd, &input_fb); > > > + igt_display_fini(&display); > > > + } > > > +} > > > diff --git a/tests/meson.build b/tests/meson.build > > > index f168fbba..9c77cfcd 100644 > > > --- a/tests/meson.build > > > +++ b/tests/meson.build > > > @@ -63,6 +63,7 @@ test_progs = [ > > > 'kms_universal_plane', > > > 'kms_vblank', > > > 'kms_vrr', > > > + 'kms_writeback', > > > 'meta_test', > > > 'panfrost_get_param', > > > 'panfrost_gem_new', > > > _______________________________________________ > > > igt-dev mailing list > > > igt-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev > > -- > Rodrigo Siqueira > https://siqueira.tech
On 07/12, Ser, Simon wrote: > On Thu, 2019-07-11 at 23:44 -0300, Rodrigo Siqueira wrote: > > On 07/10, Ser, Simon wrote: > > > Hi, > > > > > > Thanks for the patch! Here are a few comments. > > > > > > For bonus points, it would be nice to add igt_describe descriptions of > > > each sub-test. > > > > Hi Simon, > > > > First of all, thanks for your feedback; I already applied most of your > > suggestions. I just have some inline comments/questions. > > > > > On Wed, 2019-06-12 at 23:16 -0300, Brian Starkey wrote: > > > > Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and > > > > WRITEBACK_FB_ID properties on writeback connectors, ensuring their > > > > behaviour is correct. > > > > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > > [rebased and updated do_writeback_test() function to address feedback] > > > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > > > > --- > > > > tests/Makefile.sources | 1 + > > > > tests/kms_writeback.c | 314 +++++++++++++++++++++++++++++++++++++++++ > > > > tests/meson.build | 1 + > > > > 3 files changed, 316 insertions(+) > > > > create mode 100644 tests/kms_writeback.c > > > > > > > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > > > index 027ed82f..03cc8efa 100644 > > > > --- a/tests/Makefile.sources > > > > +++ b/tests/Makefile.sources > > > > @@ -77,6 +77,7 @@ TESTS_progs = \ > > > > kms_universal_plane \ > > > > kms_vblank \ > > > > kms_vrr \ > > > > + kms_writeback \ > > > > meta_test \ > > > > perf \ > > > > perf_pmu \ > > > > diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c > > > > new file mode 100644 > > > > index 00000000..66ef48a6 > > > > --- /dev/null > > > > +++ b/tests/kms_writeback.c > > > > @@ -0,0 +1,314 @@ > > > > +/* > > > > + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. > > > > + * > > > > + * Permission is hereby granted, free of charge, to any person obtaining a > > > > + * copy of this software and associated documentation files (the "Software"), > > > > + * to deal in the Software without restriction, including without limitation > > > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > > > + * and/or sell copies of the Software, and to permit persons to whom the > > > > + * Software is furnished to do so, subject to the following conditions: > > > > + * > > > > + * The above copyright notice and this permission notice (including the next > > > > + * paragraph) shall be included in all copies or substantial portions of the > > > > + * Software. > > > > + * > > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > > > + * IN THE SOFTWARE. > > > > + * > > > > + */ > > > > + > > > > +#include <errno.h> > > > > +#include <stdbool.h> > > > > +#include <stdio.h> > > > > +#include <string.h> > > > > + > > > > +#include "igt.h" > > > > +#include "igt_core.h" > > > > +#include "igt_fb.h" > > > > + > > > > +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output) > > > > +{ > > > > + drmModePropertyBlobRes *blob = NULL; > > > > + uint64_t blob_id; > > > > + int ret; > > > > + > > > > + ret = kmstest_get_property(output->display->drm_fd, > > > > + output->config.connector->connector_id, > > > > + DRM_MODE_OBJECT_CONNECTOR, > > > > + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS], > > > > + NULL, &blob_id, NULL); > > > > + if (ret) > > > > + blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id); > > > > + > > > > + igt_assert(blob); > > > > + > > > > + return blob; > > > > +} > > > > + > > > > +static bool check_writeback_config(igt_display_t *display, igt_output_t *output) > > > > +{ > > > > + igt_fb_t input_fb, output_fb; > > > > + igt_plane_t *plane; > > > > + uint32_t writeback_format = DRM_FORMAT_XRGB8888; > > > > + uint64_t tiling = igt_fb_mod_to_tiling(0); > > > > + int width, height, ret; > > > > + drmModeModeInfo override_mode = { > > > > + .clock = 25175, > > > > + .hdisplay = 640, > > > > + .hsync_start = 656, > > > > + .hsync_end = 752, > > > > + .htotal = 800, > > > > + .hskew = 0, > > > > + .vdisplay = 480, > > > > + .vsync_start = 490, > > > > + .vsync_end = 492, > > > > + .vtotal = 525, > > > > + .vscan = 0, > > > > + .vrefresh = 60, > > > > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > > > > + .name = {"640x480-60"}, > > > > + }; > > > > + igt_output_override_mode(output, &override_mode); > > > > + > > > > + width = override_mode.hdisplay; > > > > + height = override_mode.vdisplay; > > > > + > > > > + ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb); > > > > + igt_assert(ret >= 0); > > > > + > > > > + ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb); > > > > + igt_assert(ret >= 0); > > > > + > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > + igt_plane_set_fb(plane, &input_fb); > > > > + igt_output_set_writeback_fb(output, &output_fb); > > > > + > > > > + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | > > > > + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > > > > Okay, we're using atomic test-only mode to know if we can perform tests > > > with the writeback output. It's probably fine, but we don't use > > > WRITEBACK_PIXEL_FORMATS, which makes me a little bit sad. > > > > Sorry, I did not fully understand part. Did you expect to see something > > like the below code before igt_display_try_commit_atomic()? > > > > igt_output_set_prop_value(output, WRITEBACK_PIXEL_FORMATS, > > DRM_FORMAT_XRGB8888); > > Hmm, no, we cannot change the list of writeback formats (it's > immutable). > > This comment doesn't require any action, it's just me thinking aloud :P > > I'm just thinking that it would be nice to choose our format depending > on the WRITEBACK_PIXEL_FORMATS property. And probably run tests with > each supported format (or, alternatively, choose a random one). That > way we can make sure WRITEBACK_PIXEL_FORMATS isn't broken. > > However, this can be left for a later patch. > > > > > + igt_plane_set_fb(plane, NULL); > > > > + igt_remove_fb(display->drm_fd, &input_fb); > > > > + igt_remove_fb(display->drm_fd, &output_fb); > > > > + > > > > + return !ret; > > > > +} > > > > + > > > > +static igt_output_t *kms_writeback_get_output(igt_display_t *display) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < display->n_outputs; i++) { > > > > + igt_output_t *output = &display->outputs[i]; > > > > + int j; > > > > + > > > > + if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > > > > + continue; > > > > + > > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON); > > > > > > Hmm. Is this really necessary? "Real" userspace won't do this, so I > > > don't think we need to either. > > > > Hmmm, I have a little experience with userspace; however, I tested > > kms_writeback on top of vkms without this line, and everything worked > > well. If I remove this line, should I also drop the line that force > > connector to FORCE_CONNECTOR_UNSPECIFIED? > > I believe so. > > > Another question, if FORCE_CONNECTOR_ON is something that userspace > > won't want to do, why do we have it? > > We use kmstest_force_connector in injection tests: those pick a > disconnected HDMI connector and trick the kernel into thinking it's > connected. We generally also force an EDID and this is used to emulate > a screen (e.g. a screen that supports audio, 4K or 3D). > > This is only meant to be used for testing though, hence "real userspace > shouldn't use it". > Ahhh, I see. Thanks for the explanation > > > > + for (j = 0; j < igt_display_get_n_pipes(display); j++) { > > > > + igt_output_set_pipe(output, j); > > > > + > > > > + if (check_writeback_config(display, output)) { > > > > + igt_debug("Using connector %u:%s on pipe %d\n", > > > > + output->config.connector->connector_id, > > > > + output->name, j); > > > > + return output; > > > > + } > > > > > > We could probably add an igt_debug statement in case we don't use this > > > writeback output. > > > > > > > + } > > > > + > > > > + /* Restore any connectors we don't use, so we don't trip on them later */ > > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED); > > > > + } > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > +static void check_writeback_fb_id(igt_output_t *output) > > > > +{ > > > > + uint64_t check_fb_id; > > > > + > > > > + check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID); > > > > + igt_assert(check_fb_id == 0); > > > > +} > > > > + > > > > +static int do_writeback_test(igt_output_t *output, uint32_t flags, > > > > + uint32_t fb_id, int32_t *out_fence_ptr, > > > > + bool ptr_valid) > > > > > > flags seems to always be set to DRM_MODE_ATOMIC_ALLOW_MODESET. We can > > > probably remove the parameter from this function. > > > > > > > +{ > > > > + int ret; > > > > + igt_display_t *display = output->display; > > > > + struct kmstest_connector_config *config = &output->config; > > > > + > > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id); > > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id); > > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr); > > > > + > > > > + if (ptr_valid) > > > > + *out_fence_ptr = 0; > > > > + > > > > + ret = igt_display_try_commit_atomic(display, flags, NULL); > > > > + > > > > + if (ptr_valid) > > > > + igt_assert(*out_fence_ptr == -1); > > > > > > I'm confused. Why should this be -1 even if we > > > igt_display_try_commit_atomic succeeds? > > > > I inspected the drm_mode_atomic_ioctl() function and I noticed that It > > calls complete_signaling() in its turn this function assign -1 to > > out_fence_ptr. Since all of this happens at the end of atomic_commit, I > > believe that we don’t need it. I already removed it. > > I think I'm still confused :) > > I'm probably missing something. As far as I understand, we are doing an > atomic commit, sometimes with a valid WRITEBACK_FB_ID and > WRITEBACK_OUT_FENCE_PTR. In this case it should start the writeback > process in the kernel and we should get a valid out_fence_ptr? > > Why do we always get -1? FWIU userspace uses the WRITEBACK_OUT_FENCE_PTR to provide a pointer for the kernel to fill with a sync_file file descriptor, which will signal once the writeback is finished. If the signal was correctly handled, the out_fence_ptr would be set to -1, because of this we always get -1. Thanks for your feedback, I'll prepare a new version soon. > > > > + /* WRITEBACK_FB_ID must always read as zero */ > > > > + check_writeback_fb_id(output); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > +{ > > > > + int i, ret; > > > > + int32_t out_fence; > > > > + struct { > > > > + uint32_t fb_id; > > > > + bool ptr_valid; > > > > + int32_t *out_fence_ptr; > > > > + } invalid_tests[] = { > > > > + { > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > + .fb_id = 0, > > > > + .ptr_valid = true, > > > > + .out_fence_ptr = &out_fence, > > > > + }, > > > > + { > > > > + /* Invalid output buffer. */ > > > > + .fb_id = invalid_fb->fb_id, > > > > + .ptr_valid = true, > > > > + .out_fence_ptr = &out_fence, > > > > + }, > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > > > > + { > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > + .fb_id = valid_fb->fb_id, > > > > + .ptr_valid = false, > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > + }, > > > > + }; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + invalid_tests[i].fb_id, > > > > + invalid_tests[i].out_fence_ptr, > > > > + invalid_tests[i].ptr_valid); > > > > + igt_assert(ret != 0); > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > + } > > > > +} > > > > + > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > the input framebuffer. It's probably not a good idea to use the same FB > > > for input and writeback output. > > > > > > > +{ > > > > + > > > > + int ret; > > > > + > > > > + /* Valid output buffer */ > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + valid_fb->fb_id, NULL, false); > > > > + igt_assert(ret == 0); > > > > + > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + output->id, NULL, false); > > > > + igt_assert(ret == -EINVAL); > > > > + > > > > + /* Zero WRITEBACK_FB_ID */ > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + 0, NULL, false); > > > > + igt_assert(ret == 0); > > > > +} > > > > + > > > > +igt_main > > > > +{ > > > > + igt_display_t display; > > > > + igt_output_t *output; > > > > + igt_plane_t *plane; > > > > + igt_fb_t input_fb; > > > > + drmModeModeInfo mode; > > > > + int ret; > > > > + > > > > + memset(&display, 0, sizeof(display)); > > > > + > > > > + igt_fixture { > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > + igt_display_require(&display, display.drm_fd); > > > > + > > > > + kmstest_set_vt_graphics_mode(); > > > > + > > > > + igt_display_require(&display, display.drm_fd); > > > > + > > > > + igt_require(display.is_atomic); > > > > + > > > > + output = kms_writeback_get_output(&display); > > > > + igt_require(output); > > > > + > > > > + if (output->use_override_mode) > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > + else > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > + > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > + igt_require(plane); > > > > > > Maybe we can assert on this? > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > + mode.vdisplay, > > > > + DRM_FORMAT_XRGB8888, > > > > + igt_fb_mod_to_tiling(0), > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > better to make this clear. > > > > > > (Applies to other lines of this patch) > > > > > > > + &input_fb); > > > > + igt_assert(ret >= 0); > > > > + igt_plane_set_fb(plane, &input_fb); > > > > + } > > > > + > > > > + igt_subtest("writeback-pixel-formats") { > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > + unsigned int i; > > > > + char *c; > > > > + > > > > + /* > > > > + * We don't have a comprehensive list of formats, so just check > > > > + * that the blob length is sensible and that it doesn't contain > > > > + * any outlandish characters > > > > + */ > > > > + igt_assert(!(formats_blob->length % 4)); > > > > + c = formats_blob->data; > > > > + for (i = 0; i < formats_blob->length; i++) > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > + "Unexpected character %c\n", c[i]); > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > codes will be made from ASCII characters, in fact some formats already > > > have non-printable chars in them. I don't want to have to update this > > > test when a new fourcc format is added. > > > > > > We currently don't have a test for IN_FORMATS. If we really want to > > > check formats, we could have a big array of known formats and add a > > > bool is_valid_format(uint32_t fmt) function. > > > > Agree with you. How about remove this test? > > I guess we could always keep the length % 4 test, even if it's just a > sanity test. At least this one won't ever need to be changed. > > I don't feel strongly about it. > > > Thanks > > Best Regards > > > > > > + } > > > > + > > > > + igt_subtest("writeback-invalid-out-fence") { > > > > + igt_fb_t invalid_fb; > > > > > > invalid_output_fb would be a better name IMHO. > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2, > > > > + mode.vdisplay / 2, > > > > + DRM_FORMAT_XRGB8888, > > > > + igt_fb_mod_to_tiling(0), > > > > + &invalid_fb); > > > > + igt_require(ret > 0); > > > > > > We should probably use a different variable: ret is signed, > > > igt_create_fb isn't. Also ret doesn't make it clear that the return > > > value is the KMS framebuffer ID. > > > > > > (Applies to other subtests) > > > > > > > + invalid_out_fence(output, &input_fb, &invalid_fb); > > > > > > (Still not sure why we provide the input FB to this function.) > > > > > > > + igt_remove_fb(display.drm_fd, &invalid_fb); > > > > + } > > > > + > > > > + igt_subtest("writeback-fb-id") { > > > > + igt_fb_t output_fb; > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay, > > > > + DRM_FORMAT_XRGB8888, > > > > + igt_fb_mod_to_tiling(0), > > > > + &output_fb); > > > > + igt_require(ret > 0); > > > > + > > > > + writeback_fb_id(output, &input_fb, &output_fb); > > > > + > > > > + igt_remove_fb(display.drm_fd, &output_fb); > > > > + } > > > > + > > > > + igt_fixture { > > > > + igt_remove_fb(display.drm_fd, &input_fb); > > > > + igt_display_fini(&display); > > > > + } > > > > +} > > > > diff --git a/tests/meson.build b/tests/meson.build > > > > index f168fbba..9c77cfcd 100644 > > > > --- a/tests/meson.build > > > > +++ b/tests/meson.build > > > > @@ -63,6 +63,7 @@ test_progs = [ > > > > 'kms_universal_plane', > > > > 'kms_vblank', > > > > 'kms_vrr', > > > > + 'kms_writeback', > > > > 'meta_test', > > > > 'panfrost_get_param', > > > > 'panfrost_gem_new', > > > > _______________________________________________ > > > > igt-dev mailing list > > > > igt-dev@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
Thanks for the clarification! On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@arm.com wrote: > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > +{ > > > > + int i, ret; > > > > + int32_t out_fence; > > > > + struct { > > > > + uint32_t fb_id; > > > > + bool ptr_valid; > > > > + int32_t *out_fence_ptr; > > > > + } invalid_tests[] = { > > > > + { > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > + .fb_id = 0, > > > > + .ptr_valid = true, > > > > + .out_fence_ptr = &out_fence, > > > > + }, > > > > + { > > > > + /* Invalid output buffer. */ > > > > + .fb_id = invalid_fb->fb_id, > > > > + .ptr_valid = true, > > > > + .out_fence_ptr = &out_fence, > > > > + }, > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > move it to writeback_fb_id (and rename that function to test_fb?). > > It tries to test that you can't trick the driver to do any work on a fence if > the framebuffer is invalid, so the set of tests tries: no fb with valid fence, > invalid fb with valid fence, valid fb but invalid fence and assumes that no > fb with invalid fence is a NOP anyway. Yeah, that makes sense, it's just confusing to put it in a function named invalid_out_fence. Here the out fence is valid, but the output buffer isn't, so it should probably be moved away (or this function should be renamed). > > > > + { > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > + .fb_id = valid_fb->fb_id, > > > > + .ptr_valid = false, > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > + }, > > > > + }; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + invalid_tests[i].fb_id, > > > > + invalid_tests[i].out_fence_ptr, > > > > + invalid_tests[i].ptr_valid); > > > > + igt_assert(ret != 0); > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > + } > > > > +} > > > > + > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > the input framebuffer. It's probably not a good idea to use the same FB > > > for input and writeback output. > > > > > > > +{ > > > > + > > > > + int ret; > > > > + > > > > + /* Valid output buffer */ > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + valid_fb->fb_id, NULL, false); > > > > + igt_assert(ret == 0); > > > > + > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + output->id, NULL, false); > > > > + igt_assert(ret == -EINVAL); > > > > + > > > > + /* Zero WRITEBACK_FB_ID */ > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > + 0, NULL, false); > > > > + igt_assert(ret == 0); > > > > +} > > > > + > > > > +igt_main > > > > +{ > > > > + igt_display_t display; > > > > + igt_output_t *output; > > > > + igt_plane_t *plane; > > > > + igt_fb_t input_fb; > > > > + drmModeModeInfo mode; > > > > + int ret; > > > > + > > > > + memset(&display, 0, sizeof(display)); > > > > + > > > > + igt_fixture { > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > + igt_display_require(&display, display.drm_fd); > > > > + > > > > + kmstest_set_vt_graphics_mode(); > > > > + > > > > + igt_display_require(&display, display.drm_fd); > > > > + > > > > + igt_require(display.is_atomic); > > > > + > > > > + output = kms_writeback_get_output(&display); > > > > + igt_require(output); > > > > + > > > > + if (output->use_override_mode) > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > + else > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > + > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > + igt_require(plane); > > > > > > Maybe we can assert on this? > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > + mode.vdisplay, > > > > + DRM_FORMAT_XRGB8888, > > > > + igt_fb_mod_to_tiling(0), > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > better to make this clear. > > Agree. The patchset pre-dates the modifiers support (or has the same age, I > forgot) > > > > (Applies to other lines of this patch) > > > > > > > + &input_fb); > > > > + igt_assert(ret >= 0); > > > > + igt_plane_set_fb(plane, &input_fb); > > > > + } > > > > + > > > > + igt_subtest("writeback-pixel-formats") { > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > + unsigned int i; > > > > + char *c; > > > > + > > > > + /* > > > > + * We don't have a comprehensive list of formats, so just check > > > > + * that the blob length is sensible and that it doesn't contain > > > > + * any outlandish characters > > > > + */ > > > > + igt_assert(!(formats_blob->length % 4)); > > > > + c = formats_blob->data; > > > > + for (i = 0; i < formats_blob->length; i++) > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > + "Unexpected character %c\n", c[i]); > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > codes will be made from ASCII characters, in fact some formats already > > > have non-printable chars in them. I don't want to have to update this > > > test when a new fourcc format is added. > > Like the comments says, we don't have a full list of formats to check against. > Suggestions on how to improve are welcome, but I don't think we should delay > (any longer) the patchset due to this. Agreed. > Best regards, > Liviu
On Tue, 2019-07-16 at 22:21 -0300, Rodrigo Siqueira wrote: > On 07/12, Ser, Simon wrote: > > On Thu, 2019-07-11 at 23:44 -0300, Rodrigo Siqueira wrote: > > > On 07/10, Ser, Simon wrote: > > > > Hi, > > > > > > > > Thanks for the patch! Here are a few comments. > > > > > > > > For bonus points, it would be nice to add igt_describe descriptions of > > > > each sub-test. > > > > > > Hi Simon, > > > > > > First of all, thanks for your feedback; I already applied most of your > > > suggestions. I just have some inline comments/questions. > > > > > > > On Wed, 2019-06-12 at 23:16 -0300, Brian Starkey wrote: > > > > > Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and > > > > > WRITEBACK_FB_ID properties on writeback connectors, ensuring their > > > > > behaviour is correct. > > > > > > > > > > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > > > > > [rebased and updated do_writeback_test() function to address feedback] > > > > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com> > > > > > --- > > > > > tests/Makefile.sources | 1 + > > > > > tests/kms_writeback.c | 314 +++++++++++++++++++++++++++++++++++++++++ > > > > > tests/meson.build | 1 + > > > > > 3 files changed, 316 insertions(+) > > > > > create mode 100644 tests/kms_writeback.c > > > > > > > > > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > > > > index 027ed82f..03cc8efa 100644 > > > > > --- a/tests/Makefile.sources > > > > > +++ b/tests/Makefile.sources > > > > > @@ -77,6 +77,7 @@ TESTS_progs = \ > > > > > kms_universal_plane \ > > > > > kms_vblank \ > > > > > kms_vrr \ > > > > > + kms_writeback \ > > > > > meta_test \ > > > > > perf \ > > > > > perf_pmu \ > > > > > diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c > > > > > new file mode 100644 > > > > > index 00000000..66ef48a6 > > > > > --- /dev/null > > > > > +++ b/tests/kms_writeback.c > > > > > @@ -0,0 +1,314 @@ > > > > > +/* > > > > > + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. > > > > > + * > > > > > + * Permission is hereby granted, free of charge, to any person obtaining a > > > > > + * copy of this software and associated documentation files (the "Software"), > > > > > + * to deal in the Software without restriction, including without limitation > > > > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > > > > + * and/or sell copies of the Software, and to permit persons to whom the > > > > > + * Software is furnished to do so, subject to the following conditions: > > > > > + * > > > > > + * The above copyright notice and this permission notice (including the next > > > > > + * paragraph) shall be included in all copies or substantial portions of the > > > > > + * Software. > > > > > + * > > > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > > > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > > > > + * IN THE SOFTWARE. > > > > > + * > > > > > + */ > > > > > + > > > > > +#include <errno.h> > > > > > +#include <stdbool.h> > > > > > +#include <stdio.h> > > > > > +#include <string.h> > > > > > + > > > > > +#include "igt.h" > > > > > +#include "igt_core.h" > > > > > +#include "igt_fb.h" > > > > > + > > > > > +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output) > > > > > +{ > > > > > + drmModePropertyBlobRes *blob = NULL; > > > > > + uint64_t blob_id; > > > > > + int ret; > > > > > + > > > > > + ret = kmstest_get_property(output->display->drm_fd, > > > > > + output->config.connector->connector_id, > > > > > + DRM_MODE_OBJECT_CONNECTOR, > > > > > + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS], > > > > > + NULL, &blob_id, NULL); > > > > > + if (ret) > > > > > + blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id); > > > > > + > > > > > + igt_assert(blob); > > > > > + > > > > > + return blob; > > > > > +} > > > > > + > > > > > +static bool check_writeback_config(igt_display_t *display, igt_output_t *output) > > > > > +{ > > > > > + igt_fb_t input_fb, output_fb; > > > > > + igt_plane_t *plane; > > > > > + uint32_t writeback_format = DRM_FORMAT_XRGB8888; > > > > > + uint64_t tiling = igt_fb_mod_to_tiling(0); > > > > > + int width, height, ret; > > > > > + drmModeModeInfo override_mode = { > > > > > + .clock = 25175, > > > > > + .hdisplay = 640, > > > > > + .hsync_start = 656, > > > > > + .hsync_end = 752, > > > > > + .htotal = 800, > > > > > + .hskew = 0, > > > > > + .vdisplay = 480, > > > > > + .vsync_start = 490, > > > > > + .vsync_end = 492, > > > > > + .vtotal = 525, > > > > > + .vscan = 0, > > > > > + .vrefresh = 60, > > > > > + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, > > > > > + .name = {"640x480-60"}, > > > > > + }; > > > > > + igt_output_override_mode(output, &override_mode); > > > > > + > > > > > + width = override_mode.hdisplay; > > > > > + height = override_mode.vdisplay; > > > > > + > > > > > + ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb); > > > > > + igt_assert(ret >= 0); > > > > > + > > > > > + ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb); > > > > > + igt_assert(ret >= 0); > > > > > + > > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > > + igt_plane_set_fb(plane, &input_fb); > > > > > + igt_output_set_writeback_fb(output, &output_fb); > > > > > + > > > > > + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | > > > > > + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); > > > > > > > > Okay, we're using atomic test-only mode to know if we can perform tests > > > > with the writeback output. It's probably fine, but we don't use > > > > WRITEBACK_PIXEL_FORMATS, which makes me a little bit sad. > > > > > > Sorry, I did not fully understand part. Did you expect to see something > > > like the below code before igt_display_try_commit_atomic()? > > > > > > igt_output_set_prop_value(output, WRITEBACK_PIXEL_FORMATS, > > > DRM_FORMAT_XRGB8888); > > > > Hmm, no, we cannot change the list of writeback formats (it's > > immutable). > > > > This comment doesn't require any action, it's just me thinking aloud :P > > > > I'm just thinking that it would be nice to choose our format depending > > on the WRITEBACK_PIXEL_FORMATS property. And probably run tests with > > each supported format (or, alternatively, choose a random one). That > > way we can make sure WRITEBACK_PIXEL_FORMATS isn't broken. > > > > However, this can be left for a later patch. > > > > > > > + igt_plane_set_fb(plane, NULL); > > > > > + igt_remove_fb(display->drm_fd, &input_fb); > > > > > + igt_remove_fb(display->drm_fd, &output_fb); > > > > > + > > > > > + return !ret; > > > > > +} > > > > > + > > > > > +static igt_output_t *kms_writeback_get_output(igt_display_t *display) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + for (i = 0; i < display->n_outputs; i++) { > > > > > + igt_output_t *output = &display->outputs[i]; > > > > > + int j; > > > > > + > > > > > + if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > > > > > + continue; > > > > > + > > > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON); > > > > > > > > Hmm. Is this really necessary? "Real" userspace won't do this, so I > > > > don't think we need to either. > > > > > > Hmmm, I have a little experience with userspace; however, I tested > > > kms_writeback on top of vkms without this line, and everything worked > > > well. If I remove this line, should I also drop the line that force > > > connector to FORCE_CONNECTOR_UNSPECIFIED? > > > > I believe so. > > > > > Another question, if FORCE_CONNECTOR_ON is something that userspace > > > won't want to do, why do we have it? > > > > We use kmstest_force_connector in injection tests: those pick a > > disconnected HDMI connector and trick the kernel into thinking it's > > connected. We generally also force an EDID and this is used to emulate > > a screen (e.g. a screen that supports audio, 4K or 3D). > > > > This is only meant to be used for testing though, hence "real userspace > > shouldn't use it". > > > > Ahhh, I see. Thanks for the explanation > > > > > > + for (j = 0; j < igt_display_get_n_pipes(display); j++) { > > > > > + igt_output_set_pipe(output, j); > > > > > + > > > > > + if (check_writeback_config(display, output)) { > > > > > + igt_debug("Using connector %u:%s on pipe %d\n", > > > > > + output->config.connector->connector_id, > > > > > + output->name, j); > > > > > + return output; > > > > > + } > > > > > > > > We could probably add an igt_debug statement in case we don't use this > > > > writeback output. > > > > > > > > > + } > > > > > + > > > > > + /* Restore any connectors we don't use, so we don't trip on them later */ > > > > > + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED); > > > > > + } > > > > > + > > > > > + return NULL; > > > > > +} > > > > > + > > > > > +static void check_writeback_fb_id(igt_output_t *output) > > > > > +{ > > > > > + uint64_t check_fb_id; > > > > > + > > > > > + check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID); > > > > > + igt_assert(check_fb_id == 0); > > > > > +} > > > > > + > > > > > +static int do_writeback_test(igt_output_t *output, uint32_t flags, > > > > > + uint32_t fb_id, int32_t *out_fence_ptr, > > > > > + bool ptr_valid) > > > > > > > > flags seems to always be set to DRM_MODE_ATOMIC_ALLOW_MODESET. We can > > > > probably remove the parameter from this function. > > > > > > > > > +{ > > > > > + int ret; > > > > > + igt_display_t *display = output->display; > > > > > + struct kmstest_connector_config *config = &output->config; > > > > > + > > > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id); > > > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id); > > > > > + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr); > > > > > + > > > > > + if (ptr_valid) > > > > > + *out_fence_ptr = 0; > > > > > + > > > > > + ret = igt_display_try_commit_atomic(display, flags, NULL); > > > > > + > > > > > + if (ptr_valid) > > > > > + igt_assert(*out_fence_ptr == -1); > > > > > > > > I'm confused. Why should this be -1 even if we > > > > igt_display_try_commit_atomic succeeds? > > > > > > I inspected the drm_mode_atomic_ioctl() function and I noticed that It > > > calls complete_signaling() in its turn this function assign -1 to > > > out_fence_ptr. Since all of this happens at the end of atomic_commit, I > > > believe that we don’t need it. I already removed it. > > > > I think I'm still confused :) > > > > I'm probably missing something. As far as I understand, we are doing an > > atomic commit, sometimes with a valid WRITEBACK_FB_ID and > > WRITEBACK_OUT_FENCE_PTR. In this case it should start the writeback > > process in the kernel and we should get a valid out_fence_ptr? > > > > Why do we always get -1? > > FWIU userspace uses the WRITEBACK_OUT_FENCE_PTR to provide a pointer for > the kernel to fill with a sync_file file descriptor, which will signal > once the writeback is finished. If the signal was correctly handled, the > out_fence_ptr would be set to -1, because of this we always get -1. But in case a writeback operation has been successfully started, it won't be -1 (since it will be a valid FD). As I've understood it, this function will always be called without triggering a writeback operation. Maybe we should rename do_writeback_test to do_noop_writeback (or a better name) and add a comment explaining that *out_fence_ptr will be -1 because: - Either ret != 0 and we submitted a bad commit - Either ret == 0 (valid commit) but out_fence_ptr == NULL (so no writeback operation started) Maybe we could igt_assert(ret != 0 || out_fence_ptr == NULL) to make sure we don't call this function and successfully trigger a writeback operation. What do you think? > Thanks for your feedback, I'll prepare a new version soon. > > > > > > + /* WRITEBACK_FB_ID must always read as zero */ > > > > > + check_writeback_fb_id(output); > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > +{ > > > > > + int i, ret; > > > > > + int32_t out_fence; > > > > > + struct { > > > > > + uint32_t fb_id; > > > > > + bool ptr_valid; > > > > > + int32_t *out_fence_ptr; > > > > > + } invalid_tests[] = { > > > > > + { > > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > > + .fb_id = 0, > > > > > + .ptr_valid = true, > > > > > + .out_fence_ptr = &out_fence, > > > > > + }, > > > > > + { > > > > > + /* Invalid output buffer. */ > > > > > + .fb_id = invalid_fb->fb_id, > > > > > + .ptr_valid = true, > > > > > + .out_fence_ptr = &out_fence, > > > > > + }, > > > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > > > > > > + { > > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > > + .fb_id = valid_fb->fb_id, > > > > > + .ptr_valid = false, > > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > > + }, > > > > > + }; > > > > > + > > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + invalid_tests[i].fb_id, > > > > > + invalid_tests[i].out_fence_ptr, > > > > > + invalid_tests[i].ptr_valid); > > > > > + igt_assert(ret != 0); > > > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > > > + } > > > > > +} > > > > > + > > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > > the input framebuffer. It's probably not a good idea to use the same FB > > > > for input and writeback output. > > > > > > > > > +{ > > > > > + > > > > > + int ret; > > > > > + > > > > > + /* Valid output buffer */ > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + valid_fb->fb_id, NULL, false); > > > > > + igt_assert(ret == 0); > > > > > + > > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + output->id, NULL, false); > > > > > + igt_assert(ret == -EINVAL); > > > > > + > > > > > + /* Zero WRITEBACK_FB_ID */ > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + 0, NULL, false); > > > > > + igt_assert(ret == 0); > > > > > +} > > > > > + > > > > > +igt_main > > > > > +{ > > > > > + igt_display_t display; > > > > > + igt_output_t *output; > > > > > + igt_plane_t *plane; > > > > > + igt_fb_t input_fb; > > > > > + drmModeModeInfo mode; > > > > > + int ret; > > > > > + > > > > > + memset(&display, 0, sizeof(display)); > > > > > + > > > > > + igt_fixture { > > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > > + igt_display_require(&display, display.drm_fd); > > > > > + > > > > > + kmstest_set_vt_graphics_mode(); > > > > > + > > > > > + igt_display_require(&display, display.drm_fd); > > > > > + > > > > > + igt_require(display.is_atomic); > > > > > + > > > > > + output = kms_writeback_get_output(&display); > > > > > + igt_require(output); > > > > > + > > > > > + if (output->use_override_mode) > > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > > + else > > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > > + > > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > > + igt_require(plane); > > > > > > > > Maybe we can assert on this? > > > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > > + mode.vdisplay, > > > > > + DRM_FORMAT_XRGB8888, > > > > > + igt_fb_mod_to_tiling(0), > > > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > > better to make this clear. > > > > > > > > (Applies to other lines of this patch) > > > > > > > > > + &input_fb); > > > > > + igt_assert(ret >= 0); > > > > > + igt_plane_set_fb(plane, &input_fb); > > > > > + } > > > > > + > > > > > + igt_subtest("writeback-pixel-formats") { > > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > > + unsigned int i; > > > > > + char *c; > > > > > + > > > > > + /* > > > > > + * We don't have a comprehensive list of formats, so just check > > > > > + * that the blob length is sensible and that it doesn't contain > > > > > + * any outlandish characters > > > > > + */ > > > > > + igt_assert(!(formats_blob->length % 4)); > > > > > + c = formats_blob->data; > > > > > + for (i = 0; i < formats_blob->length; i++) > > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > > + "Unexpected character %c\n", c[i]); > > > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > > codes will be made from ASCII characters, in fact some formats already > > > > have non-printable chars in them. I don't want to have to update this > > > > test when a new fourcc format is added. > > > > > > > > We currently don't have a test for IN_FORMATS. If we really want to > > > > check formats, we could have a big array of known formats and add a > > > > bool is_valid_format(uint32_t fmt) function. > > > > > > Agree with you. How about remove this test? > > > > I guess we could always keep the length % 4 test, even if it's just a > > sanity test. At least this one won't ever need to be changed. > > > > I don't feel strongly about it. > > > > > Thanks > > > Best Regards > > > > > > > > + } > > > > > + > > > > > + igt_subtest("writeback-invalid-out-fence") { > > > > > + igt_fb_t invalid_fb; > > > > > > > > invalid_output_fb would be a better name IMHO. > > > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2, > > > > > + mode.vdisplay / 2, > > > > > + DRM_FORMAT_XRGB8888, > > > > > + igt_fb_mod_to_tiling(0), > > > > > + &invalid_fb); > > > > > + igt_require(ret > 0); > > > > > > > > We should probably use a different variable: ret is signed, > > > > igt_create_fb isn't. Also ret doesn't make it clear that the return > > > > value is the KMS framebuffer ID. > > > > > > > > (Applies to other subtests) > > > > > > > > > + invalid_out_fence(output, &input_fb, &invalid_fb); > > > > > > > > (Still not sure why we provide the input FB to this function.) > > > > > > > > > + igt_remove_fb(display.drm_fd, &invalid_fb); > > > > > + } > > > > > + > > > > > + igt_subtest("writeback-fb-id") { > > > > > + igt_fb_t output_fb; > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay, > > > > > + DRM_FORMAT_XRGB8888, > > > > > + igt_fb_mod_to_tiling(0), > > > > > + &output_fb); > > > > > + igt_require(ret > 0); > > > > > + > > > > > + writeback_fb_id(output, &input_fb, &output_fb); > > > > > + > > > > > + igt_remove_fb(display.drm_fd, &output_fb); > > > > > + } > > > > > + > > > > > + igt_fixture { > > > > > + igt_remove_fb(display.drm_fd, &input_fb); > > > > > + igt_display_fini(&display); > > > > > + } > > > > > +} > > > > > diff --git a/tests/meson.build b/tests/meson.build > > > > > index f168fbba..9c77cfcd 100644 > > > > > --- a/tests/meson.build > > > > > +++ b/tests/meson.build > > > > > @@ -63,6 +63,7 @@ test_progs = [ > > > > > 'kms_universal_plane', > > > > > 'kms_vblank', > > > > > 'kms_vrr', > > > > > + 'kms_writeback', > > > > > 'meta_test', > > > > > 'panfrost_get_param', > > > > > 'panfrost_gem_new', > > > > > _______________________________________________ > > > > > igt-dev mailing list > > > > > igt-dev@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
On Wed, Jul 17, 2019 at 11:46:39AM +0000, Ser, Simon wrote: > Thanks for the clarification! > > On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@arm.com wrote: > > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > +{ > > > > > + int i, ret; > > > > > + int32_t out_fence; > > > > > + struct { > > > > > + uint32_t fb_id; > > > > > + bool ptr_valid; > > > > > + int32_t *out_fence_ptr; > > > > > + } invalid_tests[] = { > > > > > + { > > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > > + .fb_id = 0, > > > > > + .ptr_valid = true, > > > > > + .out_fence_ptr = &out_fence, > > > > > + }, > > > > > + { > > > > > + /* Invalid output buffer. */ > > > > > + .fb_id = invalid_fb->fb_id, > > > > > + .ptr_valid = true, > > > > > + .out_fence_ptr = &out_fence, > > > > > + }, > > > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > It tries to test that you can't trick the driver to do any work on a fence if > > the framebuffer is invalid, so the set of tests tries: no fb with valid fence, > > invalid fb with valid fence, valid fb but invalid fence and assumes that no > > fb with invalid fence is a NOP anyway. > > Yeah, that makes sense, it's just confusing to put it in a function > named invalid_out_fence. Here the out fence is valid, but the output > buffer isn't, so it should probably be moved away (or this function > should be renamed). Don't want to offend or anything, but this does sound like bikeshedding. You have a couple of parameters that you want to have a test for because they are linked together (output framebuffer and fence) and you go through the combination of possible bad options in the test. Not sure what name we can use for the function, other than maybe 'test_invalid_parameters'? Given that 2/3 tests an invalid out fence, the name was thought to be relevant. Having invalid out buffer test separate into its own test brings no benefits, IMHO. Best regards, Liviu > > > > > > + { > > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > > + .fb_id = valid_fb->fb_id, > > > > > + .ptr_valid = false, > > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > > + }, > > > > > + }; > > > > > + > > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + invalid_tests[i].fb_id, > > > > > + invalid_tests[i].out_fence_ptr, > > > > > + invalid_tests[i].ptr_valid); > > > > > + igt_assert(ret != 0); > > > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > > > + } > > > > > +} > > > > > + > > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > > the input framebuffer. It's probably not a good idea to use the same FB > > > > for input and writeback output. > > > > > > > > > +{ > > > > > + > > > > > + int ret; > > > > > + > > > > > + /* Valid output buffer */ > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + valid_fb->fb_id, NULL, false); > > > > > + igt_assert(ret == 0); > > > > > + > > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + output->id, NULL, false); > > > > > + igt_assert(ret == -EINVAL); > > > > > + > > > > > + /* Zero WRITEBACK_FB_ID */ > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > + 0, NULL, false); > > > > > + igt_assert(ret == 0); > > > > > +} > > > > > + > > > > > +igt_main > > > > > +{ > > > > > + igt_display_t display; > > > > > + igt_output_t *output; > > > > > + igt_plane_t *plane; > > > > > + igt_fb_t input_fb; > > > > > + drmModeModeInfo mode; > > > > > + int ret; > > > > > + > > > > > + memset(&display, 0, sizeof(display)); > > > > > + > > > > > + igt_fixture { > > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > > + igt_display_require(&display, display.drm_fd); > > > > > + > > > > > + kmstest_set_vt_graphics_mode(); > > > > > + > > > > > + igt_display_require(&display, display.drm_fd); > > > > > + > > > > > + igt_require(display.is_atomic); > > > > > + > > > > > + output = kms_writeback_get_output(&display); > > > > > + igt_require(output); > > > > > + > > > > > + if (output->use_override_mode) > > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > > + else > > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > > + > > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > > + igt_require(plane); > > > > > > > > Maybe we can assert on this? > > > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > > + mode.vdisplay, > > > > > + DRM_FORMAT_XRGB8888, > > > > > + igt_fb_mod_to_tiling(0), > > > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > > better to make this clear. > > > > Agree. The patchset pre-dates the modifiers support (or has the same age, I > > forgot) > > > > > > (Applies to other lines of this patch) > > > > > > > > > + &input_fb); > > > > > + igt_assert(ret >= 0); > > > > > + igt_plane_set_fb(plane, &input_fb); > > > > > + } > > > > > + > > > > > + igt_subtest("writeback-pixel-formats") { > > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > > + unsigned int i; > > > > > + char *c; > > > > > + > > > > > + /* > > > > > + * We don't have a comprehensive list of formats, so just check > > > > > + * that the blob length is sensible and that it doesn't contain > > > > > + * any outlandish characters > > > > > + */ > > > > > + igt_assert(!(formats_blob->length % 4)); > > > > > + c = formats_blob->data; > > > > > + for (i = 0; i < formats_blob->length; i++) > > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > > + "Unexpected character %c\n", c[i]); > > > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > > codes will be made from ASCII characters, in fact some formats already > > > > have non-printable chars in them. I don't want to have to update this > > > > test when a new fourcc format is added. > > > > Like the comments says, we don't have a full list of formats to check against. > > Suggestions on how to improve are welcome, but I don't think we should delay > > (any longer) the patchset due to this. > > Agreed. > > > Best regards, > > Liviu
On Thu, 2019-07-18 at 10:49 +0100, Liviu.Dudau@arm.com wrote: > On Wed, Jul 17, 2019 at 11:46:39AM +0000, Ser, Simon wrote: > > Thanks for the clarification! > > > > On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@arm.com wrote: > > > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > +{ > > > > > > + int i, ret; > > > > > > + int32_t out_fence; > > > > > > + struct { > > > > > > + uint32_t fb_id; > > > > > > + bool ptr_valid; > > > > > > + int32_t *out_fence_ptr; > > > > > > + } invalid_tests[] = { > > > > > > + { > > > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > > > + .fb_id = 0, > > > > > > + .ptr_valid = true, > > > > > > + .out_fence_ptr = &out_fence, > > > > > > + }, > > > > > > + { > > > > > > + /* Invalid output buffer. */ > > > > > > + .fb_id = invalid_fb->fb_id, > > > > > > + .ptr_valid = true, > > > > > > + .out_fence_ptr = &out_fence, > > > > > > + }, > > > > > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > > > It tries to test that you can't trick the driver to do any work on a fence if > > > the framebuffer is invalid, so the set of tests tries: no fb with valid fence, > > > invalid fb with valid fence, valid fb but invalid fence and assumes that no > > > fb with invalid fence is a NOP anyway. > > > > Yeah, that makes sense, it's just confusing to put it in a function > > named invalid_out_fence. Here the out fence is valid, but the output > > buffer isn't, so it should probably be moved away (or this function > > should be renamed). > > Don't want to offend or anything, but this does sound like bikeshedding. You > have a couple of parameters that you want to have a test for because they are > linked together (output framebuffer and fence) and you go through the > combination of possible bad options in the test. Not sure what name we can use > for the function, other than maybe 'test_invalid_parameters'? Given that 2/3 > tests an invalid out fence, the name was thought to be relevant. > > Having invalid out buffer test separate into its own test brings no benefits, IMHO. Well, the issue is that I've been confused when reviewing the patch series. I had trouble understanding what the test does and why. I also had trouble to identify that do_writeback_test never submits a writeback operation (see other e-mail). A name that is relevant "all the time, most of the time", is not relevant at all in my opinion. It just tricks the reader into thinking the test does one thing, while it also does something else. If it would be obvious, I wouldn't mind. But here IMHO it hurts readability. So I'd prefer to rename the function. If you think it's obvious, then maybe it's just me. I'd love to hear from others if they have a different opinion. (As a side note, I agree I have a tendency to bikeshed, I try to mark my bikesheddings behind "nit:" flags.) > Best regards, > Liviu > > > > > > > + { > > > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > > > + .fb_id = valid_fb->fb_id, > > > > > > + .ptr_valid = false, > > > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > > > + }, > > > > > > + }; > > > > > > + > > > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > + invalid_tests[i].fb_id, > > > > > > + invalid_tests[i].out_fence_ptr, > > > > > > + invalid_tests[i].ptr_valid); > > > > > > + igt_assert(ret != 0); > > > > > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > > > > > + } > > > > > > +} > > > > > > + > > > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > > > the input framebuffer. It's probably not a good idea to use the same FB > > > > > for input and writeback output. > > > > > > > > > > > +{ > > > > > > + > > > > > > + int ret; > > > > > > + > > > > > > + /* Valid output buffer */ > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > + valid_fb->fb_id, NULL, false); > > > > > > + igt_assert(ret == 0); > > > > > > + > > > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > + output->id, NULL, false); > > > > > > + igt_assert(ret == -EINVAL); > > > > > > + > > > > > > + /* Zero WRITEBACK_FB_ID */ > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > + 0, NULL, false); > > > > > > + igt_assert(ret == 0); > > > > > > +} > > > > > > + > > > > > > +igt_main > > > > > > +{ > > > > > > + igt_display_t display; > > > > > > + igt_output_t *output; > > > > > > + igt_plane_t *plane; > > > > > > + igt_fb_t input_fb; > > > > > > + drmModeModeInfo mode; > > > > > > + int ret; > > > > > > + > > > > > > + memset(&display, 0, sizeof(display)); > > > > > > + > > > > > > + igt_fixture { > > > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > + > > > > > > + kmstest_set_vt_graphics_mode(); > > > > > > + > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > + > > > > > > + igt_require(display.is_atomic); > > > > > > + > > > > > > + output = kms_writeback_get_output(&display); > > > > > > + igt_require(output); > > > > > > + > > > > > > + if (output->use_override_mode) > > > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > > > + else > > > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > > > + > > > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > > > + igt_require(plane); > > > > > > > > > > Maybe we can assert on this? > > > > > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > > > + mode.vdisplay, > > > > > > + DRM_FORMAT_XRGB8888, > > > > > > + igt_fb_mod_to_tiling(0), > > > > > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > > > better to make this clear. > > > > > > Agree. The patchset pre-dates the modifiers support (or has the same age, I > > > forgot) > > > > > > > > (Applies to other lines of this patch) > > > > > > > > > > > + &input_fb); > > > > > > + igt_assert(ret >= 0); > > > > > > + igt_plane_set_fb(plane, &input_fb); > > > > > > + } > > > > > > + > > > > > > + igt_subtest("writeback-pixel-formats") { > > > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > > > + unsigned int i; > > > > > > + char *c; > > > > > > + > > > > > > + /* > > > > > > + * We don't have a comprehensive list of formats, so just check > > > > > > + * that the blob length is sensible and that it doesn't contain > > > > > > + * any outlandish characters > > > > > > + */ > > > > > > + igt_assert(!(formats_blob->length % 4)); > > > > > > + c = formats_blob->data; > > > > > > + for (i = 0; i < formats_blob->length; i++) > > > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > > > + "Unexpected character %c\n", c[i]); > > > > > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > > > codes will be made from ASCII characters, in fact some formats already > > > > > have non-printable chars in them. I don't want to have to update this > > > > > test when a new fourcc format is added. > > > > > > Like the comments says, we don't have a full list of formats to check against. > > > Suggestions on how to improve are welcome, but I don't think we should delay > > > (any longer) the patchset due to this. > > > > Agreed. > > > > > Best regards, > > > Liviu
On Thu, Jul 18, 2019 at 09:56:39AM +0000, Ser, Simon wrote: > On Thu, 2019-07-18 at 10:49 +0100, Liviu.Dudau@arm.com wrote: > > On Wed, Jul 17, 2019 at 11:46:39AM +0000, Ser, Simon wrote: > > > Thanks for the clarification! > > > > > > On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@arm.com wrote: > > > > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > +{ > > > > > > > + int i, ret; > > > > > > > + int32_t out_fence; > > > > > > > + struct { > > > > > > > + uint32_t fb_id; > > > > > > > + bool ptr_valid; > > > > > > > + int32_t *out_fence_ptr; > > > > > > > + } invalid_tests[] = { > > > > > > > + { > > > > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > > > > + .fb_id = 0, > > > > > > > + .ptr_valid = true, > > > > > > > + .out_fence_ptr = &out_fence, > > > > > > > + }, > > > > > > > + { > > > > > > > + /* Invalid output buffer. */ > > > > > > > + .fb_id = invalid_fb->fb_id, > > > > > > > + .ptr_valid = true, > > > > > > > + .out_fence_ptr = &out_fence, > > > > > > > + }, > > > > > > > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > > > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > > > > > It tries to test that you can't trick the driver to do any work on a fence if > > > > the framebuffer is invalid, so the set of tests tries: no fb with valid fence, > > > > invalid fb with valid fence, valid fb but invalid fence and assumes that no > > > > fb with invalid fence is a NOP anyway. > > > > > > Yeah, that makes sense, it's just confusing to put it in a function > > > named invalid_out_fence. Here the out fence is valid, but the output > > > buffer isn't, so it should probably be moved away (or this function > > > should be renamed). > > > > Don't want to offend or anything, but this does sound like bikeshedding. You > > have a couple of parameters that you want to have a test for because they are > > linked together (output framebuffer and fence) and you go through the > > combination of possible bad options in the test. Not sure what name we can use > > for the function, other than maybe 'test_invalid_parameters'? Given that 2/3 > > tests an invalid out fence, the name was thought to be relevant. > > > > Having invalid out buffer test separate into its own test brings no benefits, IMHO. > > Well, the issue is that I've been confused when reviewing the patch > series. I had trouble understanding what the test does and why. I also > had trouble to identify that do_writeback_test never submits a > writeback operation (see other e-mail). > > A name that is relevant "all the time, most of the time", is not > relevant at all in my opinion. It just tricks the reader into thinking > the test does one thing, while it also does something else. > > If it would be obvious, I wouldn't mind. But here IMHO it hurts > readability. So I'd prefer to rename the function. I take your comments as a valid point. Does "test_invalid_parameters" sound like a good name for the function? Is so, Rodrigo, can you please use that name in the next revision of the patch? > > If you think it's obvious, then maybe it's just me. I'd love to hear > from others if they have a different opinion. > > (As a side note, I agree I have a tendency to bikeshed, I try to mark > my bikesheddings behind "nit:" flags.) I've only said "it sounded like" :) Best regards, Liviu > > > Best regards, > > Liviu > > > > > > > > > + { > > > > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > > > > + .fb_id = valid_fb->fb_id, > > > > > > > + .ptr_valid = false, > > > > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > > > > + }, > > > > > > > + }; > > > > > > > + > > > > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > + invalid_tests[i].fb_id, > > > > > > > + invalid_tests[i].out_fence_ptr, > > > > > > > + invalid_tests[i].ptr_valid); > > > > > > > + igt_assert(ret != 0); > > > > > > > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > > > > the input framebuffer. It's probably not a good idea to use the same FB > > > > > > for input and writeback output. > > > > > > > > > > > > > +{ > > > > > > > + > > > > > > > + int ret; > > > > > > > + > > > > > > > + /* Valid output buffer */ > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > + valid_fb->fb_id, NULL, false); > > > > > > > + igt_assert(ret == 0); > > > > > > > + > > > > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > + output->id, NULL, false); > > > > > > > + igt_assert(ret == -EINVAL); > > > > > > > + > > > > > > > + /* Zero WRITEBACK_FB_ID */ > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > + 0, NULL, false); > > > > > > > + igt_assert(ret == 0); > > > > > > > +} > > > > > > > + > > > > > > > +igt_main > > > > > > > +{ > > > > > > > + igt_display_t display; > > > > > > > + igt_output_t *output; > > > > > > > + igt_plane_t *plane; > > > > > > > + igt_fb_t input_fb; > > > > > > > + drmModeModeInfo mode; > > > > > > > + int ret; > > > > > > > + > > > > > > > + memset(&display, 0, sizeof(display)); > > > > > > > + > > > > > > > + igt_fixture { > > > > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > > + > > > > > > > + kmstest_set_vt_graphics_mode(); > > > > > > > + > > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > > + > > > > > > > + igt_require(display.is_atomic); > > > > > > > + > > > > > > > + output = kms_writeback_get_output(&display); > > > > > > > + igt_require(output); > > > > > > > + > > > > > > > + if (output->use_override_mode) > > > > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > > > > + else > > > > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > > > > + > > > > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > > > > + igt_require(plane); > > > > > > > > > > > > Maybe we can assert on this? > > > > > > > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > > > > + mode.vdisplay, > > > > > > > + DRM_FORMAT_XRGB8888, > > > > > > > + igt_fb_mod_to_tiling(0), > > > > > > > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > > > > better to make this clear. > > > > > > > > Agree. The patchset pre-dates the modifiers support (or has the same age, I > > > > forgot) > > > > > > > > > > (Applies to other lines of this patch) > > > > > > > > > > > > > + &input_fb); > > > > > > > + igt_assert(ret >= 0); > > > > > > > + igt_plane_set_fb(plane, &input_fb); > > > > > > > + } > > > > > > > + > > > > > > > + igt_subtest("writeback-pixel-formats") { > > > > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > > > > + unsigned int i; > > > > > > > + char *c; > > > > > > > + > > > > > > > + /* > > > > > > > + * We don't have a comprehensive list of formats, so just check > > > > > > > + * that the blob length is sensible and that it doesn't contain > > > > > > > + * any outlandish characters > > > > > > > + */ > > > > > > > + igt_assert(!(formats_blob->length % 4)); > > > > > > > + c = formats_blob->data; > > > > > > > + for (i = 0; i < formats_blob->length; i++) > > > > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > > > > + "Unexpected character %c\n", c[i]); > > > > > > > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > > > > codes will be made from ASCII characters, in fact some formats already > > > > > > have non-printable chars in them. I don't want to have to update this > > > > > > test when a new fourcc format is added. > > > > > > > > Like the comments says, we don't have a full list of formats to check against. > > > > Suggestions on how to improve are welcome, but I don't think we should delay > > > > (any longer) the patchset due to this. > > > > > > Agreed. > > > > > > > Best regards, > > > > Liviu
On Thu, 2019-07-18 at 12:15 +0100, Liviu.Dudau@arm.com wrote: > On Thu, Jul 18, 2019 at 09:56:39AM +0000, Ser, Simon wrote: > > On Thu, 2019-07-18 at 10:49 +0100, Liviu.Dudau@arm.com wrote: > > > On Wed, Jul 17, 2019 at 11:46:39AM +0000, Ser, Simon wrote: > > > > Thanks for the clarification! > > > > > > > > On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@arm.com wrote: > > > > > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > +{ > > > > > > > > + int i, ret; > > > > > > > > + int32_t out_fence; > > > > > > > > + struct { > > > > > > > > + uint32_t fb_id; > > > > > > > > + bool ptr_valid; > > > > > > > > + int32_t *out_fence_ptr; > > > > > > > > + } invalid_tests[] = { > > > > > > > > + { > > > > > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > > > > > + .fb_id = 0, > > > > > > > > + .ptr_valid = true, > > > > > > > > + .out_fence_ptr = &out_fence, > > > > > > > > + }, > > > > > > > > + { > > > > > > > > + /* Invalid output buffer. */ > > > > > > > > + .fb_id = invalid_fb->fb_id, > > > > > > > > + .ptr_valid = true, > > > > > > > > + .out_fence_ptr = &out_fence, > > > > > > > > + }, > > > > > > > > > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > > > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > > > > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > > > > > > > It tries to test that you can't trick the driver to do any work on a fence if > > > > > the framebuffer is invalid, so the set of tests tries: no fb with valid fence, > > > > > invalid fb with valid fence, valid fb but invalid fence and assumes that no > > > > > fb with invalid fence is a NOP anyway. > > > > > > > > Yeah, that makes sense, it's just confusing to put it in a function > > > > named invalid_out_fence. Here the out fence is valid, but the output > > > > buffer isn't, so it should probably be moved away (or this function > > > > should be renamed). > > > > > > Don't want to offend or anything, but this does sound like bikeshedding. You > > > have a couple of parameters that you want to have a test for because they are > > > linked together (output framebuffer and fence) and you go through the > > > combination of possible bad options in the test. Not sure what name we can use > > > for the function, other than maybe 'test_invalid_parameters'? Given that 2/3 > > > tests an invalid out fence, the name was thought to be relevant. > > > > > > Having invalid out buffer test separate into its own test brings no benefits, IMHO. > > > > Well, the issue is that I've been confused when reviewing the patch > > series. I had trouble understanding what the test does and why. I also > > had trouble to identify that do_writeback_test never submits a > > writeback operation (see other e-mail). > > > > A name that is relevant "all the time, most of the time", is not > > relevant at all in my opinion. It just tricks the reader into thinking > > the test does one thing, while it also does something else. > > > > If it would be obvious, I wouldn't mind. But here IMHO it hurts > > readability. So I'd prefer to rename the function. > > I take your comments as a valid point. > > Does "test_invalid_parameters" sound like a good name for the function? Is so, > Rodrigo, can you please use that name in the next revision of the patch? Yes, this name sounds perfectly fine :) > > If you think it's obvious, then maybe it's just me. I'd love to hear > > from others if they have a different opinion. > > > > (As a side note, I agree I have a tendency to bikeshed, I try to mark > > my bikesheddings behind "nit:" flags.) > > I've only said "it sounded like" :) Eheh :P > Best regards, > Liviu > > > > Best regards, > > > Liviu > > > > > > > > > > > + { > > > > > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > > > > > + .fb_id = valid_fb->fb_id, > > > > > > > > + .ptr_valid = false, > > > > > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > > > > > + }, > > > > > > > > + }; > > > > > > > > + > > > > > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + invalid_tests[i].fb_id, > > > > > > > > + invalid_tests[i].out_fence_ptr, > > > > > > > > + invalid_tests[i].ptr_valid); > > > > > > > > + igt_assert(ret != 0); > > > > > > > > > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > > > > > the input framebuffer. It's probably not a good idea to use the same FB > > > > > > > for input and writeback output. > > > > > > > > > > > > > > > +{ > > > > > > > > + > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + /* Valid output buffer */ > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + valid_fb->fb_id, NULL, false); > > > > > > > > + igt_assert(ret == 0); > > > > > > > > + > > > > > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + output->id, NULL, false); > > > > > > > > + igt_assert(ret == -EINVAL); > > > > > > > > + > > > > > > > > + /* Zero WRITEBACK_FB_ID */ > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + 0, NULL, false); > > > > > > > > + igt_assert(ret == 0); > > > > > > > > +} > > > > > > > > + > > > > > > > > +igt_main > > > > > > > > +{ > > > > > > > > + igt_display_t display; > > > > > > > > + igt_output_t *output; > > > > > > > > + igt_plane_t *plane; > > > > > > > > + igt_fb_t input_fb; > > > > > > > > + drmModeModeInfo mode; > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + memset(&display, 0, sizeof(display)); > > > > > > > > + > > > > > > > > + igt_fixture { > > > > > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > > > + > > > > > > > > + kmstest_set_vt_graphics_mode(); > > > > > > > > + > > > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > > > + > > > > > > > > + igt_require(display.is_atomic); > > > > > > > > + > > > > > > > > + output = kms_writeback_get_output(&display); > > > > > > > > + igt_require(output); > > > > > > > > + > > > > > > > > + if (output->use_override_mode) > > > > > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > > > > > + else > > > > > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > > > > > + > > > > > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > > > > > + igt_require(plane); > > > > > > > > > > > > > > Maybe we can assert on this? > > > > > > > > > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > > > > > + mode.vdisplay, > > > > > > > > + DRM_FORMAT_XRGB8888, > > > > > > > > + igt_fb_mod_to_tiling(0), > > > > > > > > > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > > > > > better to make this clear. > > > > > > > > > > Agree. The patchset pre-dates the modifiers support (or has the same age, I > > > > > forgot) > > > > > > > > > > > > (Applies to other lines of this patch) > > > > > > > > > > > > > > > + &input_fb); > > > > > > > > + igt_assert(ret >= 0); > > > > > > > > + igt_plane_set_fb(plane, &input_fb); > > > > > > > > + } > > > > > > > > + > > > > > > > > + igt_subtest("writeback-pixel-formats") { > > > > > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > > > > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > > > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > > > > > + unsigned int i; > > > > > > > > + char *c; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * We don't have a comprehensive list of formats, so just check > > > > > > > > + * that the blob length is sensible and that it doesn't contain > > > > > > > > + * any outlandish characters > > > > > > > > + */ > > > > > > > > + igt_assert(!(formats_blob->length % 4)); > > > > > > > > + c = formats_blob->data; > > > > > > > > + for (i = 0; i < formats_blob->length; i++) > > > > > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > > > > > + "Unexpected character %c\n", c[i]); > > > > > > > > > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > > > > > codes will be made from ASCII characters, in fact some formats already > > > > > > > have non-printable chars in them. I don't want to have to update this > > > > > > > test when a new fourcc format is added. > > > > > > > > > > Like the comments says, we don't have a full list of formats to check against. > > > > > Suggestions on how to improve are welcome, but I don't think we should delay > > > > > (any longer) the patchset due to this. > > > > > > > > Agreed. > > > > > > > > > Best regards, > > > > > Liviu
On Thu, Jul 18, 2019 at 8:15 AM Liviu.Dudau@arm.com <Liviu.Dudau@arm.com> wrote: > > On Thu, Jul 18, 2019 at 09:56:39AM +0000, Ser, Simon wrote: > > On Thu, 2019-07-18 at 10:49 +0100, Liviu.Dudau@arm.com wrote: > > > On Wed, Jul 17, 2019 at 11:46:39AM +0000, Ser, Simon wrote: > > > > Thanks for the clarification! > > > > > > > > On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@arm.com wrote: > > > > > > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > +{ > > > > > > > > + int i, ret; > > > > > > > > + int32_t out_fence; > > > > > > > > + struct { > > > > > > > > + uint32_t fb_id; > > > > > > > > + bool ptr_valid; > > > > > > > > + int32_t *out_fence_ptr; > > > > > > > > + } invalid_tests[] = { > > > > > > > > + { > > > > > > > > + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ > > > > > > > > + .fb_id = 0, > > > > > > > > + .ptr_valid = true, > > > > > > > > + .out_fence_ptr = &out_fence, > > > > > > > > + }, > > > > > > > > + { > > > > > > > > + /* Invalid output buffer. */ > > > > > > > > + .fb_id = invalid_fb->fb_id, > > > > > > > > + .ptr_valid = true, > > > > > > > > + .out_fence_ptr = &out_fence, > > > > > > > > + }, > > > > > > > > > > > > > > This doesn't belong in this function (invalid_out_fence), since this > > > > > > > checks an invalid framebuffer, not an invalid fence. We should probably > > > > > > > move it to writeback_fb_id (and rename that function to test_fb?). > > > > > > > > > > It tries to test that you can't trick the driver to do any work on a fence if > > > > > the framebuffer is invalid, so the set of tests tries: no fb with valid fence, > > > > > invalid fb with valid fence, valid fb but invalid fence and assumes that no > > > > > fb with invalid fence is a NOP anyway. > > > > > > > > Yeah, that makes sense, it's just confusing to put it in a function > > > > named invalid_out_fence. Here the out fence is valid, but the output > > > > buffer isn't, so it should probably be moved away (or this function > > > > should be renamed). > > > > > > Don't want to offend or anything, but this does sound like bikeshedding. You > > > have a couple of parameters that you want to have a test for because they are > > > linked together (output framebuffer and fence) and you go through the > > > combination of possible bad options in the test. Not sure what name we can use > > > for the function, other than maybe 'test_invalid_parameters'? Given that 2/3 > > > tests an invalid out fence, the name was thought to be relevant. > > > > > > Having invalid out buffer test separate into its own test brings no benefits, IMHO. > > > > Well, the issue is that I've been confused when reviewing the patch > > series. I had trouble understanding what the test does and why. I also > > had trouble to identify that do_writeback_test never submits a > > writeback operation (see other e-mail). > > > > A name that is relevant "all the time, most of the time", is not > > relevant at all in my opinion. It just tricks the reader into thinking > > the test does one thing, while it also does something else. > > > > If it would be obvious, I wouldn't mind. But here IMHO it hurts > > readability. So I'd prefer to rename the function. > > I take your comments as a valid point. > > Does "test_invalid_parameters" sound like a good name for the function? Is so, > Rodrigo, can you please use that name in the next revision of the patch? Sure, I'll do that. Thanks > > > > If you think it's obvious, then maybe it's just me. I'd love to hear > > from others if they have a different opinion. > > > > (As a side note, I agree I have a tendency to bikeshed, I try to mark > > my bikesheddings behind "nit:" flags.) > > I've only said "it sounded like" :) > > Best regards, > Liviu > > > > > > Best regards, > > > Liviu > > > > > > > > > > > + { > > > > > > > > + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ > > > > > > > > + .fb_id = valid_fb->fb_id, > > > > > > > > + .ptr_valid = false, > > > > > > > > + .out_fence_ptr = (int32_t *)0x8, > > > > > > > > + }, > > > > > > > > + }; > > > > > > > > + > > > > > > > > + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + invalid_tests[i].fb_id, > > > > > > > > + invalid_tests[i].out_fence_ptr, > > > > > > > > + invalid_tests[i].ptr_valid); > > > > > > > > + igt_assert(ret != 0); > > > > > > > > > > > > > > Maybe we can check for -ret == EINVAL? > > > > > > > > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) > > > > > > > > > > > > > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to > > > > > > > the input framebuffer. It's probably not a good idea to use the same FB > > > > > > > for input and writeback output. > > > > > > > > > > > > > > > +{ > > > > > > > > + > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + /* Valid output buffer */ > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + valid_fb->fb_id, NULL, false); > > > > > > > > + igt_assert(ret == 0); > > > > > > > > + > > > > > > > > + /* Invalid object for WRITEBACK_FB_ID */ > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + output->id, NULL, false); > > > > > > > > + igt_assert(ret == -EINVAL); > > > > > > > > + > > > > > > > > + /* Zero WRITEBACK_FB_ID */ > > > > > > > > + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, > > > > > > > > + 0, NULL, false); > > > > > > > > + igt_assert(ret == 0); > > > > > > > > +} > > > > > > > > + > > > > > > > > +igt_main > > > > > > > > +{ > > > > > > > > + igt_display_t display; > > > > > > > > + igt_output_t *output; > > > > > > > > + igt_plane_t *plane; > > > > > > > > + igt_fb_t input_fb; > > > > > > > > + drmModeModeInfo mode; > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + memset(&display, 0, sizeof(display)); > > > > > > > > + > > > > > > > > + igt_fixture { > > > > > > > > + display.drm_fd = drm_open_driver_master(DRIVER_ANY); > > > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > > > + > > > > > > > > + kmstest_set_vt_graphics_mode(); > > > > > > > > + > > > > > > > > + igt_display_require(&display, display.drm_fd); > > > > > > > > + > > > > > > > > + igt_require(display.is_atomic); > > > > > > > > + > > > > > > > > + output = kms_writeback_get_output(&display); > > > > > > > > + igt_require(output); > > > > > > > > + > > > > > > > > + if (output->use_override_mode) > > > > > > > > + memcpy(&mode, &output->override_mode, sizeof(mode)); > > > > > > > > + else > > > > > > > > + memcpy(&mode, &output->config.default_mode, sizeof(mode)); > > > > > > > > + > > > > > > > > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > > > > > > > > + igt_require(plane); > > > > > > > > > > > > > > Maybe we can assert on this? > > > > > > > > > > > > > > > + ret = igt_create_fb(display.drm_fd, mode.hdisplay, > > > > > > > > + mode.vdisplay, > > > > > > > > + DRM_FORMAT_XRGB8888, > > > > > > > > + igt_fb_mod_to_tiling(0), > > > > > > > > > > > > > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be > > > > > > > better to make this clear. > > > > > > > > > > Agree. The patchset pre-dates the modifiers support (or has the same age, I > > > > > forgot) > > > > > > > > > > > > (Applies to other lines of this patch) > > > > > > > > > > > > > > > + &input_fb); > > > > > > > > + igt_assert(ret >= 0); > > > > > > > > + igt_plane_set_fb(plane, &input_fb); > > > > > > > > + } > > > > > > > > + > > > > > > > > + igt_subtest("writeback-pixel-formats") { > > > > > > > > + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); > > > > > > > > > > > > > > Need to drmModeFreePropertyBlob this. > > > > > > > > > > > > > > > + const char *valid_chars = "0123456 ABCGNRUVXY"; > > > > > > > > + unsigned int i; > > > > > > > > + char *c; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * We don't have a comprehensive list of formats, so just check > > > > > > > > + * that the blob length is sensible and that it doesn't contain > > > > > > > > + * any outlandish characters > > > > > > > > + */ > > > > > > > > + igt_assert(!(formats_blob->length % 4)); > > > > > > > > + c = formats_blob->data; > > > > > > > > + for (i = 0; i < formats_blob->length; i++) > > > > > > > > + igt_assert_f(strchr(valid_chars, c[i]), > > > > > > > > + "Unexpected character %c\n", c[i]); > > > > > > > > > > > > > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc > > > > > > > codes will be made from ASCII characters, in fact some formats already > > > > > > > have non-printable chars in them. I don't want to have to update this > > > > > > > test when a new fourcc format is added. > > > > > > > > > > Like the comments says, we don't have a full list of formats to check against. > > > > > Suggestions on how to improve are welcome, but I don't think we should delay > > > > > (any longer) the patchset due to this. > > > > > > > > Agreed. > > > > > > > > > Best regards, > > > > > Liviu > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 027ed82f..03cc8efa 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -77,6 +77,7 @@ TESTS_progs = \ kms_universal_plane \ kms_vblank \ kms_vrr \ + kms_writeback \ meta_test \ perf \ perf_pmu \ diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c new file mode 100644 index 00000000..66ef48a6 --- /dev/null +++ b/tests/kms_writeback.c @@ -0,0 +1,314 @@ +/* + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +#include <errno.h> +#include <stdbool.h> +#include <stdio.h> +#include <string.h> + +#include "igt.h" +#include "igt_core.h" +#include "igt_fb.h" + +static drmModePropertyBlobRes *get_writeback_formats_blob(igt_output_t *output) +{ + drmModePropertyBlobRes *blob = NULL; + uint64_t blob_id; + int ret; + + ret = kmstest_get_property(output->display->drm_fd, + output->config.connector->connector_id, + DRM_MODE_OBJECT_CONNECTOR, + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_PIXEL_FORMATS], + NULL, &blob_id, NULL); + if (ret) + blob = drmModeGetPropertyBlob(output->display->drm_fd, blob_id); + + igt_assert(blob); + + return blob; +} + +static bool check_writeback_config(igt_display_t *display, igt_output_t *output) +{ + igt_fb_t input_fb, output_fb; + igt_plane_t *plane; + uint32_t writeback_format = DRM_FORMAT_XRGB8888; + uint64_t tiling = igt_fb_mod_to_tiling(0); + int width, height, ret; + drmModeModeInfo override_mode = { + .clock = 25175, + .hdisplay = 640, + .hsync_start = 656, + .hsync_end = 752, + .htotal = 800, + .hskew = 0, + .vdisplay = 480, + .vsync_start = 490, + .vsync_end = 492, + .vtotal = 525, + .vscan = 0, + .vrefresh = 60, + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, + .name = {"640x480-60"}, + }; + igt_output_override_mode(output, &override_mode); + + width = override_mode.hdisplay; + height = override_mode.vdisplay; + + ret = igt_create_fb(display->drm_fd, width, height, DRM_FORMAT_XRGB8888, tiling, &input_fb); + igt_assert(ret >= 0); + + ret = igt_create_fb(display->drm_fd, width, height, writeback_format, tiling, &output_fb); + igt_assert(ret >= 0); + + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); + igt_plane_set_fb(plane, &input_fb); + igt_output_set_writeback_fb(output, &output_fb); + + ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); + igt_plane_set_fb(plane, NULL); + igt_remove_fb(display->drm_fd, &input_fb); + igt_remove_fb(display->drm_fd, &output_fb); + + return !ret; +} + +static igt_output_t *kms_writeback_get_output(igt_display_t *display) +{ + int i; + + for (i = 0; i < display->n_outputs; i++) { + igt_output_t *output = &display->outputs[i]; + int j; + + if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) + continue; + + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_ON); + + for (j = 0; j < igt_display_get_n_pipes(display); j++) { + igt_output_set_pipe(output, j); + + if (check_writeback_config(display, output)) { + igt_debug("Using connector %u:%s on pipe %d\n", + output->config.connector->connector_id, + output->name, j); + return output; + } + } + + /* Restore any connectors we don't use, so we don't trip on them later */ + kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED); + } + + return NULL; +} + +static void check_writeback_fb_id(igt_output_t *output) +{ + uint64_t check_fb_id; + + check_fb_id = igt_output_get_prop(output, IGT_CONNECTOR_WRITEBACK_FB_ID); + igt_assert(check_fb_id == 0); +} + +static int do_writeback_test(igt_output_t *output, uint32_t flags, + uint32_t fb_id, int32_t *out_fence_ptr, + bool ptr_valid) +{ + int ret; + igt_display_t *display = output->display; + struct kmstest_connector_config *config = &output->config; + + igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id); + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_FB_ID, fb_id); + igt_output_set_prop_value(output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, (uint64_t)out_fence_ptr); + + if (ptr_valid) + *out_fence_ptr = 0; + + ret = igt_display_try_commit_atomic(display, flags, NULL); + + if (ptr_valid) + igt_assert(*out_fence_ptr == -1); + + /* WRITEBACK_FB_ID must always read as zero */ + check_writeback_fb_id(output); + + return ret; +} + +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) +{ + int i, ret; + int32_t out_fence; + struct { + uint32_t fb_id; + bool ptr_valid; + int32_t *out_fence_ptr; + } invalid_tests[] = { + { + /* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */ + .fb_id = 0, + .ptr_valid = true, + .out_fence_ptr = &out_fence, + }, + { + /* Invalid output buffer. */ + .fb_id = invalid_fb->fb_id, + .ptr_valid = true, + .out_fence_ptr = &out_fence, + }, + { + /* Invalid WRITEBACK_OUT_FENCE_PTR. */ + .fb_id = valid_fb->fb_id, + .ptr_valid = false, + .out_fence_ptr = (int32_t *)0x8, + }, + }; + + for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) { + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, + invalid_tests[i].fb_id, + invalid_tests[i].out_fence_ptr, + invalid_tests[i].ptr_valid); + igt_assert(ret != 0); + } +} + +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb) +{ + + int ret; + + /* Valid output buffer */ + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, + valid_fb->fb_id, NULL, false); + igt_assert(ret == 0); + + /* Invalid object for WRITEBACK_FB_ID */ + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, + output->id, NULL, false); + igt_assert(ret == -EINVAL); + + /* Zero WRITEBACK_FB_ID */ + ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET, + 0, NULL, false); + igt_assert(ret == 0); +} + +igt_main +{ + igt_display_t display; + igt_output_t *output; + igt_plane_t *plane; + igt_fb_t input_fb; + drmModeModeInfo mode; + int ret; + + memset(&display, 0, sizeof(display)); + + igt_fixture { + display.drm_fd = drm_open_driver_master(DRIVER_ANY); + igt_display_require(&display, display.drm_fd); + + kmstest_set_vt_graphics_mode(); + + igt_display_require(&display, display.drm_fd); + + igt_require(display.is_atomic); + + output = kms_writeback_get_output(&display); + igt_require(output); + + if (output->use_override_mode) + memcpy(&mode, &output->override_mode, sizeof(mode)); + else + memcpy(&mode, &output->config.default_mode, sizeof(mode)); + + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); + igt_require(plane); + + ret = igt_create_fb(display.drm_fd, mode.hdisplay, + mode.vdisplay, + DRM_FORMAT_XRGB8888, + igt_fb_mod_to_tiling(0), + &input_fb); + igt_assert(ret >= 0); + igt_plane_set_fb(plane, &input_fb); + } + + igt_subtest("writeback-pixel-formats") { + drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output); + const char *valid_chars = "0123456 ABCGNRUVXY"; + unsigned int i; + char *c; + + /* + * We don't have a comprehensive list of formats, so just check + * that the blob length is sensible and that it doesn't contain + * any outlandish characters + */ + igt_assert(!(formats_blob->length % 4)); + c = formats_blob->data; + for (i = 0; i < formats_blob->length; i++) + igt_assert_f(strchr(valid_chars, c[i]), + "Unexpected character %c\n", c[i]); + } + + igt_subtest("writeback-invalid-out-fence") { + igt_fb_t invalid_fb; + ret = igt_create_fb(display.drm_fd, mode.hdisplay / 2, + mode.vdisplay / 2, + DRM_FORMAT_XRGB8888, + igt_fb_mod_to_tiling(0), + &invalid_fb); + igt_require(ret > 0); + + invalid_out_fence(output, &input_fb, &invalid_fb); + + igt_remove_fb(display.drm_fd, &invalid_fb); + } + + igt_subtest("writeback-fb-id") { + igt_fb_t output_fb; + ret = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay, + DRM_FORMAT_XRGB8888, + igt_fb_mod_to_tiling(0), + &output_fb); + igt_require(ret > 0); + + writeback_fb_id(output, &input_fb, &output_fb); + + igt_remove_fb(display.drm_fd, &output_fb); + } + + igt_fixture { + igt_remove_fb(display.drm_fd, &input_fb); + igt_display_fini(&display); + } +} diff --git a/tests/meson.build b/tests/meson.build index f168fbba..9c77cfcd 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -63,6 +63,7 @@ test_progs = [ 'kms_universal_plane', 'kms_vblank', 'kms_vrr', + 'kms_writeback', 'meta_test', 'panfrost_get_param', 'panfrost_gem_new',