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

[Pekka Paalanen]

On Mon, 29 Jan 2018 09:17:49 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Pekka,
> Thanks again for all the review! I'm really grateful you got this far,
> and surprised: I'd expected the assign_planes() rewrite to be the much
> more hairy part of the series, and state to be relatively
> straightforward. \_o_/

Hi Daniel,

you're welcome!

Heh, maybe getting state straight has rippled through assing_planes()
and simplified that as well. :-)

> Most everything I've seen going past I agree with at least on the face
> of it. I'm going through fixing it up in various bits of downtime, and
> will reply to them all when I've had a chance to go through and do a
> last pass before sending out. So I don't forget things like, e.g.:
> https://gitlab.collabora.com/pq/weston/commit/81f74def4ed1285ad3794f80d93178039be3cae1

Please, do send out v15 only up to "Atomic modesetting support" first.

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

> Our assign_planes() loop is top (closest to eye / least occluded) to
> bottom (furthest from eye / most occluded) through the stack of views.
> If we encounter a view which is not in any way occluded and there is a
> free format-compatible (etc) overlay plane, we run a TEST_ONLY commit,
> with whatever the current state is + view on overlay plane, and keep
> the assignment if that succeeds. (Fullscreen views skip the occlusion
> test and we attempt to assign them to the scanout plane.)
> 
> In the steady state, this isn't really a problem: we have the mode
> from the CRTC state (which doesn't change), the scanout plane from the
> _previous_ state (which we temporarily reuse), then we apply overlay
> planes on top of this to test.
> 
> On the other hand, if we're newly enabling an output, switching modes,
> etc, the top-to-bottom ordering implies that without the previous
> state, we'll be testing mode/connectors (from CRTC state) + overlay
> planes + not-well-known scanout plane state (either disabled by
> state_invalid, or whatever it was previously; probably disabled). So
> testing isn't meaningful at all.
> 
> 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.

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.

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?

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);
}


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/20180129/9ca6699a/attachment-0001.sig>