[PATCH weston 15/68] compositor-drm: Add explicit type member to drm_fb

[Pekka Paalanen]

On Fri,  9 Dec 2016 19:57:30 +0000
Daniel Stone <daniels at collabora.com> wrote:

> Rather than magically trying to infer what the buffer is and what we
> should do with it when we go to destroy it, add an explicit type
> instead.
> 
> Differential Revision: https://phabricator.freedesktop.org/D1488
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 50 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 4ef7343..217db32 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -129,11 +129,19 @@ struct drm_mode {
>  	drmModeModeInfo mode_info;
>  };
>  
> +enum drm_fb_type {
> +	BUFFER_INVALID = 0, /**< never used */
> +	BUFFER_CLIENT, /**< directly sourced from client */
> +	BUFFER_PIXMAN_DUMB, /**< internal Pixman rendering */
> +	BUFFER_GBM_SURFACE, /**< internal EGL rendering */
> +};

Hi,

cool.

> +
>  struct drm_fb {
> +	enum drm_fb_type type;
> +
>  	uint32_t fb_id, stride, handle, size;
>  	int width, height;
>  	int fd;
> -	int is_client_buffer;
>  	struct weston_buffer_reference buffer_ref;
>  
>  	/* Used by gbm fbs */
> @@ -290,6 +298,7 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int height,
>  	if (ret)
>  		goto err_fb;
>  
> +	fb->type = BUFFER_PIXMAN_DUMB;
>  	fb->handle = create_arg.handle;
>  	fb->stride = create_arg.pitch;
>  	fb->size = create_arg.size;
> @@ -352,6 +361,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
>  {
>  	struct drm_mode_destroy_dumb destroy_arg;
>  
> +	assert(fb->type == BUFFER_PIXMAN_DUMB);
> +
>  	if (!fb->map)
>  		return;
>  
> @@ -370,8 +381,8 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
>  }
>  
>  static struct drm_fb *
> -drm_fb_get_from_bo(struct gbm_bo *bo,
> -		   struct drm_backend *backend, uint32_t format)
> +drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
> +		   uint32_t format, enum drm_fb_type type)
>  {
>  	struct drm_fb *fb = gbm_bo_get_user_data(bo);
>  	uint32_t handles[4] = { 0 }, pitches[4] = { 0 }, offsets[4] = { 0 };

For the shortcut return:
assert(type == fb->type)?

> @@ -384,6 +395,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo,
>  	if (fb == NULL)
>  		return NULL;
>  
> +	fb->type = type;
>  	fb->bo = bo;
>  
>  	fb->width = gbm_bo_get_width(bo);
> @@ -440,9 +452,6 @@ static void
>  drm_fb_set_buffer(struct drm_fb *fb, struct weston_buffer *buffer)
>  {
>  	assert(fb->buffer_ref.buffer == NULL);
> -
> -	fb->is_client_buffer = 1;
> -

assert(fb->type == BUFFER_CLIENT)?

>  	weston_buffer_reference(&fb->buffer_ref, buffer);
>  }
>  
> @@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, struct drm_fb *fb)
>  	if (!fb)
>  		return;
>  
> -	if (fb->map &&
> -            (fb != output->dumb[0] && fb != output->dumb[1])) {
> -		drm_fb_destroy_dumb(fb);

This piece sent me into a recursive "well, actually..." loop.

It looked like dead code at first hand, as this gets hit when
output->dumb and fb don't match. When would that be? I would guess when
video mode changed.

I think changing resolutions would hit this path, when flipping to a
new dumb buffer of a different size than one coming out of scanout
which cannot be destroyed until pageflip completed.

Except I am wrong in a couple of ways: destroying the buffer is fine,
the kernel will keep it referenced as long as necessary anyway. And,
drm_output_switch_mode() does destroy everything immediately.

So this bit is ok. Unless there is a third well-actually.

> -	} else if (fb->bo) {
> -		if (fb->is_client_buffer)
> -			gbm_bo_destroy(fb->bo);
> -		else
> -			gbm_surface_release_buffer(output->gbm_surface,
> -						   fb->bo);
> +	switch (fb->type) {
> +	case BUFFER_PIXMAN_DUMB:
> +		/* nothing: pixman buffers are destroyed manually */
> +		break;
> +	case BUFFER_CLIENT:
> +		gbm_bo_destroy(fb->bo);
> +		break;
> +	case BUFFER_GBM_SURFACE:
> +		gbm_surface_release_buffer(output->gbm_surface, fb->bo);
> +		break;
> +	default:
> +		assert(NULL);
> +		break;
>  	}
>  }
>  
> @@ -559,7 +572,7 @@ drm_output_prepare_scanout_view(struct drm_output *output,
>  		return NULL;
>  	}
>  
> -	output->next = drm_fb_get_from_bo(bo, b, format);
> +	output->next = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
>  	if (!output->next) {
>  		gbm_bo_destroy(bo);
>  		return NULL;
> @@ -585,7 +598,8 @@ drm_output_render_gl(struct drm_output *output, pixman_region32_t *damage)
>  		return;
>  	}
>  
> -	output->next = drm_fb_get_from_bo(bo, b, output->gbm_format);
> +	output->next = drm_fb_get_from_bo(bo, b, output->gbm_format,
> +					  BUFFER_GBM_SURFACE);
>  	if (!output->next) {
>  		weston_log("failed to get drm_fb for bo\n");
>  		gbm_surface_release_buffer(output->gbm_surface, bo);
> @@ -1054,7 +1068,7 @@ drm_output_prepare_overlay_view(struct drm_output *output,
>  		return NULL;
>  	}
>  
> -	s->next = drm_fb_get_from_bo(bo, b, format);
> +	s->next = drm_fb_get_from_bo(bo, b, format, BUFFER_CLIENT);
>  	if (!s->next) {
>  		gbm_bo_destroy(bo);
>  		return NULL;

Yeah, it looks fine, I only proposed some added asserts which are
non-essential, so:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170221/59ffa926/attachment.sig>