[PATCH v14 35/41] compositor-drm: Add modes to drm_output_propose_state

[Daniel Stone]

Hi,
On Mon, 29 Jan 2018 at 10:55, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Mon, 29 Jan 2018 09:17:49 +0000 Daniel Stone <daniel at fooishbar.org> wrote:
> > On 26 January 2018 at 14:04, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > > On Wed, 20 Dec 2017 12:26:52 +0000
> > > Daniel Stone <daniels at collabora.com> wrote:
> > >> +enum drm_output_propose_state_mode {
> > >> +     DRM_OUTPUT_PROPOSE_STATE_MIXED, /**< mix renderer & planes */
> > >> +     DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY, /**< only assign to planes */
> > >> +};
> > >
> > > what is the reason to have a planes-only mode? I saw the patch
> > > "compositor-drm: Add test-only mode to state application" will first
> > > attempt a planes-only mode and then fall back to mixed mode. Why does
> > > the planes-only mode not fall out of mixed mode naturally when it runs
> > > out of views to show? This would be good to explain in the commit
> > > message to justify the modes.
> >
> > This could again go into a comment or documentation I suppose, but I'm
> > not sure where I'd put it without adding so much clutter to the code
> > as to hide it. Ideas?
>
> Commit message would be fine by me, you can be as verbose as you want
> without a worry.

This got missed.

> > We have two options: either punt when we don't have valid scanout
> > state, or do what we do here. Which is, speculatively build a state
> > composed purely of planes with no intermediate checks, and test once
> > at the end. If it works, then we use it. If not, we build a
> > renderer-only (i.e. renderer + cursor) state and just use that. In the
> > next repaint, we will use the normal 'mixed' mode.
> >
> > (All of the above described is the intention. If these patches differ
> > from that description, then the patches are wrong.)
>
> Ok, this is something that never occurred to me when reading this
> patch. This is going to a great length to avoid allocating a
> framebuffer for the renderer unless absolutely necessary.

Not just avoiding allocating, but avoiding putting up one frame where
everything has gone through the GPU, followed by another where
everything's on the overlay, for no real reason. That could cause
flickers when playing media, due to different colourspace conversions
etc. But yes, we do bend over backwards to use planes where we can.

> A quick check on "compositor-drm: Add test-only mode to state
> application" revealed code:
>
> drm_assign_planes(struct weston_output *output_base, void *repaint_data)
> {
> ...
>         state = drm_output_propose_state(output_base, pending_state,
>                                          DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
>         if (!state)
>                 state = drm_output_propose_state(output_base, pending_state,
>                                                  DRM_OUTPUT_PROPOSE_STATE_MIXED);
>
> from which I made wrong conclusions on what the two modes do. An
> important bit that I missed was the hunk added to
> drm_output_propose_state() that fundamentally changes how 'planes_ok'
> gets set.
>
> Another reason I was confused was that STATE_PLANES_ONLY is attempted
> on every frame. Should that not be skipped if we have an existing
> primary fb to test with? Then the renderer-not-needed case might be
> slightly more obvious to fall out from STATE_MIXED.

It could still be useful. For instance, if the hardware has one
detiling unit shared between all planes, with the GPU rendering to
tiled buffers and the media decoder linear. The client has given us
one GPU buffer placed on top of one fullscreen media buffer. In this
case, if we test compositor rendering buffer (tiled) + client GPU
buffer (tiled), we will fail the test and not promote the client GPU
buffer to an overlay. There's no guarantee that whatever you want to
put on fullscreen is compatible with the renderer output.

> Maybe it would be more clear if the hunk that sets up the real
> planes_ok logic in "compositor-drm: Add test-only mode to state
> application" would be moved into this patch?

Sure, I think that could work.

> Or thinking these patches from a higher level...
>
> Essentially there are three modes: renderer-only, mixed, and
> planes-only. In this patch series, the renderer-only mode is a hidden
> mode under the mixed mode, and it's not obvious why the planes-only
> mode exists. Mixed mode is a bit special, because it can natually fall
> into renderer-only or planes-only setups.
>
> What would you think of exposing all three modes explicitly, and having
> the logic to choose one mode and the fallback to renderer-only mode in
> drm_assing_planes()?
>
> You could have a helper like
>         bool drm_output_has_testable_scanout_fb()
> to make it more self-documenting.
>
> Something along the lines of
>
> if (drm_output_has_testable_scanout_fb()) {
>         state = drm_output_propose_state(MIXED);
> } else {
>         state = drm_output_propose_state(PLANES_ONLY);
>         if (!state)
>                 state = drm_output_propose_state(RENDERER_ONLY);
> }

Hm, I think what we really want is:

/* First try to bypass the renderer completely, and construct a scene
entirely from client views on planes. */
state = drm_output_propose_state(PLANES_ONLY);
if (!state)
    if (drm_output_has_testable_scanout_fb())
        state = drm_output_propose_state(MIXED); /* still use planes
where we can, but assuming that we'll have at least some things
through the renderer, so no fullscreen scanout */
    else
        state = drm_output_propose_state(RENDERER_ONLY); /* a buffer
from the renderer will go to the primary/scanout plane, but we have no
previous buffers to test against, so we can't use planes; force
everything but cursor through the renderer for now */

Does that make sense? If so, I'm happy to write it up now.

Cheers,
Daniel