[PATCH weston 19/68] compositor-drm: Refcount drm_fb

[Pekka Paalanen]

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

> Sometimes we need to duplicate an existing drm_fb, e.g. when
> pageflipping to the same buffer to kickstart the repaint loop. To handle
> situations like these, and simplify resource management for dumb and
> cursor buffers, refcount drm_fb.
> 
> Differential Revision: https://phabricator.freedesktop.org/D1491
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index eb735b2..a684ac9 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -140,6 +140,8 @@ enum drm_fb_type {
>  struct drm_fb {
>  	enum drm_fb_type type;
>  
> +	int refcnt;
> +
>  	uint32_t fb_id, stride, handle, size;
>  	const struct pixel_format_info *format;
>  	int width, height;
> @@ -302,6 +304,8 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int height,
>  	if (!fb)
>  		return NULL;
>  
> +	fb->refcnt = 1;
> +
>  	fb->format = pixel_format_get_info(format);
>  	if (!fb->format) {
>  		weston_log("failed to look up format 0x%lx\n",
> @@ -312,6 +316,7 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int height,
>  	if (!fb->format->depth || !fb->format->bpp) {
>  		weston_log("format 0x%lx is not compatible with dumb buffers\n",
>  			   (unsigned long) format);
> +		goto err_fb;

Hi,

this hunk belongs in an earlier patch.

>  	}
>  
>  	memset(&create_arg, 0, sizeof create_arg);
> @@ -384,6 +389,13 @@ err_fb:
>  }
>  
>  static struct drm_fb *
> +drm_fb_ref(struct drm_fb *fb)
> +{
> +	fb->refcnt++;
> +	return fb;
> +}
> +
> +static struct drm_fb *
>  drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
>  		   uint32_t format, enum drm_fb_type type)
>  {
> @@ -392,13 +404,14 @@ drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
>  	int ret;
>  
>  	if (fb)
> -		return fb;
> +		return drm_fb_ref(fb);

This path is now different from before, and requires adding an
equivalent drm_fb_unref() call somewhere, but I don't see it.
Previously unref would have released it immediately, now the first call
will be a no-op.

Or, if this is a bug fix, it requires an explanation in the commit
message how in some cases drm_fb_unref() got called twice.

Maybe this hunk belongs in a different patch instead?

>  
>  	fb = zalloc(sizeof *fb);
>  	if (fb == NULL)
>  		return NULL;
>  
>  	fb->type = type;
> +	fb->refcnt = 1;
>  	fb->bo = bo;
>  
>  	fb->width = gbm_bo_get_width(bo);
> @@ -472,6 +485,10 @@ drm_fb_unref(struct drm_fb *fb)
>  	if (!fb)
>  		return;
>  
> +	assert(fb->refcnt > 0);
> +	if (--fb->refcnt > 0)
> +		return;
> +
>  	switch (fb->type) {
>  	case BUFFER_PIXMAN_DUMB:
>  		/* nothing: pixman buffers are destroyed manually */

It took a while to see the paths:

drm_fb_unref -> gbm_bo_destroy -> drm_fb_destroy_gbm

drm_fb_unref -> gbm_surface_release_buffer
		gbm_surface_destroy -> drm_fb_destroy_gbm

drm_output_fini_pixman -> drm_fb_destroy_dumb

The goal is to solidify drm_fb_unref() as the main entry point for
destruction, but it's not quite there yet. Very good.

If you move both of the hunks I pointed out into other patches, then
this patch gets:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
If that's not the right thing to do, I'll review the next revision.


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/ba1d2af4/attachment.sig>