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

[Pekka Paalanen]

On Tue, 16 Jan 2018 15:42:25 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 16 January 2018 at 14:30, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Wed, 20 Dec 2017 12:26:23 +0000
> > Daniel Stone <daniels at collabora.com> wrote:  
> >> @@ -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().  
> 
> Inside gbm_surface_destroy(). This destroys every BO created for the
> surface.

It does not seem like that to me.

main/gbm.c:

GBM_EXPORT void
gbm_surface_destroy(struct gbm_surface *surf)
{
   surf->gbm->surface_destroy(surf);
}


backends/dri/gbm_dri.c:

static void
gbm_dri_surface_destroy(struct gbm_surface *_surf)
{
   struct gbm_dri_surface *surf = gbm_dri_surface(_surf);

   free(surf->base.modifiers);
   free(surf);
}

But when I looked into Mesa EGL, dri2_drm_destroy_surface() has a loop
to call gbm_bo_destroy() on all color buffers.

Anyway, doesn't really matter which one does it, as both are called.

> 
> >>  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.  
> 
> Yes, the wording is too imprecise, and I'm thinking of the
> pixman_image_t, which doesn't have a ref on the drm_fb. I'll try to
> think of some better wording.

Cool, but don't let it stop you from landing this. ;-)


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/20180117/94c4594b/attachment.sig>