Re: [PATCH v2 weston 00/16] Atomic modesetting support
Hi, On 11 July 2015 at 02:05, Bryce Harrington br...@osg.samsung.com wrote: On Fri, Jun 26, 2015 at 02:16:55PM -0500, Derek Foreman wrote: On 22/06/15 11:25 AM, Daniel Stone wrote: Hi, Thanks to everyone who reviewed the previous series. This new series cleans up the previous patches, introduces a few fixes (e.g. not relying on a repaint to pull us out of DPMS), and crucially adds support for the libdrm TEST_ONLY interface (allowing us to check before we commit, e.g. that a particular plane combination is workable), using the new cursor API from libdrm. Everything up to and including 8/16 seems like beneficial clean-up or bug fix that could land independently of later bits? For that half, Reviewed-By: Derek Foreman der...@osg.samsung.com (The rest I haven't had a chance to look at yet) At least for patches 1-6 I tend to agree; they're good cleanup or refactoring, and no reason not to just get them landed. Patch 7 started to look a bit more ambitious to me so I stopped there. But with 1-6, might save time having to rebase them. Unfortunately they already don't apply, but if you can split them out from the remainder of the patchset, I'd be +1 for just landing them immediately. Thanks for this. I've rebased the first six across the libweston divide and pushed those now. I think 7-10 should be very uninvasive once rebased as well; it's only where we get to 11 and upwards that it starts getting a bit hairy. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 weston 00/16] Atomic modesetting support
On Fri, 10 Jul 2015 18:05:48 -0700 Bryce Harrington br...@osg.samsung.com wrote: On Fri, Jun 26, 2015 at 02:16:55PM -0500, Derek Foreman wrote: On 22/06/15 11:25 AM, Daniel Stone wrote: Hi, Thanks to everyone who reviewed the previous series. This new series cleans up the previous patches, introduces a few fixes (e.g. not relying on a repaint to pull us out of DPMS), and crucially adds support for the libdrm TEST_ONLY interface (allowing us to check before we commit, e.g. that a particular plane combination is workable), using the new cursor API from libdrm. Everything up to and including 8/16 seems like beneficial clean-up or bug fix that could land independently of later bits? For that half, Reviewed-By: Derek Foreman der...@osg.samsung.com (The rest I haven't had a chance to look at yet) At least for patches 1-6 I tend to agree; they're good cleanup or refactoring, and no reason not to just get them landed. Patch 7 started to look a bit more ambitious to me so I stopped there. But with 1-6, might save time having to rebase them. Unfortunately they already don't apply, but if you can split them out from the remainder of the patchset, I'd be +1 for just landing them immediately. Sure. I've been holding off on them with the prospect of landing Mario Kleiner's compositor patches first, just in case there was any conflict, but since I redid Mario's patches and I'm still waiting for the final ack on my analysis which means I can (and probably already need to) rebase them, I'm totally fine landing these clean-up/preparation patches. Of the current libweston series, there are still changes coming to compositor-drm.c for config handling, but I don't think they should conflict much. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 weston 00/16] Atomic modesetting support
On Fri, Jun 26, 2015 at 02:16:55PM -0500, Derek Foreman wrote: On 22/06/15 11:25 AM, Daniel Stone wrote: Hi, Thanks to everyone who reviewed the previous series. This new series cleans up the previous patches, introduces a few fixes (e.g. not relying on a repaint to pull us out of DPMS), and crucially adds support for the libdrm TEST_ONLY interface (allowing us to check before we commit, e.g. that a particular plane combination is workable), using the new cursor API from libdrm. Everything up to and including 8/16 seems like beneficial clean-up or bug fix that could land independently of later bits? For that half, Reviewed-By: Derek Foreman der...@osg.samsung.com (The rest I haven't had a chance to look at yet) At least for patches 1-6 I tend to agree; they're good cleanup or refactoring, and no reason not to just get them landed. Patch 7 started to look a bit more ambitious to me so I stopped there. But with 1-6, might save time having to rebase them. Unfortunately they already don't apply, but if you can split them out from the remainder of the patchset, I'd be +1 for just landing them immediately. Bryce This still relies on support not yet mainlined. Currently I am using: git://git.collabora.co.uk/git/user/daniels/linux.git#wip/4.2.x/atomic-i915 git://git.collabora.co.uk/git/user/daniels/libdrm.git#atomic Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 weston 00/16] Atomic modesetting support
Hi,
On 29 June 2015 at 07:53, Pekka Paalanen ppaala...@gmail.com wrote:
On Fri, 26 Jun 2015 14:27:48 +0100
Daniel Stone dan...@fooishbar.org wrote:
On 24 June 2015 at 13:11, Pekka Paalanen ppaala...@gmail.com wrote:
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?
Gaining anything from a req snippet cache
Yes, I don't think Weston would gain anything from caching request
snippets. As I said, it's not applicable to us, but I can certainly
imagine usecases _for other non-Weston users_ where req snippets would
come in handy, so I added it to the API as another way to go about
things. drmModeAtomic{Duplicate,Merge} are not relevant to Weston.
depends on the need to submit
already submitted state again. The only significant reason to do that,
that I can imagine, would be if the algorithm unconditionally
initialized the req with either that cached state or disabled state.
If we avoid unconditionally initializing a req like that, I don't see
what we would gain from a req snippet cache. And we can avoid it,
because the kernel maintain all state we do not set.
I assume the cases where we need to re-program all state are rare
enough to not warrant this added code.
Therefore, if we only program state that has changed, the cache would
be a net cost rather than a win, as we'd (almost) never hit the cache.
Assuming this about Duplicate/Merge, I'll just leave it here, since we
agree that it's an inappropriate API to use for Weston, which is why I
haven't used it for Weston.
Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 weston 00/16] Atomic modesetting support
On Fri, 26 Jun 2015 14:27:48 +0100 Daniel Stone dan...@fooishbar.org wrote: Hi, On 24 June 2015 at 13:11, Pekka Paalanen ppaala...@gmail.com wrote: 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? Gaining anything from a req snippet cache depends on the need to submit already submitted state again. The only significant reason to do that, that I can imagine, would be if the algorithm unconditionally initialized the req with either that cached state or disabled state. If we avoid unconditionally initializing a req like that, I don't see what we would gain from a req snippet cache. And we can avoid it, because the kernel maintain all state we do not set. I assume the cases where we need to re-program all state are rare enough to not warrant this added code. Therefore, if we only program state that has changed, the cache would be a net cost rather than a win, as we'd (almost) never hit the cache. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 weston 00/16] Atomic modesetting support
On 22/06/15 11:25 AM, Daniel Stone wrote: Hi, Thanks to everyone who reviewed the previous series. This new series cleans up the previous patches, introduces a few fixes (e.g. not relying on a repaint to pull us out of DPMS), and crucially adds support for the libdrm TEST_ONLY interface (allowing us to check before we commit, e.g. that a particular plane combination is workable), using the new cursor API from libdrm. Everything up to and including 8/16 seems like beneficial clean-up or bug fix that could land independently of later bits? For that half, Reviewed-By: Derek Foreman der...@osg.samsung.com (The rest I haven't had a chance to look at yet) This still relies on support not yet mainlined. Currently I am using: git://git.collabora.co.uk/git/user/daniels/linux.git#wip/4.2.x/atomic-i915 git://git.collabora.co.uk/git/user/daniels/libdrm.git#atomic Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 weston 00/16] Atomic modesetting support
Hi, On 24 June 2015 at 13:11, Pekka Paalanen ppaala...@gmail.com wrote: On Wed, 24 Jun 2015 12:22:15 +0100 Daniel Stone dan...@fooishbar.org wrote: On 23 June 2015 at 13:28, Pekka Paalanen ppaala...@gmail.com wrote: On Tue, 23 Jun 2015 11:48:56 +0100 Daniel Stone dan...@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
Re: [PATCH v2 weston 00/16] Atomic modesetting support
Hello On Wed, 24 Jun 2015 12:22:15 +0100 Daniel Stone dan...@fooishbar.org wrote: Hi, On 23 June 2015 at 13:28, Pekka Paalanen ppaala...@gmail.com wrote: On Tue, 23 Jun 2015 11:48:56 +0100 Daniel Stone dan...@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. 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. 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
Re: [PATCH v2 weston 00/16] Atomic modesetting support
Hi, On 23 June 2015 at 13:28, Pekka Paalanen ppaala...@gmail.com wrote: On Tue, 23 Jun 2015 11:48:56 +0100 Daniel Stone dan...@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
Re: [PATCH v2 weston 00/16] Atomic modesetting support
On Tue, 23 Jun 2015 11:48:56 +0100 Daniel Stone dan...@fooishbar.org wrote: 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. 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
Re: [PATCH v2 weston 00/16] Atomic modesetting support
On Tue, Jun 23, 2015 at 12:48 PM, Daniel Stone dan...@fooishbar.org wrote: 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. Just a quick clarification discussed with Daniel on irc: Restrictions on requiring an enabled primary plane are mostly just software since the default primary plane enabled by universal planes can't do anything else. It can also only do rgbx. But atomic (and already even the transitional plane helpers) lift that restriction, and drivers must take explicit action in their atomic_check callbacks to refuse a config with enabled crtc but disabled primary. The default assumption is that primary/cursor planes aren't anything special at all compared to other planes and only used for implementing backwards compatibility with old userspace. The only thing where this differes is that for cursors and primary planes we have dedicated properties to tell userspace the size limits, for sprites there's nothing like that. But givin that max size is just a small part of figuring out what kind of surfaces a driver supports I don't think that's a big deal really ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2 weston 00/16] Atomic modesetting support
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
Re: [PATCH v2 weston 00/16] Atomic modesetting support
On Mon, 22 Jun 2015 17:25:05 +0100
Daniel Stone dani...@collabora.com wrote:
Hi,
Thanks to everyone who reviewed the previous series. This new series
cleans up the previous patches, introduces a few fixes (e.g. not relying
on a repaint to pull us out of DPMS), and crucially adds support for the
libdrm TEST_ONLY interface (allowing us to check before we commit, e.g.
that a particular plane combination is workable), using the new cursor
API from libdrm.
This still relies on support not yet mainlined. Currently I am using:
git://git.collabora.co.uk/git/user/daniels/linux.git#wip/4.2.x/atomic-i915
git://git.collabora.co.uk/git/user/daniels/libdrm.git#atomic
Hi Daniel,
you asked me to take a look at the assing_planes code wrt. TEST_ONLY
and backtracking.
The selected code quotations below are from after the complete series.
static void
drm_plane_update_begin(struct drm_plane *plane)
{
plane-props.value_pend_mask = 0;
}
/**
* Try to use the primary DRM plane to directly display a full-screen view
*
* If a surface covers an entire output and is unoccluded, attempt to directly
* pageflip the primary DRM plane (not to be confused with the primary Weston
* plane) to the buffer. In legacy DRM usage, this will use drmModePageFlip;
* in atomic, this is just another plane.
*
* @param output Output to target
* @param ev View to prospectively use the primary plane
*/
static struct weston_plane *
drm_output_prepare_scanout_view(struct drm_output *output,
struct weston_view *ev)
{
struct drm_compositor *c =
(struct drm_compositor *) output-base.compositor;
struct weston_buffer *buffer = ev-surface-buffer_ref.buffer;
struct weston_buffer_viewport *viewport = ev-surface-buffer_viewport;
struct gbm_bo *bo;
uint32_t format;
if (ev-geometry.x != output-base.x ||
ev-geometry.y != output-base.y ||
buffer == NULL || c-gbm == NULL ||
buffer-width != output-base.current_mode-width ||
buffer-height != output-base.current_mode-height ||
output-base.transform != viewport-buffer.transform ||
ev-transform.enabled)
return NULL;
if (ev-geometry.scissor_enabled)
return NULL;
bo = gbm_bo_import(c-gbm, GBM_BO_IMPORT_WL_BUFFER,
buffer-resource, GBM_BO_USE_SCANOUT);
/* Unable to use the buffer for scanout */
if (!bo)
return NULL;
format = drm_output_check_scanout_format(output, ev-surface, bo);
if (format == 0) {
gbm_bo_destroy(bo);
return NULL;
}
output-primary_plane-next = drm_fb_get_from_bo(bo, c, format);
if (!output-primary_plane-next) {
gbm_bo_destroy(bo);
return NULL;
}
drm_fb_set_buffer(output-primary_plane-next, buffer);
return output-fb_plane;
}
static struct weston_plane *
drm_output_prepare_overlay_view(struct drm_output *output,
struct weston_view *ev)
{
struct weston_compositor *ec = output-base.compositor;
struct drm_compositor *c = (struct drm_compositor *)ec;
struct weston_buffer_viewport *viewport = ev-surface-buffer_viewport;
struct drm_plane *p;
struct drm_plane *cur = NULL;
int found = 0;
struct gbm_bo *bo;
pixman_region32_t dest_rect, src_rect;
pixman_box32_t *box, tbox;
uint32_t format;
wl_fixed_t sx1, sy1, sx2, sy2;
if (c-gbm == NULL)
return NULL;
if (viewport-buffer.transform != output-base.transform)
return NULL;
if (viewport-buffer.scale != output-base.current_scale)
return NULL;
if (c-sprites_are_broken)
return NULL;
if (ev-output_mask != (1u output-base.id))
return NULL;
if (ev-surface-buffer_ref.buffer == NULL)
return NULL;
if (ev-alpha != 1.0f)
return NULL;
if (wl_shm_buffer_get(ev-surface-buffer_ref.buffer-resource))
return NULL;
if (!drm_view_transform_supported(ev))
return NULL;
/* Find the current view this plane is assigned to, if any. */
wl_list_for_each(p, c-plane_list, link) {
if (p-view != ev || p-output != output)
continue;
cur = p;
break;
}
bo = gbm_bo_import(c-gbm, GBM_BO_IMPORT_WL_BUFFER,
ev-surface-buffer_ref.buffer-resource,
GBM_BO_USE_SCANOUT);
wl_list_for_each(p, c-plane_list, link) {
if (!bo)
continue;
if (!drm_plane_crtc_supported(output, p-possible_crtcs))
continue;
