[PATCH v2 weston 00/16] Atomic modesetting support

[Pekka Paalanen]

Hello

On Wed, 24 Jun 2015 12:22:15 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 23 June 2015 at 13:28, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Tue, 23 Jun 2015 11:48:56 +0100
> > Daniel Stone <daniel at 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.

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

> >> > If the TEST_ONLY commit succeeds, rewind to cursor B and try the next
> >> > plane. If it fails, rewind to cursor A, fill in the rest of disables,
> >> > and be done with it.
> >> >
> >> > Does this make sense?
> >>
> >> Hmm, I was thinking more of, build a list of disables first, then try
> >> to insert enables one-by-one, and apply them if they succeed.
> >
> > Yeah, that works too, I suppose, I didn't think of allowing the setting
> > of the same properties multiple times in a single atomic req. Less
> > rewinding, indeed.
> >
> > I just didn't spot any code doing the disables at all for TEST_ONLY. :-)
> 
> Yeah, this is correct, so we will generate false negatives. Following
> the approach I've taken above, we would have to do a first pass to
> cull unused planes, followed by a second pass to add new planes;
> following the destroy-and-rebuild approach that assign_planes used to
> take, we wouldn't need this, because we can steal any plane which was
> once assigned to our output, or isn't assigned to any output.

Ah, yeah. Filling in the details in between, we agree. :-)

> >> > This is assuming we cannot do without the primary plane. If it was
> >> > possible to drive truely universal planes, we would not know if we need
> >> > a primary plane until we know if there is anything on it. You'd have to
> >> > first assume the primary plane is needed, test the additional plane
> >> > setups, and then if we don't need the primary plane, test once more
> >> > without it.
> >>
> >> Sure, in theory the primary plane can be disabled, but that's not the
> >> reality of a lot of common hardware, so we'll have to deal with that
> >> assumption for a very long time to come.
> >
> > Cool, one thing less to worry.
> >
> > Sounds like the libdrm API, and hence the kernel ABI, is sufficient now.
> > You're not using the Duplicate and Merge functions from libdrm, but
> > maybe they would be useful for a state cache?
> 
> Thanks. Definitely still a fair few details to be worked through on
> the Weston implementation side, but nothing I can see that would
> impact on the kernel/libdrm.
> 
> 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. :-)

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.


Thanks,
pq