Hi Maxime, thanks for the ping on irc. I had accidently deleted v2 of this patch. Sam On Mon, Aug 30, 2021 at 11:49:09AM +0200, Maxime Ripard wrote: > The drm_helper_hpd_irq_event() function is iterating over all the > connectors when an hotplug event is detected. > > During that iteration, it will call each connector detect function and > figure out if its status changed. > > Finally, if any connector changed, it will notify the user-space and the > clients that something changed on the DRM device. > > This is supposed to be used for drivers that don't have a hotplug > interrupt for individual connectors. However, drivers that can use an > interrupt for a single connector are left in the dust and can either > reimplement the logic used during the iteration for each connector or > use that helper and iterate over all connectors all the time. > > Since both are suboptimal, let's create a helper that will only perform > the status detection on a single connector. > > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx> A few comments in the following. Sam > > --- > > Changes from v1: > - Rename the shared function > - Move the hotplug event notification out of the shared function > - Added missing locks > - Improve the documentation > - Switched to drm_dbg_kms > --- > drivers/gpu/drm/drm_probe_helper.c | 120 ++++++++++++++++++++--------- > include/drm/drm_probe_helper.h | 1 + > 2 files changed, 86 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 5606bca3caa8..fcf32ec0b0c8 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -795,6 +795,86 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_kms_helper_poll_fini); > > +static bool check_connector_changed(struct drm_connector *connector) > +{ > + struct drm_device *dev = connector->dev; > + enum drm_connector_status old_status; > + u64 old_epoch_counter; > + bool changed = false; > + > + /* Only handle HPD capable connectors. */ > + drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD)); > + > + drm_WARN_ON(dev, !mutex_is_locked(&dev->mode_config.mutex)); > + > + old_status = connector->status; > + old_epoch_counter = connector->epoch_counter; > + > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Old epoch counter %llu\n", > + connector->base.id, > + connector->name, > + old_epoch_counter); > + > + connector->status = drm_helper_probe_detect(connector, NULL, false); > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] status updated from %s to %s\n", > + connector->base.id, > + connector->name, > + drm_get_connector_status_name(old_status), > + drm_get_connector_status_name(connector->status)); > + > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] New epoch counter %llu\n", > + connector->base.id, > + connector->name, > + connector->epoch_counter); > + > + /* > + * Check if epoch counter had changed, meaning that we need > + * to send a uevent. > + */ > + if (old_epoch_counter != connector->epoch_counter) > + changed = true; If logging is included here we could do it like: if (old_epoch_counter != connector->epoch_counter) { drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Changed epoch counter %llu => %llu\n", connector->base.id, connector->name, old_epoch_counter, connector->epoch_counter); return true; } else { drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Same epoch counter %llu\n", connector->base.id, connector->name, connector->epoch_counter); return false; } } So we only log epoch counter once. And the changed local variable is gone. Just a suggestion. > + > + return changed; > +} > + > +/** > + * drm_connector_helper_hpd_irq_event - hotplug processing > + * @connector: drm_connector > + * > + * Drivers can use this helper function to run a detect cycle on a connector > + * which has the DRM_CONNECTOR_POLL_HPD flag set in its &polled member. > + * > + * This helper function is useful for drivers which can track hotplug > + * interrupts for a single connector. Drivers that want to send a > + * hotplug event for all connectors or can't track hotplug interrupts > + * per connector need to use drm_helper_hpd_irq_event(). > + * > + * This function must be called from process context with no mode > + * setting locks held. > + * > + * Note that a connector can be both polled and probed from the hotplug > + * handler, in case the hotplug interrupt is known to be unreliable. Documentation of the return value would be nice. > + */ > +bool drm_connector_helper_hpd_irq_event(struct drm_connector *connector) > +{ > + struct drm_device *dev = connector->dev; > + bool changed; > + > + mutex_lock(&dev->mode_config.mutex); > + changed = check_connector_changed(connector); > + mutex_unlock(&dev->mode_config.mutex); > + > + if (changed) { > + drm_kms_helper_hotplug_event(dev); > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] Sent hotplug event\n", > + connector->base.id, > + connector->name); > + } > + > + return changed; > +} > +EXPORT_SYMBOL(drm_connector_helper_hpd_irq_event); > + > /** > * drm_helper_hpd_irq_event - hotplug processing > * @dev: drm_device > @@ -808,9 +888,10 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini); > * interrupts for each connector. > * > * Drivers which support hotplug interrupts for each connector individually and > - * which have a more fine-grained detect logic should bypass this code and > - * directly call drm_kms_helper_hotplug_event() in case the connector state > - * changed. > + * which have a more fine-grained detect logic can use > + * drm_connector_helper_hpd_irq_event(). Alternatively, they should bypass this > + * code and directly call drm_kms_helper_hotplug_event() in case the connector > + * state changed. > * > * This function must be called from process context with no mode > * setting locks held. Documentation of the return value would also be nice here. There are a few uses of the return value so we cannot just ignore it. > @@ -822,9 +903,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) > { > struct drm_connector *connector; > struct drm_connector_list_iter conn_iter; > - enum drm_connector_status old_status; > bool changed = false; > - u64 old_epoch_counter; > > if (!dev->mode_config.poll_enabled) > return false; > @@ -832,37 +911,8 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev) > 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; The old code skips connectors without DRM_CONNECTOR_POLL_HPD, where the new code will WARN is DRM_CONNECTOR_POLL_HPD is not set. This seems like a bigger change than what is intended. > - > - old_status = connector->status; > - > - old_epoch_counter = connector->epoch_counter; > - > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old epoch counter %llu\n", connector->base.id, > - connector->name, > - old_epoch_counter); > - > - connector->status = drm_helper_probe_detect(connector, NULL, false); > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", > - connector->base.id, > - connector->name, > - drm_get_connector_status_name(old_status), > - drm_get_connector_status_name(connector->status)); > - > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New epoch counter %llu\n", > - connector->base.id, > - connector->name, > - connector->epoch_counter); > - > - /* > - * Check if epoch counter had changed, meaning that we need > - * to send a uevent. > - */ > - if (old_epoch_counter != connector->epoch_counter) > + if (check_connector_changed(connector)) > changed = true; > - > } > drm_connector_list_iter_end(&conn_iter); > mutex_unlock(&dev->mode_config.mutex); > diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h > index 8d3ed2834d34..04c57564c397 100644 > --- a/include/drm/drm_probe_helper.h > +++ b/include/drm/drm_probe_helper.h > @@ -18,6 +18,7 @@ int drm_helper_probe_detect(struct drm_connector *connector, > void drm_kms_helper_poll_init(struct drm_device *dev); > void drm_kms_helper_poll_fini(struct drm_device *dev); > bool drm_helper_hpd_irq_event(struct drm_device *dev); > +bool drm_connector_helper_hpd_irq_event(struct drm_connector *connector); > void drm_kms_helper_hotplug_event(struct drm_device *dev); > > void drm_kms_helper_poll_disable(struct drm_device *dev); > -- > 2.31.1 >