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

[Daniel Stone]

Hi Pekka,

On 21 February 2017 at 15:19, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri,  9 Dec 2016 19:57:34 +0000 Daniel Stone <daniels at collabora.com> wrote:
>> @@ -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;
>
> this hunk belongs in an earlier patch.

As you already pointed out whilst reviewing that patch. :)

>> @@ -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?

It's indeed almost certainly a bugfix: if you have one client buffer
with multiple views, both of which could be promoted to scanout,
you'll get the same user data (drm_fb) handed back for the same BO,
and could attempt to destroy it twice. Previously you'd probably
mostly luck out due to the memory not yet being reused, but now you'd
be near-guaranteed to hit the assert in drm_fb_unref, which seemed
harsh. So I squashed it in here.

drm_fb_ref doesn't exist before this patch, so the split would have to
be this in a later patch. Would that work for you? I don't mind either
way.

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

Slowly but surely ...

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

I'll leave you to review the next revision and/or just continue this thread on.

Cheers,
Daniel