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

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

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

On Tue, Nov 11, 2025 at 09:13:06PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 11, 2025 at 08:54:59PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 11, 2025 at 06:23:35PM +0200, Marius Vlad wrote:
> > > Introduce 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 change would make use of this connector status to propagate
> > > per-connector udev hotplug events.
> > > 
> > > The connector status is set in the drm_connector_funcs.fill_modes/vkms
> > > ConfigFS connector's status and cleared out when firing out KMS uevents.
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > > ---
> > >  drivers/gpu/drm/drm_connector.c      |  1 +
> > >  drivers/gpu/drm/drm_probe_helper.c   | 17 +++++++++++++++++
> > >  drivers/gpu/drm/drm_sysfs.c          |  1 +
> > >  drivers/gpu/drm/vkms/vkms_configfs.c |  6 ++++++
> > >  include/drm/drm_connector.h          |  3 +++
> > >  5 files changed, 28 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 09b12c30df69..f0474368e98d 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -629,6 +629,8 @@ 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;
> > 
> > .fill_modes() gets exectued from the getconnector() ioctl which userspace
> > issues in response to the uevent. Not the other way around. So looks
> > like you have the chicken and egg the wrong way around here.
> 
> Oh, and we already have a connector->epoch_counter which is supposed
> track whether anything on the connector has changed. So I'm not sure
> what extra this new boolean is supposed to achieve on top of that?
epoch_counter doesn't take all cases into consideration. For the
instance the connector list loop in output_poll_execute will be a no-op
if we try to simulate a hot-plug event with drm_sysfs. I suppose we
could've have used it for (real) HW hot-plug.

There's a few use-cases:

- 2 with real HW hot-plug - one for polling and one for HPD 
- 2 for with virtual connectors - one for VKMS and one for drm_sysfs

The bool helps to know -- in case you have multiple connectors -- which
of them had a hot-plug event.
> 
> > >  	}
> > >  
> > >  	/*
> > > @@ -732,6 +734,17 @@ 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;
> > > +
> > > +	mutex_lock(&dev->mode_config.mutex);
> > > +	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);
> > > +	mutex_unlock(&dev->mode_config.mutex);
> > > +
> > >  	drm_sysfs_hotplug_event(dev);
> > >  	drm_client_dev_hotplug(dev);
> > >  }
> > > @@ -748,6 +761,10 @@ void drm_kms_helper_connector_hotplug_event(struct drm_connector *connector)
> > >  {
> > >  	struct drm_device *dev = connector->dev;
> > >  
> > > +	mutex_lock(&dev->mode_config.mutex);
> > > +	connector->status_changed = false;
> > > +	mutex_unlock(&dev->mode_config.mutex);
> > > +
> > >  	drm_sysfs_connector_hotplug_event(connector);
> > >  	drm_client_dev_hotplug(dev);
> > >  }
> > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > > index b01ffa4d6509..bd9161490116 100644
> > > --- a/drivers/gpu/drm/drm_sysfs.c
> > > +++ b/drivers/gpu/drm/drm_sysfs.c
> > > @@ -199,6 +199,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/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > index 506666e21c91..6d6dd1a2c3a6 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > > @@ -537,8 +537,14 @@ static ssize_t connector_status_store(struct config_item *item,
> > >  {
> > >  	struct vkms_configfs_connector *connector;
> > >  	enum drm_connector_status status;
> > > +	struct vkms_connector *vkms_connector;
> > > +	struct vkms_device *vkms_dev;
> > >  
> > >  	connector = connector_item_to_vkms_configfs_connector(item);
> > > +	vkms_connector = connector->config->connector;
> > > +	vkms_dev = connector->config->config->dev;
> > > +	scoped_guard(mutex, &vkms_dev->drm.mode_config.mutex)
> > > +		vkms_connector->base.status_changed = true;
> > >  
> > >  	if (kstrtouint(page, 10, &status))
> > >  		return -EINVAL;
> > > 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Ville Syrjälä
> Intel

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