[Intel-gfx] [PATCH i-g-t v2 02/14] lib/igt_kms: Rework plane properties to be more atomic, v5.
Mika Kahola
mika.kahola at intel.com
Fri Oct 20 08:03:58 UTC 2017
On Thu, 2017-10-19 at 11:44 +0200, Maarten Lankhorst wrote:
> 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.c
> > > om>
> > > ---
> > > 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(disp
> > > lay,
> > > 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.
Ok. We'll keep it as it is.
Reviewed-by: Mika Kahola <mika.kahola at intel.com>
>
--
Mika Kahola - Intel OTC
More information about the Intel-gfx
mailing list