Hi Dmitry,
On Sat, Aug 23, 2025 at 12:30:27AM +0300, Dmitry Baryshkov wrote:
> On Tue, Jul 29, 2025 at 07:57:07PM +0300, Marius Vlad wrote:
> > This patch introduces a new boolean variable used to track connector's
> > connect/disconnect status and it is being used on both polling and
> > the HPD (Hot Plug Detect) paths.
> >
> > A subsequent patch would make use of this connector status to propagate
> > per-connector udev hot plug events. This allows user-space to receive
> > the connector's ID, rather than having a generic hot-plug event for all
> > connectors, or in the HPD path, just the first one found with a
> > connection status change.
>
> This looks good, the main question would be, what prevents a races
> during modifications of this field? I think it should be toggled under
> the dev->mode_config.mutex.
Thanks the review (been away for awhile and just saw your replies).
Sent out a v3 update:
https://lore.kernel.org/dri-devel/[email protected]/
>
> >
> > Signed-off-by: Marius Vlad <[email protected]>
> > ---
> > drivers/gpu/drm/drm_connector.c | 1 +
> > drivers/gpu/drm/drm_probe_helper.c | 12 ++++++++++++
> > drivers/gpu/drm/drm_sysfs.c | 1 +
> > include/drm/drm_connector.h | 3 +++
> > 4 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index 272d6254ea47..3c6628ee3096 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -274,6 +274,7 @@ static int drm_connector_init_only(struct drm_device
> > *dev,
> >
> > /* provide ddc symlink in sysfs */
> > connector->ddc = ddc;
> > + connector->status_changed = false;
> >
> > INIT_LIST_HEAD(&connector->head);
> > INIT_LIST_HEAD(&connector->global_connector_list_entry);
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 6b3541159c0f..761766181e99 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -628,6 +628,7 @@ int drm_helper_probe_single_connector_modes(struct
> > drm_connector *connector,
> > mod_delayed_work(system_wq,
> > &dev->mode_config.output_poll_work,
> > 0);
> > + connector->status_changed = true;
> > }
> >
> > /*
> > @@ -731,6 +732,15 @@ EXPORT_SYMBOL(drm_helper_probe_single_connector_modes);
> > */
> > void drm_kms_helper_hotplug_event(struct drm_device *dev)
> > {
> > + struct drm_connector *connector;
> > + struct drm_connector_list_iter conn_iter;
> > +
> > + drm_connector_list_iter_begin(dev, &conn_iter);
> > + drm_for_each_connector_iter(connector, &conn_iter) {
> > + connector->status_changed = false;
> > + }
> > + drm_connector_list_iter_end(&conn_iter);
> > +
> > drm_sysfs_hotplug_event(dev);
> > drm_client_dev_hotplug(dev);
> > }
> > @@ -747,6 +757,7 @@ void drm_kms_helper_connector_hotplug_event(struct
> > drm_connector *connector)
> > {
> > struct drm_device *dev = connector->dev;
> >
> > + connector->status_changed = false;
> > drm_sysfs_connector_hotplug_event(connector);
> > drm_client_dev_hotplug(dev);
>
>
> What would be the rule? Should it be unset before or after calling all
> the notifiers? Otherwise it's really strange. In the chunk below you set
> the flag, then it calls this function and the flags gets immediately
> unset.
Yeah, saw that was well. I've removed it entirely and just kept the unset bit
in drm_kms_helper_hotplug_event/drm_kms_helper_connector_hotplug_event.
>
> > }
> > @@ -1041,6 +1052,7 @@ bool drm_connector_helper_hpd_irq_event(struct
> > drm_connector *connector)
> > mutex_unlock(&dev->mode_config.mutex);
> >
> > if (changed) {
> > + connector->status_changed = true;
> > drm_kms_helper_connector_hotplug_event(connector);
> > drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Sent hotplug event\n",
> > connector->base.id,
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index 60c1f26edb6f..77f880654d6a 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -196,6 +196,7 @@ static ssize_t status_store(struct device *device,
> > return ret;
> >
> > old_force = connector->force;
> > + connector->status_changed = true;
> >
> > if (sysfs_streq(buf, "detect"))
> > connector->force = 0;
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 8f34f4b8183d..e4310df3d55c 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -2146,6 +2146,9 @@ struct drm_connector {
> > /** @force: a DRM_FORCE_<foo> state for forced mode sets */
> > enum drm_connector_force force;
> >
> > + /** @status_changed: if the old status doesn't match current connection
> > status */
> > + bool status_changed;
> > +
> > /**
> > * @edid_override: Override EDID set via debugfs.
> > *
> > --
> > 2.47.2
> >
>
> --
> With best wishes
> Dmitry
signature.asc
Description: PGP signature
