Re: [PATCH i-g-t v4 5/7] lib/igt_kms: Rework plane properties to be more atomic, v4.

[Mika Kahola <mika.kahola@intel.com>]

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@linux.intel.com>
> ---
>  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()
 
>  	}
>  
>  	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);
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx