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

[Pekka Paalanen]

On Mon, 21 Nov 2016 19:41:33 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> 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.

Hi,

about the surface/output overlap here... we certainly do handle a
surface overlapping several disjoint outputs, but I still have not
convinced myself that we handle mutually overlapping outputs properly.

Why do I say that? Because struct weston_plane has a member called
'damage', and drm_output_render() directly subtracts damage from the
primary plane. Pretty much every backend does that.


Thanks,
pq

> 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.
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161122/22c916d7/attachment.sig>