Re: [PATCH v2 1/2] drm: Introduce a new connector status

[Marius Vlad <marius.vlad@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5573 bytes --]

On Tue, Sep 23, 2025 at 11:42:42AM +0300, Marius Vlad wrote:
> 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/20250729165708.9947-1-marius.vlad@collabora.com/
Err, this one points to the correct version: https://lore.kernel.org/dri-devel/20250923083636.4749-1-marius.vlad@collabora.com/T/#t
> > 
> > > 
> > > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > > ---
> > >  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



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]