Re: [PATCH v8 2/2] drm: Send per-connector hotplug events

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 22, 2026 at 02:35:32PM +0200, Nicolas Frattaroli wrote:
> Try to send per-connector hotplug events as often as possible, rather
> than connector-less global hotplug events. This does result in more
> hotplug events if multiple connectors changed at the same time, but
> give userspace more actionable information.
> 
> Since the hotplug event needs to be sent outside of the mode_config
> mutex to avoid a deadlock, pointers to all the changed connectors are
> stored in a newly allocated array.
> 
> The "changed" boolean in output_poll_execute now only serves to signal
> that a non-connector-specific hotplug event needs to be sent from a
> prior event that was delayed. So, rename it from "changed" to
> "delayed_hp" to avoid any confusion.
> 
> Co-developed-by: Marius Vlad <marius.vlad@collabora.com>
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 68 ++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index d4dc8cb45bce..3beed8aa32fe 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -757,17 +757,19 @@ static void output_poll_execute(struct work_struct *work)
>  {
>  	struct delayed_work *delayed_work = to_delayed_work(work);
>  	struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
> +	struct drm_connector **changed_conns;
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	enum drm_connector_status old_status;
> -	bool repoll = false, changed;
> +	unsigned int num_changed = 0, i;
> +	bool repoll = false, delayed_hp;
>  	u64 old_epoch_counter;
>  
>  	if (!dev->mode_config.poll_enabled)
>  		return;
>  
>  	/* Pick up any changes detected by the probe functions. */
> -	changed = dev->mode_config.delayed_event;
> +	delayed_hp = dev->mode_config.delayed_event;
>  	dev->mode_config.delayed_event = false;
>  
>  	if (!drm_kms_helper_poll) {
> @@ -783,6 +785,13 @@ static void output_poll_execute(struct work_struct *work)
>  		goto out;
>  	}
>  
> +	changed_conns = kmalloc_array(dev->mode_config.num_connector,
> +				sizeof(*changed_conns), GFP_KERNEL);
> +	if (!changed_conns) {
> +		repoll = true;
> +		goto out;
> +	}
> +
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	drm_for_each_connector_iter(connector, &conn_iter) {
>  		/* Ignore forced connectors. */
> @@ -836,15 +845,23 @@ static void output_poll_execute(struct work_struct *work)
>  				    connector->base.id, connector->name,
>  				    old_epoch_counter, connector->epoch_counter);
>  
> -			changed = true;
> +			drm_connector_get(connector);
> +			changed_conns[num_changed++] = connector;
>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
>  
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> +	for (i = 0; i < num_changed; i++) {
> +		drm_kms_helper_connector_hotplug_event(changed_conns[i]);
> +		drm_connector_put(changed_conns[i]);
> +	}
> +
> +	kfree(changed_conns);
> +
>  out:
> -	if (changed)
> +	if (delayed_hp)
>  		drm_kms_helper_hotplug_event(dev);
>  
>  	if (repoll)
> @@ -1081,39 +1098,40 @@ EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event);
>   */
>  bool drm_helper_hpd_irq_event(struct drm_device *dev)
>  {
> -	struct drm_connector *connector, *first_changed_connector = NULL;
>  	struct drm_connector_list_iter conn_iter;
> -	int changed = 0;
> +	struct drm_connector **changed_conns;
> +	struct drm_connector *connector;
> +	unsigned int changed = 0, i;
>  
>  	if (!dev->mode_config.poll_enabled)
>  		return false;
>  
> -	mutex_lock(&dev->mode_config.mutex);
> -	drm_connector_list_iter_begin(dev, &conn_iter);
> -	drm_for_each_connector_iter(connector, &conn_iter) {
> -		/* Only handle HPD capable connectors. */
> -		if (!(connector->polled & DRM_CONNECTOR_POLL_HPD))
> -			continue;
> +	scoped_guard(mutex, &dev->mode_config.mutex) {
> +		changed_conns = kmalloc_array(dev->mode_config.num_connector,
> +					sizeof(*changed_conns), GFP_KERNEL);
> +		if (!changed_conns)
> +			return false;

I'm thinking you could avoid all this kmalloc_array() complication
by doing the drm_sysfs_connector_hotplug_event() calls directly
from the loop, and only defer the drm_client_dev_hotplug() to the
end (to avoid locking issues).

And ideally I think drm_client_dev_hotplug() should even queue its
own work and do things asynchronously, but that's a much bigger
change and would need more thought.

>  
> -		if (check_connector_changed(connector)) {
> -			if (!first_changed_connector) {
> +		drm_connector_list_iter_begin(dev, &conn_iter);
> +		drm_for_each_connector_iter(connector, &conn_iter) {
> +			/* Only handle HPD capable connectors. */
> +			if (!(connector->polled & DRM_CONNECTOR_POLL_HPD))
> +				continue;
> +
> +			if (check_connector_changed(connector)) {
>  				drm_connector_get(connector);
> -				first_changed_connector = connector;
> +				changed_conns[changed++] = connector;
>  			}
> -
> -			changed++;
>  		}
> +		drm_connector_list_iter_end(&conn_iter);
>  	}
> -	drm_connector_list_iter_end(&conn_iter);
> -	mutex_unlock(&dev->mode_config.mutex);
>  
> -	if (changed == 1)
> -		drm_kms_helper_connector_hotplug_event(first_changed_connector);
> -	else if (changed > 0)
> -		drm_kms_helper_hotplug_event(dev);
> +	for (i = 0; i < changed; i++) {
> +		drm_kms_helper_connector_hotplug_event(changed_conns[i]);
> +		drm_connector_put(changed_conns[i]);
> +	}
>  
> -	if (first_changed_connector)
> -		drm_connector_put(first_changed_connector);
> +	kfree(changed_conns);
>  
>  	return changed;
>  }
> 
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel