[PATCH weston 15/68] compositor-drm: Add explicit type member to drm_fb

[Daniel Stone]

Hi Pekka,
Thanks for the thoughtful review, as ever.

On 22 February 2017 at 13:45, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 21 Feb 2017 15:29:09 +0200 Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> On Fri,  9 Dec 2016 19:57:30 +0000 Daniel Stone <daniels at 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