Hi Marius,
On Fri, 27 Jun 2025 at 14:18, Marius Vlad <marius.v...@collabora.com> wrote:
> Manually setting/forcing the connector status in sysfs does not
> propagate the CONNECTOR ID. For drivers that use polling, including
> manually setting it up with sysfs this would similarly to the HDP IRQ
> path which can pass it through drm_connector_helper_hpd_irq_event().
s/HDP/HPD/g
> out:
> - if (changed)
> - drm_kms_helper_hotplug_event(dev);
> + if (changed) {
> + struct drm_connector *first_changed_connector = NULL;
> + if (!mutex_trylock(&dev->mode_config.mutex)) {
> + repoll = true;
> + goto repoll;
> + }
>
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + if (connector->sysfs_hotplug) {
> + drm_connector_get(connector);
> + first_changed_connector = connector;
> + connector->sysfs_hotplug = false;
> + break;
> + }
> + }
> + drm_connector_list_iter_end(&conn_iter);
> + mutex_unlock(&dev->mode_config.mutex);
> +
> + if (first_changed_connector) {
> + drm_sysfs_connector_hotplug_event(connector);
> + drm_connector_put(first_changed_connector);
> + } else {
> + drm_kms_helper_hotplug_event(dev);
> + }
> + }
> +repoll:
> if (repoll)
> schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
> }
There are a couple of different things in here, so I think it would be
good to split into multiple commits.
This commit as-is is not correct: if we force multiple connectors to a
given status at the 'same time' (i.e. before the output-poll worker
can run in between), we will only send a hotplug event for the first
connector in the list, and not the other.
The good news is that this is easy to fix: we don't need to hold the
big mode_config mutex over this list, because it's only needed to call
the connector probe, unless I'm very much mistaken. So you could drop
the locking and just call drm_sysfs_connector_hotplug_event() for
every changed connector. But then we're introducing a specialist path
for sysfs, which is not ideal.
I think first we should aim to let output_poll_execute() send
per-connector events for _any_ kind of HPD event it finds. To that
effect, we could have a connector->status_changed bool which was set
whenever this happened, e.g. in drm_connector_helper_hpd_irq_event() /
check_connector_changed() for the connector-aware path, or
drm_helper_probe_single_connector_modes() for the general path. That
could then be cleared by drm_kms_helper_connector_hotplug_event() for
a single connector, or drm_kms_helper_hotplug_event() for all
connectors.
In the second commit, you could change output_poll_execute() to send
per-connector events, regardless of how they arrived. That would just
be iterating the connector list (without the mode_config mutex held)
and calling drm_kms_helper_connector_hotplug_event() for every changed
connector.
That would give us a nice logical progression: first we start tracking
whether a connector's status changed in a more robust way, which would
work across the multiple detection methods we have and also be OK for
sending events for delayed work. Then we start sending fine-grained
hotplug events when they come out of the poll helper. At the end of
the series, as a happy side effect of making code more generally
robust, we get HPD events for forcing connector status via sysfs.
Cheers,
Daniel
