On Tue, 16 Jan 2018 16:40:47 +0000 Daniel Stone <dan...@fooishbar.org> wrote:
> Hi Pekka,
>
> On 16 January 2018 at 15:19, Pekka Paalanen <ppaala...@gmail.com> wrote:
> > On Wed, 20 Dec 2017 12:26:26 +0000
> > Daniel Stone <dani...@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 <dani...@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
pgp0yGp7PFJpv.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
