Re: [PATCH i-g-t v2 02/14] lib/igt_kms: Rework plane properties to be more atomic, v5.

From: Mika Kahola <mika.kahola@intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH i-g-t v2 02/14] lib/igt_kms: Rework plane properties to be more atomic, v5.
Date: Fri, 20 Oct 2017 11:03:58 +0300	[thread overview]
Message-ID: <1508486638.3274.140.camel@intel.com> (raw)
In-Reply-To: <114d694a-fd87-96ef-0e2f-614cf07875a3@linux.intel.com>

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@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@intel.com>

> 
-- 
Mika Kahola - Intel OTC

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