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

[Daniel Stone]

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.

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

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

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

Thanks a lot for the review!

Cheers,
Daniel