[PATCH 2/2] compositor-drm: don't create and destroy kms fbs on every frame

[Kristian Hoegsberg]

On Mon, Apr 30, 2012 at 01:31:29PM +0300, Ander Conselvan de Oliveira wrote:
> Use the new gbm_get/set_user_data() to reuse the kms fbs if the gbm
> surface's bo's are reused.

This doesn't look right.  drm_output_render() always allocates and
sets user data, which overwrite previously set user data.  That leaks
the KMS FB and we have to create a new one every frame.  Also, caching
the KMS FB in user data is useful for gbm bos we get from
drm_output_prepare_scanout_surface() as well.  An application may be
flipping between the same two buffers, and in that case we'll want to
reuse the KMS FBs as well.

Instead, I was thinking that we could just move the drmModeAddFB call
into a get_fb_for_bo() helper function, which would look something like this:

  data = get_user_data(bo);
  if (!data)
    return data->id;

  data = malloc(sizeof *data);
  drmModeAddFB(..., &data->id);

  set_user_data(bo, data);

  return data->id;

Kristian

> ---
>  src/compositor-drm.c |   94 +++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 75 insertions(+), 19 deletions(-)
> 
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index e7433f7..e9b556c 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -199,12 +199,51 @@ drm_output_prepare_scanout_surface(struct drm_output *output)
>  	return 0;
>  }
>  
> +struct bo_user_data {
> +	uint32_t fb_id;
> +};
> +
> +static void
> +bo_set_fb_id(struct gbm_bo *bo, uint32_t fb_id)
> +{
> +	struct bo_user_data *user_data= gbm_bo_get_user_data(bo);
> +
> +	if (!user_data)
> +		return;
> +
> +	user_data->fb_id = fb_id;
> +}
> +
> +static uint32_t
> +bo_get_fb_id(struct gbm_bo *bo)
> +{
> +	struct bo_user_data *user_data= gbm_bo_get_user_data(bo);
> +
> +	if (user_data)
> +		return user_data->fb_id;
> +	else
> +		return 0;
> +}
> +
> +static void
> +bo_free_user_data(struct gbm_bo *bo, void *data)
> +{
> +	struct gbm_device *gbm = gbm_bo_get_device(bo);
> +	struct bo_user_data *user_data = data;
> +
> +	if (user_data->fb_id)
> +		drmModeRmFB(gbm_device_get_fd(gbm), user_data->fb_id);
> +
> +	free(data);
> +}
> +
>  static void
>  drm_output_render(struct drm_output *output, pixman_region32_t *damage)
>  {
>  	struct drm_compositor *compositor =
>  		(struct drm_compositor *) output->base.compositor;
>  	struct weston_surface *surface;
> +	struct bo_user_data *data;
>  
>  	if (!eglMakeCurrent(compositor->base.display, output->egl_surface,
>  			    output->egl_surface, compositor->base.context)) {
> @@ -223,6 +262,15 @@ drm_output_render(struct drm_output *output, pixman_region32_t *damage)
>  		fprintf(stderr, "failed to lock front buffer: %m\n");
>  		return;
>  	}
> +
> +	data = calloc(1, sizeof *data);
> +	if (!data) {
> +		gbm_surface_release_buffer(output->surface, output->next_bo);
> +		output->next_bo = NULL;
> +		return;
> +	}
> +
> +	gbm_bo_set_user_data(output->next_bo, data, bo_free_user_data);
>  }
>  
>  static void
> @@ -241,6 +289,8 @@ drm_output_repaint(struct weston_output *output_base,
>  	if (!output->next_bo)
>  		drm_output_render(output, damage);
>  
> +	output->next_fb_id = bo_get_fb_id(output->next_bo);
> +
>  	stride = gbm_bo_get_pitch(output->next_bo);
>  	handle = gbm_bo_get_handle(output->next_bo).u32;
>  
> @@ -252,15 +302,20 @@ drm_output_repaint(struct weston_output *output_base,
>  		output->next_bo = NULL;
>  	}
>  
> -	ret = drmModeAddFB(compositor->drm.fd,
> -			   output->base.current->width,
> -			   output->base.current->height,
> -			   24, 32, stride, handle, &output->next_fb_id);
> -	if (ret) {
> -		fprintf(stderr, "failed to create kms fb: %m\n");
> -		gbm_surface_release_buffer(output->surface, output->next_bo);
> -		output->next_bo = NULL;
> -		return;
> +	if (!output->next_fb_id) {
> +		ret = drmModeAddFB(compositor->drm.fd,
> +				   output->base.current->width,
> +				   output->base.current->height,
> +				   24, 32, stride, handle, &output->next_fb_id);
> +		if (ret) {
> +			fprintf(stderr, "failed to create kms fb: %m\n");
> +			gbm_surface_release_buffer(output->surface, output->next_bo);
> +			output->next_bo = NULL;
> +			return;
> +		}
> +
> +		if (output->next_bo)
> +			bo_set_fb_id(output->next_bo, output->next_fb_id);
>  	}
>  
>  	mode = container_of(output->base.current, struct drm_mode, base);
> @@ -355,22 +410,23 @@ page_flip_handler(int fd, unsigned int frame,
>  		(struct drm_compositor *) output->base.compositor;
>  	uint32_t msecs;
>  
> -	if (output->current_fb_id)
> -		drmModeRmFB(c->drm.fd, output->current_fb_id);
> -	output->current_fb_id = output->next_fb_id;
> -	output->next_fb_id = 0;
> -
> -	if (output->scanout_buffer) {
> -		weston_buffer_post_release(output->scanout_buffer);
> -		wl_list_remove(&output->scanout_buffer_destroy_listener.link);
> -		output->scanout_buffer = NULL;
> -	} else if (output->current_bo) {
> +	if (output->current_bo) {
>  		gbm_surface_release_buffer(output->surface,
>  					   output->current_bo);
> +	} else {
> +		if (output->scanout_buffer) {
> +			weston_buffer_post_release(output->scanout_buffer);
> +			wl_list_remove(&output->scanout_buffer_destroy_listener.link);
> +			output->scanout_buffer = NULL;
> +		}
> +		if (output->current_fb_id)
> +			drmModeRmFB(c->drm.fd, output->current_fb_id);
>  	}
>  
>  	output->current_bo = output->next_bo;
>  	output->next_bo = NULL;
> +	output->current_fb_id = output->next_fb_id;
> +	output->next_fb_id = 0;
>  
>  	if (output->pending_scanout_buffer) {
>  		output->scanout_buffer = output->pending_scanout_buffer;
> -- 
> 1.7.4.1
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel