[Intel-gfx] [PATCH i-g-t 1/5] kms: Add universal plane support

Damien Lespiau damien.lespiau at intel.com
Fri Jun 13 18:21:01 CEST 2014


On Thu, May 29, 2014 at 08:09:13AM -0700, Matt Roper wrote:
> Add support for universal planes.  This involves revamping the existing
> plane handling a bit to allow primary & cursor planes to come from the
> DRM plane list, rather than always being manually added.  Also,
> eliminate the hard-coded position of primary & cursor in the plane list
> since the DRM could return them in any order.
> 
> To minimize impact for existing tests, we add a new
> igt_display_use_universal_commits() call to toggle between universal and
> legacy API's for programming the primary and cursor planes.
> 
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>

With the tiny change below, that patch looks reasonable. So:

Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>

One thing we may want to clean up when we bring in even more ways to do
commits is how we store the states to flush. What I mean by that is:

- In the *_set_*() functions (eg. igt_plane_set_fb()), we store the
  states that change (eg. in set_fb() we only set fb_changed to true and
  don't touch need_set_crtc/need_set_cursor)

- In the commit, we resolve what we need to do depending on the states
  changed and the commit method.

> ---
>  lib/igt_kms.c | 132 +++++++++++++++++++++++++++++++++++++++++++++-------------
>  lib/igt_kms.h |   5 +++
>  2 files changed, 107 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index d00250d..97e329b 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -500,27 +500,24 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  	 */
>  	display->n_pipes = resources->count_crtcs;
>  
> +	drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);


Can we have:

#ifndef DRM_CLIENT_CAP_UNIVERSAL_PLANES
#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
#endif

As an easy way to make sure that's always defined, even if we don't have
the (unreleased) libdrm with this support?

