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@xxxxxxx>
> > > [rebased and updated do_writeback_test() function to address feedback]
> > > Signed-off-by: Liviu Dudau <liviu.dudau@xxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
> --
> Rodrigo Siqueira
> https://siqueira.tech
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
