[PATCH v14 09/41] compositor-drm: Track unused connectors and CRTCs

[Pekka Paalanen]

On Tue, 16 Jan 2018 16:40:47 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Pekka,
> 
> On 16 January 2018 at 15:19, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Wed, 20 Dec 2017 12:26:26 +0000
> > Daniel Stone <daniels at collabora.com> wrote:  
> >> Rather than a more piecemeal approach at backend creation, explicitly
> >> track connectors and CRTCs we do not intend to use, so we can ensure
> >> they are disabled where appropriate.
> >>
> >> Signed-off-by: Daniel Stone <daniels at collabora.com>
> >> ---
> >>  libweston/compositor-drm.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 86 insertions(+)
> >>
> >> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> >> index 04d52f2c6..9c7564383 100644
> >> --- a/libweston/compositor-drm.c
> >> +++ b/libweston/compositor-drm.c
> >> @@ -188,6 +188,9 @@ struct drm_backend {
> >>
> >>       void *repaint_data;
> >>
> >> +     struct wl_array unused_connectors;
> >> +     struct wl_array unused_crtcs;
> >> +
> >>       int cursors_are_broken;
> >>
> >>       bool universal_planes;
> >> @@ -1363,6 +1387,12 @@ drm_output_assign_state(struct drm_output_state *state,
> >>               else if (plane->type == WDRM_PLANE_TYPE_PRIMARY)
> >>                       output->page_flip_pending = 1;
> >>       }
> >> +
> >> +     if (output->dpms == WESTON_DPMS_ON) {
> >> +             wl_array_remove_uint32(&b->unused_connectors,
> >> +                                    output->connector_id);
> >> +             wl_array_remove_uint32(&b->unused_crtcs, output->crtc_id);
> >> +     }  
> >
> > This gets called from drm_repaint_flush(). In the following patch,
> > drm_repaint_flush() starts to explicitly disable unused CRTCs. That
> > happens before calling drm_output_assign_state(). It would seem that it
> > is possible to disable a CRTC that is being taken into use.
> >
> > The CRTC and connector should probably be removed from the unused lists
> > already under drm_output_repaint() if it's to be done by state
> > application.  
> 
> You're correct here, and I think this is probably a good idea. AFAICT,
> the failure case you're describing is that we would disable a CRTC in
> drm_repaint_flush() immediately before we re-enable it in the same
> function, right? When we get to atomic, this becomes a non-issue, as
> the disabling property set is superseded by the later enables by
> drmModeAtomicCommit() in libdrm, as well as by the kernel. But for the
> non-atomic API, this could indeed cause enable -> disable -> enable,
> rather than enable -> enable, at startup.

Right. I wasn't too sure of the ordering, but I very much suspected the
exact things you describe.

> > On another thought, "unused" at this patch means "no owning drm_output"
> > if we look at drm_backend_update_unused_outputs(), but OTOH it means
> > "not being driven" if we look at drm_output_deinit() called from
> > drm_output_disable() which still leaves the drm_output existing.  
> 
> Hm. We want to disable any unused CRTCs before making any enabling
> modesets, to maximise the shared global resources, e.g. CRTC clocks
> available for the enabling modeset, giving it the greatest chance of
> success. This means that we need to do the disables before any
> enabling modesets. Maybe we could do this by initially having every
> CRTC and connector in the unused list, and only removing them in
> drm_repaint_flush(), first doing a scan over every output in the
> pending_state and removing them from the unused list, then disabling
> everything on the unused list, then applying the output states.

If we have a consistent meaning for the "unused", what does it
benefit to go through the hassle of scanning over pending_states? Why
couldn't the arrays of unused be correct to begin with?

> If we're doing this, we probably want to (again) alter the 'move
> repaint state application to flush' commit, to commit any DPMS-off
> outputs before we commit any enabling states, in order to be
> consistent. Fun.

Maybe postpone that for later as an additional optimization. It's not
like anything is regressing without it, is it?

> > - Any hotplug event will rewrite all "unused" arrays according to "no
> >   owning drm_output".
> >
> > - Disable, then enable an output without a hotplug even in between
> >   would probably cause the CRTC to be erroneuously turned off by the
> >   following patch.
> >
> > Could you explain the intention here, what does "unused" mean?
> >
> > I suppose, as disabled outputs do not go through repaint, disabled but
> > connected (drm_output exists for the CRTC and connector) should be part
> > of the unused arrays, which implies that the SetCrtc vs. array_remove
> > ordering will be wrong and drm_backend_update_unused_outputs() should
> > check the drm_output is enabled as well.  
> 
> Right, I think that's the best thing to do.
> 
> > One of these patches should change the behaviour from "do not touch an
> > output unless explicitly enabled or disabled" to "disable all outputs
> > not explicitly enabled", as keeping the don't-touch mode seems awkward.  
> 
> That's the intention, at least by the time we get to 'Move repaint
> state application to flush'.

As long as the commit message of such patch identifies this change in
behaviour. :-)

> My intention is, after some testing tomorrow, merge patches 1-8, fix
> up Philipp's comments on the two patches, merge your fixes to 13
> ('Don't restore original CRTC mode') in, and send out a v15. Probably
> all of this points out the need for more comments/documentation, on
> both this and the lifetime of atomic state objects: if those existed,
> these discussions would've pointed out that the code didn't match the
> documentation, or the documentation was nonsensical. ;)

Sounds good!


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180117/9a7c8077/attachment.sig>