[PATCH v14 06/41] compositor-drm: Use drm_plane for scanout plane

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:23 +0000
Daniel Stone <daniels at collabora.com> wrote:

> Use a real drm_plane to back the scanout plane, displacing
> output->fb_{last,cur,pending} to their plane-tracked equivalents.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor-drm.c | 222 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 149 insertions(+), 73 deletions(-)
> 
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index a600ef40b..6ad616ebd 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c

> @@ -3355,6 +3398,19 @@ drm_output_init_egl(struct drm_output *output, struct drm_backend *b)
>  static void
>  drm_output_fini_egl(struct drm_output *output)
>  {
> +	struct drm_backend *b = to_drm_backend(output->base.compositor);
> +
> +	/* Destroying the GBM surface will destroy all our GBM buffers,
> +	 * regardless of refcount. Ensure we destroy them here. */

I suppose that happens inside eglDestroySurface().

> +	if (!b->shutting_down &&
> +	    output->scanout_plane->state_cur->fb &&
> +	    output->scanout_plane->state_cur->fb->type == BUFFER_GBM_SURFACE) {
> +		drm_plane_state_free(output->scanout_plane->state_cur, true);
> +		output->scanout_plane->state_cur =
> +			drm_plane_state_alloc(NULL, output->scanout_plane);
> +		output->scanout_plane->state_cur->complete = true;
> +	}
> +
>  	gl_renderer->output_destroy(&output->base);
>  	gbm_surface_destroy(output->gbm_surface);
>  	drm_output_fini_cursor_egl(output);
> @@ -3420,8 +3476,20 @@ err:
>  static void
>  drm_output_fini_pixman(struct drm_output *output)
>  {
> +	struct drm_backend *b = to_drm_backend(output->base.compositor);
>  	unsigned int i;
>  
> +	/* Destroying the Pixman surface will destroy all our buffers,
> +	 * regardless of refcount. Ensure we destroy them here. */

I think this is only half-true. The pixman_image_t does get destroyed,
but drm_fb is still refcounted and recount honoured it seems. Only
drm_output_render_pixman() uses the image, and I assume that won't
called anymore. drm_fb is the one that maintains the backing storage in
this case.

Therefore this workaround here is not necessary for avoiding
use-after-free, but maybe we want this to ensure the drm_fb really does
get destroyed anyway?

At most, the comment here could be adjusted.

> +	if (!b->shutting_down &&
> +	    output->scanout_plane->state_cur->fb &&
> +	    output->scanout_plane->state_cur->fb->type == BUFFER_PIXMAN_DUMB) {
> +		drm_plane_state_free(output->scanout_plane->state_cur, true);
> +		output->scanout_plane->state_cur =
> +			drm_plane_state_alloc(NULL, output->scanout_plane);
> +		output->scanout_plane->state_cur->complete = true;
> +	}
> +
>  	pixman_renderer_output_destroy(&output->base);
>  	pixman_region32_fini(&output->previous_damage);
>  

Anyway,

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: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180116/1804e20b/attachment.sig>