On Mon, 27 Feb 2017 22:58:48 +0000 Daniel Stone <dan...@fooishbar.org> wrote:
> Hi Pekka,
>
> On 21 February 2017 at 15:19, Pekka Paalanen <ppaala...@gmail.com> wrote:
> > On Fri, 9 Dec 2016 19:57:34 +0000 Daniel Stone <dani...@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.
I think I'd be fine if those points were explained in the commit
message. To me, it's important to justify every hunk in a patch if it
somehow stands out as not being part of the main topic. And this one
caugh my eye as an unexplained change.
The important thing to document about this change is that is exchanges
a (almost never hit) double-unref into a (always hit) leak of the
drm_fb. It would be even better to mention what patch shall fix the
introduced leak.
To clarify, the leak I am imagining is: drm_fb gets created with
refcount=1, then every drm_fb_get_from_bo() will increment the
refcount, but according to the old not-refcounted architecture, there
is only ever a single call to unref, which means it will never be
released. And that would apply to *all* drm_fbs ever used. Am I wrong?
Seems like a fairly big temporary regression compared to the
double-free which can never happen in Weston since we don't use
multiple views (yet).
Thanks,
pq
> >> @@ -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.paala...@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
pgpoohd9iZruU.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
