Hi Pekka, Thanks for the thoughtful review, as ever. On 22 February 2017 at 13:45, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Tue, 21 Feb 2017 15:29:09 +0200 Pekka Paalanen <ppaala...@gmail.com> wrote: >> On Fri, 9 Dec 2016 19:57:30 +0000 Daniel Stone <dani...@collabora.com> >> wrote: >> > @@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, >> > struct drm_fb *fb) >> > if (!fb) >> > return; >> > >> > - if (fb->map && >> > - (fb != output->dumb[0] && fb != output->dumb[1])) { >> > - drm_fb_destroy_dumb(fb); >> >> This piece sent me into a recursive "well, actually..." loop. >> >> It looked like dead code at first hand, as this gets hit when >> output->dumb and fb don't match. When would that be? I would guess when >> video mode changed. >> >> I think changing resolutions would hit this path, when flipping to a >> new dumb buffer of a different size than one coming out of scanout >> which cannot be destroyed until pageflip completed. >> >> Except I am wrong in a couple of ways: destroying the buffer is fine, >> the kernel will keep it referenced as long as necessary anyway. And, >> drm_output_switch_mode() does destroy everything immediately. >> >> So this bit is ok. Unless there is a third well-actually. > > And there is it: > > < MrCooper> pq: IME KMS framebuffers aren't reference-counted in the > kernel; destroying an fb which is still being scanned out by any CRTCs > turns off those CRTCs > > Is it then so, that drm_output_switch_mode() is wrong to destroy the FB > immediately before even flipping a new one, but as it does so, there is > no leak introduced by removing the above drm_fb_destroy_dumb() call? > > I.e. the patch does not regress anything because of a bug elsewhere? > > If that is so, my R-b stands, as fixing switch_mode is a whole another > matter. Also the refcounting introduced later in this series offer a > much better basis for fixing switch_mode.
Correct on all counts, especially on switch_mode. ;) I wrote up my findings but decided not to conflate that and atomic: https://phabricator.freedesktop.org/T7621 - if anyone else wants to pursue that, I'm more than happy to help out and talk you through it. I think it'd be a really interesting task to attack, and it'd definitely be really helpful. Michel is indeed right that calling drmModeRmFB will disable any CRTC currently scanning out of that buffer. I don't think your analysis is quite correct though: drm_output_release_fb() gets called from switch_mode, but _before_ the drm_output_fini_pixman() -> drm_output_init_pixman() cycle, which is what will set new output->dumb pointers. So I believe this call was a no-op. Even if it wasn't a no-op, drm_output_fini_pixman() will destroy those buffers anyway; if the release_fb() call _wasn't_ a no-op, then drm_output_fini_pixman() would crash because you'd destroy the buffers twice. Great. tl;dr: I think it's fine, you think it's fine, so I'll take the asserts and your R-b, thankyou ;) Cheers, Daniel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel