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 <[email protected]> wrote: > On Wed, 20 Dec 2017 12:26:52 +0000 > Daniel Stone <[email protected]> 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
