Re: [PATCH 1/5] drm/i915: Add sys PSR toggle interface

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> This interface allows an immediate enabling of PSR feature. What
> allow us
> to see immediately the PSR savings and will allow us to expose this
> through sysfs interface for powertop to leverage its functionality.
> 
> Signed-off-by: Alexandra Yates <alexandra.yates@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  1 +
>  drivers/gpu/drm/i915/i915_sysfs.c | 79
> +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_ddi.c  |  6 ++-
>  drivers/gpu/drm/i915/intel_dp.c   | 11 ++++--
>  drivers/gpu/drm/i915/intel_drv.h  |  4 +-
>  drivers/gpu/drm/i915/intel_psr.c  | 36 ++++++++++++++----
>  6 files changed, 122 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index f5c91b0..c96da4d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -976,6 +976,7 @@ struct i915_psr {
>  	bool psr2_support;
>  	bool aux_frame_sync;
>  	bool link_standby;
> +	bool sysfs_set;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index 2d576b7..208c6ec 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -106,12 +106,84 @@ show_media_rc6_ms(struct device *kdev, struct
> device_attribute *attr, char *buf)
>  	return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
>  }
>  
> +static ssize_t
> +show_psr(struct device *kdev, struct device_attribute *attr, char
> *buf)
> +{
> +	struct drm_minor *dminor = dev_to_drm_minor(kdev);
> +	struct drm_device *dev = dminor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	ssize_t ret;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->psr.enabled 
> ?
> +			"enabled":"disabled");
> +	mutex_unlock(&dev_priv->psr.lock);
> +	return ret;
> +}
> +
> +static ssize_t
> +toggle_psr(struct device *kdev, struct device_attribute *attr,
> +	const char *buf, size_t count)
> +{
> +	struct drm_minor *dminor = dev_to_drm_minor(kdev);
> +	struct drm_device *dev = dminor->dev;
> +	struct intel_connector *connector;
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *crtc = NULL;
> +	u32 val;
> +	ssize_t ret;
> +	struct intel_dp *intel_dp = NULL;
> +	bool sysfs_set = true;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	for_each_intel_connector(dev, connector) {
> +		if (!connector->base.encoder)
> +			continue;
> +		encoder = to_intel_encoder(connector->base.encoder);
> +		crtc = to_intel_crtc(encoder->base.crtc);
> +		intel_dp = enc_to_intel_dp(&encoder->base);
> +	}

Same problem here: no guarantee that the encoder is DP, nor that it
supports PSR, nor that it is enabled and has a mode set.

> +
> +	if (!crtc)
> +		return -ENODEV;
> +	switch (val) {
> +	case 0:
> +		ret = intel_psr_disable(intel_dp, sysfs_set);
> +		if (ret)
> +			return ret;
> +		break;
> +	case 1:
> +		ret = intel_psr_enable(intel_dp, sysfs_set);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +
> +	}
> +	return count;
> +}
> +
> +static DEVICE_ATTR(psr_enable, S_IRUGO | S_IWUSR, show_psr,
> toggle_psr);
>  static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL);
>  static DEVICE_ATTR(rc6_residency_ms, S_IRUGO, show_rc6_ms, NULL);
>  static DEVICE_ATTR(rc6p_residency_ms, S_IRUGO, show_rc6p_ms, NULL);
>  static DEVICE_ATTR(rc6pp_residency_ms, S_IRUGO, show_rc6pp_ms,
> NULL);
>  static DEVICE_ATTR(media_rc6_residency_ms, S_IRUGO,
> show_media_rc6_ms, NULL);
>  
> +static struct attribute *psr_attrs[] = {
> +	&dev_attr_psr_enable.attr,
> +	NULL
> +};
> +
> +static struct attribute_group psr_attr_group = {
> +	.name = power_group_name,
> +	.attrs = psr_attrs
> +};
> +
>  static struct attribute *rc6_attrs[] = {
>  	&dev_attr_rc6_enable.attr,
>  	&dev_attr_rc6_residency_ms.attr,
> @@ -596,6 +668,12 @@ void i915_setup_sysfs(struct drm_device *dev)
>  	int ret;
>  
>  #ifdef CONFIG_PM
> +	if (HAS_PSR(dev)) {
> +		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
> +					&psr_attr_group);
> +		if (ret)
> +			DRM_ERROR("PSR sysfs setup failed\n");
> +	}
>  	if (HAS_RC6(dev)) {
>  		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
>  					&rc6_attr_group);
> @@ -652,6 +730,7 @@ void i915_teardown_sysfs(struct drm_device *dev)
>  	device_remove_bin_file(dev->primary->kdev,  &dpf_attrs_1);
>  	device_remove_bin_file(dev->primary->kdev,  &dpf_attrs);
>  #ifdef CONFIG_PM
> +	sysfs_unmerge_group(&dev->primary->kdev->kobj,
> &psr_attr_group);
>  	sysfs_unmerge_group(&dev->primary->kdev->kobj,
> &rc6_attr_group);
>  	sysfs_unmerge_group(&dev->primary->kdev->kobj,
> &rc6p_attr_group);
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 921edf1..8e384e5 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1689,7 +1689,8 @@ static void intel_enable_ddi(struct
> intel_encoder *intel_encoder)
>  			intel_dp_stop_link_train(intel_dp);
>  
>  		intel_edp_backlight_on(intel_dp);
> -		intel_psr_enable(intel_dp);
> +		if (dev_priv->psr.sysfs_set != true)
> +			intel_psr_enable(intel_dp, dev_priv-
> >psr.sysfs_set);
>  		intel_edp_drrs_enable(intel_dp);
>  	}
>  
> @@ -1717,7 +1718,8 @@ static void intel_disable_ddi(struct
> intel_encoder *intel_encoder)
>  		struct intel_dp *intel_dp =
> enc_to_intel_dp(encoder);
>  
>  		intel_edp_drrs_disable(intel_dp);
> -		intel_psr_disable(intel_dp);
> +		if (dev_priv->psr.sysfs_set != true)
> +			intel_psr_disable(intel_dp, dev_priv-
> >psr.sysfs_set);
>  		intel_edp_backlight_off(intel_dp);
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 7523558..183a60a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2421,13 +2421,16 @@ static void intel_disable_dp(struct
> intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  
>  	if (crtc->config->has_audio)
>  		intel_audio_codec_disable(encoder);
>  
> -	if (HAS_PSR(dev) && !HAS_DDI(dev))
> -		intel_psr_disable(intel_dp);
> +	if (HAS_PSR(dev) && !HAS_DDI(dev)) {
> +		if (dev_priv->psr.sysfs_set != true)
> +			intel_psr_disable(intel_dp, dev_priv-
> >psr.sysfs_set);
> +	}
>  
>  	/* Make sure the panel is off before trying to change the
> mode. But also
>  	 * ensure that we have vdd while we switch off the panel. */
> @@ -2689,9 +2692,11 @@ static void g4x_enable_dp(struct intel_encoder
> *encoder)
>  static void vlv_enable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
>  
>  	intel_edp_backlight_on(intel_dp);
> -	intel_psr_enable(intel_dp);
> +	if (dev_priv->psr.sysfs_set != true)
> +		intel_psr_enable(intel_dp, dev_priv->psr.sysfs_set);
>  }
>  
>  static void g4x_pre_enable_dp(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index e0fcfa1..199e8cc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1446,8 +1446,8 @@ void intel_backlight_unregister(struct
> drm_device *dev);
>  
>  
>  /* intel_psr.c */
> -void intel_psr_enable(struct intel_dp *intel_dp);
> -void intel_psr_disable(struct intel_dp *intel_dp);
> +int intel_psr_enable(struct intel_dp *intel_dp, bool sysfs_set);
> +int intel_psr_disable(struct intel_dp *intel_dp, bool sysfs_set);
>  void intel_psr_invalidate(struct drm_device *dev,
>  			  unsigned frontbuffer_bits);
>  void intel_psr_flush(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index c3abae4..64cb714 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -378,34 +378,43 @@ static void intel_psr_activate(struct intel_dp
> *intel_dp)
>  /**
>   * intel_psr_enable - Enable PSR
>   * @intel_dp: Intel DP
> + * @sysfs_set: Identifies if this feature is set from sysfs
>   *
>   * This function can only be called after the pipe is fully trained
> and enabled.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise.
>   */
> -void intel_psr_enable(struct intel_dp *intel_dp)
> +int intel_psr_enable(struct intel_dp *intel_dp, bool sysfs_set)
>  {
>  	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> >base.base.crtc);
> +	int ret = 0;
> +
>  
>  	if (!HAS_PSR(dev)) {
>  		DRM_DEBUG_KMS("PSR not supported on this
> platform\n");
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	if (!is_edp_psr(intel_dp)) {
>  		DRM_DEBUG_KMS("PSR not supported by this panel\n");
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (dev_priv->psr.enabled) {
>  		DRM_DEBUG_KMS("PSR already in use\n");
> +		ret = -EALREADY;
>  		goto unlock;
>  	}
>  
> -	if (!intel_psr_match_conditions(intel_dp))
> +	if (!intel_psr_match_conditions(intel_dp)) {
> +		ret = -ENOTTY;
>  		goto unlock;
> +	}
>  
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  
> @@ -464,8 +473,12 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  				      msecs_to_jiffies(intel_dp-
> >panel_power_cycle_delay * 5));
>  
>  	dev_priv->psr.enabled = intel_dp;
> +	if (sysfs_set)
> +		dev_priv->psr.sysfs_set = sysfs_set;
> +
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
> +	return ret;
>  }
>  
>  static void vlv_psr_disable(struct intel_dp *intel_dp)
> @@ -520,10 +533,11 @@ static void hsw_psr_disable(struct intel_dp
> *intel_dp)
>  /**
>   * intel_psr_disable - Disable PSR
>   * @intel_dp: Intel DP
> + * @sysfs_set: Identifies if this feature is set from sysfs
>   *
>   * This function needs to be called before disabling pipe.
>   */
> -void intel_psr_disable(struct intel_dp *intel_dp)
> +int intel_psr_disable(struct intel_dp *intel_dp, bool sysfs_set)
>  {
>  	struct intel_digital_port *intel_dig_port =
> dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
> @@ -532,7 +546,7 @@ void intel_psr_disable(struct intel_dp *intel_dp)
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (!dev_priv->psr.enabled) {
>  		mutex_unlock(&dev_priv->psr.lock);
> -		return;
> +		return -EALREADY;
>  	}
>  
>  	/* Disable PSR on Source */
> @@ -545,9 +559,13 @@ void intel_psr_disable(struct intel_dp
> *intel_dp)
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
>  
>  	dev_priv->psr.enabled = NULL;
> +	if (sysfs_set)
> +		dev_priv->psr.sysfs_set = sysfs_set;
> +
>  	mutex_unlock(&dev_priv->psr.lock);
>  
>  	cancel_delayed_work_sync(&dev_priv->psr.work);
> +	return 0;
>  }
>  
>  static void intel_psr_work(struct work_struct *work)
> @@ -781,8 +799,10 @@ void intel_psr_init(struct drm_device *dev)
>  
>  	/* Per platform default */
>  	if (i915.enable_psr == -1) {
> -		if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -			i915.enable_psr = 1;
> +		if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +			if (dev_priv->psr.sysfs_set != true)
> +				i915.enable_psr = 1;
> +		}
>  		else
>  			i915.enable_psr = 0;
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx