[PATCH weston] compositor: Assign new views to the primary plane

[Pekka Paalanen]

On Wed, 14 Dec 2016 14:53:37 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Pekka,
> 
> On 14 December 2016 at 14:17, Pekka Paalanen
> <pekka.paalanen at collabora.co.uk> wrote:
> > On Fri, 9 Dec 2016 17:27:13 +0000 Daniel Stone <daniel at fooishbar.org> 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 up happily repainting & re-renderering
> all the time for no reason.
> 
> So, with both of these two patches, we get both desired behaviours: no
> unnecessary renderer wakeups when both planes are active; no
> unnecessary repaints. This patch is necessary, but not sufficient, to
> reach that goal.
> 
> > 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. :-)


Thanks,
pq