[PATCH v2 weston 00/16] Atomic modesetting support

[Pekka Paalanen]

On Tue, 23 Jun 2015 11:48:56 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 23 June 2015 at 11:26, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > you asked me to take a look at the assing_planes code wrt. TEST_ONLY
> > and backtracking.
> 
> Thanks!
> 
> As a general comment, carried over from IRC: I think we should get
> Mario's series reviewed and merged first. It looks like good work, and
> I don't have any objection to the DRM bits, but am entirely
> underqualified to review the shell patches. I'd look to get this
> merged in two parts, with the drm_plane and universal plane conversion
> coming first, and then the atomic conversion coming later when it's a
> bit more bedded down and some of those mega-comments have gone away.
> 
> >                 /* XXX: At this point, we need two runs through assign_planes;
> >                  *      one to prepare any necessary views, and see if there
> >                  *      are any currently-unused planes. This process may
> >                  *      actually free up some planes for other views to use;
> >                  *      if any planes have been freed up, we should do another
> >                  *      pass to see if any planeless views can use any planes
> >                  *      which have just been freed. But we want to cache what
> >                  *      we can, so we're not, e.g., calling gbm_bo_import
> >                  *      twice. */
> 
> This is relevant ...
> 
> > Yeah, the logic for populating the atomic req seems mostly ok, but is
> > it missing the primary weston_plane assignment to the primary DRM plane?
> > Or if we are going to do a modeset, shouldn't that be here somehow too?
> >
> > I mean for cases where the primary DRM plane will actually change. If
> > it doesn't change, then I'd assume DRM maintains the state that is not
> > touched.
> 
> Right, exactly. The atomic ioctl starts by duplicating existing state,
> and then all property changes are strictly additive; otherwise we
> wouldn't have any use for the cache at all.
> 
> Mind you, the cache is broken, because it assumes a strictly linear
> set of changes, which this breaks, e.g.:
>   + initial modeset
>     - generate new req
>     - populate internal cache with complete desired state
>     - post new req to kernel
>   + repaint
>      - generate new req
>      - populate internal cache with values for new primary fb
>      - post new req to kernel
>   + repaint
>      - assign_planes
>        - generate new req
>        - populate internal cache with speculative values for checking plane
>        - submit req as TEST_ONLY
>      - generate new req
>      - populate internal cache with same values as previous; they're
> identical, so no change
>      - req doesn't have plane values since our cache says they're unchanged
> 
> 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.

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.

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.

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

> > The TEST_ONLY call should contain everything the final atomic commit
> > call will, right? So once assign_planes is done, we should already have
> > the complete atomic req, and in output_repaint what's left to do is to
> > actually render the leftovers and commit.
> >
> > I suppose this means we need to prepare the next buffer for a primary
> > weston_plane already at assign_planes start and use it in the TEST_ONLY
> > commit, before we even render to it. I hope it won't screw up any
> > fencing in DRM or EGL. But can we get it out of EGL/GBM *before* we
> > render it? We need an equivalent buffer that the primary DRM plane's
> > buffer is going to be for TEST_ONLY commits, so that we can check what
> > can go to overlays, so we know what we will need to render.
> 
> Indeed; the key is an equivalent buffer, rather than a rendered
> buffer, because we can't know what to render (think planes with alpha)
> until we've assigned our planes.
> 
> <daniels> pq: definitely when the primary plane has changed, we'll
> need to avoid assign_planes the first time around
> <daniels> pq: so that would be both on mode changes, and also when
> changing from pixman to egl rendering
> <daniels> pq: (so many creative failure cases, e.g. when you have
> shared detiling units, and moving from dumb/linear to gpu/tiled
> buffers means that you'll occupy an additional scaler for the primary)
> <daniels> pq: my thinking there was that during that transition, you'd
> just block planes completely, i.e. a transient version of
> sprites_are_broken
> <daniels> pq: i had originally thought in that case, when you finish
> repaint, that you'd then schedule another repaint to do a pass through
> assign_planes to get everything into its 'optimal' (most-planed) form
> <daniels> pq: but i don't think that holds true, because if your scene
> is static, then you'll use _more_ rather than less power through
> overlays
> <daniels> pq: so it seems like just a temporary block would be enough
> 
> So we can add a third case to that: transitioning from displaying a
> client buffer for scanout, back to GBM/Pixman.

Okay, so we would start using the "overlay disable" switch temporarily
a lot more. Weston has that already for making screenshots.

> > The atomic req in the process of being built would look like this:
> >
> > - primary plane setup
> > - other plane setup 1 (succeeded)
> > - other plane setup 2 (succeeded)
> > <- cursor A
> > - other plane setup 3 (to be tested)
> > <- cursor B
> > - other plane setup 4 (disable)
> > ...
> > - other plane setup N (disable)
> >
> > 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. :-)

> > 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,
pq