Op 26-06-18 om 10:38 schreef Liviu Dudau:
> On Mon, Jun 25, 2018 at 02:48:47PM +0200, Maarten Lankhorst wrote:
>> Op 01-03-18 om 18:38 schreef Liviu Dudau:
>>> From: Brian Starkey <brian.starkey@xxxxxxx>
>>>
>>> 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>
>>> ---
>>> lib/igt_kms.c | 7 +-
>>> lib/igt_kms.h | 8 ++
>>> tests/Makefile.sources | 1 +
>>> tests/kms_writeback.c | 352 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> tests/meson.build | 1 +
>>> 5 files changed, 365 insertions(+), 4 deletions(-)
>>> create mode 100644 tests/kms_writeback.c
>>>
>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>> index 00d9d2e2..d558c1b8 100644
>>> --- a/lib/igt_kms.c
>>> +++ b/lib/igt_kms.c
>>> @@ -2267,8 +2267,7 @@ static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>>> /*
>>> * Add position and fb changes of a plane to the atomic property set
>>> */
>>> -static void
>>> -igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> drmModeAtomicReq *req)
>>> {
>>> igt_display_t *display = pipe->display;
>>> @@ -2878,7 +2877,7 @@ igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe, enum igt_atomic_crtc_properties
>>> /*
>>> * Add crtc property changes to the atomic property set
>>> */
>>> -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req)
>>> {
>>> int i;
>>>
>>> @@ -2902,7 +2901,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe
>>> /*
>>> * Add connector property changes to the atomic property set
>>> */
>>> -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req)
>>> {
>>>
>>> int i;
>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>>> index 59ccc4c6..f38fd0a0 100644
>>> --- a/lib/igt_kms.h
>>> +++ b/lib/igt_kms.h
>>> @@ -581,6 +581,12 @@ extern void igt_output_replace_prop_blob(igt_output_t *output,
>>> enum igt_atomic_connector_properties prop,
>>> const void *ptr, size_t length);
>>>
>>> +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe,
>>> + drmModeAtomicReq *req);
>>> +
>>> +
>>> +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req);
>>> +
>>> /**
>>> * igt_pipe_obj_has_prop:
>>> * @pipe: Pipe to check.
>>> @@ -670,6 +676,8 @@ extern void igt_pipe_obj_replace_prop_blob(igt_pipe_t *pipe,
>>>
>>> void igt_pipe_refresh(igt_display_t *display, enum pipe pipe, bool force);
>>>
>>> +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req);
>>> +
>>> void igt_enable_connectors(void);
>>> void igt_reset_connectors(void);
>> Please don't..
> More context on what I should not do/say/write would be helpful.
>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 23f859be..9cfa474d 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -212,6 +212,7 @@ TESTS_progs = \
>>> kms_tv_load_detect \
>>> kms_universal_plane \
>>> kms_vblank \
>>> + 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..d922213d
>>> --- /dev/null
>>> +++ b/tests/kms_writeback.c
>>> @@ -0,0 +1,352 @@
>>> +/*
>>> + * (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"
>>> +
>>> +/* We need to define these ourselves until we get an updated libdrm */
>>> +#ifndef DRM_MODE_CONNECTOR_WRITEBACK
>>> +#define DRM_MODE_CONNECTOR_WRITEBACK 18
>>> +#endif
>>> +
>>> +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);
>> Could we perhaps update igt_output_is_connected to always return true when mode override
>> is set, so we don't have to force enable the connector?
> Yes, that is now obsolete as we have a client cap as well that will hide
> the connector unless set, so this patch needs updating as well.
>
>>> + 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)
>>> +{
>>> + bool found;
>>> + uint64_t check_fb_id;
>>> +
>>> + found = kmstest_get_property(output->display->drm_fd, output->id,
>>> + DRM_MODE_OBJECT_CONNECTOR,
>>> + igt_connector_prop_names[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> + NULL, &check_fb_id, NULL);
>>> + igt_assert(found && (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;
>>> + enum pipe pipe;
>>> + drmModeAtomicReq *req;
>>> + igt_display_t *display = output->display;
>>> + struct kmstest_connector_config *config = &output->config;
>>> +
>>> + req = drmModeAtomicAlloc();
>>> + drmModeAtomicSetCursor(req, 0);
>>> +
>>> + for_each_pipe(display, pipe) {
>>> + igt_pipe_t *pipe_obj = &display->pipes[pipe];
>>> + igt_plane_t *plane;
>>> +
>>> + /*
>>> + * Add CRTC Properties to the property set
>>> + */
>>> + igt_atomic_prepare_crtc_commit(pipe_obj, req);
>>> +
>>> + for_each_plane_on_pipe(display, pipe, plane) {
>>> + igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
>>> + }
>>> + }
>>> +
>>> + igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> + output->props[IGT_CONNECTOR_CRTC_ID],
>>> + config->crtc->crtc_id));
>> igt_output_set_pipe() ?
>> Or failing that, igt_output_set_property(output, IGT_CONNECTOR_CRTC_ID);
> Thanks for the hint, will update the patch.
>
>>> + igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,
>>> + output->props[IGT_CONNECTOR_WRITEBACK_FB_ID],
>>> + fb_id));
>> igt_output_set_property(output, IGT_CONNECTOR_FB_ID); or the convenience function from v1.. which also saves you from having to do the call below.
>>
>> I would really keep all the logic for the atomic properties internal, please don't override igt_display_commit_atomic.. we've made it too easy to set properties to anything you want.
> Understood. On the other hand, some flexibility in the test framework
> for whacky values in order to do adversarial testing could be useful.
We do allow this explicitly, see kms_atomic, crtc_invalid_params tries setting an invalid mode:
/* Pass a series of invalid object IDs for the mode ID. */
igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, plane->drm_plane->plane_id);
crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);
igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, pipe->crtc_id);
crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);
igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, output->id);
crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);
igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_MODE_ID, fb->fb_id);
crtc_commit_atomic_err(pipe, plane, ATOMIC_RELAX_NONE, EINVAL);
I couldn't put in a pointer to the kitchen sink, else I would have. O:)
Same works for plane and connector too, this is why you don't need to
override atomic commit().
You can put together an atomic test without ever using anything but
igt_plane_set_prop_value, igt_output_set_prop_value and igt_pipe(,_obj)_set_prop_value.
The convenience functions are just for allowing legacy support to continue working.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
