[PATCH weston v9 06/62] compositor-drm: Refactor destroy drm_fb function

[Emil Velikov]

Hi Dan,

On 10 March 2017 at 15:12, Daniel Stone <daniel at fooishbar.org> wrote:
> Hey Emil,
>
> On 9 March 2017 at 23:51, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 3 March 2017 at 23:05, Daniel Stone <daniels at collabora.com> wrote:
>>> +static void
>>> +drm_fb_destroy_dumb(struct drm_fb *fb)
>>> +{
>>> +       struct drm_mode_destroy_dumb destroy_arg;
>>> +
>>> +       assert(fb->type == BUFFER_PIXMAN_DUMB);
>>> +
>>> +       if (fb->map && fb->size > 0)
>>> +               munmap(fb->map, fb->size);
>>> +
>>> +       memset(&destroy_arg, 0, sizeof(destroy_arg));
>>> +       destroy_arg.handle = fb->handle;
>>> +       drmIoctl(fb->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_arg);
>>> +
>>> +       drm_fb_destroy(fb);
>>
>> Things looks wrong here. Afaict we should unmap, rmfb ('detach' for
>> dumb) and then dumb_destroy.
>> See the drm_fb_create_dumb() error path - perhaps we can drop that in
>> favour of the updated drm_fb_destroy_dumb() ?
>
> Honestly, I don't think it makes any practical difference. The dumb
> buffer itself has a reference held by the fb, so won't actually get
> destroyed until both the buffer has been destroyed and the FB has been
> as well. When we unmap also makes no difference, since we're not
> touching the memory anymore.
Considering the "fun" experiences Maarten had in the area (admittedly
due to different assumptions by userspace) might be better to be
pedantic... It shouldn't hurt weston too much.
Although yes, my suggestion is quite paranoid ;-)

-Emil