Hi,
On 23 June 2015 at 11:26, Pekka Paalanen <ppaala...@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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
