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@xxxxxxxxxxx>
> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Karol Herbst <kherbst@xxxxxxxxxx>
> ---
> 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
