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

[Daniel Stone]

Hi,

On 22 March 2017 at 14:35, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri,  3 Mar 2017 23:05:25 +0000
> Daniel Stone <daniels at collabora.com> wrote:
>> Clean up some ambiguity around current/next: current could previously
>> have referred to a buffer which was being displayed, or the last buffer
>> being displayed whilst we waited for a configuration we'd requested to
>> take effect.
>>
>> Introduce a new variable, fb_last, which exclusively holds the latter
>> case, thus leaving us with three unambiguous members: fb_pending is used
>> as a scratch space for a buffer we're about to post, fb_current holds
>> the buffer we last requested to display (whether active yet or not), and
>> fb_last is again scratch space for a buffer which is no longer being
>> displayed after a previous configuration reqeust.
>
> the minor problem I have with this patch is that "current" is no longer
> the current fb on screen if we have programmed a new flip. Your
> new definition "(whether active yet or not)" contains very much an
> ambiguity: whether it is actually on screen or not, which I think would
> be a fairly important difference.
>
> 'current' has never referred to an FB that is no longer on screen,
> because page_flip_handler() replaces it with the FB that just came on
> screen.

'current' is no longer on screen, in between the hardware completing
the pageflip, and Weston processing the kernel's flip-complete event.

> I would argue an FB is "pending" until the flip completes. That makes
> me think of the following terminology:
>
> pending: the FB in a programmed flip, but not completed the flip yet.
>
> current: the FB currently on screen

The reason I landed on pending/current/last is because I was looking
at it from the point of view of the kernel's state tracking. pending
is something we haven't submitted to the kernel yet, current is what
the kernel last accepted as state, and last is the thing which _was_
current before the last update, but hasn't yet been released by the
kernel. I'm trying to look at it from that point of view rather than
'FB turned to light' / pageflip events, because the latter is subject
to a race we can't close, and is also entirely uninteresting to our
state machinery.

Before this patch, 'next' was a mixture of a staging area whilst
building the state, and state which had been submitted to the kernel
but we had not received a pageflip event for (your 'pending' above).
'current' was a mixture of what is the last set of state to be
submitted, and what is the last-but-one state to be submitted.

As soon as the kernel accepts an update, there is no possibility for
it to later reject it. Any updates which are going to be made, will be
made incrementally on the last state submitted. This patch means that
the latter ('last state submitted') is _always_
fb_current/state_current. Before this patch, this was either
output->pending or output->current, depending on if we're in
assign_planes/repaint or not.

> And there is no need for a third state, because when 'current' stop
> being current, it only needs to be unreffed, it does not need to be
> stored anymore.
>
> But that's how it was, and you need a third state.
>
> If we look at it from the FB point of view:
>
> prepared: the FB (and state) we are building up for the next screen
> update.
>
> queued: the FB in a programmed flip, but not completed the flip
> yet.
>
> current: the FB currently on screen.
>
> 'prepared' would become 'queued' when we issue drmModePageFlip().
> When the pageflip completes, 'current' is unreffed, and 'queued'
> becomes 'current'.
>
>
> However, that's a "wrong" point of view. I believe your ultimate aim is
> *not* to track FB state, but to track collections of atomic state. An
> atomic state set is constructed (fb_pending), programmed (fb_current),
> and finished (fb_last). When an atomic state set is finished and
> destroyed, the FB is only "current" as it is now on screen. You have no
> need to keep the atomic state set that is currently on screen, do you?
>
> 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.

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 ... ?

I need to retain the programmed (state_current, with the update not
yet complete), because all future state updates are incremental upon
the current state. So I need to be able to compare state_current to my
target state in order to figure out what to do. Is that what you were
asking instead ... ?

>> @@ -833,6 +839,8 @@ drm_output_repaint(struct weston_output *output_base,
>>                       .request.sequence = 1,
>>               };
>>
>> +             /* XXX: Set output much earlier, so we don't attempt to place
>> +              *      planes on entirely the wrong output. */
>
> 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.

>> @@ -1003,9 +1014,8 @@ page_flip_handler(int fd, unsigned int frame,
>>        * we just want to page flip to the current buffer to get an accurate
>>        * timestamp */
>>       if (output->page_flip_pending) {
>> -             drm_fb_unref(output->fb_current);
>> -             output->fb_current = output->fb_pending;
>> -             output->fb_pending = NULL;
>> +             drm_fb_unref(output->fb_last);
>> +             output->fb_last = NULL;
>
> Hmm, would this change actually make output->page_flip_pending
> unnecessary? At least here? Hmm, probably not for this patch, anyway.

Probably not - see 16/62 for how page_flip_pending is, at this point
in the series, frustratingly special.

>> @@ -1535,10 +1545,16 @@ drm_output_switch_mode(struct weston_output *output_base, struct weston_mode *mo
>>       output->base.current_mode->flags =
>>               WL_OUTPUT_MODE_CURRENT | WL_OUTPUT_MODE_PREFERRED;
>>
>> -     /* reset rendering stuff. */
>> +     /* XXX: This drops our current buffer too early, before we've started
>> +      *      displaying it. Ideally this should be much more atomic and
>> +      *      integrated with a full repaint cycle, rather than doing a
>> +      *      sledgehammer modeswitch first, and only later showing new
>> +      *      content.
>> +      */
>
> True dat.

Also it's not introduced by this patch; it's pre-existing behaviour
which I have lovingly preserved at this point.

>>       drm_fb_unref(output->fb_current);
>> -     drm_fb_unref(output->fb_pending);
>> -     output->fb_current = output->fb_pending = NULL;
>> +     assert(!output->fb_last);
>> +     assert(!output->fb_pending);
>> +     output->fb_last = output->fb_current = NULL;
>
> Unnecessary to set fb_last to NULL if it already must be NULL.

Yes, true.

>> @@ -2753,6 +2769,7 @@ create_sprites(struct drm_backend *b)
>>
>>               sprite->possible_crtcs = plane->possible_crtcs;
>>               sprite->plane_id = plane->plane_id;
>> +             sprite->fb_last = NULL;
>>               sprite->fb_current = NULL;
>>               sprite->fb_pending = NULL;
>>               sprite->backend = b;
>> @@ -2784,8 +2801,9 @@ destroy_sprites(struct drm_backend *backend)
>>                               sprite->plane_id,
>>                               output->crtc_id, 0, 0,
>>                               0, 0, 0, 0, 0, 0, 0, 0);
>> +             drm_fb_unref(sprite->fb_last);
>>               drm_fb_unref(sprite->fb_current);
>> -             drm_fb_unref(sprite->fb_pending);
>> +             assert(!sprite->fb_pending);
>>               weston_plane_release(&sprite->plane);
>>               free(sprite);
>>       }
>
> Shouldn't drm_output_deinit() have some asserts or unrefs added?

Yes, probably.

> 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.

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?

Cheers,
Daniel