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

[Daniel Stone]

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?

Thanks for the review!

Cheers,
Daniel