[PATCH weston 7/8] compositor-drm: Implement clone mode, refactor output into logical ones

[Daniel Stone]

Hi Emmanuel,

On 2 May 2016 at 22:40, Emmanuel Gil Peyrot
<emmanuel.peyrot at collabora.com> wrote:
> Introduces a “same-as” configuration option for each output, which
> bypasses the rest of the output configuration (mode, scale, transform
> and seat) and instead makes it a clone of the specified output.
>
> This is implemented by splitting the drm_output struct into the
> per-connector drm_output and the per-weston_output drm_logical_output,
> with the latter containing one or more of the former in a wl_list.

Hmm. Let me stop you there ...

> +struct drm_logical_output {
> +       struct weston_output   base;
> +       struct wl_list output_list;
> +
> +       int page_flip_refcount;

This to me is a red flag. I think you've built this patch at the wrong level.

There are really two kinds of clone mode: same-CRTC and different
CRTC. Same-CRTC can share planes, and they share timing as well. I
think same-CRTC clones should be ganged together within the backend,
and different-CRTC clones should appear entirely disjoint from the
backend's point of view.

With the caveat that my review of Armin's drm_keep_current_mode patch
shows that I comprehensively don't yet understand the new output
configuration API - so I'd like his input on this as well - I'd
suggest the following approach.

Firstly, keep every connector as a separate weston_output, as it is today.

When you parse an outputB same-as outputA configuration, call a new
backend vfunc: outputA->chain_output(outputB). If this returns
success, mark output B as chained from output A: not appearing in a
surface's output_mask[0], not having dpms/repaint/... called on it,
etc. If this returns failure, then continue to consider them as
totally separate outputs. In the DRM backend, this would simply mean
that we passed multiple connectors to drmModeSetCrtc and multiple
connector_states bound to the same CRTC.

This would remove a lot of complexity from the DRM backend, and also
allow planes to work when we can clone multiple connectors on the same
CRTC. It would also mean we avoid a possible drop to 30fps, if the two
CRTCs end up clocked half the repaint cycle apart, and thus didn't
call weston_output_finish_frame until too late. Plus it has the added
benefit of working better when we have a constrained number of CRTCs,
or constrained CRTC capability, or whatever.

The core already has to deal with surfaces overlapping multiple 'real'
outputs - i.e. running on different paint cycles - today, so I don't
see that punting clones running on disjoint CRTCs makes that problem
worse in terms of complexity for the core. We could certainly do
better with repaint-cycle time reporting[1] in the core, but just
punting down to the backend only makes that worse rather than better,
I think.

Doing it this way would make also make the patchset a lot less
invasive, particularly if rebased on top of the
atomic/plane_state/output_state patchset ... ;)

Sorry to be the bearer of bad news.

Cheers,
Daniel

[0]: Well. The client should still see the output for
wl_surface::{enter,leave} events, but the backend should only see the
'real' outputs, not the chained ones. Maybe we need a
client_output_mask, and a backend_output_mask ... ?
[1]: Have a surface span two weston_outputs with disjoint paint
clocks. Watch the surface's frame-event timing bounce between the two
outputs, with resultant jittery framerate. Presumably we should pick
one 'master' surface to clock from, and stick to that.