[Intel-gfx] [PATCH i-g-t v2 02/14] lib/igt_kms: Rework plane properties to be more atomic, v5.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Thu Oct 19 09:44:32 UTC 2017
Op 19-10-17 om 11:08 schreef Mika Kahola:
> On Thu, 2017-10-12 at 13:54 +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.
>> Changes since v4:
>> - Back out resetting crtc position in igt_plane_set_fb() and
>> document it during init. Tests appear to rely on it being
>> preserved.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>> lib/igt_kms.c | 299 +++++++++++++++++----------
>> ------------
>> lib/igt_kms.h | 59 ++++----
>> tests/kms_atomic_interruptible.c | 12 +-
>> tests/kms_rotation_crc.c | 4 +-
>> 4 files changed, 165 insertions(+), 209 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index e6fa8f4af455..e77ae5d696da 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,27 @@ 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 info on
>> first commit. */
>> + igt_plane_set_prop_changed(plane,
>> IGT_PLANE_FB_ID);
>> + igt_plane_set_prop_changed(plane,
>> IGT_PLANE_CRTC_ID);
>> +
>> + /*
>> + * CRTC_X/Y are not changed in
>> igt_plane_set_fb, so
>> + * force them to be sanitized in case they
>> contain
>> + * garbage.
>> + */
>> + igt_plane_set_prop_changed(plane,
>> IGT_PLANE_CRTC_X);
>> + igt_plane_set_prop_changed(plane,
>> IGT_PLANE_CRTC_Y);
>> }
>>
>> /*
>> @@ -1805,9 +1814,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 +2076,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 +2085,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 +2093,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;
> Could we add a macro here too? Like 'for_each_prop_on_plane()' or
> similar?
Outside of commit it doesn't make much sense to iterate over the properties.
The only test that enumerates over the array is the reworked kms_atomic, to
see if the value committed is the same as the value returned by getprop.
And the only reason the test does so is because it was easier to store in an
array to memcmp plane->values against kernel_values.
More information about the Intel-gfx
mailing list