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

Submitted by Alexandra Yates on April 12, 2016, 7:18 p.m.

Details

Message ID 1460488728-23319-2-git-send-email-alexandra.yates@linux.intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Alexandra Yates April 12, 2016, 7:18 p.m.
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(-)

Patch hide | download patch | download mbox

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);
+	}
+
+	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;
 	}

Comments

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;

>  	}
On Wed, 2016-04-13 at 13:26 +0000, Zanoni, Paulo R wrote:
> 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.


I agree. Also we have an issue with new platforms if we end up having
any laptop with 2 eDPs supporting PSR things could get crazy.

So we could actually create a 
struct intel_dp *edp_with_psr_support; inside psr structure that we set
in the moment we find the first eDP panel that supports PSR
and change the enabled from intel_dp pointer to a boolean.

> 

> > +

> > +	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;
On Wed, 13 Apr 2016, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote:
> On Wed, 2016-04-13 at 13:26 +0000, Zanoni, Paulo R wrote:
>> 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.
>
> I agree. Also we have an issue with new platforms if we end up having
> any laptop with 2 eDPs supporting PSR things could get crazy.
>
> So we could actually create a 
> struct intel_dp *edp_with_psr_support; inside psr structure that we set
> in the moment we find the first eDP panel that supports PSR
> and change the enabled from intel_dp pointer to a boolean.

It seems to me at that point the sane thing to do is to move all
sink/encoder specific stuff to struct intel_dp, and only leave common
source specific stuff to dev_priv->psr.

It might make sense to move towards that direction already now, like
we've done for all backlight related stuff. All the backlight stuff is
under connector->panel even though we globally only support one
connector with backlight.

BR,
Jani.

>
>> 
>> > +
>> > +	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