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

[Pekka Paalanen]

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:
> 
> > Rather than magically trying to infer what the buffer is and what we
> > should do with it when we go to destroy it, add an explicit type
> > instead.
> > 
> > Differential Revision: https://phabricator.freedesktop.org/D1488
> > 
> > Signed-off-by: Daniel Stone <daniels at collabora.com>
> > ---
> >  libweston/compositor-drm.c | 50 +++++++++++++++++++++++++++++-----------------
> >  1 file changed, 32 insertions(+), 18 deletions(-)

> >  	weston_buffer_reference(&fb->buffer_ref, buffer);
> >  }
> >  
> > @@ -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.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170222/9d3cafbd/attachment.sig>