[PATCH weston v9 14/62] compositor-drm: Introduce fb_last member

[Daniel Stone]

Hi Pekka,

On 24 March 2017 at 10:30, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Thu, 23 Mar 2017 15:56:55 +0000
> Daniel Stone <daniel at fooishbar.org> wrote:
>> On 22 March 2017 at 14:35, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > I think this patch could use a better explanation, particularly if all
>> > my speculations were in fact incorrect. ;-)
>>
>> Yeah, your speculation (and how I've outlined my thinking / view of
>> things above) is accurate, up until the question, which I don't
>> entirely understand. I'll phrase this entirely in terms of atomic
>> state rather than fb, because this is purely an incremental step along
>> the way.
>
> The question came from the fact that page_flip_handler() sets fb_last
> to NULL rather than keeping it around for further use. To me that looks
> like you have no need to keep atomic state that is currently on screen,
> if not kept in some other place.

Er ... ?

Let's say buffer A is current on screen, with an idle repaint loop.
last == NULL, current == A. (I'm ignoring pending, since it is purely
a staging area used between repaint_begin and repaint_flush.)

A repaint operation generates buffer B to be displayed, and calls
drmModePageFlip. last == A, current == B. A is still displayed on
screen at the point drmModePageFlip is called.

The page flip operation completes: B has now turned to light. last (A)
is unref'ed: it is not part of the current state, nor is it being
displayed on screen (since page_flip_handler signals that the last
completed state request, has completed _in hardware_ as well). We have
no need for it. last == NULL, current == B.

Does page_flip_handler need more commentary on how it signals the
completion of drmModePageFlip? :\

>> I need to retain the superseded (state_last) state, because that needs
>> to get freed when an update completes. Mind you, this can be
>> 'floating', with some work: we could retain the drm_pending_state
>> ownership of the drm_output_state until we get a completion event
>> (page_flip_handler/atomic_complete_handler), and not retain it in
>> drm_output. This would mean that
>> {vblank,page_flip,atomic_complete,destroy,disable,dpms_off}_complete
>> could be replaced by ... two enums? One for where we are in the
>> repaint cycle (between begin/flush, post-flush but pre-complete,
>> idle), and one for our target state when we can next change it
>> (active, dpms off, disable, destroy). This would even make some of the
>> ownership tracking slightly easier, I think, and drm_output would only
>> have state rather than state_last. Was that what you were asking ... ?
>
> Umm... maybe? :-)
>
> I might be dense here, but why not just destroy the state instead of
> putting it into state_last?

We don't want to send a wl_buffer.release until the hardware has
stopped using the buffer. That only occurs when the completion event
(page_flip_handler) for the request which made that state _not_
current, has been delivered.

> I notice that state_last does not get destroyed without
> page_flip_pending, which means you keep it around only because of
> start_repaint_loop.

It doesn't have anything to do with start_repaint_loop ...

state_last only gets populated (set non-NULL) when we submit new state
which will be asynchronously applied. This occurs when we use
drmModePageFlip in the fallback path for start_repaint_loop, later in
the series (cf. the page_flip_pending flag, which is currently set
when we're flipping to a new buffer and should avoid releasing the
old; I can expand further on this if it's helpful to do so, but the
later commit does have a fairly complete commit message I think). This
also occurs when we use drmModePageFlip in the regular
drm_output_repaint path.

page_flip_handler is called for both these cases. Its job is to note
that the buffer last requested has now turned to light
(drm_output_finish_frame -> presentation time feedback). Its job is
also to release the old buffer which is no longer used anywhere
(drm_fb_unref).

> Ahh, but the actual use of fb_last comes in patch 16 which I had not
> read yet! I was missing the greater reason on why do this in the first
> place.

'Actual use'? It's very much used here, per the example above.

>> > That's an interesting comment, not quite sure what it does in this
>> > patch.
>>
>> It's a non-sequitur. Since I fix it later in the series, I could just
>> drop it from this patch.
>
> So... does it mean that we may at this point (or already in upstream?)
> attempt to place planes on a wrong output?

Upstream already does viciously incorrect things with planes on
multihead. But luckily, thanks to the issue which 'Ignore views on
other outputs' solves, planes on multi-head are completely broken
anyway.

>> > Wait, even before this patch, what unrefs all the FBs in
>> > current/pending/next when an output is disabled or destroyed?
>> >
>> > Did we miss something with "compositor-drm: Refcount drm_fb" after all?
>> > Reading "compositor-drm: Use refcounted FBs for Pixman" it seems we do
>> > unref 'dumb', we ref for 'next', but do not unref for 'current' on
>> > destroy but only when being replaced by 'next' in page_flip_handler().
>>
>> Yes, I think you're right. Mind you, since gbm_bos are not themselves
>> refcounted, destroying the gbm_surface in drm_output_fini_egl will
>> inherently destroy the gbm_bo, which will call our destroy handler to
>> remove the FB, etc etc. But we're still leaking a ref.
>
> What about pixman buffers? Are we not leaking those ever since
> "compositor-drm: Refcount drm_fb" at least one for each output
> hot-unplug?
>
> Actually, maybe not that patch, but the following that makes
> refcounting work for dumb buffers. Very subtle.

I think what I need to do to make this work is to squash the 'refcount
drm_fb' and 'refcount dumb buffer' patches together, as well as
introduce a drm_fb_unref(fb->current) in drm_output_deinit from there.
That seems to make all my fbs drop to 0 ref on unplug.

>> This is solved later with state by unref'ing state_cur, but I think
>> you're right that when we add the refcounting, we need to balance it
>> with an unref on destroy. Would doing that solve your concerns?
>
> Yes, please!
>
> Then I'd like to go over that patch once more to verify the feeling of
> not adding up is gone. (Referring to
> https://lists.freedesktop.org/archives/wayland-devel/2017-February/033182.html
> )

I'm pretty sure that it's just one unref missing in deinit, and the
rest is fine. If you can think of any others off the top of your head,
please let me know before I rebase everything on top of this patch
tomorrow?

Cheers,
Daniel