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

[Pekka Paalanen]

On Fri, 9 Dec 2016 17:27:13 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 9 December 2016 at 16:35, Daniel Stone <daniels at collabora.com> 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 <pekka.paalanen at collabora.co.uk>


Thanks,
pq