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

[Daniel Stone]

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.

>>  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.

Cheers,
Daniel