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

[Derek Foreman]

On 21/05/15 02:29 AM, Daniel Stone 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,

Should (can?) that comment be resolved before landing this?
My lightning fast reading of drm_crtc.c (out of the wrong branch like a
boss) indicates that the crtc isn't even looked up when fb id is 0, just
passed on as null crtc...

So seems like the comment can just go away and we can happily pass 0 (or
whatever we want)?

> +			0, 0, 0, 0, 0, 0, 0, 0);
> +	drm_output_release_fb(plane->output, plane->current);
> +	drm_output_release_fb(plane->output, plane->next);
> +	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);
> +		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);
>  	}
>  
>  	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);
> -
> -	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
>