Re: [PATCH v2] drm/i915/debugfs: Add i915_hpd_storm_ctl

[Jani Nikula]

On Thu, 09 Feb 2017, Lyude <lyude@xxxxxxxxxx> wrote:
> This adds a file in i915's debugfs directory that allows userspace to
> manually control HPD storm detection. This is mainly for hotplugging
> tests, where we might want to test HPD storm functionality or disable
> storm detection to speed up hotplugging tests without breaking anything.
>
> Changes since v1:
> - Make HPD storm interval configurable
> - Misc code cleanup
>
> Signed-off-by: Lyude <lyude@xxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Tomeu Vizoso <tomeu@xxxxxxxxxxxxxxx>

Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx>


> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 78 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_drv.c      |  1 +
>  drivers/gpu/drm/i915/i915_drv.h      |  4 ++
>  drivers/gpu/drm/i915/i915_irq.c      |  2 +
>  drivers/gpu/drm/i915/intel_hotplug.c | 15 ++++---
>  5 files changed, 94 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1ccc297..b55547d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4641,6 +4641,81 @@ static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
>  	return drm_add_fake_info_node(minor, ent, &i915_forcewake_fops);
>  }
>  
> +static int i915_hpd_storm_ctl_show(struct seq_file *m, void *data)
> +{
> +	struct drm_i915_private *dev_priv = m->private;
> +	struct i915_hotplug *hotplug = &dev_priv->hotplug;
> +
> +	seq_printf(m, "Threshold: %d\n", hotplug->hpd_storm_threshold);
> +	seq_printf(m, "Detected: %s\n",
> +		   yesno(delayed_work_pending(&hotplug->reenable_work)));
> +
> +	return 0;
> +}
> +
> +static ssize_t i915_hpd_storm_ctl_write(struct file *file,
> +					const char __user *ubuf, size_t len,
> +					loff_t *offp)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct drm_i915_private *dev_priv = m->private;
> +	struct i915_hotplug *hotplug = &dev_priv->hotplug;
> +	unsigned int new_threshold;
> +	int i;
> +	char *newline;
> +	char tmp[16];
> +
> +	if (len >= sizeof(tmp))
> +		return -EINVAL;
> +
> +	if (copy_from_user(tmp, ubuf, len))
> +		return -EFAULT;
> +
> +	tmp[len] = '\0';
> +
> +	/* Strip newline, if any */
> +	newline = strchr(tmp, '\n');
> +	if (newline)
> +		*newline = '\0';
> +
> +	if (strcmp(tmp, "reset") == 0)
> +		new_threshold = HPD_STORM_DEFAULT_THRESHOLD;
> +	else if (kstrtouint(tmp, 10, &new_threshold) != 0)
> +		return -EINVAL;
> +
> +	if (new_threshold > 0)
> +		DRM_DEBUG_KMS("Setting HPD storm detection threshold to %d\n",
> +			      new_threshold);
> +	else
> +		DRM_DEBUG_KMS("Disabling HPD storm detection\n");
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	hotplug->hpd_storm_threshold = new_threshold;
> +	/* Reset the HPD storm stats so we don't accidentally trigger a storm */
> +	for_each_hpd_pin(i)
> +		hotplug->stats[i].count = 0;
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	/* Re-enable hpd immediately if we were in an irq storm */
> +	flush_delayed_work(&dev_priv->hotplug.reenable_work);
> +
> +	return len;
> +}
> +
> +static int i915_hpd_storm_ctl_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, i915_hpd_storm_ctl_show, inode->i_private);
> +}
> +
> +static const struct file_operations i915_hpd_storm_ctl_fops = {
> +	.owner = THIS_MODULE,
> +	.open = i915_hpd_storm_ctl_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +	.write = i915_hpd_storm_ctl_write
> +};
> +
>  static int i915_debugfs_create(struct dentry *root,
>  			       struct drm_minor *minor,
>  			       const char *name,
> @@ -4734,7 +4809,8 @@ static const struct i915_debugfs_files {
>  	{"i915_dp_test_data", &i915_displayport_test_data_fops},
>  	{"i915_dp_test_type", &i915_displayport_test_type_fops},
>  	{"i915_dp_test_active", &i915_displayport_test_active_fops},
> -	{"i915_guc_log_control", &i915_guc_log_control_fops}
> +	{"i915_guc_log_control", &i915_guc_log_control_fops},
> +	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops}
>  };
>  
>  int i915_debugfs_register(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d48c02a7..f23b568 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -827,6 +827,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>  	spin_lock_init(&dev_priv->gpu_error.lock);
>  	mutex_init(&dev_priv->backlight_lock);
>  	spin_lock_init(&dev_priv->uncore.lock);
> +
>  	spin_lock_init(&dev_priv->mm.object_stat_lock);
>  	spin_lock_init(&dev_priv->mmio_flip_lock);
>  	spin_lock_init(&dev_priv->wm.dsparb_lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 604b20f..2b7b317 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -384,6 +384,8 @@ enum hpd_pin {
>  #define for_each_hpd_pin(__pin) \
>  	for ((__pin) = (HPD_NONE + 1); (__pin) < HPD_NUM_PINS; (__pin)++)
>  
> +#define HPD_STORM_DEFAULT_THRESHOLD 5
> +
>  struct i915_hotplug {
>  	struct work_struct hotplug_work;
>  
> @@ -407,6 +409,8 @@ struct i915_hotplug {
>  	struct work_struct poll_init_work;
>  	bool poll_enabled;
>  
> +	unsigned int hpd_storm_threshold;
> +
>  	/*
>  	 * if we get a HPD irq from DP and a HPD irq from non-DP
>  	 * the non-DP HPD could block the workqueue on a mode config
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9eb4c9c..27c13193 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4277,6 +4277,8 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	if (!IS_GEN2(dev_priv))
>  		dev->vblank_disable_immediate = true;
>  
> +	dev_priv->hotplug.hpd_storm_threshold = HPD_STORM_DEFAULT_THRESHOLD;
> +
>  	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
>  	dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
>  
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index b62e3f8..6a9c165 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -100,7 +100,6 @@ bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port)
>  }
>  
>  #define HPD_STORM_DETECT_PERIOD		1000
> -#define HPD_STORM_THRESHOLD		5
>  #define HPD_STORM_REENABLE_DELAY	(2 * 60 * 1000)
>  
>  /**
> @@ -112,9 +111,13 @@ bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port)
>   * storms. Only the pin specific stats and state are changed, the caller is
>   * responsible for further action.
>   *
> - * @HPD_STORM_THRESHOLD irqs are allowed within @HPD_STORM_DETECT_PERIOD ms,
> - * otherwise it's considered an irq storm, and the irq state is set to
> - * @HPD_MARK_DISABLED.
> + * The number of irqs that are allowed within @HPD_STORM_DETECT_PERIOD is
> + * stored in @dev_priv->hotplug.hpd_storm_threshold which defaults to
> + * @HPD_STORM_DEFAULT_THRESHOLD. If this threshold is exceeded, it's
> + * considered an irq storm and the irq state is set to @HPD_MARK_DISABLED.
> + *
> + * The HPD threshold can be controlled through i915_hpd_storm_ctl in debugfs,
> + * and should only be adjusted for automated hotplug testing.
>   *
>   * Return true if an irq storm was detected on @pin.
>   */
> @@ -123,13 +126,15 @@ static bool intel_hpd_irq_storm_detect(struct drm_i915_private *dev_priv,
>  {
>  	unsigned long start = dev_priv->hotplug.stats[pin].last_jiffies;
>  	unsigned long end = start + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD);
> +	const int threshold = dev_priv->hotplug.hpd_storm_threshold;
>  	bool storm = false;
>  
>  	if (!time_in_range(jiffies, start, end)) {
>  		dev_priv->hotplug.stats[pin].last_jiffies = jiffies;
>  		dev_priv->hotplug.stats[pin].count = 0;
>  		DRM_DEBUG_KMS("Received HPD interrupt on PIN %d - cnt: 0\n", pin);
> -	} else if (dev_priv->hotplug.stats[pin].count > HPD_STORM_THRESHOLD) {
> +	} else if (dev_priv->hotplug.stats[pin].count > threshold &&
> +		   threshold) {
>  		dev_priv->hotplug.stats[pin].state = HPD_MARK_DISABLED;
>  		DRM_DEBUG_KMS("HPD interrupt storm detected on PIN %d\n", pin);
>  		storm = true;

-- 
Jani Nikula, Intel Open Source Technology Center