[PATCH v2 weston 00/16] Atomic modesetting support

[Daniel Stone]

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.

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

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

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

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

Cheers,
Daniel