[igt-dev] [PATCH i-g-t v3] lib/igt_kms: Fix commits for planes with multiple possible CRTCs

Daniel Vetter daniel at ffwll.ch
Thu Feb 28 10:23:32 UTC 2019


On Wed, Feb 27, 2019 at 08:13:37PM +0000, Wentland, Harry wrote:
> On 2019-02-25 10:42 a.m., Nicholas Kazlauskas wrote:
> > An igt_plane_t is defined per igt_pipe_t. It is treated as its
> > own independent resource but DRM planes can be exposed to being used on
> > a number of possible CRTCs - making it a shared resource.
> > 
> > In IGT planes with multiple possible CRTCs are added to the plane list
> > for each pipe that the plane supports. The internal state remains
> > independent in IGT so when the same plane is modified for multiple
> > pipes in a single commit the last pipe to modify it is the one whose
> > state gets fully applied.
> > 
> > This situation happens fairly often in practice - resetting the display
> > at the start of the test before a commit will reset the CRTC ID and FB
> > ID for each plane.
> > 
> > For an example, consider the
> > igt at kms_plane_alpha_blend@pipe-a-constant-alpha-max test.
> > 
> > This test will fail for any overlay plane exposed for multiple CRTCs.
> > 
> > The test tries to set a framebuffer for pipe A but has all the other
> > pipes reset the plane state in the same commit. If there are multiple
> > pipes on the hardware then the last pipe will be the one to set all the
> > plane properties. The driver will receive a commit with no overlay
> > plane enabled since the last pipe set CRTC ID and FB ID to 0, disabling
> > the plane. The reference CRC capture will be incorrect since not all
> > the planes have been enabled and the subsequent CRC captures will
> > not match, failing the test.
> > 
> > The simplest (but hacky) fix to this problem is to only set the
> > properties for the plane for the pipe that last had it bound.
> > 
> > This patch introduces a global plane list on igt_display_t that keeps
> > track of the pipe that pipe that last called igt_plane_set_fb. The
> > properties for the plane will only be applied from that single pipe
> > when commiting the state to DRM.
> > 
> > No behavioral changes should be introduced by this patch for hardware
> > whose planes are only ever exposed one CRTC.
> > 
> > It would likely be best to eventually modify the igt_pipe_t plane list
> > to be a list of pointers to planes instead (igt_plane_t**)
> > instead of being the actual plane objects, but that can come later.
> > 
> > Many areas of the code like to make use of the backpointer to the pipe
> > on the plane which makes refactoring the code in that manner a little
> > trickier.
> > 
> > v2: Add igt_plane_set_fb, use igt_plane_t for global plane list (Daniel)
> > v3: Leave TODO for filling in all state/props on global planes
> > 
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> 
> Acked-by: Harry Wentland <harry.wentland at amd.com>
> 
> Daniel, ping.

Sry, drowning and behind mails. I acked the general plan already, as long
as you don't upset CI all fine with me. Once it's all done would be good
to make a fine combed pass over docs and make sure all the fixed
pipe/plane association language is also gone.
-Daniel