>  	plane_resources = drmModeGetPlaneResources(display->drm_fd);
>  	igt_assert(plane_resources);
>  
>  	for (i = 0; i < display->n_pipes; i++) {
>  		igt_pipe_t *pipe = &display->pipes[i];
> -		igt_plane_t *plane;
> -		int p, j;
> +		igt_plane_t *plane = NULL;
> +		int p = 0, j;
>  
>  		pipe->display = display;
>  		pipe->pipe = i;
>  
> -		/* primary plane */
> -		p = IGT_PLANE_PRIMARY;
> -		plane = &pipe->planes[p];
> -		plane->pipe = pipe;
> -		plane->index = p;
> -		plane->is_primary = true;
> -
>  		/* add the planes that can be used with that pipe */
>  		for (j = 0; j < plane_resources->count_planes; j++) {
>  			drmModePlane *drm_plane;
> +			drmModeObjectPropertiesPtr proplist;
> +			drmModePropertyPtr prop;
> +			int k;
>  
>  			drm_plane = drmModeGetPlane(display->drm_fd,
>  						    plane_resources->planes[j]);
> @@ -531,21 +528,67 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  				continue;
>  			}
>  
> -			p++;
>  			plane = &pipe->planes[p];
>  			plane->pipe = pipe;
> -			plane->index = p;
> +			plane->index = p++;
>  			plane->drm_plane = drm_plane;
> +
> +			prop = NULL;
> +			proplist = drmModeObjectGetProperties(display->drm_fd,
> +							      plane_resources->planes[j],
> +							      DRM_MODE_OBJECT_PLANE);
> +			for (k = 0; k < proplist->count_props; k++) {
> +				prop = drmModeGetProperty(display->drm_fd, proplist->props[k]);
> +				if (!prop)
> +					continue;
> +
> +				if (strcmp(prop->name, "type") != 0) {
> +					drmModeFreeProperty(prop);
> +					continue;
> +				}
> +
> +				switch (proplist->prop_values[k]) {
> +				case DRM_PLANE_TYPE_PRIMARY:
> +					plane->is_primary = 1;
> +					display->has_universal_planes = 1;
> +					break;
> +				case DRM_PLANE_TYPE_CURSOR:
> +					plane->is_cursor = 1;
> +					pipe->has_cursor = 1;
> +					display->has_universal_planes = 1;
> +					break;
> +				}
> +
> +				drmModeFreeProperty(prop);
> +				break;
> +			}
> +
>  		}
>  
> -		/* cursor plane */
> -		p++;
> -		plane = &pipe->planes[p];
> -		plane->pipe = pipe;
> -		plane->index = p;
> -		plane->is_cursor = true;
> +		/*
> +		 * Add a drm_plane-less primary plane for kernels without
> +		 * universal plane support.
> +		 */
> +		if (!display->has_universal_planes) {
> +			plane = &pipe->planes[p];
> +			plane->pipe = pipe;
> +			plane->index = p++;
> +			plane->is_primary = true;
> +		}
>  
> -		pipe->n_planes = ++p;
> +		/*
> +		 * Add drm_plane-less cursor plane for kernels that don't
> +		 * expose a universal cursor plane.
> +		 */
> +		if (!pipe->has_cursor) {
> +			/* cursor plane */
> +			plane = &pipe->planes[p];
> +			plane->pipe = pipe;
> +			plane->index = p++;
> +			plane->is_cursor = true;
> +		}
> +
> +		pipe->n_planes = p;
>  
>  		/* make sure we don't overflow the plane array */
>  		igt_assert(pipe->n_planes <= IGT_MAX_PLANES);
> @@ -700,16 +743,29 @@ static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, enum igt_plane plane)
>  {
>  	int idx;
>  
> -	/* Cursor plane is always the upper plane */
> -	if (plane == IGT_PLANE_CURSOR)
> -		idx = pipe->n_planes - 1;
> -	else {
> -		igt_assert_f(plane >= 0 && plane < (pipe->n_planes),
> -			     "plane=%d\n", plane);
> -		idx = plane;
> +	for (idx = 0; idx < pipe->n_planes; idx++) {
> +		switch (plane) {
> +		case IGT_PLANE_PRIMARY:
> +			if (pipe->planes[idx].is_primary)
> +				return &pipe->planes[idx];
> +			break;
> +		case IGT_PLANE_CURSOR:
> +			if (pipe->planes[idx].is_cursor)
> +				return &pipe->planes[idx];
> +			break;
> +		case IGT_PLANE_2:
> +			if (!pipe->planes[idx].is_primary &&
> +			    !pipe->planes[idx].is_cursor)
> +				return &pipe->planes[idx];
> +		default:
> +			if (!pipe->planes[idx].is_primary &&
> +			    !pipe->planes[idx].is_cursor)
> +				/* Consume this overlay and keep searching for another */
> +				plane--;
> +		}
>  	}
>  
> -	return &pipe->planes[idx];
> +	return NULL;
>  }
>  
>  static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
> @@ -761,6 +817,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
>  	uint32_t fb_id, crtc_id;
>  	int ret;
>  
> +	igt_assert(plane->drm_plane);
> +
>  	fb_id = igt_plane_get_fb_id(plane);
>  	crtc_id = output->config.crtc->crtc_id;
>  	pipe = igt_output_get_driving_pipe(output);
> @@ -820,7 +878,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
>  
>  static int igt_plane_commit(igt_plane_t *plane, igt_output_t *output)
>  {
> -	if (plane->is_cursor) {
> +	struct igt_display *display = plane->pipe->display;
> +
> +	if (display->commit_universal && plane->drm_plane) {
> +		igt_drm_plane_commit(plane, output);
> +	} else if (plane->is_cursor) {
>  		igt_cursor_commit(plane, output);
>  	} else if (plane->is_primary) {
>  		/* state updated by SetCrtc */
> @@ -838,8 +900,8 @@ static int igt_output_commit(igt_output_t *output)
>  	int i;
>  
>  	pipe = igt_output_get_driving_pipe(output);
> -	if (pipe->need_set_crtc) {
> -		igt_plane_t *primary = &pipe->planes[0];
> +	if (pipe->need_set_crtc && !display->commit_universal) {
> +		igt_plane_t *primary = igt_pipe_get_plane(pipe, IGT_PLANE_PRIMARY);
>  		drmModeModeInfo *mode;
>  		uint32_t fb_id, crtc_id;
>  		int ret;
> @@ -889,7 +951,7 @@ static int igt_output_commit(igt_output_t *output)
>  		primary->fb_changed = false;
>  	}
>  
> -	if (pipe->need_set_cursor) {
> +	if (pipe->need_set_cursor && !display->commit_universal) {
>  		igt_plane_t *cursor;
>  		uint32_t gem_handle, crtc_id;
>  		int ret;
> @@ -940,6 +1002,16 @@ static int igt_output_commit(igt_output_t *output)
>  	return 0;
>  }
>  
> +/*
> + * Indicate whether future display commits should use universal plane API's
> + * or legacy API's.
> + */
> +void igt_display_use_universal_commits(igt_display_t *display,
> +				       bool use_universal)
> +{
> +	display->commit_universal = use_universal;
> +}
> +
>  int igt_display_commit(igt_display_t *display)
>  {
>  	int i;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 8e80d4b..4955fc8 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -121,6 +121,7 @@ struct igt_pipe {
>  	unsigned int need_set_crtc        : 1;
>  	unsigned int need_set_cursor      : 1;
>  	unsigned int need_wait_for_vblank : 1;
> +	unsigned int has_cursor           : 1;
>  #define IGT_MAX_PLANES	4
>  	int n_planes;
>  	igt_plane_t planes[IGT_MAX_PLANES];
> @@ -143,6 +144,8 @@ struct igt_display {
>  	unsigned long pipes_in_use;
>  	igt_output_t *outputs;
>  	igt_pipe_t pipes[I915_MAX_PIPES];
> +	bool has_universal_planes;
> +	bool commit_universal;
>  };
>  
>  /* set vt into graphics mode, required to prevent fbcon from interfering */
> @@ -150,6 +153,8 @@ void igt_set_vt_graphics_mode(void);
>  
>  void igt_display_init(igt_display_t *display, int drm_fd);
>  void igt_display_fini(igt_display_t *display);
> +void igt_display_use_universal_commits(igt_display_t *display,
> +				       bool use_universal);
>  int  igt_display_commit(igt_display_t *display);
>  int  igt_display_get_n_pipes(igt_display_t *display);
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



More information about the Intel-gfx mailing list