Hi, On 24 June 2015 at 13:11, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Wed, 24 Jun 2015 12:22:15 +0100 > Daniel Stone <dan...@fooishbar.org> wrote: >> On 23 June 2015 at 13:28, Pekka Paalanen <ppaala...@gmail.com> wrote: >> > On Tue, 23 Jun 2015 11:48:56 +0100 >> > Daniel Stone <dan...@fooishbar.org> wrote: >> >> > So, the DRM planes we have not assigned yet but were enabled, shouldn't >> >> > they be disabled in the TEST_ONLY commit? Otherwise they will contain >> >> > old state and we validate against some new DRM plane states with some >> >> > previous DRM plane states? That might lead to unnecessary failures from >> >> > TEST_ONLY commit if we would eventually end up disabling or updating >> >> > the already enabled DRM planes. >> >> >> >> The relevant comment above leads to that, yeah. We really need to >> >> split the walk up into two passes: firstly find anything currently on >> >> a plane which shouldn't be and free those planes up, then walk through >> >> and assign everything. I suspect this will be particularly relevant >> >> for, e.g., using the primary plane to directly scan out a client >> >> buffer. >> > >> > But can you do that in two passes? How can you know if a view is no >> > longer good for a plane if you don't already assing planes to begin >> > with and try with TEST_ONLY commit? >> >> Easily: look at the battery of tests we have for prepare_overlay_view >> etc. Is it a SHM buffer? Is it partially occluded? Is the format >> compatible? And so on, and so forth. When we associate views with >> planes, we can work out in a first pass that the view currently being >> carried on a plane is no longer any good for that plane, and throw it >> away early. > > But that depends on also on whether the TEST_ONLY commit succeeds or > not. The battery of tests is not sufficient alone, DRM may still reject > something. That causes a new plane to be freed when you are doing the > second(?) pass with test commits, and then that plane needs explicit > disabling again to not affect the following tests. > > So yes, can find views that definitely are not good for planes anymore, > but there could be even more of them.
Yes - it would be necessary but not sufficient. Think of it as an easy first cull. >> I guess this really comes down to differing views on how plane >> assignment should work. At the moment, on every output repaint, we >> essentially throw away every single plane on the entire device (not >> just those currently on our output - naughty ...) and build a >> configuration back from scratch. In this case, the incremental/caching >> approach we have doesn't really buy us much, because every time we >> walk assign_planes(), the only thing we're caching is the modeset >> parameters, which aren't touched anyway. >> >> I started from trying to address the multiple-output problem (if a >> plane is assigned to output B, then don't touch it in output A's >> repaint loop, otherwise bad things happen), and ended up with a much >> stronger association between the current state and the drm_planes, >> e.g. tracking view/output. This naturally seemed to lead to a more >> incremental approach wrt how we generate DRM atomic state, and one of >> the things that jumped out from that was that you could then do a >> first pass to cull any assumptions that no longer held true (view A is >> suitable to display on plane B), giving you a baseline to build on. >> >> I don't have a strong opinion on which is ultimately better. > > Ok. > > Hmm, maybe I'm slowly getting to what you have been trying to explain... > > First pass is to just check the old plane assignments that still hold > (nothing to atomic req for these, except state changes if needed), and > free others (add disables to the req). Then a test commit will ensure > they still hold. If not, start the 2nd pass from a all planes disabled > req instead. > > Then the seconds pass will try to find additional views for free planes > in a similar fashion to what the from-scratch algorithm would do: add > one plane. Test. If ok, add another plane, repeat. If not ok, add > disables for that plane instead, and stop (or try to find other views). > > The base req built on the first pass ensures all planes that have not > already been assigned and tested ok, are to be disabled. > > Was this your idea? > > The point I kept missing was that we assumed the req already contained > properties to disable all unused-at-this-point planes. > > A confusion between the code and what it should be. I think that's a reasonable expression of where I was aiming with that 'XXX' comment block, yeah. >> I did indeed have exactly that in mind, where individual updates would >> build their own AtomicReq, and then merge them together, e.g.: >> - generate base req for CRTC/connector A >> - duplicate req >> - apply plane updates for that output into duplicated req >> - call TEST_ONLY on duplicated req >> - use duplicated req if possible, or failing that go back to original >> - do exactly the same for CRTC/connector/planes B >> - merge two reqs together >> - issue actual atomic modeset >> >> It's a little bit contrived perhaps, but there are other ways of >> tracking state for which I can see that being a better approach. > > We should also avoid premature optimization, I suppose. :-) It's not necessarily even about premature optimisation, but just about the model you use internally to represent state. > I think the best way to cache state is to let it stay in the kernel, > and avoid submitting a disable followed by the old state that's already > there. If we manage that, maintaining cached atomic req pieces in > weston would be moot. Can you expand a bit on what you mean here? Cheers, Daniel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel