[PATCH v2 weston 00/16] Atomic modesetting support

[Daniel Stone]

Hi,

On 24 June 2015 at 13:11, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Wed, 24 Jun 2015 12:22:15 +0100
> Daniel Stone <daniel at fooishbar.org> wrote:
>> 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.

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