On Tue, 10 Jul 2018 08:53:54 +0100 Daniel Stone <[email protected]> wrote:
> Hi, > On Mon, 29 Jan 2018 at 10:55, Pekka Paalanen <[email protected]> wrote: > > On Mon, 29 Jan 2018 09:17:49 +0000 Daniel Stone <[email protected]> > > wrote: > > > 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. > > 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
pgp8B0MYSm_Uk.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
