On Mon, Nov 14, 2016 at 06:59:22PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> ---
> tests/kms_atomic.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 115 insertions(+), 9 deletions(-)
>
> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> index 8b648eb..58fc0dd 100644
> --- a/tests/kms_atomic.c
> +++ b/tests/kms_atomic.c
> @@ -44,6 +44,7 @@
> #include "drmtest.h"
> #include "igt.h"
> #include "igt_aux.h"
> +#include "sw_sync.h"
>
> #ifndef DRM_CLIENT_CAP_ATOMIC
> #define DRM_CLIENT_CAP_ATOMIC 3
> @@ -126,6 +127,7 @@ struct kms_atomic_plane_state {
> uint32_t fb_id; /* 0 to disable */
> uint32_t src_x, src_y, src_w, src_h; /* 16.16 fixed-point */
> uint32_t crtc_x, crtc_y, crtc_w, crtc_h; /* normal integers */
> + int32_t fence_fd;
> };
>
> struct kms_atomic_crtc_state {
> @@ -133,6 +135,7 @@ struct kms_atomic_crtc_state {
> uint32_t obj;
> int idx;
> bool active;
> + uint64_t out_fence_ptr;
> struct kms_atomic_blob mode;
> };
>
> @@ -190,11 +193,13 @@ static uint32_t blob_duplicate(int fd, uint32_t id_orig)
> crtc_populate_req(crtc, req); \
> plane_populate_req(plane, req); \
> do_atomic_commit((crtc)->state->desc->fd, req, flags); \
> - crtc_check_current_state(crtc, plane, relax); \
> - plane_check_current_state(plane, relax); \
> + if (!(flags & DRM_MODE_ATOMIC_TEST_ONLY)) { \
> + crtc_check_current_state(crtc, plane, relax); \
> + plane_check_current_state(plane, relax); \
> + } \
> }
>
> -#define crtc_commit_atomic_err(crtc, plane, crtc_old, plane_old, req, relax, e) { \
> +#define crtc_commit_atomic_err(crtc, plane, crtc_old, plane_old, req, flags, relax, e) { \
> drmModeAtomicSetCursor(req, 0); \
> crtc_populate_req(crtc, req); \
> plane_populate_req(plane, req); \
> @@ -299,6 +304,9 @@ find_connector(struct kms_atomic_state *state,
> static void plane_populate_req(struct kms_atomic_plane_state *plane,
> drmModeAtomicReq *req)
> {
> + if (plane->fence_fd)
> + plane_set_prop(req, plane, IGT_PLANE_IN_FENCE_FD, plane->fence_fd);
> +
> plane_set_prop(req, plane, IGT_PLANE_CRTC_ID, plane->crtc_id);
> plane_set_prop(req, plane, IGT_PLANE_FB_ID, plane->fb_id);
> plane_set_prop(req, plane, IGT_PLANE_SRC_X, plane->src_x);
> @@ -424,6 +432,10 @@ find_plane(struct kms_atomic_state *state, enum plane_type type,
> static void crtc_populate_req(struct kms_atomic_crtc_state *crtc,
> drmModeAtomicReq *req)
> {
> + if (crtc->out_fence_ptr)
> + crtc_set_prop(req, crtc, IGT_CRTC_OUT_FENCE_PTR,
> + crtc->out_fence_ptr);
> +
> crtc_set_prop(req, crtc, IGT_CRTC_MODE_ID, crtc->mode.id);
> crtc_set_prop(req, crtc, IGT_CRTC_ACTIVE, crtc->active);
> }
> @@ -986,6 +998,7 @@ static void plane_invalid_params(struct kms_atomic_crtc_state *crtc,
> uint32_t format = plane_get_igt_format(&plane);
> drmModeAtomicReq *req = drmModeAtomicAlloc();
> struct igt_fb fb;
> + int timeline, fence_fd;
>
> /* Pass a series of invalid object IDs for the FB ID. */
> plane.fb_id = plane.obj;
> @@ -1024,6 +1037,23 @@ static void plane_invalid_params(struct kms_atomic_crtc_state *crtc,
> plane_commit_atomic_err(&plane, plane_old, req,
> ATOMIC_RELAX_NONE, EINVAL);
>
> + /* invalid fence fd */
> + plane.fence_fd = plane.state->desc->fd;
> + plane.crtc_id = plane_old->crtc_id;
> + plane_commit_atomic_err(&plane, plane_old, req,
> + ATOMIC_RELAX_NONE, EINVAL);
> +
> + /* Valid fence_fd but invalid CRTC */
> + timeline = sw_sync_timeline_create();
> + fence_fd = sw_sync_fence_create(timeline, 1);
> + plane.fence_fd = fence_fd;
> + plane.crtc_id = ~0U;
> + plane_commit_atomic_err(&plane, plane_old, req,
> + ATOMIC_RELAX_NONE, EINVAL);
> + close(fence_fd);
> + close(timeline);
> +
> + plane.fence_fd = -1;
> plane.crtc_id = plane_old->crtc_id;
> plane_commit_atomic(&plane, req, ATOMIC_RELAX_NONE);
>
> @@ -1058,27 +1088,98 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
> {
> struct kms_atomic_crtc_state crtc = *crtc_old;
> drmModeAtomicReq *req = drmModeAtomicAlloc();
> + int timeline, fence_fd, *out_fence;
>
> igt_assert(req);
>
> /* Pass a series of invalid object IDs for the mode ID. */
> crtc.mode.id = plane->obj;
> - crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> ATOMIC_RELAX_NONE, EINVAL);
>
> crtc.mode.id = crtc.obj;
> - crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> ATOMIC_RELAX_NONE, EINVAL);
>
> crtc.mode.id = conn->obj;
> - crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> ATOMIC_RELAX_NONE, EINVAL);
>
> crtc.mode.id = plane->fb_id;
> - crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> ATOMIC_RELAX_NONE, EINVAL);
>
> + /* invalid out_fence_ptr */
> + crtc.mode.id = crtc_old->mode.id;
> + crtc.out_fence_ptr = (uint64_t) crtc_invalid_params;
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> + ATOMIC_RELAX_NONE, EFAULT);
> +
> + /* invalid out_fence_ptr */
> + crtc.mode.id = crtc_old->mode.id;
> + crtc.out_fence_ptr = (uint64_t) 0x8;
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> + ATOMIC_RELAX_NONE, EFAULT);
> + crtc.out_fence_ptr = (uint64_t) 0;
> +
> + /* valid in fence but not allowed prop on crtc */
> + timeline = sw_sync_timeline_create();
> + fence_fd = sw_sync_fence_create(timeline, 1);
> + plane->fence_fd = fence_fd;
> + crtc.active = false;
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> + ATOMIC_RELAX_NONE, EINVAL);
> +
> + out_fence = malloc(sizeof(uint64_t));
> + igt_assert(out_fence);
> +
> +
> + /* valid out fence ptr and flip event but not allowed prop on crtc */
> + crtc.out_fence_ptr = (uint64_t) out_fence;
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> + ATOMIC_RELAX_NONE, EINVAL);
> +
> + /* valid out fence ptr and flip event but not allowed prop on crtc */
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + DRM_MODE_PAGE_FLIP_EVENT,
> + ATOMIC_RELAX_NONE, EINVAL);
> +
> + /* valid page flip event but not allowed prop on crtc */
> + crtc.out_fence_ptr = (uint64_t) 0;
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + DRM_MODE_PAGE_FLIP_EVENT,
> + ATOMIC_RELAX_NONE, EINVAL);
> + crtc.active = true;
> +
> + /* valid out fence ptr and flip event but invalid prop on crtc */
> + crtc.out_fence_ptr = (uint64_t) out_fence;
> + crtc.mode.id = plane->fb_id;
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> + ATOMIC_RELAX_NONE, EINVAL);
> +
> + /* valid out fence ptr and flip event but invalid prop on crtc */
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + DRM_MODE_PAGE_FLIP_EVENT,
> + ATOMIC_RELAX_NONE, EINVAL);
> +
> + /* valid page flip event but invalid prop on crtc */
> + crtc.out_fence_ptr = (uint64_t) 0;
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + DRM_MODE_PAGE_FLIP_EVENT,
> + ATOMIC_RELAX_NONE, EINVAL);
> +
> + /* successful TEST_ONLY with fences set */
> + plane->fence_fd = fence_fd;
Smashing all invalid tests into one function&subtest is imo bad style.
gem_exec_params.c is probably overkill in splitting stuff up, but for this
here at least making some groups of invalid tests as subgroups would be
good imo.
E.g. replacing all your comments with somewhat meaningful subtests names
also helps with making things self-documenting a bit more. Old rule of
thumb that if you write a comment explaining what the code does, it needs
to be refactored to make it obvious what it does.
-Daniel
> crtc.mode.id = crtc_old->mode.id;
> + crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE,
> + DRM_MODE_ATOMIC_TEST_ONLY);
> + igt_assert(*out_fence == -1);
> + close(fence_fd);
> + close(timeline);
> +
> + /* reset fences */
> + plane->fence_fd = -1;
> + crtc.out_fence_ptr = (uint64_t) 0;
> crtc_commit_atomic(&crtc, plane, req, ATOMIC_RELAX_NONE, 0);
>
> /* Create a blob which is the wrong size to be a valid mode. */
> @@ -1086,7 +1187,7 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
> crtc.mode.data,
> sizeof(struct drm_mode_modeinfo) - 1,
> &crtc.mode.id));
> - crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> ATOMIC_RELAX_NONE, EINVAL);
>
>
> @@ -1094,12 +1195,17 @@ static void crtc_invalid_params(struct kms_atomic_crtc_state *crtc_old,
> crtc.mode.data,
> sizeof(struct drm_mode_modeinfo) + 1,
> &crtc.mode.id));
> - crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req,
> + crtc_commit_atomic_err(&crtc, plane, crtc_old, plane, req, 0,
> ATOMIC_RELAX_NONE, EINVAL);
>
> + /* out fence ptr but not page flip event */
> + crtc.out_fence_ptr = (uint64_t) out_fence;
> + crtc_commit_atomic(crtc_old, plane, req, ATOMIC_RELAX_NONE, 0);
> +
> /* Restore the CRTC and check the state matches the old. */
> crtc_commit_atomic(crtc_old, plane, req, ATOMIC_RELAX_NONE, 0);
>
> + free(out_fence);
> drmModeAtomicFree(req);
> }
>
> --
> 2.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
