[PATCH v2 1/2] drm/probe-helper: Create a HPD IRQ event helper for a single connector

[Sam Ravnborg]

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 at cerno.tech>

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
>