On Mon, 29 Jan 2018 09:17:49 +0000 Daniel Stone <[email protected]> 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 <[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? 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
pgphXAVLicY5c.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
