[PATCH v2 weston 00/16] Atomic modesetting support

[Daniel Stone]

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:
>> This is the reason for the large 'XXX' comment when calling
>> populate_atomic_plane inside repaint_atomic. So far, my thought has
>> been to add yet another parameter (ugh) to populate_atomic_plane, and
>> thus to plane_add_atomic, which would just call SetProp and not fill
>> the internal cache. A much more flexible solution would be to wrap
>> every drmModeAtomicReq inside a new drm_atomic_state struct, which
>> would contain our internal cache (wdrm_plane_properties etc), but I
>> haven't yet seen anything so far which would demand this more complex
>> solution. The main argument for taking the complex approach is that
>> this is what the kernel does: drm_atomic_state (and its children for
>> CRTC/plane/connector state) are duplicated internally for each
>> request.
>
> I think all those problems go away, if you build the final atomic req
> once and for all, like I suggested below. I think it's fairly
> error-prone to build it once, test, and build (almost) the same again,
> not to mention a little wasteful.

That's a fair comment, yeah.

> There would be some fiddling still when you need to back up because a
> TEST commit said "no", but then it'd be only that plane's worth of
> state.

Well, we already have that, in that if a test commit fails, we just
back the cursor up to where it was before. Committing a request only
commits up to the cursor, and merges get appended from the cursor. So
I don't see how it's really any more complicated than what we have
now, of:
  - add plane state A
  - test, success!
  - add plane state B
  - oh no, failed; back cursor up to A
  - add plane state C
  - test, success!
  - req now contains A + C

> We could simply add a pending value in addition to the current value
> stored in struct property_item, but yeah, this is all just details.

Mmm, yeah. Or just read everything back from the kernel any time a
request fails.

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

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.

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

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

Cheers,
Daniel