Re: [PATCH weston] compositor: Assign new views to the primary plane
Hi, On 14 December 2016 at 15:21, Pekka Paalanen wrote: > On Wed, 14 Dec 2016 14:53:37 + Daniel Stone wrote: >> On 14 December 2016 at 14:17, Pekka Paalanen >> wrote: >> > So, reading both versions of the commit message, I think I was able >> > reconstruct enough of the idea to see what's going on, but please have >> > a third try on explaining it. ;-) >> >> Did that (though too verbose for a commit message) help, at least? > > Yes! > > Don't be shy to just dump it into the commit message, because writing > it nicely and shortly is probably not humanly possible, or at least not > worthwhile. I'd prefer to err on documenting too much than too > little. :-) OK, I've pushed this with your review and a fourth attempt at a coherent commit message. Fingers crossed it's readable. :) Thanks! Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] compositor: Assign new views to the primary plane
On Wed, 14 Dec 2016 14:53:37 + Daniel Stone wrote: > Hi Pekka, > > On 14 December 2016 at 14:17, Pekka Paalanen > wrote: > > On Fri, 9 Dec 2016 17:27:13 + Daniel Stone > > wrote: > >> However, this is undesirable for DRM. With multi-output, when > >> assign_planes() is called, any view which wasn't on a plane for our > >> putput was moved to the primary plane, thus causing damage, and an > > > > Putput! \o/ > > /o\ > > >> output repaint. Fixing this, to ignore views which do not touch our > >> output at all, means that the view wouldn't have a plane assigned until > >> the other output eventually ran through repaint. > >> > >> For large SHM buffers (weston_surface->keep_buffer == false), this means > >> that by the time the other output assigned it to a plane, the buffer may > >> have been discarded on the client side. > > > > Oh, upload garbage, right. That's a very interesting side-effect of > > weston_surface_damage(). > > Yes, you noted it quite well in e508ce6a. ;) > > > To me it makes perfect sense to assign views to the primary plane by > > default, on its own. > > > > "definitely the wrong way to fix" what? The weston_surface_damage() > > issue? I'd probably not even make the connection there. > > Well, it's not a complete fix for the entire surface damage system. It > is, however, _a_ correct fix, in that by bypassing the issue, we > completely prevent the problem recurring. More below. > > > The > > DRM output/plane assignment? Is there something that makes a decision > > based on which plane a view is already on? > > > > Oh right, there has to be, because we don't support having the same > > view on several planes, right? (Hmm, why was that... let me guess: > > damage tracking?) > > Right, because it just can't work at the moment. compositor-drm uses > weston_plane as a 'base class' of drm_plane (drm_sprite), so if > multiple outputs were to both promote the same view to a plane, it > would be inconsistent as each repaint cycle assigned it to a different > plane; a nightmare for state tracking. If one output promotes the view > to a plane and the other does not, then it just goes missing from the > other output, since renderer repaint no longer shows it. > > > I just couldn't spot where we actually check the plane assignment. Is > > that something you are adding with atomic prep? > > No, nothing so obvious. It only trips with 'Ignore views on other > outputs', when you have multiple outputs in Weston, and are using > something which paints a SHM buffer once rather than continuously. > > Simplified slightly, this is the behaviour in current master: > - views A and Z get created for outputs B and Y, respectively, > initial with view->plane == NULL > - SHM buffers are attached to A and Z, and uploaded to the renderer > - output B repaint gets called > - assign_planes for output B observes that buffer for views A and Z > can never be promoted to a plane as it is SHM, so sets keep_buffer == > false > - assign_planes assigns view A to the primary plane; as the initial > plane was NULL, the comparison in weston_view_assign_to_plane does not > trigger, and weston_surface_damage is called > - assign_planes also assigns view Z to the primary plane, with the same > effect > - after repaint, as there is no need for the buffer content to be > kept, the buffers are released > - output Y repaint gets called > - assign_planes for output Y has no effect on keep_buffer here, as > it is already false (i.e. this is a static/constant/deterministic > calculation) > - assign_planes for output Y assigns view Z to the primary plane, > which is a no-op as the plane was already set by output B repaint > - everyone lives happily ever after > > The specific failure I saw with the 'Ignore views on other outputs' > patch, but _without_ this patch, was: > - assign_planes for output B does _not_ assign view Z to the primary > plane anymore > - assign_planes for output Y _does_ assign view Z to the primary > plane, however this is _not_ a no-op as the previous plane was NULL > - weston_surface_damage is called, uploading content from ... somewhere > > This patch fixes this specific breakage, by assigning views to the > primary plane at time of creation. The only way > weston_view_assign_to_plane can be called with something other than > the primary plane, is if the view is assigned to a plane by the > backend. If that ever happens, keep_buffer is guaranteed to not be > false: we very conservatively set keep_buffer for _all_ views globally > on every output repaint, for exactly this reason. Given all the above, > I think this is a complete fix for the problem I've just described. > > FWIW, the reason I wrote 'Ignore views on other outputs' is in the > commit message, but elaborated, if you had multiple outputs with > planes in use, you were in for a bad time. assign_planes would walk > the view list, placing views either on a drm_plane if they were active > o
Re: [PATCH weston] compositor: Assign new views to the primary plane
Hi Pekka, On 14 December 2016 at 14:17, Pekka Paalanen wrote: > On Fri, 9 Dec 2016 17:27:13 + Daniel Stone wrote: >> However, this is undesirable for DRM. With multi-output, when >> assign_planes() is called, any view which wasn't on a plane for our >> putput was moved to the primary plane, thus causing damage, and an > > Putput! \o/ /o\ >> output repaint. Fixing this, to ignore views which do not touch our >> output at all, means that the view wouldn't have a plane assigned until >> the other output eventually ran through repaint. >> >> For large SHM buffers (weston_surface->keep_buffer == false), this means >> that by the time the other output assigned it to a plane, the buffer may >> have been discarded on the client side. > > Oh, upload garbage, right. That's a very interesting side-effect of > weston_surface_damage(). Yes, you noted it quite well in e508ce6a. ;) > To me it makes perfect sense to assign views to the primary plane by > default, on its own. > > "definitely the wrong way to fix" what? The weston_surface_damage() > issue? I'd probably not even make the connection there. Well, it's not a complete fix for the entire surface damage system. It is, however, _a_ correct fix, in that by bypassing the issue, we completely prevent the problem recurring. More below. > The > DRM output/plane assignment? Is there something that makes a decision > based on which plane a view is already on? > > Oh right, there has to be, because we don't support having the same > view on several planes, right? (Hmm, why was that... let me guess: > damage tracking?) Right, because it just can't work at the moment. compositor-drm uses weston_plane as a 'base class' of drm_plane (drm_sprite), so if multiple outputs were to both promote the same view to a plane, it would be inconsistent as each repaint cycle assigned it to a different plane; a nightmare for state tracking. If one output promotes the view to a plane and the other does not, then it just goes missing from the other output, since renderer repaint no longer shows it. > I just couldn't spot where we actually check the plane assignment. Is > that something you are adding with atomic prep? No, nothing so obvious. It only trips with 'Ignore views on other outputs', when you have multiple outputs in Weston, and are using something which paints a SHM buffer once rather than continuously. Simplified slightly, this is the behaviour in current master: - views A and Z get created for outputs B and Y, respectively, initial with view->plane == NULL - SHM buffers are attached to A and Z, and uploaded to the renderer - output B repaint gets called - assign_planes for output B observes that buffer for views A and Z can never be promoted to a plane as it is SHM, so sets keep_buffer == false - assign_planes assigns view A to the primary plane; as the initial plane was NULL, the comparison in weston_view_assign_to_plane does not trigger, and weston_surface_damage is called - assign_planes also assigns view Z to the primary plane, with the same effect - after repaint, as there is no need for the buffer content to be kept, the buffers are released - output Y repaint gets called - assign_planes for output Y has no effect on keep_buffer here, as it is already false (i.e. this is a static/constant/deterministic calculation) - assign_planes for output Y assigns view Z to the primary plane, which is a no-op as the plane was already set by output B repaint - everyone lives happily ever after The specific failure I saw with the 'Ignore views on other outputs' patch, but _without_ this patch, was: - assign_planes for output B does _not_ assign view Z to the primary plane anymore - assign_planes for output Y _does_ assign view Z to the primary plane, however this is _not_ a no-op as the previous plane was NULL - weston_surface_damage is called, uploading content from ... somewhere This patch fixes this specific breakage, by assigning views to the primary plane at time of creation. The only way weston_view_assign_to_plane can be called with something other than the primary plane, is if the view is assigned to a plane by the backend. If that ever happens, keep_buffer is guaranteed to not be false: we very conservatively set keep_buffer for _all_ views globally on every output repaint, for exactly this reason. Given all the above, I think this is a complete fix for the problem I've just described. FWIW, the reason I wrote 'Ignore views on other outputs' is in the commit message, but elaborated, if you had multiple outputs with planes in use, you were in for a bad time. assign_planes would walk the view list, placing views either on a drm_plane if they were active on that output, or primary_plane otherwise. So for views on other outputs which were promoted to planes, the other output's assign_planes would assign the view to the primary plane, causing damage and a repaint on the other output: the other output would do _exactly the same_, so you end
Re: [PATCH weston] compositor: Assign new views to the primary plane
On Fri, 9 Dec 2016 17:27:13 + Daniel Stone wrote: > Hi, > > On 9 December 2016 at 16:35, Daniel Stone wrote: > > However, this is undesirable for DRM. In a multi-output situation, > > we would see a view only visible on another output, reasonably decide we > > didn't want it in a plane on our output, and move it to the primary > > plane, causing damage, and an output repaint. The plane wouldn't be > > assigned until the other output ran through repaint. > > > > For large SHM buffers (weston_surface->keep_buffer as false), this means > > that the other output would assign it to a plane later, which caused > > weston_surface_damage to be called - in the exact way the comment says > > it shouldn't - which triggered a flush and buffer upload. By this stage, > > the buffer content would be gone and we would upload garbage. > > Er, I can't English this afternoon. Imagine the above two paragraphs > never happened, and mentally replace them with: > > However, this is undesirable for DRM. With multi-output, when > assign_planes() is called, any view which wasn't on a plane for our > putput was moved to the primary plane, thus causing damage, and an Putput! \o/ > output repaint. Fixing this, to ignore views which do not touch our > output at all, means that the view wouldn't have a plane assigned until > the other output eventually ran through repaint. > > For large SHM buffers (weston_surface->keep_buffer == false), this means > that by the time the other output assigned it to a plane, the buffer may > have been discarded on the client side. Oh, upload garbage, right. That's a very interesting side-effect of weston_surface_damage(). To me it makes perfect sense to assign views to the primary plane by default, on its own. "definitely the wrong way to fix" what? The weston_surface_damage() issue? I'd probably not even make the connection there. The DRM output/plane assignment? Is there something that makes a decision based on which plane a view is already on? Oh right, there has to be, because we don't support having the same view on several planes, right? (Hmm, why was that... let me guess: damage tracking?) I just couldn't spot where we actually check the plane assignment. Is that something you are adding with atomic prep? So, reading both versions of the commit message, I think I was able reconstruct enough of the idea to see what's going on, but please have a third try on explaining it. ;-) Anyway, the change itself is: Reviewed-by: Pekka Paalanen Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] compositor: Assign new views to the primary plane
Hi, On 9 December 2016 at 16:35, Daniel Stone wrote: > However, this is undesirable for DRM. In a multi-output situation, > we would see a view only visible on another output, reasonably decide we > didn't want it in a plane on our output, and move it to the primary > plane, causing damage, and an output repaint. The plane wouldn't be > assigned until the other output ran through repaint. > > For large SHM buffers (weston_surface->keep_buffer as false), this means > that the other output would assign it to a plane later, which caused > weston_surface_damage to be called - in the exact way the comment says > it shouldn't - which triggered a flush and buffer upload. By this stage, > the buffer content would be gone and we would upload garbage. Er, I can't English this afternoon. Imagine the above two paragraphs never happened, and mentally replace them with: However, this is undesirable for DRM. With multi-output, when assign_planes() is called, any view which wasn't on a plane for our putput was moved to the primary plane, thus causing damage, and an output repaint. Fixing this, to ignore views which do not touch our output at all, means that the view wouldn't have a plane assigned until the other output eventually ran through repaint. For large SHM buffers (weston_surface->keep_buffer == false), this means that by the time the other output assigned it to a plane, the buffer may have been discarded on the client side. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston] compositor: Assign new views to the primary plane
When we create a new view, assign it to the primary plane from the beginning. This made no difference before, since the next surface repaint would forcibly assign all views to a plane, either through DRM's assign_planes() hook, or the fallback inside core repaint. However, this is undesirable for DRM. In a multi-output situation, we would see a view only visible on another output, reasonably decide we didn't want it in a plane on our output, and move it to the primary plane, causing damage, and an output repaint. The plane wouldn't be assigned until the other output ran through repaint. For large SHM buffers (weston_surface->keep_buffer as false), this means that the other output would assign it to a plane later, which caused weston_surface_damage to be called - in the exact way the comment says it shouldn't - which triggered a flush and buffer upload. By this stage, the buffer content would be gone and we would upload garbage. Avoid this problem for now by assigning the view to the primary plane on creation, thus short-circuiting weston_view_move_to_plane when we do call it during output repaint. This is arguably the right thing to do if definitely the wrong way to fix it, but the atomic series is long enough for now. Signed-off-by: Daniel Stone Cc: Pekka Paalanen --- libweston/compositor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libweston/compositor.c b/libweston/compositor.c index 6a69394..895a040 100644 --- a/libweston/compositor.c +++ b/libweston/compositor.c @@ -269,6 +269,7 @@ weston_view_create(struct weston_surface *surface) return NULL; view->surface = surface; + view->plane = &surface->compositor->primary_plane; /* Assign to surface */ wl_list_insert(&surface->views, &view->surface_link); -- 2.9.3 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
