Op 03-10-17 om 14:05 schreef Mika Kahola:
> On Fri, 2017-09-29 at 11:59 +0200, Maarten Lankhorst wrote:
>> In the future I want to allow tests to commit more properties,
>> but for this to work I have to fix all properties to work better
>> with atomic commit. Instead of special casing each
>> property make a bitmask for all property changed flags, and try to
>> commit all properties.
>>
>> Changes since v1:
>> - Remove special dumping of src and crtc coordinates.
>> - Dump all modified coordinates.
>> Changes since v2:
>> - Move igt_plane_set_prop_changed up slightly.
>> Changes since v3:
>> - Fix wrong ordering of set_position in kms_plane_lowres causing a
>> test failure.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> lib/igt_kms.c | 293 +++++++++++++++++----------
>> ------------
>> lib/igt_kms.h | 59 ++++----
>> tests/kms_atomic_interruptible.c | 12 +-
>> tests/kms_plane_lowres.c | 2 +-
>> tests/kms_rotation_crc.c | 4 +-
>> 5 files changed, 160 insertions(+), 210 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 07d2074c2b1a..6e0052ebe7a7 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -192,11 +192,11 @@ const char
>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = {
>>
>> /*
>> * Retrieve all the properies specified in props_name and store them
>> into
>> - * plane->atomic_props_plane.
>> + * plane->props.
>> */
>> static void
>> -igt_atomic_fill_plane_props(igt_display_t *display, igt_plane_t
>> *plane,
>> - int num_props, const char **prop_names)
>> +igt_fill_plane_props(igt_display_t *display, igt_plane_t *plane,
>> + int num_props, const char **prop_names)
>> {
>> drmModeObjectPropertiesPtr props;
>> int i, j, fd;
>> @@ -214,7 +214,7 @@ igt_atomic_fill_plane_props(igt_display_t
>> *display, igt_plane_t *plane,
>> if (strcmp(prop->name, prop_names[j]) != 0)
>> continue;
>>
>> - plane->atomic_props_plane[j] = props-
>>> props[i];
>> + plane->props[j] = props->props[i];
>> break;
>> }
>>
>> @@ -1659,7 +1659,6 @@ void igt_display_init(igt_display_t *display,
>> int drm_fd)
>> drmModeRes *resources;
>> drmModePlaneRes *plane_resources;
>> int i;
>> - int is_atomic = 0;
>>
>> memset(display, 0, sizeof(igt_display_t));
>>
>> @@ -1679,7 +1678,9 @@ void igt_display_init(igt_display_t *display,
>> int drm_fd)
>> igt_assert_f(display->pipes, "Failed to allocate memory for
>> %d pipes\n", display->n_pipes);
>>
>> drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
>> - is_atomic = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC,
>> 1);
>> + if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
>> + display->is_atomic = 1;
>> +
>> plane_resources = drmModeGetPlaneResources(display->drm_fd);
>> igt_assert(plane_resources);
>>
>> @@ -1776,19 +1777,19 @@ void igt_display_init(igt_display_t *display,
>> int drm_fd)
>> plane->type = type;
>> plane->pipe = pipe;
>> plane->drm_plane = drm_plane;
>> - plane->fence_fd = -1;
>> + plane->values[IGT_PLANE_IN_FENCE_FD] =
>> ~0ULL;
>>
>> - if (is_atomic == 0) {
>> - display->is_atomic = 1;
>> - igt_atomic_fill_plane_props(display,
>> plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
>> - }
>> + igt_fill_plane_props(display, plane,
>> IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
>>
>> get_plane_property(display->drm_fd,
>> drm_plane->plane_id,
>> "rotation",
>> - &plane-
>>> rotation_property,
>> - &prop_value,
>> + &plane-
>>> props[IGT_PLANE_ROTATION],
>> + &plane-
>>> values[IGT_PLANE_ROTATION],
>> NULL);
>> - plane->rotation =
>> (igt_rotation_t)prop_value;
>> +
>> + /* Clear any residual framebuffer on first
>> commit. */
>> + igt_plane_set_prop_changed(plane,
>> IGT_PLANE_FB_ID);
>> + igt_plane_set_prop_changed(plane,
>> IGT_PLANE_CRTC_ID);
>> }
>>
>> /*
>> @@ -1805,9 +1806,6 @@ void igt_display_init(igt_display_t *display,
>> int drm_fd)
>>
>> pipe->n_planes = n_planes;
>>
>> - for_each_plane_on_pipe(display, i, plane)
>> - plane->fb_changed = true;
>> -
>> pipe->mode_changed = true;
>> }
>>
>> @@ -2070,18 +2068,7 @@ bool igt_pipe_get_property(igt_pipe_t *pipe,
>> const char *name,
>>
>> static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>> {
>> - if (plane->fb)
>> - return plane->fb->fb_id;
>> - else
>> - return 0;
>> -}
>> -
>> -static uint32_t igt_plane_get_fb_gem_handle(igt_plane_t *plane)
>> -{
>> - if (plane->fb)
>> - return plane->fb->gem_handle;
>> - else
>> - return 0;
>> + return plane->values[IGT_PLANE_FB_ID];
>> }
>>
>> #define CHECK_RETURN(r, fail) { \
>> @@ -2090,9 +2077,6 @@ static uint32_t
>> igt_plane_get_fb_gem_handle(igt_plane_t *plane)
>> igt_assert_eq(r, 0); \
>> }
>>
>> -
>> -
>> -
>> /*
>> * Add position and fb changes of a plane to the atomic property set
>> */
>> @@ -2101,63 +2085,31 @@ igt_atomic_prepare_plane_commit(igt_plane_t
>> *plane, igt_pipe_t *pipe,
>> drmModeAtomicReq *req)
>> {
>> igt_display_t *display = pipe->display;
>> - uint32_t fb_id, crtc_id;
>> + int i;
>>
>> igt_assert(plane->drm_plane);
>>
>> - /* it's an error to try an unsupported feature */
>> - igt_assert(igt_plane_supports_rotation(plane) ||
>> - !plane->rotation_changed);
>> -
>> - fb_id = igt_plane_get_fb_id(plane);
>> - crtc_id = pipe->crtc_id;
>> -
>> LOG(display,
>> "populating plane data: %s.%d, fb %u\n",
>> kmstest_pipe_name(pipe->pipe),
>> plane->index,
>> - fb_id);
>> -
>> - if (plane->fence_fd >= 0) {
>> - uint64_t fence_fd = (int64_t) plane->fence_fd;
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_IN_FENCE_FD, fence_fd);
>> - }
>> + igt_plane_get_fb_id(plane));
>>
>> - if (plane->fb_changed) {
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_CRTC_ID, fb_id ? crtc_id : 0);
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_FB_ID, fb_id);
>> - }
>> -
>> - if (plane->position_changed || plane->size_changed) {
>> - uint32_t src_x = IGT_FIXED(plane->src_x, 0); /*
>> src_x */
>> - uint32_t src_y = IGT_FIXED(plane->src_y, 0); /*
>> src_y */
>> - uint32_t src_w = IGT_FIXED(plane->src_w, 0); /*
>> src_w */
>> - uint32_t src_h = IGT_FIXED(plane->src_h, 0); /*
>> src_h */
>> - int32_t crtc_x = plane->crtc_x;
>> - int32_t crtc_y = plane->crtc_y;
>> - uint32_t crtc_w = plane->crtc_w;
>> - uint32_t crtc_h = plane->crtc_h;
>> + for (i = 0; i < IGT_NUM_PLANE_PROPS; i++) {
>> + if (!igt_plane_is_prop_changed(plane, i))
>> + continue;
>>
>> - LOG(display,
>> - "src = (%d, %d) %u x %u "
>> - "dst = (%d, %d) %u x %u\n",
>> - src_x >> 16, src_y >> 16, src_w >> 16, src_h >> 16,
>> - crtc_x, crtc_y, crtc_w, crtc_h);
>> + /* it's an error to try an unsupported feature */
>> + igt_assert(plane->props[i]);
>>
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_SRC_X, src_x);
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_SRC_Y, src_y);
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_SRC_W, src_w);
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_SRC_H, src_h);
>> + igt_debug("plane %s.%d: Setting property \"%s\" to
>> 0x%"PRIx64"/%"PRIi64"\n",
>> + kmstest_pipe_name(pipe->pipe), plane->index,
>> igt_plane_prop_names[i],
>> + plane->values[i], plane->values[i]);
>>
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_CRTC_X, crtc_x);
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_CRTC_Y, crtc_y);
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_CRTC_W, crtc_w);
>> - igt_atomic_populate_plane_req(req, plane,
>> IGT_PLANE_CRTC_H, crtc_h);
>> + igt_assert_lt(0, drmModeAtomicAddProperty(req,
>> plane->drm_plane->plane_id,
>> + plane->props[i],
>> + plane-
>>> values[i]));
>> }
>> -
>> - if (plane->rotation_changed)
>> - igt_atomic_populate_plane_req(req, plane,
>> - IGT_PLANE_ROTATION, plane->rotation);
>> }
>>
>>
>> @@ -2183,17 +2135,20 @@ static int igt_drm_plane_commit(igt_plane_t
>> *plane,
>> int32_t crtc_y;
>> uint32_t crtc_w;
>> uint32_t crtc_h;
>> + bool setplane =
>> + igt_plane_is_prop_changed(plane, IGT_PLANE_FB_ID) ||
>> + plane->changed & IGT_PLANE_COORD_CHANGED_MASK;
>>
>> igt_assert(plane->drm_plane);
>>
>> /* it's an error to try an unsupported feature */
>> igt_assert(igt_plane_supports_rotation(plane) ||
>> - !plane->rotation_changed);
>> + !igt_plane_is_prop_changed(plane,
>> IGT_PLANE_ROTATION));
>>
>> fb_id = igt_plane_get_fb_id(plane);
>> crtc_id = pipe->crtc_id;
>>
>> - if ((plane->fb_changed || plane->size_changed) && fb_id ==
>> 0) {
>> + if (setplane && fb_id == 0) {
>> LOG(display,
>> "SetPlane pipe %s, plane %d, disabling\n",
>> kmstest_pipe_name(pipe->pipe),
>> @@ -2212,16 +2167,15 @@ static int igt_drm_plane_commit(igt_plane_t
>> *plane,
>> IGT_FIXED(0,0) /* src_h */);
>>
>> CHECK_RETURN(ret, fail_on_error);
>> - } else if (plane->fb_changed || plane->position_changed ||
>> - plane->size_changed) {
>> - src_x = IGT_FIXED(plane->src_x,0); /* src_x */
>> - src_y = IGT_FIXED(plane->src_y,0); /* src_y */
>> - src_w = IGT_FIXED(plane->src_w,0); /* src_w */
>> - src_h = IGT_FIXED(plane->src_h,0); /* src_h */
>> - crtc_x = plane->crtc_x;
>> - crtc_y = plane->crtc_y;
>> - crtc_w = plane->crtc_w;
>> - crtc_h = plane->crtc_h;
>> + } else if (setplane) {
>> + src_x = plane->values[IGT_PLANE_SRC_X];
>> + src_y = plane->values[IGT_PLANE_SRC_Y];
>> + src_w = plane->values[IGT_PLANE_SRC_W];
>> + src_h = plane->values[IGT_PLANE_SRC_H];
>> + crtc_x = plane->values[IGT_PLANE_CRTC_X];
>> + crtc_y = plane->values[IGT_PLANE_CRTC_Y];
>> + crtc_w = plane->values[IGT_PLANE_CRTC_W];
>> + crtc_h = plane->values[IGT_PLANE_CRTC_H];
>>
>> LOG(display,
>> "SetPlane %s.%d, fb %u, src = (%d, %d) "
>> @@ -2245,9 +2199,10 @@ static int igt_drm_plane_commit(igt_plane_t
>> *plane,
>> CHECK_RETURN(ret, fail_on_error);
>> }
>>
>> - if (plane->rotation_changed) {
>> - ret = igt_plane_set_property(plane, plane-
>>> rotation_property,
>> - plane->rotation);
>> + if (igt_plane_is_prop_changed(plane, IGT_PLANE_ROTATION)) {
>> + ret = igt_plane_set_property(plane,
>> + plane-
>>> props[IGT_PLANE_ROTATION],
>> + plane-
>>> values[IGT_PLANE_ROTATION]);
>>
>> CHECK_RETURN(ret, fail_on_error);
>> }
>> @@ -2269,35 +2224,30 @@ static int
>> igt_cursor_commit_legacy(igt_plane_t *cursor,
>> uint32_t crtc_id = pipe->crtc_id;
>> int ret;
>>
>> - if (cursor->fb_changed) {
>> - uint32_t gem_handle =
>> igt_plane_get_fb_gem_handle(cursor);
>> -
>> - if (gem_handle) {
>> + if (igt_plane_is_prop_changed(cursor, IGT_PLANE_FB_ID)) {
>> + if (cursor->gem_handle)
>> LOG(display,
>> "SetCursor pipe %s, fb %u %dx%d\n",
>> kmstest_pipe_name(pipe->pipe),
>> - gem_handle,
>> - cursor->crtc_w, cursor->crtc_h);
>> -
>> - ret = drmModeSetCursor(display->drm_fd,
>> crtc_id,
>> - gem_handle,
>> - cursor->crtc_w,
>> - cursor->crtc_h);
>> - } else {
>> + cursor->gem_handle,
>> + (unsigned)cursor-
>>> values[IGT_PLANE_CRTC_W],
>> + (unsigned)cursor-
>>> values[IGT_PLANE_CRTC_H]);
>> + else
>> LOG(display,
>> "SetCursor pipe %s, disabling\n",
>> kmstest_pipe_name(pipe->pipe));
>>
>> - ret = drmModeSetCursor(display->drm_fd,
>> crtc_id,
>> - 0, 0, 0);
>> - }
>> -
>> + ret = drmModeSetCursor(display->drm_fd, crtc_id,
>> + cursor->gem_handle,
>> + cursor-
>>> values[IGT_PLANE_CRTC_W],
>> + cursor-
>>> values[IGT_PLANE_CRTC_H]);
>> CHECK_RETURN(ret, fail_on_error);
>> }
>>
>> - if (cursor->position_changed) {
>> - int x = cursor->crtc_x;
>> - int y = cursor->crtc_y;
>> + if (igt_plane_is_prop_changed(cursor, IGT_PLANE_CRTC_X) ||
>> + igt_plane_is_prop_changed(cursor, IGT_PLANE_CRTC_Y)) {
>> + int x = cursor->values[IGT_PLANE_CRTC_X];
>> + int y = cursor->values[IGT_PLANE_CRTC_Y];
>>
>> LOG(display,
>> "MoveCursor pipe %s, (%d, %d)\n",
>> @@ -2326,13 +2276,14 @@ static int
>> igt_primary_plane_commit_legacy(igt_plane_t *primary,
>> int ret;
>>
>> /* Primary planes can't be windowed when using a legacy
>> commit */
>> - igt_assert((primary->crtc_x == 0 && primary->crtc_y == 0));
>> + igt_assert((primary->values[IGT_PLANE_CRTC_X] == 0 &&
>> primary->values[IGT_PLANE_CRTC_Y] == 0));
>>
>> /* nor rotated */
>> - igt_assert(!primary->rotation_changed);
>> + igt_assert(!igt_plane_is_prop_changed(primary,
>> IGT_PLANE_ROTATION));
>>
>> - if (!primary->fb_changed && !primary->position_changed &&
>> - !primary->size_changed && !primary->pipe->mode_changed)
>> + if (!igt_plane_is_prop_changed(primary, IGT_PLANE_FB_ID) &&
>> + !(primary->changed & IGT_PLANE_COORD_CHANGED_MASK) &&
>> + !primary->pipe->mode_changed)
>> return 0;
>>
>> crtc_id = pipe->crtc_id;
>> @@ -2343,19 +2294,22 @@ static int
>> igt_primary_plane_commit_legacy(igt_plane_t *primary,
>> mode = NULL;
>>
>> if (fb_id) {
>> + uint32_t src_x = primary->values[IGT_PLANE_SRC_X] >>
>> 16;
>> + uint32_t src_y = primary->values[IGT_PLANE_SRC_Y] >>
>> 16;
>> +
>> LOG(display,
>> "%s: SetCrtc pipe %s, fb %u, src (%d, %d), "
>> "mode %dx%d\n",
>> igt_output_name(output),
>> kmstest_pipe_name(pipe->pipe),
>> fb_id,
>> - primary->src_x, primary->src_y,
>> + src_x, src_y,
>> mode->hdisplay, mode->vdisplay);
>>
>> ret = drmModeSetCrtc(display->drm_fd,
>> crtc_id,
>> fb_id,
>> - primary->src_x, primary->src_y,
>> + src_x, src_y,
>> &output->id,
>> 1,
>> mode);
>> @@ -2608,18 +2562,27 @@ display_commit_changed(igt_display_t
>> *display, enum igt_commit_style s)
>> }
>>
>> for_each_plane_on_pipe(display, pipe, plane) {
>> - plane->fb_changed = false;
>> - plane->position_changed = false;
>> - plane->size_changed = false;
>> + if (s == COMMIT_ATOMIC) {
>> + int fd;
>> + plane->changed = 0;
>>
>> - if (s != COMMIT_LEGACY ||
>> - !(plane->type == DRM_PLANE_TYPE_PRIMARY
>> ||
>> - plane->type == DRM_PLANE_TYPE_CURSOR))
>> - plane->rotation_changed = false;
>> + fd = plane-
>>> values[IGT_PLANE_IN_FENCE_FD];
>> + if (fd != -1)
>> + close(fd);
>>
>> - if (s == COMMIT_ATOMIC)
>> /* reset fence_fd to prevent it from
>> being set for the next commit */
>> - igt_plane_set_fence_fd(plane, -1);
>> + plane->values[IGT_PLANE_IN_FENCE_FD]
>> = -1;
>> + } else {
>> + plane->changed &=
>> ~IGT_PLANE_COORD_CHANGED_MASK;
>> +
>> + igt_plane_clear_prop_changed(plane,
>> IGT_PLANE_CRTC_ID);
>> + igt_plane_clear_prop_changed(plane,
>> IGT_PLANE_FB_ID);
>> +
>> + if (s != COMMIT_LEGACY ||
>> + !(plane->type ==
>> DRM_PLANE_TYPE_PRIMARY ||
>> + plane->type ==
>> DRM_PLANE_TYPE_CURSOR))
>> + igt_plane_clear_prop_changed
>> (plane, IGT_PLANE_ROTATION);
>> + }
>> }
>> }
>>
>> @@ -2913,30 +2876,31 @@ void igt_plane_set_fb(igt_plane_t *plane,
>> struct igt_fb *fb)
>> LOG(display, "%s.%d: plane_set_fb(%d)\n",
>> kmstest_pipe_name(pipe->pipe),
>> plane->index, fb ? fb->fb_id : 0);
>>
>> - plane->fb = fb;
>> + igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID, fb ?
>> pipe->crtc_id : 0);
>> + igt_plane_set_prop_value(plane, IGT_PLANE_FB_ID, fb ? fb-
>>> fb_id : 0);
>> +
>> + if (plane->type == DRM_PLANE_TYPE_CURSOR && fb)
>> + plane->gem_handle = fb->gem_handle;
>> + else
>> + plane->gem_handle = 0;
>> +
>> /* hack to keep tests working that don't call
>> igt_plane_set_size() */
>> if (fb) {
>> /* set default plane size as fb size */
>> - plane->crtc_w = fb->width;
>> - plane->crtc_h = fb->height;
>> + igt_plane_set_position(plane, 0, 0);
>> + igt_plane_set_size(plane, fb->width, fb->height);
>>
>> /* set default src pos/size as fb size */
>> - plane->src_x = 0;
>> - plane->src_y = 0;
>> - plane->src_w = fb->width;
>> - plane->src_h = fb->height;
>> + igt_fb_set_position(fb, plane, 0, 0);
>> + igt_fb_set_size(fb, plane, fb->width, fb->height);
>> } else {
>> - plane->src_x = 0;
>> - plane->src_y = 0;
>> - plane->src_w = 0;
>> - plane->src_h = 0;
>> + igt_plane_set_position(plane, 0, 0);
>> + igt_plane_set_size(plane, 0, 0);
>>
>> - plane->crtc_w = 0;
>> - plane->crtc_h = 0;
>> + /* set default src pos/size as fb size */
>> + igt_fb_set_position(fb, plane, 0, 0);
>> + igt_fb_set_size(fb, plane, 0, 0);
>> }
>> -
>> - plane->fb_changed = true;
>> - plane->size_changed = true;
>> }
>>
>> /**
>> @@ -2949,12 +2913,19 @@ void igt_plane_set_fb(igt_plane_t *plane,
>> struct igt_fb *fb)
>> */
>> void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd)
>> {
>> - close(plane->fence_fd);
>> + int64_t fd;
>>
>> - if (fcntl(fence_fd, F_GETFD) != -1)
>> - plane->fence_fd = dup(fence_fd);
>> - else
>> - plane->fence_fd = -1;
>> + fd = plane->values[IGT_PLANE_IN_FENCE_FD];
>> + if (fd != -1)
>> + close(fd);
>> +
>> + if (fence_fd != -1) {
>> + fd = dup(fence_fd);
>> + igt_fail_on(fd == -1);
>> + } else
>> + fd = -1;
>> +
>> + igt_plane_set_prop_value(plane, IGT_PLANE_IN_FENCE_FD, fd);
>> }
>>
>> void igt_plane_set_position(igt_plane_t *plane, int x, int y)
>> @@ -2965,10 +2936,8 @@ void igt_plane_set_position(igt_plane_t
>> *plane, int x, int y)
>> LOG(display, "%s.%d: plane_set_position(%d,%d)\n",
>> kmstest_pipe_name(pipe->pipe), plane->index, x, y);
>>
>> - plane->crtc_x = x;
>> - plane->crtc_y = y;
>> -
>> - plane->position_changed = true;
>> + igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_X, x);
>> + igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_Y, y);
>> }
>>
>> /**
>> @@ -2989,10 +2958,8 @@ void igt_plane_set_size(igt_plane_t *plane,
>> int w, int h)
>> LOG(display, "%s.%d: plane_set_size (%dx%d)\n",
>> kmstest_pipe_name(pipe->pipe), plane->index, w, h);
>>
>> - plane->crtc_w = w;
>> - plane->crtc_h = h;
>> -
>> - plane->size_changed = true;
>> + igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_W, w);
>> + igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_H, h);
>> }
>>
>> /**
>> @@ -3014,10 +2981,8 @@ void igt_fb_set_position(struct igt_fb *fb,
>> igt_plane_t *plane,
>> LOG(display, "%s.%d: fb_set_position(%d,%d)\n",
>> kmstest_pipe_name(pipe->pipe), plane->index, x, y);
>>
>> - plane->src_x = x;
>> - plane->src_y = y;
>> -
>> - plane->fb_changed = true;
>> + igt_plane_set_prop_value(plane, IGT_PLANE_SRC_X,
>> IGT_FIXED(x, 0));
>> + igt_plane_set_prop_value(plane, IGT_PLANE_SRC_Y,
>> IGT_FIXED(y, 0));
>> }
>>
>> /**
>> @@ -3040,10 +3005,8 @@ void igt_fb_set_size(struct igt_fb *fb,
>> igt_plane_t *plane,
>> LOG(display, "%s.%d: fb_set_size(%dx%d)\n",
>> kmstest_pipe_name(pipe->pipe), plane->index, w, h);
>>
>> - plane->src_w = w;
>> - plane->src_h = h;
>> -
>> - plane->fb_changed = true;
>> + igt_plane_set_prop_value(plane, IGT_PLANE_SRC_W,
>> IGT_FIXED(w, 0));
>> + igt_plane_set_prop_value(plane, IGT_PLANE_SRC_H,
>> IGT_FIXED(h, 0));
>> }
>>
>> static const char *rotation_name(igt_rotation_t rotation)
>> @@ -3071,9 +3034,7 @@ void igt_plane_set_rotation(igt_plane_t *plane,
>> igt_rotation_t rotation)
>> kmstest_pipe_name(pipe->pipe),
>> plane->index, rotation_name(rotation));
>>
>> - plane->rotation = rotation;
>> -
>> - plane->rotation_changed = true;
>> + igt_plane_set_prop_value(plane, IGT_PLANE_ROTATION,
>> rotation);
>> }
>>
>> /**
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 1ef10e7d525c..f87f8be31421 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -252,6 +252,9 @@ enum igt_atomic_plane_properties {
>> IGT_PLANE_CRTC_W,
>> IGT_PLANE_CRTC_H,
>>
>> +/* Append new properties after IGT_PLANE_COORD_CHANGED_MASK */
>> +#define IGT_PLANE_COORD_CHANGED_MASK 0xff
>> +
>> IGT_PLANE_FB_ID,
>> IGT_PLANE_CRTC_ID,
>> IGT_PLANE_IN_FENCE_FD,
>> @@ -286,37 +289,19 @@ typedef struct {
>> int index;
>> /* capabilities */
>> int type;
>> - /* state tracking */
>> - unsigned int fb_changed : 1;
>> - unsigned int position_changed : 1;
>> - unsigned int rotation_changed : 1;
>> - unsigned int size_changed : 1;
>> +
>> /*
>> * drm_plane can be NULL for primary and cursor planes (when
>> not
>> * using the atomic modeset API)
>> */
>> drmModePlane *drm_plane;
>> - struct igt_fb *fb;
>> -
>> - uint32_t rotation_property;
>> -
>> - /* position within pipe_src_w x pipe_src_h */
>> - int crtc_x, crtc_y;
>> - /* size within pipe_src_w x pipe_src_h */
>> - int crtc_w, crtc_h;
>>
>> - /* position within the framebuffer */
>> - uint32_t src_x;
>> - uint32_t src_y;
>> - /* size within the framebuffer*/
>> - uint32_t src_w;
>> - uint32_t src_h;
>> + /* gem handle for fb */
>> + uint32_t gem_handle;
>>
>> - igt_rotation_t rotation;
>> -
>> - /* in fence fd */
>> - int fence_fd;
>> - uint32_t atomic_props_plane[IGT_NUM_PLANE_PROPS];
>> + uint64_t changed;
>> + uint32_t props[IGT_NUM_PLANE_PROPS];
>> + uint64_t values[IGT_NUM_PLANE_PROPS];
>> } igt_plane_t;
>>
>> struct igt_pipe {
>> @@ -407,7 +392,7 @@ bool igt_pipe_get_property(igt_pipe_t *pipe,
>> const char *name,
>>
>> static inline bool igt_plane_supports_rotation(igt_plane_t *plane)
>> {
>> - return plane->rotation_property != 0;
>> + return plane->props[IGT_PLANE_ROTATION] != 0;
>> }
>> void igt_pipe_request_out_fence(igt_pipe_t *pipe);
>> void igt_pipe_set_degamma_lut(igt_pipe_t *pipe, void *ptr, size_t
>> length);
>> @@ -527,16 +512,20 @@ static inline bool
>> igt_output_is_connected(igt_output_t *output)
>>
>> #define IGT_FIXED(i,f) ((i) << 16 | (f))
>>
>> -/**
>> - * igt_atomic_populate_plane_req:
>> - * @req: A pointer to drmModeAtomicReq
>> - * @plane: A pointer igt_plane_t
>> - * @prop: one of igt_atomic_plane_properties
>> - * @value: the value to add
>> - */
>> -#define igt_atomic_populate_plane_req(req, plane, prop, value) \
>> - igt_assert_lt(0, drmModeAtomicAddProperty(req, plane-
>>> drm_plane->plane_id,\
>> - plane-
>>> atomic_props_plane[prop], value))
>> +#define igt_plane_is_prop_changed(plane, prop) \
>> + (!!((plane)->changed & (1 << (prop))))
>> +
>> +#define igt_plane_set_prop_changed(plane, prop) \
>> + (plane)->changed |= 1 << (prop)
>> +
>> +#define igt_plane_clear_prop_changed(plane, prop) \
>> + (plane)->changed &= ~(1 << (prop))
>> +
>> +#define igt_plane_set_prop_value(plane, prop, value) \
>> + do { \
>> + plane->values[prop] = value; \
>> + igt_plane_set_prop_changed(plane, prop); \
>> + } while (0)
>>
>> /**
>> * igt_atomic_populate_crtc_req:
>> diff --git a/tests/kms_atomic_interruptible.c
>> b/tests/kms_atomic_interruptible.c
>> index 2d19fe967809..5570854390ea 100644
>> --- a/tests/kms_atomic_interruptible.c
>> +++ b/tests/kms_atomic_interruptible.c
>> @@ -161,12 +161,12 @@ static void run_plane_test(igt_display_t
>> *display, enum pipe pipe, igt_output_t
>> /* connector: 1 prop */
>> output-
>>> props[IGT_CONNECTOR_CRTC_ID],
>> /* plane: remainder props */
>> - plane-
>>> atomic_props_plane[IGT_PLANE_CRTC_ID],
>> - plane-
>>> atomic_props_plane[IGT_PLANE_FB_ID],
>> - plane-
>>> atomic_props_plane[IGT_PLANE_SRC_W],
>> - plane-
>>> atomic_props_plane[IGT_PLANE_SRC_H],
>> - plane-
>>> atomic_props_plane[IGT_PLANE_CRTC_W],
>> - plane-
>>> atomic_props_plane[IGT_PLANE_CRTC_H]
>> + plane-
>>> props[IGT_PLANE_CRTC_ID],
>> + plane-
>>> props[IGT_PLANE_FB_ID],
>> + plane-
>>> props[IGT_PLANE_SRC_W],
>> + plane-
>>> props[IGT_PLANE_SRC_H],
>> + plane-
>>> props[IGT_PLANE_CRTC_W],
>> + plane-
>>> props[IGT_PLANE_CRTC_H]
>> };
>> uint64_t prop_vals[] = {
>> /* crtc */
>> diff --git a/tests/kms_plane_lowres.c b/tests/kms_plane_lowres.c
>> index b16c8cd433b2..613f68899601 100644
>> --- a/tests/kms_plane_lowres.c
>> +++ b/tests/kms_plane_lowres.c
>> @@ -227,8 +227,8 @@ test_setup(data_t *data, enum pipe pipe, uint64_t
>> modifier, int flags,
>> 1.0, 1.0, 0.0,
>> &data->fb[i]);
>>
>> - igt_plane_set_position(data->plane[i], x, y);
>> igt_plane_set_fb(data->plane[i], &data->fb[i]);
>> + igt_plane_set_position(data->plane[i], x, y);
> This part could be split into a separate patch as it fixes an error.
> BTW why do we need to igt_plane_set_fb() before
> igt_plane_set_position()
It wasn't an error before, igt_plane_set_fb cleared everything to defaults except position, but with this patch I felt it should clear position too because why keep it special?
Hmm though it seems multiple tests assume that position is untouched by igt_plane_set_fb, I think it might be better to revert this hunk. Perhaps clean it up later..
I'll just drop this part for now. Reality is clashing with ideals again. :(
>> }
>>
>> return mode;
>> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
>> index 4d2ef1c184f0..4932a0d44410 100644
>> --- a/tests/kms_rotation_crc.c
>> +++ b/tests/kms_rotation_crc.c
>> @@ -122,11 +122,11 @@ static void prepare_crtc(data_t *data,
>> igt_output_t *output, enum pipe pipe,
>> igt_plane_set_fb(primary, &data->fb_modeset);
>>
>> if (commit < COMMIT_ATOMIC) {
>> - primary->rotation_changed = false;
>> + igt_plane_clear_prop_changed(primary,
>> IGT_PLANE_ROTATION);
>> igt_display_commit(display);
>>
>> if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>> - primary->rotation_changed = true;
>> + igt_plane_set_prop_changed(primary,
>> IGT_PLANE_ROTATION);
>> }
>>
>> igt_plane_set_fb(plane, NULL);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