> 
> Harry
> 
> > ---
> >  lib/igt_kms.c | 103 ++++++++++++++++++++++++++++++++++++--------------
> >  lib/igt_kms.h |   6 ++-
> >  2 files changed, 79 insertions(+), 30 deletions(-)
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 761bb193..a68a9087 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -1905,6 +1905,26 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  	plane_resources = drmModeGetPlaneResources(display->drm_fd);
> >  	igt_assert(plane_resources);
> >  
> > +	display->n_planes = plane_resources->count_planes;
> > +	display->planes = calloc(sizeof(igt_plane_t), display->n_planes);
> > +	igt_assert_f(display->planes, "Failed to allocate memory for %d planes\n", display->n_planes);
> > +
> > +	for (i = 0; i < plane_resources->count_planes; ++i) {
> > +		igt_plane_t *plane = &display->planes[i];
> > +		uint32_t id = plane_resources->planes[i];
> > +
> > +		plane->drm_plane = drmModeGetPlane(display->drm_fd, id);
> > +		igt_assert(plane->drm_plane);
> > +
> > +		plane->type = get_drm_plane_type(display->drm_fd, id);
> > +
> > +		/*
> > +		 * TODO: Fill in the rest of the plane properties here and
> > +		 * move away from the plane per pipe model to align closer
> > +		 * to the DRM KMS model.
> > +		 */
> > +	}
> > +
> >  	for_each_pipe(display, i) {
> >  		igt_pipe_t *pipe = &display->pipes[i];
> >  		igt_plane_t *plane;
> > @@ -1922,17 +1942,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  		igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
> >  
> >  		/* count number of valid planes */
> > -		for (j = 0; j < plane_resources->count_planes; j++) {
> > -			drmModePlane *drm_plane;
> > -
> > -			drm_plane = drmModeGetPlane(display->drm_fd,
> > -						    plane_resources->planes[j]);
> > +		for (j = 0; j < display->n_planes; j++) {
> > +			drmModePlane *drm_plane = display->planes[j].drm_plane;
> >  			igt_assert(drm_plane);
> >  
> >  			if (drm_plane->possible_crtcs & (1 << i))
> >  				n_planes++;
> > -
> > -			drmModeFreePlane(drm_plane);
> >  		}
> >  
> >  		igt_assert_lt(0, n_planes);
> > @@ -1941,20 +1956,14 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  		last_plane = n_planes - 1;
> >  
> >  		/* add the planes that can be used with that pipe */
> > -		for (j = 0; j < plane_resources->count_planes; j++) {
> > -			drmModePlane *drm_plane;
> > -
> > -			drm_plane = drmModeGetPlane(display->drm_fd,
> > -						    plane_resources->planes[j]);
> > -			igt_assert(drm_plane);
> > +		for (j = 0; j < display->n_planes; j++) {
> > +			igt_plane_t *global_plane = &display->planes[j];
> > +			drmModePlane *drm_plane = global_plane->drm_plane;
> >  
> > -			if (!(drm_plane->possible_crtcs & (1 << i))) {
> > -				drmModeFreePlane(drm_plane);
> > +			if (!(drm_plane->possible_crtcs & (1 << i)))
> >  				continue;
> > -			}
> >  
> > -			type = get_drm_plane_type(display->drm_fd,
> > -						  plane_resources->planes[j]);
> > +			type = global_plane->type;
> >  
> >  			if (type == DRM_PLANE_TYPE_PRIMARY && pipe->plane_primary == -1) {
> >  				plane = &pipe->planes[0];
> > @@ -1975,6 +1984,14 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  			plane->pipe = pipe;
> >  			plane->drm_plane = drm_plane;
> >  			plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
> > +			plane->ref = global_plane;
> > +
> > +			/*
> > +			 * HACK: point the global plane to the first pipe that
> > +			 * it can go on.
> > +			 */
> > +			if (!global_plane->ref)
> > +				igt_plane_set_pipe(plane, pipe);
> >  
> >  			igt_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
> >  
> > @@ -2127,17 +2144,6 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
> >  
> >  static void igt_pipe_fini(igt_pipe_t *pipe)
> >  {
> > -	int i;
> > -
> > -	for (i = 0; i < pipe->n_planes; i++) {
> > -		igt_plane_t *plane = &pipe->planes[i];
> > -
> > -		if (plane->drm_plane) {
> > -			drmModeFreePlane(plane->drm_plane);
> > -			plane->drm_plane = NULL;
> > -		}
> > -	}
> > -
> >  	free(pipe->planes);
> >  	pipe->planes = NULL;
> >  
> > @@ -2163,6 +2169,15 @@ void igt_display_fini(igt_display_t *display)
> >  {
> >  	int i;
> >  
> > +	for (i = 0; i < display->n_planes; ++i) {
> > +		igt_plane_t *plane = &display->planes[i];
> > +
> > +		if (plane->drm_plane) {
> > +			drmModeFreePlane(plane->drm_plane);
> > +			plane->drm_plane = NULL;
> > +		}
> > +	}
> > +
> >  	for (i = 0; i < display->n_pipes; i++)
> >  		igt_pipe_fini(&display->pipes[i]);
> >  
> > @@ -2172,6 +2187,8 @@ void igt_display_fini(igt_display_t *display)
> >  	display->outputs = NULL;
> >  	free(display->pipes);
> >  	display->pipes = NULL;
> > +	free(display->planes);
> > +	display->planes = NULL;
> >  }
> >  
> >  static void igt_display_refresh(igt_display_t *display)
> > @@ -2789,6 +2806,10 @@ static int igt_pipe_commit(igt_pipe_t *pipe,
> >  	for (i = 0; i < pipe->n_planes; i++) {
> >  		igt_plane_t *plane = &pipe->planes[i];
> >  
> > +		/* skip planes that are handled by another pipe */
> > +		if (plane->ref->pipe != pipe)
> > +			continue;
> > +
> >  		ret = igt_plane_commit(plane, pipe, s, fail_on_error);
> >  		CHECK_RETURN(ret, fail_on_error);
> >  	}
> > @@ -3177,6 +3198,10 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
> >  			igt_atomic_prepare_crtc_commit(pipe_obj, req);
> >  
> >  		for_each_plane_on_pipe(display, pipe, plane) {
> > +			/* skip planes that are handled by another pipe */
> > +			if (plane->ref->pipe != pipe_obj)
> > +				continue;
> > +
> >  			if (plane->changed)
> >  				igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
> >  		}
> > @@ -3709,6 +3734,9 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
> >  		if (igt_plane_has_prop(plane, IGT_PLANE_COLOR_RANGE))
> >  			igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_RANGE,
> >  				igt_color_range_to_str(fb->color_range));
> > +
> > +		/* Hack to prioritize the plane on the pipe that last set fb */
> > +		igt_plane_set_pipe(plane, pipe);
> >  	} else {
> >  		igt_plane_set_size(plane, 0, 0);
> >  
> > @@ -3743,6 +3771,23 @@ void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd)
> >  	igt_plane_set_prop_value(plane, IGT_PLANE_IN_FENCE_FD, fd);
> >  }
> >  
> > +/**
> > + * igt_plane_set_pipe:
> > + * @plane: Target plane pointer
> > + * @pipe: The pipe to assign the plane to
> > + *
> > + */
> > +void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe)
> > +{
> > +	/*
> > +	 * HACK: Point the global plane back to the local plane.
> > +	 * This is used to help apply the correct atomic state while
> > +	 * we're moving away from the single pipe per plane model.
> > +	 */
> > +	plane->ref->ref = plane;
> > +	plane->ref->pipe = pipe;
> > +}
> > +
> >  /**
> >   * igt_plane_set_position:
> >   * @plane: Plane pointer for which position is to be set
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 5755b160..e1c5c967 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -300,9 +300,10 @@ typedef enum {
> >  #define IGT_ROTATION_MASK \
> >  	(IGT_ROTATION_0 | IGT_ROTATION_90 | IGT_ROTATION_180 | IGT_ROTATION_270)
> >  
> > -typedef struct {
> > +typedef struct igt_plane {
> >  	/*< private >*/
> >  	igt_pipe_t *pipe;
> > +	struct igt_plane *ref;
> >  	int index;
> >  	/* capabilities */
> >  	int type;
> > @@ -372,8 +373,10 @@ struct igt_display {
> >  	int drm_fd;
> >  	int log_shift;
> >  	int n_pipes;
> > +	int n_planes;
> >  	int n_outputs;
> >  	igt_output_t *outputs;
> > +	igt_plane_t *planes;
> >  	igt_pipe_t *pipes;
> >  	bool has_cursor_plane;
> >  	bool is_atomic;
> > @@ -413,6 +416,7 @@ void igt_pipe_request_out_fence(igt_pipe_t *pipe);
> >  
> >  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);
> > +void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);
> >  void igt_plane_set_position(igt_plane_t *plane, int x, int y);
> >  void igt_plane_set_size(igt_plane_t *plane, int w, int h);
> >  void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation);
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the igt-dev mailing list