[RFC weston 07/14] compositor-drm: Refactor sprite create/destroy into helpers

[Pekka Paalanen]

On Thu, 21 May 2015 08:29:04 +0100
Daniel Stone <daniels at collabora.com> wrote:

> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> This moves the single sprite creation code from create_sprites() into a
> new function. The readability clean-up is small, but my intention is to
> write an alternate version of create_sprites(), and sharing the single
> sprite creation code is useful.
> 
> [daniels: Genericised from drm_sprite to drm_plane, moving some of the
>           logic back into create_sprites(), also symmetrical
> 	  drm_plane_destroy.]
> 
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  src/compositor-drm.c | 121 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 86 insertions(+), 35 deletions(-)
> 
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 15ecec2..5d07955 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -2284,12 +2284,82 @@ err_free:
>  	return -1;
>  }
>  
> +/**
> + * Create a drm_plane for a hardware plane
> + *
> + * Creates one drm_plane structure for a hardware plane, and initialises its
> + * properties and formats.
> + *
> + * This function does not add the plane to the list of usable planes in Weston
> + * itself; the caller is responsible for this.
> + *
> + * Call drm_plane_destroy to clean up the plane.
> + *
> + * @param ec Compositor to create plane for
> + * @param kplane DRM plane to create
> + */
> +static struct drm_plane *
> +drm_plane_create(struct drm_compositor *ec, const drmModePlane *kplane)
> +{
> +	struct drm_plane *plane;
> +
> +	plane = zalloc(sizeof(*plane) + ((sizeof(uint32_t)) *
> +					  kplane->count_formats));
> +	if (!plane) {
> +		weston_log("%s: out of memory\n", __func__);
> +		return NULL;
> +	}
> +
> +	weston_plane_init(&plane->base, &ec->base, 0, 0);
> +	wl_list_init(&plane->link);
> +	plane->possible_crtcs = kplane->possible_crtcs;
> +	plane->plane_id = kplane->plane_id;
> +	plane->current = NULL;
> +	plane->next = NULL;
> +	plane->compositor = ec;
> +	plane->count_formats = kplane->count_formats;
> +	memcpy(plane->formats, kplane->formats,
> +	       kplane->count_formats * sizeof(kplane->formats[0]));
> +
> +	return plane;
> +}
> +
> +/**
> + * Destroy one DRM plane
> + *
> + * Destroy a DRM plane, removing it from screen and releasing its retained
> + * buffers in the process. The counterpart to drm_plane_create.
> + *
> + * @param plane Plane to deallocate (will be freed)
> + */
> +static void
> +drm_plane_destroy(struct drm_plane *plane)
> +{
> +	drmModeSetPlane(plane->compositor->drm.fd,
> +			plane->plane_id, 0 /* XXX: CRTC ID? */, 0, 0,
> +			0, 0, 0, 0, 0, 0, 0, 0);
> +	drm_output_release_fb(plane->output, plane->current);
> +	drm_output_release_fb(plane->output, plane->next);

drm_plane_create() does not set plane->output to non-NULL, so this is
relying on something else, but as explained below, it doesn't really
matter at all at this point of the patch series.

> +	weston_plane_release(&plane->base);
> +	wl_list_remove(&plane->link);
> +	free(plane);
> +}
> +
> +/**
> + * Initialise sprites (overlay planes)
> + *
> + * Walk the list of provided DRM planes, and add overlay planes.
> + *
> + * Call destroy_sprites to free these planes.
> + *
> + * @param ec Compositor to create sprites for.
> + */
>  static void
>  create_sprites(struct drm_compositor *ec)
>  {
> -	struct drm_plane *plane;
>  	drmModePlaneRes *kplane_res;
>  	drmModePlane *kplane;
> +	struct drm_plane *drm_plane;
>  	uint32_t i;
>  
>  	kplane_res = drmModeGetPlaneResources(ec->drm.fd);
> @@ -2304,53 +2374,34 @@ create_sprites(struct drm_compositor *ec)
>  		if (!kplane)
>  			continue;
>  
> -		plane = zalloc(sizeof(*plane) + ((sizeof(uint32_t)) *
> -						  kplane->count_formats));
> -		if (!plane) {
> -			weston_log("%s: out of memory\n",
> -				__func__);
> -			drmModeFreePlane(kplane);
> +		drm_plane = drm_plane_create(ec, kplane);
> +		drmModeFreePlane(kplane);
> +		if (!drm_plane)
>  			continue;
> -		}
>  
> -		plane->possible_crtcs = kplane->possible_crtcs;
> -		plane->plane_id = kplane->plane_id;
> -		plane->current = NULL;
> -		plane->next = NULL;
> -		plane->compositor = ec;
> -		plane->count_formats = kplane->count_formats;
> -		memcpy(plane->formats, kplane->formats,
> -		       kplane->count_formats * sizeof(kplane->formats[0]));
> -		drmModeFreePlane(kplane);
> -		weston_plane_init(&plane->base, &ec->base, 0, 0);
> -		weston_compositor_stack_plane(&ec->base, &plane->base,
> +		weston_plane_init(&drm_plane->base, &ec->base, 0, 0);

drm_plane_create() already called weston_plane_init().

> +		weston_compositor_stack_plane(&ec->base, &drm_plane->base,
>  					      &ec->base.primary_plane);
> -
> -		wl_list_insert(&ec->sprite_list, &plane->link);
> +		wl_list_insert(&ec->sprite_list, &drm_plane->link);

These do not set the plane->output either.

>  	}
>  
>  	drmModeFreePlaneResources(kplane_res);
>  }
>  
> +/**
> + * Clean up sprites (overlay planes)
> + *
> + * The counterpart to create_sprites.
> + *
> + * @param compositor Compositor to deallocate sprites for.
> + */
>  static void
>  destroy_sprites(struct drm_compositor *compositor)
>  {
>  	struct drm_plane *plane, *next;
> -	struct drm_output *output;
>  
> -	output = container_of(compositor->base.output_list.next,
> -			      struct drm_output, base.link);

This is using an arbitrary output. For sprites, the drm_fb is always a
client buffer AFAIU, which means drm_output_release_fb() does not
really need an output, which is why it works.

> -
> -	wl_list_for_each_safe(plane, next, &compositor->sprite_list, link) {
> -		drmModeSetPlane(compositor->drm.fd,
> -				plane->plane_id,
> -				output->crtc_id, 0, 0,
> -				0, 0, 0, 0, 0, 0, 0, 0);
> -		drm_output_release_fb(output, plane->current);
> -		drm_output_release_fb(output, plane->next);
> -		weston_plane_release(&plane->base);
> -		free(plane);
> -	}
> +	wl_list_for_each_safe(plane, next, &compositor->sprite_list, link)
> +		drm_plane_destroy(plane);
>  }
>  
>  static int


Thanks,
pq