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

[Daniel Stone]

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_/

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

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?

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

>> @@ -3049,13 +3054,17 @@ err:
>>
>>  static struct drm_output_state *
>>  drm_output_propose_state(struct weston_output *output_base,
>> -                      struct drm_pending_state *pending_state)
>> +                      struct drm_pending_state *pending_state,
>> +                      enum drm_output_propose_state_mode mode)
>>  {
>>       struct drm_output *output = to_drm_output(output_base);
>> +     struct drm_backend *b = to_drm_backend(output_base->compositor);
>>       struct drm_output_state *state;
>>       struct weston_view *ev;
>>       pixman_region32_t surface_overlap, renderer_region, occluded_region;
>>       struct weston_plane *primary = &output_base->compositor->primary_plane;
>> +     bool renderer_ok = (mode != DRM_OUTPUT_PROPOSE_STATE_PLANES_ONLY);
>> +     bool planes_ok = !b->sprites_are_broken;
>
> Would you want to remove the sprites_are_broken check from
> drm_output_prepare_overlay_view()?

Yeah, or just make it an assert.

>> @@ -3118,36 +3127,49 @@ drm_output_propose_state(struct weston_output *output_base,
>>                       next_plane = primary;
>>               pixman_region32_fini(&surface_overlap);
>>
>> -             if (next_plane == NULL)
>> +             /* The cursor plane is 'special' in the sense that we can still
>> +              * place it in the legacy API, and we gate that with a separate
>> +              * cursors_are_broken flag. */
>> +             if (next_plane == NULL && !b->cursors_are_broken)
>
> Would you want to remove the cursors_are_broken check from
> drm_output_prepare_cursor_view()?

Ditto.

Cheers,
Daniel