Nouveau - Jun 2023 - [PATCH] drm/nouveau/disp: use drm_kms_helper_connector_hotplug_event()

Looks alright! Some comments below

On Tue, 2023-06-20 at 18:15 +0000, Simon Ser wrote:
> This adds more information to the hotplug uevent and lets user-space
> know that it's about a particular connector only.
> 
> Signed-off-by: Simon Ser <contact at emersion.fr>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Karol Herbst <kherbst at redhat.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_display.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
> index ec3ffff487fc..99977e5fe716 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -465,7 +465,8 @@ nouveau_display_hpd_work(struct work_struct *work)
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	u32 pending;
> -	bool changed = false;
> +	int changed = 0;
> +	struct drm_connector *first_changed_connector = NULL;
>  
>  	pm_runtime_get_sync(dev->dev);
>  
> @@ -509,7 +510,12 @@ nouveau_display_hpd_work(struct work_struct *work)
>  		if (old_epoch_counter == connector->epoch_counter)
>  			continue;
>  
> -		changed = true;
> +		changed++;
> +		if (!first_changed_connector) {
> +			drm_connector_get(connector);
> +			first_changed_connector = connector;
> +		}
> +
>  		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] status updated from %s to %s
(epoch counter %llu->%llu)\n",
>  			    connector->base.id, connector->name,
>  			    drm_get_connector_status_name(old_status),
> @@ -520,9 +526,14 @@ nouveau_display_hpd_work(struct work_struct *work)
>  	drm_connector_list_iter_end(&conn_iter);
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> -	if (changed)
> +	if (changed == 1)
> +		drm_kms_helper_connector_hotplug_event(first_changed_connector);
> +	else if (changed > 0)
>  		drm_kms_helper_hotplug_event(dev);
I'm curious if you think there might be an advantage to doing this per-
connector even with multiple connectors? Seems like we could do that if we
stored changed connectors as a bitmask.
>  
> +	if (first_changed_connector)
> +		drm_connector_put(first_changed_connector);
> +
>  	pm_runtime_mark_last_busy(drm->dev->dev);
>  noop:
>  	pm_runtime_put_autosuspend(dev->dev);
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Hi Lyude!

On Wednesday, June 21st, 2023 at 23:56, Lyude Paul <lyude at redhat.com>
wrote:
> > -	if (changed)
> > +	if (changed == 1)
> > +		drm_kms_helper_connector_hotplug_event(first_changed_connector);
> > +	else if (changed > 0)
> >  		drm_kms_helper_hotplug_event(dev);
> 
> I'm curious if you think there might be an advantage to doing this per-
> connector even with multiple connectors? Seems like we could do that if we
> stored changed connectors as a bitmask.
I believe firing off multiple per-connector uevents would be worse than a
single per-device uevent because user-space would see intermediate states
instead of an atomic change.

Let's say I have 3 connectors connected, and different settings set in my
compositor when 1, 2 or 3 screens are connected. Let's say an unplug
disconnects 2 monitors at the same time. With a single uevent, the compositor
refreshes the whole device state, and then applies the 1-screen settings. With
2 uevents (one for each connector), the compositor refreshes the first
disconnected connector state, then applies the 2-screen settings, then
refreshes the second disconnected connector state, then applies the 1-screen
settings. Applying the 2-screen settings might cause an unnecessarily glitchy
intermediate state, where windows move around, modesets are performed, user
configured commands are run, just to un-do everything right after.

I hope this makes sense,

Simon