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

Tue Jul 10 09:59:33 UTC 2018

On Tue, 10 Jul 2018 08:53:54 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

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

Aha, good to realize.

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

Yes, I think that makes sense.

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