* [PATCH 1/5] drm/i915: Add sys PSR toggle interface
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
@ 2016-04-12 19:18 ` Alexandra Yates
2016-04-13 13:26 ` Zanoni, Paulo R
2016-04-12 19:18 ` [PATCH 2/5] drm/i915: Add sys FBC " Alexandra Yates
` (6 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Alexandra Yates @ 2016-04-12 19:18 UTC (permalink / raw)
To: intel-gfx, rodrigo.vivi, nivedita.swaminathan, joe.konno; +Cc: Alexandra Yates
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);
+ }
+
+ 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;
}
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/5] drm/i915: Add sys PSR toggle interface
2016-04-12 19:18 ` [PATCH 1/5] drm/i915: Add sys PSR toggle interface Alexandra Yates
@ 2016-04-13 13:26 ` Zanoni, Paulo R
2016-04-13 20:43 ` Vivi, Rodrigo
0 siblings, 1 reply; 23+ messages in thread
From: Zanoni, Paulo R @ 2016-04-13 13:26 UTC (permalink / raw)
To: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo,
alexandra.yates@linux.intel.com, Konno, Joe,
Swaminathan, Nivedita
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/5] drm/i915: Add sys PSR toggle interface
2016-04-13 13:26 ` Zanoni, Paulo R
@ 2016-04-13 20:43 ` Vivi, Rodrigo
2016-04-14 7:58 ` Jani Nikula
0 siblings, 1 reply; 23+ messages in thread
From: Vivi, Rodrigo @ 2016-04-13 20:43 UTC (permalink / raw)
To: intel-gfx@lists.freedesktop.org, alexandra.yates@linux.intel.com,
Konno, Joe, Zanoni, Paulo R, Swaminathan, Nivedita
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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/5] drm/i915: Add sys PSR toggle interface
2016-04-13 20:43 ` Vivi, Rodrigo
@ 2016-04-14 7:58 ` Jani Nikula
0 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2016-04-14 7:58 UTC (permalink / raw)
To: Vivi, Rodrigo, intel-gfx@lists.freedesktop.org,
alexandra.yates@linux.intel.com, Konno, Joe, Zanoni, Paulo R,
Swaminathan, Nivedita
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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/5] drm/i915: Add sys FBC toggle interface
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
2016-04-12 19:18 ` [PATCH 1/5] drm/i915: Add sys PSR toggle interface Alexandra Yates
@ 2016-04-12 19:18 ` Alexandra Yates
2016-04-13 12:48 ` Zanoni, Paulo R
2016-04-12 19:18 ` [PATCH 3/5] drm/i915: Add sys RC6 " Alexandra Yates
` (5 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Alexandra Yates @ 2016-04-12 19:18 UTC (permalink / raw)
To: intel-gfx, rodrigo.vivi, nivedita.swaminathan, joe.konno; +Cc: Alexandra Yates
This interface allows an immediate enabling of FBC feature. What allow us
to see immediately the FBC power management 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 | 77 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_display.c | 15 +++++--
drivers/gpu/drm/i915/intel_drv.h | 4 +-
drivers/gpu/drm/i915/intel_fbc.c | 29 +++++++++++---
5 files changed, 115 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c96da4d..0f3a37f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -936,6 +936,7 @@ struct intel_fbc {
} work;
const char *no_fbc_reason;
+ bool sysfs_set;
};
/**
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 208c6ec..50d45ef 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -107,6 +107,65 @@ show_media_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf)
}
static ssize_t
+show_fbc(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->fbc.lock);
+ ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->fbc.enabled ?
+ "enabled":"disabled");
+ mutex_unlock(&dev_priv->fbc.lock);
+ return ret;
+}
+
+static ssize_t
+toggle_fbc(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;
+ 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);
+ }
+
+ if (!crtc)
+ return -ENODEV;
+ switch (val) {
+ case 0:
+ ret = intel_fbc_disable(crtc, sysfs_set);
+ if (ret)
+ return ret;
+ break;
+ case 1:
+ ret = intel_fbc_enable(crtc, sysfs_set);
+ if (ret)
+ return ret;
+ break;
+ default:
+ return -EINVAL;
+
+ }
+ return count;
+}
+
+static ssize_t
show_psr(struct device *kdev, struct device_attribute *attr, char *buf)
{
struct drm_minor *dminor = dev_to_drm_minor(kdev);
@@ -167,6 +226,7 @@ toggle_psr(struct device *kdev, struct device_attribute *attr,
return count;
}
+static DEVICE_ATTR(fbc_enable, S_IRUGO | S_IWUSR, show_fbc, toggle_fbc);
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);
@@ -174,6 +234,16 @@ 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 *fbc_attrs[] = {
+ &dev_attr_fbc_enable.attr,
+ NULL
+};
+
+static struct attribute_group fbc_attr_group = {
+ .name = power_group_name,
+ .attrs = fbc_attrs
+};
+
static struct attribute *psr_attrs[] = {
&dev_attr_psr_enable.attr,
NULL
@@ -668,6 +738,12 @@ void i915_setup_sysfs(struct drm_device *dev)
int ret;
#ifdef CONFIG_PM
+ if (HAS_FBC(dev)) {
+ ret = sysfs_merge_group(&dev->primary->kdev->kobj,
+ &fbc_attr_group);
+ if (ret)
+ DRM_ERROR("FBC sysfs setup failed\n");
+ }
if (HAS_PSR(dev)) {
ret = sysfs_merge_group(&dev->primary->kdev->kobj,
&psr_attr_group);
@@ -730,6 +806,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, &fbc_attr_group);
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);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bf9e347..3d252b9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6254,7 +6254,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
encoder->base.crtc = NULL;
- intel_fbc_disable(intel_crtc);
+ if (dev_priv->fbc.sysfs_set != true)
+ intel_fbc_disable(intel_crtc, dev_priv->fbc.sysfs_set);
intel_update_watermarks(crtc);
intel_disable_shared_dpll(intel_crtc);
@@ -13586,7 +13587,10 @@ static int intel_atomic_commit(struct drm_device *dev,
intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
dev_priv->display.crtc_disable(crtc);
intel_crtc->active = false;
- intel_fbc_disable(intel_crtc);
+
+ if (dev_priv->fbc.sysfs_set != true)
+ intel_fbc_disable(intel_crtc,
+ dev_priv->fbc.sysfs_set);
intel_disable_shared_dpll(intel_crtc);
/*
@@ -13632,8 +13636,11 @@ static int intel_atomic_commit(struct drm_device *dev,
intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
if (crtc->state->active &&
- drm_atomic_get_existing_plane_state(state, crtc->primary))
- intel_fbc_enable(intel_crtc);
+ drm_atomic_get_existing_plane_state(state, crtc->primary)) {
+ if (dev_priv->fbc.sysfs_set != true)
+ intel_fbc_enable(intel_crtc,
+ dev_priv->fbc.sysfs_set);
+ }
if (crtc->state->active &&
(crtc->state->planes_changed || update_pipe))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 199e8cc..93d09cc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1373,8 +1373,8 @@ void intel_fbc_pre_update(struct intel_crtc *crtc);
void intel_fbc_post_update(struct intel_crtc *crtc);
void intel_fbc_init(struct drm_i915_private *dev_priv);
void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv);
-void intel_fbc_enable(struct intel_crtc *crtc);
-void intel_fbc_disable(struct intel_crtc *crtc);
+int intel_fbc_enable(struct intel_crtc *crtc, bool sysfs_set);
+int intel_fbc_disable(struct intel_crtc *crtc, bool sysfs_set);
void intel_fbc_global_disable(struct drm_i915_private *dev_priv);
void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d5a7cfe..d6154e5 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1083,19 +1083,24 @@ out:
/**
* intel_fbc_enable: tries to enable FBC on the CRTC
* @crtc: the CRTC
+ * @sysfs_set: Identifies if this featudre is set from sysfs.
*
* This function checks if the given CRTC was chosen for FBC, then enables it if
* possible. Notice that it doesn't activate FBC. It is valid to call
* intel_fbc_enable multiple times for the same pipe without an
* intel_fbc_disable in the middle, as long as it is deactivated.
+ *
+ * Returns:
+ * 0 on success and -errno otherwise
*/
-void intel_fbc_enable(struct intel_crtc *crtc)
+int intel_fbc_enable(struct intel_crtc *crtc, bool sysfs_set)
{
struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
struct intel_fbc *fbc = &dev_priv->fbc;
+ int ret = 0;
if (!fbc_supported(dev_priv))
- return;
+ return -EINVAL;
mutex_lock(&fbc->lock);
@@ -1105,11 +1110,14 @@ void intel_fbc_enable(struct intel_crtc *crtc)
WARN_ON(!crtc->config->enable_fbc);
WARN_ON(fbc->active);
}
+ ret = -EALREADY;
goto out;
}
- if (!crtc->config->enable_fbc)
+ if (!crtc->config->enable_fbc) {
+ ret = -EINVAL;
goto out;
+ }
WARN_ON(fbc->active);
WARN_ON(fbc->crtc != NULL);
@@ -1117,6 +1125,7 @@ void intel_fbc_enable(struct intel_crtc *crtc)
intel_fbc_update_state_cache(crtc);
if (intel_fbc_alloc_cfb(crtc)) {
fbc->no_fbc_reason = "not enough stolen memory";
+ ret = -EINVAL;
goto out;
}
@@ -1125,8 +1134,11 @@ void intel_fbc_enable(struct intel_crtc *crtc)
fbc->enabled = true;
fbc->crtc = crtc;
+ if (sysfs_set)
+ dev_priv->fbc.sysfs_set = sysfs_set;
out:
mutex_unlock(&fbc->lock);
+ return ret;
}
/**
@@ -1157,26 +1169,33 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
/**
* intel_fbc_disable - disable FBC if it's associated with crtc
* @crtc: the CRTC
+ * @sysfs_set: Identifies if this featudre is set from sysfs.
*
* This function disables FBC if it's associated with the provided CRTC.
+ *
+ * Returns:
+ * 0 on success and -errno otherwise
*/
-void intel_fbc_disable(struct intel_crtc *crtc)
+int intel_fbc_disable(struct intel_crtc *crtc, bool sysfs_set)
{
struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
struct intel_fbc *fbc = &dev_priv->fbc;
if (!fbc_supported(dev_priv))
- return;
+ return -EINVAL;
mutex_lock(&fbc->lock);
if (fbc->crtc == crtc) {
WARN_ON(!fbc->enabled);
WARN_ON(fbc->active);
__intel_fbc_disable(dev_priv);
+ if (sysfs_set)
+ dev_priv->fbc.sysfs_set = sysfs_set;
}
mutex_unlock(&fbc->lock);
cancel_work_sync(&fbc->work.work);
+ return 0;
}
/**
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 2/5] drm/i915: Add sys FBC toggle interface
2016-04-12 19:18 ` [PATCH 2/5] drm/i915: Add sys FBC " Alexandra Yates
@ 2016-04-13 12:48 ` Zanoni, Paulo R
0 siblings, 0 replies; 23+ messages in thread
From: Zanoni, Paulo R @ 2016-04-13 12:48 UTC (permalink / raw)
To: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo,
alexandra.yates@linux.intel.com, Konno, Joe,
Swaminathan, Nivedita
Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> This interface allows an immediate enabling of FBC feature. What
> allow us
> to see immediately the FBC
There's no way to guarantee the user will immediately see any FBC
savings. FBC depends on a lot of conditions (e.g., X tiling, correct
pipe, correct buffer size and placement, correct pixel format, correct
mode, gpu idleness, etc), so users may see no power saving changes by
toggling the powertop buttons for FBC and incorrectly assume that FBC
saves no power, while they are just in a state that doesn't immediately
allow FBC to be activated. Do we plan to provide easily-accessible
documentation explaining such things?
> power management 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 | 77
> ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_display.c | 15 +++++--
> drivers/gpu/drm/i915/intel_drv.h | 4 +-
> drivers/gpu/drm/i915/intel_fbc.c | 29 +++++++++++---
> 5 files changed, 115 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index c96da4d..0f3a37f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -936,6 +936,7 @@ struct intel_fbc {
> } work;
>
> const char *no_fbc_reason;
> + bool sysfs_set;
> };
>
> /**
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index 208c6ec..50d45ef 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -107,6 +107,65 @@ show_media_rc6_ms(struct device *kdev, struct
> device_attribute *attr, char *buf)
> }
>
> static ssize_t
> +show_fbc(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->fbc.lock);
> + ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->fbc.enabled
> ?
> + "enabled":"disabled");
Whenever this was discussed, I actually thought this interface would
expose "is support for the FBC feature enabled in the Kernel?" instead
of "is FBC currently software-enabled on a CRTC?". For completeness,
notice that a third option would be "is FBC currently enabled in
hardware?". Why did you pick the current approach instead of the
others?
> + mutex_unlock(&dev_priv->fbc.lock);
Locking/unlocking doesn't help much here.
> + return ret;
> +}
> +
> +static ssize_t
> +toggle_fbc(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;
> + 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);
> + }
Why are we picking a random connected CRTC? There's no guarantee it
supports FBC, nor that it even has a mode.
> +
> + if (!crtc)
> + return -ENODEV;
> + switch (val) {
> + case 0:
> + ret = intel_fbc_disable(crtc, sysfs_set);
> + if (ret)
> + return ret;
> + break;
> + case 1:
> + ret = intel_fbc_enable(crtc, sysfs_set);
The current FBC code was designed in a way where the appropriate CRTC
is chosen during the atomic commits, and then later
intel_fbc_{en,dis}able is called. This change is bypassing
intel_fbc_can_choose(), and creating a new - and unplanned for - use
case where FBC can be toggled without a modeset. While we could make
this work somehow, we'd be adding additional complexity (aka possible
bugs) just to enable users to switch their machines to unsafe defaults
using powertop (and maybe reaching wrong conclusions due to the lack of
guarantee that FBC will be enabled in hardware), so I'm not sure the
benefits outweigh the maintenance costs.
So my suggestion would be to make the sysfs interface just toggle
i915.enable_fbc, which would require a modeset for things to actually
take place. I thought this was the original plan.
By the way, aren't you hitting the WARNs from
intel_fbc_update_state_cache() with the current code?
> + if (ret)
> + return ret;
> + break;
> + default:
> + return -EINVAL;
> +
> + }
> + return count;
> +}
> +
> +static ssize_t
> show_psr(struct device *kdev, struct device_attribute *attr, char
> *buf)
> {
> struct drm_minor *dminor = dev_to_drm_minor(kdev);
> @@ -167,6 +226,7 @@ toggle_psr(struct device *kdev, struct
> device_attribute *attr,
> return count;
> }
>
> +static DEVICE_ATTR(fbc_enable, S_IRUGO | S_IWUSR, show_fbc,
> toggle_fbc);
> 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);
> @@ -174,6 +234,16 @@ 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 *fbc_attrs[] = {
> + &dev_attr_fbc_enable.attr,
> + NULL
> +};
> +
> +static struct attribute_group fbc_attr_group = {
> + .name = power_group_name,
> + .attrs = fbc_attrs
> +};
> +
> static struct attribute *psr_attrs[] = {
> &dev_attr_psr_enable.attr,
> NULL
> @@ -668,6 +738,12 @@ void i915_setup_sysfs(struct drm_device *dev)
> int ret;
>
> #ifdef CONFIG_PM
> + if (HAS_FBC(dev)) {
> + ret = sysfs_merge_group(&dev->primary->kdev->kobj,
> + &fbc_attr_group);
> + if (ret)
> + DRM_ERROR("FBC sysfs setup failed\n");
> + }
> if (HAS_PSR(dev)) {
> ret = sysfs_merge_group(&dev->primary->kdev->kobj,
> &psr_attr_group);
> @@ -730,6 +806,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,
> &fbc_attr_group);
> 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);
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index bf9e347..3d252b9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6254,7 +6254,8 @@ static void intel_crtc_disable_noatomic(struct
> drm_crtc *crtc)
> for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
> encoder->base.crtc = NULL;
>
> - intel_fbc_disable(intel_crtc);
> + if (dev_priv->fbc.sysfs_set != true)
> + intel_fbc_disable(intel_crtc, dev_priv-
> >fbc.sysfs_set);
> intel_update_watermarks(crtc);
> intel_disable_shared_dpll(intel_crtc);
>
> @@ -13586,7 +13587,10 @@ static int intel_atomic_commit(struct
> drm_device *dev,
> intel_crtc_disable_planes(crtc,
> old_crtc_state->plane_mask);
> dev_priv->display.crtc_disable(crtc);
> intel_crtc->active = false;
> - intel_fbc_disable(intel_crtc);
> +
> + if (dev_priv->fbc.sysfs_set != true)
> + intel_fbc_disable(intel_crtc,
> + dev_priv-
> >fbc.sysfs_set);
> intel_disable_shared_dpll(intel_crtc);
>
> /*
> @@ -13632,8 +13636,11 @@ static int intel_atomic_commit(struct
> drm_device *dev,
> intel_pre_plane_update(to_intel_crtc_state(o
> ld_crtc_state));
>
> if (crtc->state->active &&
> - drm_atomic_get_existing_plane_state(state, crtc-
> >primary))
> - intel_fbc_enable(intel_crtc);
> + drm_atomic_get_existing_plane_state(state, crtc-
> >primary)) {
> + if (dev_priv->fbc.sysfs_set != true)
> + intel_fbc_enable(intel_crtc,
> + dev_priv-
> >fbc.sysfs_set);
> + }
>
> if (crtc->state->active &&
> (crtc->state->planes_changed || update_pipe))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 199e8cc..93d09cc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1373,8 +1373,8 @@ void intel_fbc_pre_update(struct intel_crtc
> *crtc);
> void intel_fbc_post_update(struct intel_crtc *crtc);
> void intel_fbc_init(struct drm_i915_private *dev_priv);
> void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv);
> -void intel_fbc_enable(struct intel_crtc *crtc);
> -void intel_fbc_disable(struct intel_crtc *crtc);
> +int intel_fbc_enable(struct intel_crtc *crtc, bool sysfs_set);
> +int intel_fbc_disable(struct intel_crtc *crtc, bool sysfs_set);
> void intel_fbc_global_disable(struct drm_i915_private *dev_priv);
> void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> unsigned int frontbuffer_bits,
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index d5a7cfe..d6154e5 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1083,19 +1083,24 @@ out:
> /**
> * intel_fbc_enable: tries to enable FBC on the CRTC
> * @crtc: the CRTC
> + * @sysfs_set: Identifies if this featudre is set from sysfs.
> *
> * This function checks if the given CRTC was chosen for FBC, then
> enables it if
> * possible. Notice that it doesn't activate FBC. It is valid to
> call
> * intel_fbc_enable multiple times for the same pipe without an
> * intel_fbc_disable in the middle, as long as it is deactivated.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise
> */
> -void intel_fbc_enable(struct intel_crtc *crtc)
> +int intel_fbc_enable(struct intel_crtc *crtc, bool sysfs_set)
> {
> struct drm_i915_private *dev_priv = crtc->base.dev-
> >dev_private;
> struct intel_fbc *fbc = &dev_priv->fbc;
> + int ret = 0;
>
> if (!fbc_supported(dev_priv))
> - return;
> + return -EINVAL;
>
> mutex_lock(&fbc->lock);
>
> @@ -1105,11 +1110,14 @@ void intel_fbc_enable(struct intel_crtc
> *crtc)
> WARN_ON(!crtc->config->enable_fbc);
> WARN_ON(fbc->active);
> }
> + ret = -EALREADY;
> goto out;
> }
>
> - if (!crtc->config->enable_fbc)
> + if (!crtc->config->enable_fbc) {
> + ret = -EINVAL;
> goto out;
> + }
>
> WARN_ON(fbc->active);
> WARN_ON(fbc->crtc != NULL);
> @@ -1117,6 +1125,7 @@ void intel_fbc_enable(struct intel_crtc *crtc)
> intel_fbc_update_state_cache(crtc);
> if (intel_fbc_alloc_cfb(crtc)) {
> fbc->no_fbc_reason = "not enough stolen memory";
> + ret = -EINVAL;
> goto out;
> }
>
> @@ -1125,8 +1134,11 @@ void intel_fbc_enable(struct intel_crtc *crtc)
>
> fbc->enabled = true;
> fbc->crtc = crtc;
> + if (sysfs_set)
> + dev_priv->fbc.sysfs_set = sysfs_set;
> out:
> mutex_unlock(&fbc->lock);
> + return ret;
> }
>
> /**
> @@ -1157,26 +1169,33 @@ static void __intel_fbc_disable(struct
> drm_i915_private *dev_priv)
> /**
> * intel_fbc_disable - disable FBC if it's associated with crtc
> * @crtc: the CRTC
> + * @sysfs_set: Identifies if this featudre is set from sysfs.
> *
> * This function disables FBC if it's associated with the provided
> CRTC.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise
> */
> -void intel_fbc_disable(struct intel_crtc *crtc)
> +int intel_fbc_disable(struct intel_crtc *crtc, bool sysfs_set)
> {
> struct drm_i915_private *dev_priv = crtc->base.dev-
> >dev_private;
> struct intel_fbc *fbc = &dev_priv->fbc;
>
> if (!fbc_supported(dev_priv))
> - return;
> + return -EINVAL;
>
> mutex_lock(&fbc->lock);
> if (fbc->crtc == crtc) {
> WARN_ON(!fbc->enabled);
> WARN_ON(fbc->active);
> __intel_fbc_disable(dev_priv);
> + if (sysfs_set)
> + dev_priv->fbc.sysfs_set = sysfs_set;
> }
> mutex_unlock(&fbc->lock);
>
> cancel_work_sync(&fbc->work.work);
> + return 0;
> }
>
> /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/5] drm/i915: Add sys RC6 toggle interface
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
2016-04-12 19:18 ` [PATCH 1/5] drm/i915: Add sys PSR toggle interface Alexandra Yates
2016-04-12 19:18 ` [PATCH 2/5] drm/i915: Add sys FBC " Alexandra Yates
@ 2016-04-12 19:18 ` Alexandra Yates
2016-04-12 19:18 ` [PATCH 4/5] drm/i915: Add sys drrs " Alexandra Yates
` (4 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Alexandra Yates @ 2016-04-12 19:18 UTC (permalink / raw)
To: intel-gfx, rodrigo.vivi, nivedita.swaminathan, joe.konno; +Cc: Alexandra Yates
This interface allows enabling/disabling of RC6 feature. It allows
to see immediately the RC6 power management savings and will allow
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.c | 5 ++--
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_sysfs.c | 42 ++++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_display.c | 3 +-
drivers/gpu/drm/i915/intel_drv.h | 5 ++--
drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++--
6 files changed, 99 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 29b4e79..6ffbdfb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -950,7 +950,7 @@ int i915_reset(struct drm_device *dev)
* of re-init after reset.
*/
if (INTEL_INFO(dev)->gen > 5)
- intel_enable_gt_powersave(dev);
+ intel_enable_gt_powersave(dev, false);
return 0;
}
@@ -1600,7 +1600,8 @@ static int intel_runtime_resume(struct device *device)
if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
intel_hpd_init(dev_priv);
- intel_enable_gt_powersave(dev);
+ if (dev_priv->rps.sysfs_set != true)
+ intel_enable_gt_powersave(dev, dev_priv->rps.sysfs_set);
enable_rpm_wakeref_asserts(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f3a37f..bbe189f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1158,6 +1158,7 @@ struct intel_gen6_power_mgmt {
* talking to hw - so only take it when talking to hw!
*/
struct mutex hw_lock;
+ bool sysfs_set;
};
/* defined intel_pm.c */
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 50d45ef..81aa534 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -71,7 +71,45 @@ static ssize_t
show_rc6_mask(struct device *kdev, struct device_attribute *attr, char *buf)
{
struct drm_minor *dminor = dev_to_drm_minor(kdev);
- return snprintf(buf, PAGE_SIZE, "%x\n", intel_enable_rc6(dminor->dev));
+ struct drm_device *dev = dminor->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ int enable_rc6;
+
+ enable_rc6 = sanitize_rc6_option(dev, intel_enable_rc6(dminor->dev));
+ return snprintf(buf, PAGE_SIZE, "%x\n",
+ (dev_priv->rps.enabled && enable_rc6));
+}
+
+static ssize_t
+toggle_rc6(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;
+ u32 val;
+ ssize_t ret;
+ bool sysfs_set = true;
+
+ ret = kstrtou32(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ switch (val) {
+ case 0:
+ ret = intel_disable_rc_powersave(dev, sysfs_set);
+ if (ret)
+ return ret;
+ break;
+ case 1:
+ ret = intel_enable_gt_powersave(dev, sysfs_set);
+ if (ret)
+ return ret;
+ break;
+ default:
+ return -EINVAL;
+
+ }
+ return count;
}
static ssize_t
@@ -228,7 +266,7 @@ toggle_psr(struct device *kdev, struct device_attribute *attr,
static DEVICE_ATTR(fbc_enable, S_IRUGO | S_IWUSR, show_fbc, toggle_fbc);
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_enable, S_IRUGO | S_IWUSR, show_rc6_mask, toggle_rc6);
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);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3d252b9..4a671e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15256,7 +15256,8 @@ void intel_modeset_init_hw(struct drm_device *dev)
dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
intel_init_clock_gating(dev);
- intel_enable_gt_powersave(dev);
+ if (dev_priv->rps.sysfs_set != true)
+ intel_enable_gt_powersave(dev, dev_priv->rps.sysfs_set);
}
/*
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 93d09cc..d280847 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1586,8 +1586,9 @@ void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
void intel_gpu_ips_teardown(void);
void intel_init_gt_powersave(struct drm_device *dev);
void intel_cleanup_gt_powersave(struct drm_device *dev);
-void intel_enable_gt_powersave(struct drm_device *dev);
-void intel_disable_gt_powersave(struct drm_device *dev);
+int intel_enable_gt_powersave(struct drm_device *dev, bool sysfs_set);
+int intel_disable_gt_powersave(struct drm_device *dev);
+int intel_disable_rc_powersave(struct drm_device *dev, bool sysfs_set);
void intel_suspend_gt_powersave(struct drm_device *dev);
void intel_reset_gt_powersave(struct drm_device *dev);
void gen6_update_ring_freq(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b8e395a..9ce9cda 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4587,6 +4587,13 @@ void intel_set_rps(struct drm_device *dev, u8 val)
gen6_set_rps(dev, val);
}
+static void gen9_disable_rc(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ I915_WRITE(GEN6_RC_CONTROL, 0);
+}
+
static void gen9_disable_rps(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4595,6 +4602,13 @@ static void gen9_disable_rps(struct drm_device *dev)
I915_WRITE(GEN9_PG_ENABLE, 0);
}
+static void gen6_disable_rc(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ I915_WRITE(GEN6_RC_CONTROL, 0);
+}
+
static void gen6_disable_rps(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -6255,7 +6269,33 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
gen6_rps_idle(dev_priv);
}
-void intel_disable_gt_powersave(struct drm_device *dev)
+int intel_disable_rc_powersave(struct drm_device *dev, bool sysfs_set)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ if (IS_IRONLAKE_M(dev)) {
+ ironlake_disable_drps(dev);
+ } else if (INTEL_INFO(dev)->gen >= 6) {
+ intel_suspend_gt_powersave(dev);
+
+ mutex_lock(&dev_priv->rps.hw_lock);
+ if (INTEL_INFO(dev)->gen >= 9)
+ gen9_disable_rc(dev);
+ else if (IS_CHERRYVIEW(dev))
+ cherryview_disable_rps(dev);
+ else if (IS_VALLEYVIEW(dev))
+ valleyview_disable_rps(dev);
+ else
+ gen6_disable_rc(dev);
+
+ dev_priv->rps.enabled = false;
+ dev_priv->rps.sysfs_set = sysfs_set;
+ mutex_unlock(&dev_priv->rps.hw_lock);
+ }
+ return 0;
+}
+
+int intel_disable_gt_powersave(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -6277,6 +6317,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
dev_priv->rps.enabled = false;
mutex_unlock(&dev_priv->rps.hw_lock);
}
+ return 0;
}
static void intel_gen6_powersave_work(struct work_struct *work)
@@ -6322,13 +6363,14 @@ static void intel_gen6_powersave_work(struct work_struct *work)
intel_runtime_pm_put(dev_priv);
}
-void intel_enable_gt_powersave(struct drm_device *dev)
+int intel_enable_gt_powersave(struct drm_device *dev, bool sysfs_set)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ int ret = 0;
/* Powersaving is controlled by the host when inside a VM */
if (intel_vgpu_active(dev))
- return;
+ return -ENOTTY;
if (IS_IRONLAKE_M(dev)) {
ironlake_enable_drps(dev);
@@ -6352,6 +6394,11 @@ void intel_enable_gt_powersave(struct drm_device *dev)
round_jiffies_up_relative(HZ)))
intel_runtime_pm_get_noresume(dev_priv);
}
+
+ mutex_lock(&dev_priv->rps.hw_lock);
+ dev_priv->rps.sysfs_set = sysfs_set;
+ mutex_unlock(&dev_priv->rps.hw_lock);
+ return ret;
}
void intel_reset_gt_powersave(struct drm_device *dev)
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 4/5] drm/i915: Add sys drrs toggle interface
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
` (2 preceding siblings ...)
2016-04-12 19:18 ` [PATCH 3/5] drm/i915: Add sys RC6 " Alexandra Yates
@ 2016-04-12 19:18 ` Alexandra Yates
2016-04-12 19:18 ` [PATCH 5/5] drm-i915: Add sys IPS " Alexandra Yates
` (3 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Alexandra Yates @ 2016-04-12 19:18 UTC (permalink / raw)
To: intel-gfx, rodrigo.vivi, nivedita.swaminathan, joe.konno; +Cc: Alexandra Yates
This interface allows enabling/disabling of DRRS feature. It allows
to see immediately the power management savings and will allow
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 | 86 +++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_ddi.c | 9 +++-
drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++---
drivers/gpu/drm/i915/intel_drv.h | 4 +-
5 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bbe189f..4c5eea6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -964,6 +964,7 @@ struct i915_drrs {
unsigned busy_frontbuffer_bits;
enum drrs_refresh_rate_type refresh_rate_type;
enum drrs_support_type type;
+ bool sysfs_set;
};
struct i915_psr {
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 81aa534..f489ab6 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -264,6 +264,72 @@ toggle_psr(struct device *kdev, struct device_attribute *attr,
return count;
}
+static ssize_t
+show_drrs(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->drrs.mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->drrs.dp ?
+ "enabled":"disabled");
+ mutex_unlock(&dev_priv->drrs.mutex);
+ return ret;
+}
+
+static ssize_t
+toggle_drrs(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;
+ struct intel_dp *intel_dp = NULL;
+ u32 val;
+ ssize_t ret;
+ 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_edp_drrs_disable(intel_dp, sysfs_set);
+ if (ret)
+ return ret;
+ break;
+ case 1:
+ if (encoder->type == INTEL_OUTPUT_EDP) {
+ ret = intel_edp_drrs_enable(intel_dp, sysfs_set);
+ if (ret)
+ return ret;
+ }
+ break;
+ default:
+ return -EINVAL;
+
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR(drrs_enable, S_IRUGO | S_IWUSR, show_drrs, toggle_drrs);
static DEVICE_ATTR(fbc_enable, S_IRUGO | S_IWUSR, show_fbc, toggle_fbc);
static DEVICE_ATTR(psr_enable, S_IRUGO | S_IWUSR, show_psr, toggle_psr);
static DEVICE_ATTR(rc6_enable, S_IRUGO | S_IWUSR, show_rc6_mask, toggle_rc6);
@@ -323,6 +389,17 @@ static struct attribute_group media_rc6_attr_group = {
.name = power_group_name,
.attrs = media_rc6_attrs
};
+
+static struct attribute *drrs_attrs[] = {
+ &dev_attr_drrs_enable.attr,
+ NULL
+};
+
+static struct attribute_group drrs_attr_group = {
+ .name = power_group_name,
+ .attrs = drrs_attrs
+};
+
#endif
static int l3_access_valid(struct drm_device *dev, loff_t offset)
@@ -788,6 +865,14 @@ void i915_setup_sysfs(struct drm_device *dev)
if (ret)
DRM_ERROR("PSR sysfs setup failed\n");
}
+
+ if (HAS_PSR(dev)) {
+ ret = sysfs_merge_group(&dev->primary->kdev->kobj,
+ &drrs_attr_group);
+ if (ret)
+ DRM_ERROR("DRRS sysfs setup failed\n");
+ }
+
if (HAS_RC6(dev)) {
ret = sysfs_merge_group(&dev->primary->kdev->kobj,
&rc6_attr_group);
@@ -844,6 +929,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, &drrs_attr_group);
sysfs_unmerge_group(&dev->primary->kdev->kobj, &fbc_attr_group);
sysfs_unmerge_group(&dev->primary->kdev->kobj, &psr_attr_group);
sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6_attr_group);
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 8e384e5..eb6f0f9 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1691,7 +1691,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
intel_edp_backlight_on(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);
+ if (dev_priv->drrs.sysfs_set != true)
+ intel_edp_drrs_enable(intel_dp,
+ dev_priv->drrs.sysfs_set);
}
if (intel_crtc->config->has_audio) {
@@ -1717,7 +1719,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
if (type == INTEL_OUTPUT_EDP) {
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
- intel_edp_drrs_disable(intel_dp);
+ if (dev_priv->drrs.sysfs_set != true)
+ intel_edp_drrs_disable(intel_dp,
+ dev_priv->drrs.sysfs_set);
+
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 183a60a..ec4bd12 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5413,42 +5413,53 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
/**
* intel_edp_drrs_enable - init drrs struct if supported
* @intel_dp: DP struct
+ * @sysfs_set: Identifies if this featudre is set from sysfs.
*
* Initializes frontbuffer_bits and drrs.dp
+ *
+ * Returns:
+ * 0 on success and -errno otherwise.
*/
-void intel_edp_drrs_enable(struct intel_dp *intel_dp)
+int intel_edp_drrs_enable(struct intel_dp *intel_dp, bool sysfs_set)
{
struct drm_device *dev = intel_dp_to_dev(intel_dp);
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct drm_crtc *crtc = dig_port->base.base.crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ int ret = 0;
if (!intel_crtc->config->has_drrs) {
DRM_DEBUG_KMS("Panel doesn't support DRRS\n");
- return;
+ return -EINVAL;
}
mutex_lock(&dev_priv->drrs.mutex);
if (WARN_ON(dev_priv->drrs.dp)) {
DRM_ERROR("DRRS already enabled\n");
+ ret = -EALREADY;
goto unlock;
}
dev_priv->drrs.busy_frontbuffer_bits = 0;
dev_priv->drrs.dp = intel_dp;
-
+ if (sysfs_set)
+ dev_priv->drrs.sysfs_set = sysfs_set;
unlock:
mutex_unlock(&dev_priv->drrs.mutex);
+ return ret;
}
/**
* intel_edp_drrs_disable - Disable DRRS
* @intel_dp: DP struct
+ * @sysfs_set: Identifies if this featudre is set from sysfs.
*
+ * Returns:
+ * 0 on success and -errno otherwise.
*/
-void intel_edp_drrs_disable(struct intel_dp *intel_dp)
+int intel_edp_drrs_disable(struct intel_dp *intel_dp, bool sysfs_set)
{
struct drm_device *dev = intel_dp_to_dev(intel_dp);
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5457,12 +5468,12 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp)
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
if (!intel_crtc->config->has_drrs)
- return;
+ return -EINVAL;
mutex_lock(&dev_priv->drrs.mutex);
if (!dev_priv->drrs.dp) {
mutex_unlock(&dev_priv->drrs.mutex);
- return;
+ return -EALREADY;
}
if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
@@ -5471,9 +5482,12 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp)
fixed_mode->vrefresh);
dev_priv->drrs.dp = NULL;
+ if (sysfs_set)
+ dev_priv->drrs.sysfs_set = sysfs_set;
mutex_unlock(&dev_priv->drrs.mutex);
cancel_delayed_work_sync(&dev_priv->drrs.work);
+ return 0;
}
static void intel_edp_drrs_downclock_work(struct work_struct *work)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d280847..eda84ae 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1299,8 +1299,8 @@ void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv);
uint32_t intel_dp_pack_aux(const uint8_t *src, int src_bytes);
void intel_plane_destroy(struct drm_plane *plane);
-void intel_edp_drrs_enable(struct intel_dp *intel_dp);
-void intel_edp_drrs_disable(struct intel_dp *intel_dp);
+int intel_edp_drrs_enable(struct intel_dp *intel_dp, bool sysfs_set);
+int intel_edp_drrs_disable(struct intel_dp *intel_dp, bool sysfs_set);
void intel_edp_drrs_invalidate(struct drm_device *dev,
unsigned frontbuffer_bits);
void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 5/5] drm-i915: Add sys IPS toggle interface
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
` (3 preceding siblings ...)
2016-04-12 19:18 ` [PATCH 4/5] drm/i915: Add sys drrs " Alexandra Yates
@ 2016-04-12 19:18 ` Alexandra Yates
2016-04-13 10:21 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Daniel Vetter
` (2 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Alexandra Yates @ 2016-04-12 19:18 UTC (permalink / raw)
To: intel-gfx, rodrigo.vivi, nivedita.swaminathan, joe.konno; +Cc: Alexandra Yates
This interface allows enabling/disabling of IPS feature. It allows
to see immediately the power management savings and will allow
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_debugfs.c | 5 ++-
drivers/gpu/drm/i915/i915_drv.h | 8 ++++
drivers/gpu/drm/i915/i915_sysfs.c | 78 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_color.c | 12 ++++--
drivers/gpu/drm/i915/intel_display.c | 37 +++++++++++++----
drivers/gpu/drm/i915/intel_dp.c | 11 +++--
drivers/gpu/drm/i915/intel_drv.h | 4 +-
7 files changed, 136 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2d11b49..e338c8a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4022,6 +4022,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
enum intel_display_power_domain power_domain;
u32 val = 0; /* shut up gcc */
int ret;
+ bool sysfs_set = false;
if (pipe_crc->source == source)
return 0;
@@ -4071,7 +4072,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
* user space can't make reliable use of the CRCs, so let's just
* completely disable it.
*/
- hsw_disable_ips(crtc);
+ hsw_disable_ips(crtc, sysfs_set);
spin_lock_irq(&pipe_crc->lock);
kfree(pipe_crc->entries);
@@ -4116,7 +4117,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
else if (IS_HASWELL(dev) && pipe == PIPE_A)
hsw_trans_edp_pipe_A_crc_wa(dev, false);
- hsw_enable_ips(crtc);
+ hsw_enable_ips(crtc, sysfs_set);
}
ret = 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4c5eea6..0df0030 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -967,6 +967,12 @@ struct i915_drrs {
bool sysfs_set;
};
+struct i915_ips {
+ struct mutex lock;
+ int enable;
+ bool sysfs_set;
+};
+
struct i915_psr {
struct mutex lock;
bool sink_support;
@@ -1888,6 +1894,8 @@ struct drm_i915_private {
struct i915_power_domains power_domains;
+ struct i915_ips hsw_ips;
+
struct i915_psr psr;
struct i915_gpu_error gpu_error;
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index f489ab6..3bc830b 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -265,6 +265,64 @@ toggle_psr(struct device *kdev, struct device_attribute *attr,
}
static ssize_t
+show_ips(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->hsw_ips.lock);
+ ret = snprintf(buf, PAGE_SIZE, "%s\n", dev_priv->hsw_ips.enable ?
+ "enabled":"disabled");
+ mutex_unlock(&dev_priv->hsw_ips.lock);
+ return ret;
+}
+
+static ssize_t
+toggle_ips(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;
+ bool sysfs_set = true;
+ u32 val;
+ ssize_t ret;
+
+ 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);
+ }
+ if (!crtc)
+ return -ENODEV;
+
+ switch (val) {
+ case 0:
+ ret = hsw_disable_ips(crtc, sysfs_set);
+ if (ret)
+ return ret;
+ break;
+ case 1:
+ ret = hsw_enable_ips(crtc, sysfs_set);
+ if (ret)
+ return ret;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return count;
+}
+
+static ssize_t
show_drrs(struct device *kdev, struct device_attribute *attr, char *buf)
{
struct drm_minor *dminor = dev_to_drm_minor(kdev);
@@ -329,6 +387,7 @@ toggle_drrs(struct device *kdev, struct device_attribute *attr,
return count;
}
+static DEVICE_ATTR(ips_enable, S_IRUGO | S_IWUSR, show_ips, toggle_ips);
static DEVICE_ATTR(drrs_enable, S_IRUGO | S_IWUSR, show_drrs, toggle_drrs);
static DEVICE_ATTR(fbc_enable, S_IRUGO | S_IWUSR, show_fbc, toggle_fbc);
static DEVICE_ATTR(psr_enable, S_IRUGO | S_IWUSR, show_psr, toggle_psr);
@@ -348,6 +407,16 @@ static struct attribute_group fbc_attr_group = {
.attrs = fbc_attrs
};
+static struct attribute *ips_attrs[] = {
+ &dev_attr_ips_enable.attr,
+ NULL
+};
+
+static struct attribute_group ips_attr_group = {
+ .name = power_group_name,
+ .attrs = ips_attrs
+};
+
static struct attribute *psr_attrs[] = {
&dev_attr_psr_enable.attr,
NULL
@@ -859,6 +928,14 @@ void i915_setup_sysfs(struct drm_device *dev)
if (ret)
DRM_ERROR("FBC sysfs setup failed\n");
}
+
+ if (HAS_IPS(dev)) {
+ ret = sysfs_merge_group(&dev->primary->kdev->kobj,
+ &ips_attr_group);
+ if (ret)
+ DRM_ERROR("IPS sysfs setup failed\n");
+ }
+
if (HAS_PSR(dev)) {
ret = sysfs_merge_group(&dev->primary->kdev->kobj,
&psr_attr_group);
@@ -931,6 +1008,7 @@ void i915_teardown_sysfs(struct drm_device *dev)
#ifdef CONFIG_PM
sysfs_unmerge_group(&dev->primary->kdev->kobj, &drrs_attr_group);
sysfs_unmerge_group(&dev->primary->kdev->kobj, &fbc_attr_group);
+ sysfs_unmerge_group(&dev->primary->kdev->kobj, &ips_attr_group);
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);
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 1b3f974..27660a6 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -325,8 +325,10 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
*/
if (IS_HASWELL(dev) && intel_crtc->config->ips_enabled &&
(intel_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
- hsw_disable_ips(intel_crtc);
- reenable_ips = true;
+ if (dev_priv->hsw_ips.sysfs_set != true){
+ hsw_disable_ips(intel_crtc, dev_priv->hsw_ips.sysfs_set);
+ reenable_ips = true;
+ }
}
intel_crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
@@ -334,8 +336,10 @@ static void haswell_load_luts(struct drm_crtc_state *crtc_state)
i9xx_load_luts(crtc_state);
- if (reenable_ips)
- hsw_enable_ips(intel_crtc);
+ if (reenable_ips){
+ if (dev_priv->hsw_ips.sysfs_set != true)
+ hsw_enable_ips(intel_crtc, dev_priv->hsw_ips.sysfs_set);
+ }
}
/* Loads the palette/gamma unit for the CRTC on Broadwell+. */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4a671e3..7a66d00 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4400,13 +4400,14 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
}
}
-void hsw_enable_ips(struct intel_crtc *crtc)
+int hsw_enable_ips(struct intel_crtc *crtc, bool sysfs_set)
{
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ int ret = 0;
if (!crtc->config->ips_enabled)
- return;
+ return -EINVAL;
/*
* We can only enable IPS after we enable a plane and wait for a vblank
@@ -4431,18 +4432,28 @@ void hsw_enable_ips(struct intel_crtc *crtc)
* and don't wait for vblanks until the end of crtc_enable, then
* the HW state readout code will complain that the expected
* IPS_CTL value is not the one we read. */
- if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50))
+ if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50)){
DRM_ERROR("Timed out waiting for IPS enable\n");
+ ret = -ETIMEDOUT;
+ }
}
+
+ mutex_lock(&dev_priv->hsw_ips.lock);
+ dev_priv->hsw_ips.enable = 1;
+ dev_priv->hsw_ips.sysfs_set = sysfs_set;
+ mutex_unlock(&dev_priv->hsw_ips.lock);
+
+ return ret;
}
-void hsw_disable_ips(struct intel_crtc *crtc)
+int hsw_disable_ips(struct intel_crtc *crtc, bool sysfs_set)
{
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ int ret = 0;
if (!crtc->config->ips_enabled)
- return;
+ return -EINVAL;
assert_plane_enabled(dev_priv, crtc->plane);
if (IS_BROADWELL(dev)) {
@@ -4450,15 +4461,23 @@ void hsw_disable_ips(struct intel_crtc *crtc)
WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0));
mutex_unlock(&dev_priv->rps.hw_lock);
/* wait for pcode to finish disabling IPS, which may take up to 42ms */
- if (wait_for((I915_READ(IPS_CTL) & IPS_ENABLE) == 0, 42))
+ if (wait_for((I915_READ(IPS_CTL) & IPS_ENABLE) == 0, 42)){
DRM_ERROR("Timed out waiting for IPS disable\n");
+ ret = -ETIMEDOUT;
+ }
} else {
I915_WRITE(IPS_CTL, 0);
POSTING_READ(IPS_CTL);
}
+ mutex_lock(&dev_priv->hsw_ips.lock);
+ dev_priv->hsw_ips.enable = 0;
+ dev_priv->hsw_ips.sysfs_set = sysfs_set;
+ mutex_unlock(&dev_priv->hsw_ips.lock);
+
/* We need to wait for a vblank before we can disable the plane. */
intel_wait_for_vblank(dev, crtc->pipe);
+ return ret;
}
static void intel_crtc_dpms_overlay_disable(struct intel_crtc *intel_crtc)
@@ -4503,7 +4522,8 @@ intel_post_enable_primary(struct drm_crtc *crtc)
* when going from primary only to sprite only and vice
* versa.
*/
- hsw_enable_ips(intel_crtc);
+ if (dev_priv->hsw_ips.sysfs_set != true)
+ hsw_enable_ips(intel_crtc, dev_priv->hsw_ips.sysfs_set);
/*
* Gen2 reports pipe underruns whenever all planes are disabled.
@@ -4544,7 +4564,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
* when going from primary only to sprite only and vice
* versa.
*/
- hsw_disable_ips(intel_crtc);
+ if (dev_priv->hsw_ips.sysfs_set != true)
+ hsw_disable_ips(intel_crtc, dev_priv->hsw_ips.sysfs_set);
}
/* FIXME get rid of this and use pre_plane_update */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ec4bd12..0fff2bd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3906,6 +3906,7 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
{
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = dig_port->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
u8 buf;
int ret = 0;
@@ -3942,7 +3943,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
}
out:
- hsw_enable_ips(intel_crtc);
+ if (dev_priv->hsw_ips.sysfs_set != true)
+ hsw_enable_ips(intel_crtc, dev_priv->hsw_ips.sysfs_set);
return ret;
}
@@ -3950,6 +3952,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
{
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = dig_port->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
u8 buf;
int ret;
@@ -3969,11 +3972,13 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp)
return ret;
}
- hsw_disable_ips(intel_crtc);
+ if (dev_priv->hsw_ips.sysfs_set != true)
+ hsw_disable_ips(intel_crtc, dev_priv->hsw_ips.sysfs_set);
if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
buf | DP_TEST_SINK_START) < 0) {
- hsw_enable_ips(intel_crtc);
+ if (dev_priv->hsw_ips.sysfs_set != true)
+ hsw_enable_ips(intel_crtc, dev_priv->hsw_ips.sysfs_set);
return -EIO;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index eda84ae..c616cb6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1244,8 +1244,8 @@ bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
int chv_calc_dpll_params(int refclk, intel_clock_t *pll_clock);
bool intel_crtc_active(struct drm_crtc *crtc);
-void hsw_enable_ips(struct intel_crtc *crtc);
-void hsw_disable_ips(struct intel_crtc *crtc);
+int hsw_enable_ips(struct intel_crtc *crtc, bool sysfs_set);
+int hsw_disable_ips(struct intel_crtc *crtc, bool sysfs_set);
enum intel_display_power_domain
intel_display_port_power_domain(struct intel_encoder *intel_encoder);
enum intel_display_power_domain
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
` (4 preceding siblings ...)
2016-04-12 19:18 ` [PATCH 5/5] drm-i915: Add sys IPS " Alexandra Yates
@ 2016-04-13 10:21 ` Daniel Vetter
2016-04-13 10:24 ` Jani Nikula
2016-04-13 12:59 ` Zanoni, Paulo R
7 siblings, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2016-04-13 10:21 UTC (permalink / raw)
To: Alexandra Yates; +Cc: intel-gfx, joe.konno, nivedita.swaminathan, rodrigo.vivi
On Tue, Apr 12, 2016 at 12:18:43PM -0700, Alexandra Yates wrote:
> This project is explained in detail on the HAS
> https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnhD1Tes3f08UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
>
> Summary:
> Permits the user to identify and toggle values for PSR, FBC, RC6, DRRS, and IPS
> under /sys/class/drm/card0/power/. By enabling these features I'm looking to
> empower our customers, such as, power team, chrome OS, and platform integration teams
> to debug graphics power management features.
>
> From the current patchset PSR, FBC, RC6, and, DRRS are complete and past BAT, modset,
> and suspend/resume tests. For IPS I'm looking for feedback since IPS seems to change
> during runtime dynamically. Additionally, as a future project IPS would be better
> served if it is implemented by using the atomic check, pre-commit, commit, post-commit,
> a good example of this is the PSR enable/disable implementation.
>
> For this project to be completed needs to have in place the following components:
> (Need to be developed)
> * IPS toggle interface flesh-out
> * Tests added to intel-gpu-tools repo
> * Documentation for all sysfs added interfaces
> * PowerTOP component named: GFX-TOP. With the following requirements:
> * It would be available only to developers
> * It would allow the developers to toggle the interfaces from
> the PowerTOP user interface.
> * It would show the Power Consumption impact of toggling on and off
> these settings.
>
> Your review of this patchset is intended to contribute to its full
> maturity so that when we reach the PowerTOP development all pieces would
> be ready for commit.
Nack. Testing PM stuff accurately is already a combinatorial nightmare, I
don't want more of this stuff. And for debugging the module options should
be good enough.
-Daniel
>
> Thank you in advance,
>
> Alexandra Yates (5):
> drm/i915: Add sys PSR toggle interface
> drm/i915: Add sys FBC toggle interface
> drm/i915: Add sys RC6 toggle interface
> drm/i915: Add sys drrs toggle interface
> drm-i915: Add sys IPS toggle interface
>
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.h | 12 ++
> drivers/gpu/drm/i915/i915_sysfs.c | 362 ++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_color.c | 12 +-
> drivers/gpu/drm/i915/intel_ddi.c | 15 +-
> drivers/gpu/drm/i915/intel_display.c | 55 ++++--
> drivers/gpu/drm/i915/intel_dp.c | 48 +++--
> drivers/gpu/drm/i915/intel_drv.h | 21 +-
> drivers/gpu/drm/i915/intel_fbc.c | 29 ++-
> drivers/gpu/drm/i915/intel_pm.c | 53 ++++-
> drivers/gpu/drm/i915/intel_psr.c | 36 +++-
> 12 files changed, 588 insertions(+), 65 deletions(-)
>
> --
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
` (5 preceding siblings ...)
2016-04-13 10:21 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Daniel Vetter
@ 2016-04-13 10:24 ` Jani Nikula
2016-04-13 12:59 ` Zanoni, Paulo R
7 siblings, 0 replies; 23+ messages in thread
From: Jani Nikula @ 2016-04-13 10:24 UTC (permalink / raw)
To: Alexandra Yates, intel-gfx, rodrigo.vivi, nivedita.swaminathan,
joe.konno
Cc: Daniel Vetter
On Tue, 12 Apr 2016, Alexandra Yates <alexandra.yates@linux.intel.com> wrote:
> Permits the user to identify and toggle values for PSR, FBC, RC6,
> DRRS, and IPS under /sys/class/drm/card0/power/. By enabling these
> features I'm looking to empower our customers, such as, power team,
> chrome OS, and platform integration teams to debug graphics power
> management features.
Most of these features have module parameters to enable or disable them,
with platform specific defaults. We enable and support each feature on
each platform where we have confidence it actually works. Conversely, we
don't support the features on platforms where we know they don't work,
and are therefore disabled by default. To underline this, the module
parameters are labeled "unsafe", and setting them taints the kernel. You
change the defaults, you're on your own.
If you google for the i915 module parameters, you'll find plenty of
pages recommending the users to set the various module parameters,
typically to gain power savings. It seems to me more often than not
people don't understand what they're doing when they enable the
features. It's not uncommon to see dmesgs in bug reports with all kinds
of module parameters set, regardless of whether they are relevant on the
platform in question or not, and it's not uncommon for the module
parameters to be the reason for the issue the bug reporter is seeing.
With this background, I am feeling rather hesitant about exposing all of
these features as an ABI through the sysfs. I fear the end result is
going to be PowerTOP recommending users to change the settings, leading
to more bug reports. I understand your intention is to help developers,
but from the ABI perspective there's really no such thing as "sysfs for
developers only". I'd probably feel less uneasy about having these in
the debugfs, because that sets the expectations right.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-12 19:18 ` [PATCH 0/5] PowerManagement Toggle for PowerTOP Alexandra Yates
` (6 preceding siblings ...)
2016-04-13 10:24 ` Jani Nikula
@ 2016-04-13 12:59 ` Zanoni, Paulo R
2016-04-13 14:50 ` Daniel Vetter
7 siblings, 1 reply; 23+ messages in thread
From: Zanoni, Paulo R @ 2016-04-13 12:59 UTC (permalink / raw)
To: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo,
alexandra.yates@linux.intel.com, Konno, Joe,
Swaminathan, Nivedita
Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> This project is explained in detail on the HAS
> https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnhD1Tes3f0
> 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
>
> Summary:
> Permits the user to identify and toggle values for PSR, FBC, RC6,
> DRRS, and IPS
> under /sys/class/drm/card0/power/. By enabling these features I'm
> looking to
> empower our customers, such as, power team, chrome OS, and platform
> integration teams
> to debug graphics power management features.
>
> From the current patchset PSR, FBC, RC6, and, DRRS are complete and
> past BAT, modset,
> and suspend/resume tests. For IPS I'm looking for feedback since IPS
> seems to change
> during runtime dynamically. Additionally, as a future project IPS
> would be better
> served if it is implemented by using the atomic check, pre-commit,
> commit, post-commit,
> a good example of this is the PSR enable/disable implementation.
>
> For this project to be completed needs to have in place the following
> components:
> (Need to be developed)
> * IPS toggle interface flesh-out
> * Tests added to intel-gpu-tools repo
> * Documentation for all sysfs added interfaces
> * PowerTOP component named: GFX-TOP. With the following requirements:
> * It would be available only to developers
> * It would allow the developers to toggle the interfaces from
> the PowerTOP user interface.
> * It would show the Power Consumption impact of toggling on and
> off
> these settings.
>
A small suggestion:
In the past, I've seen people testing i915.ko power saving features
just by "toggling" the features through the i915 module parameters
without doing anything else. In most of these cases, the machine was
not properly configured for power savings, so the conclusion was often
that the feature didn't save any power. While this was true (toggling
the feature didn't save any power), these people would have reached
different conclusions if they had, for example, changed their disk
power management policies. Do we have any sort of plan to try to
educate the powertop/gfxtop users regarding these things?
Maybe the powertop/gfxtop interface could expose some very-easy-to-
reach text explaining these things.
> Your review of this patchset is intended to contribute to its full
> maturity so that when we reach the PowerTOP development all pieces
> would
> be ready for commit.
>
> Thank you in advance,
>
> Alexandra Yates (5):
> drm/i915: Add sys PSR toggle interface
> drm/i915: Add sys FBC toggle interface
> drm/i915: Add sys RC6 toggle interface
> drm/i915: Add sys drrs toggle interface
> drm-i915: Add sys IPS toggle interface
>
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.c | 5 +-
> drivers/gpu/drm/i915/i915_drv.h | 12 ++
> drivers/gpu/drm/i915/i915_sysfs.c | 362
> ++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_color.c | 12 +-
> drivers/gpu/drm/i915/intel_ddi.c | 15 +-
> drivers/gpu/drm/i915/intel_display.c | 55 ++++--
> drivers/gpu/drm/i915/intel_dp.c | 48 +++--
> drivers/gpu/drm/i915/intel_drv.h | 21 +-
> drivers/gpu/drm/i915/intel_fbc.c | 29 ++-
> drivers/gpu/drm/i915/intel_pm.c | 53 ++++-
> drivers/gpu/drm/i915/intel_psr.c | 36 +++-
> 12 files changed, 588 insertions(+), 65 deletions(-)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-13 12:59 ` Zanoni, Paulo R
@ 2016-04-13 14:50 ` Daniel Vetter
2016-04-13 20:38 ` Vivi, Rodrigo
0 siblings, 1 reply; 23+ messages in thread
From: Daniel Vetter @ 2016-04-13 14:50 UTC (permalink / raw)
To: Zanoni, Paulo R
Cc: intel-gfx@lists.freedesktop.org, Konno, Joe,
Swaminathan, Nivedita, Vivi, Rodrigo
On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> > This project is explained in detail on the HAS
> > https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnhD1Tes3f0
> > 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
> >
> > Summary:
> > Permits the user to identify and toggle values for PSR, FBC, RC6,
> > DRRS, and IPS
> > under /sys/class/drm/card0/power/. By enabling these features I'm
> > looking to
> > empower our customers, such as, power team, chrome OS, and platform
> > integration teams
> > to debug graphics power management features.
> >
> > From the current patchset PSR, FBC, RC6, and, DRRS are complete and
> > past BAT, modset,
> > and suspend/resume tests. For IPS I'm looking for feedback since IPS
> > seems to change
> > during runtime dynamically. Additionally, as a future project IPS
> > would be better
> > served if it is implemented by using the atomic check, pre-commit,
> > commit, post-commit,
> > a good example of this is the PSR enable/disable implementation.
> >
> > For this project to be completed needs to have in place the following
> > components:
> > (Need to be developed)
> > * IPS toggle interface flesh-out
> > * Tests added to intel-gpu-tools repo
> > * Documentation for all sysfs added interfaces
> > * PowerTOP component named: GFX-TOP. With the following requirements:
> > * It would be available only to developers
> > * It would allow the developers to toggle the interfaces from
> > the PowerTOP user interface.
> > * It would show the Power Consumption impact of toggling on and
> > off
> > these settings.
> >
>
> A small suggestion:
>
> In the past, I've seen people testing i915.ko power saving features
> just by "toggling" the features through the i915 module parameters
> without doing anything else. In most of these cases, the machine was
> not properly configured for power savings, so the conclusion was often
> that the feature didn't save any power. While this was true (toggling
> the feature didn't save any power), these people would have reached
> different conclusions if they had, for example, changed their disk
> power management policies. Do we have any sort of plan to try to
> educate the powertop/gfxtop users regarding these things?
>
> Maybe the powertop/gfxtop interface could expose some very-easy-to-
> reach text explaining these things.
I think trying to extract/reuse those from the igt testcases might be
really useful. I.e. add docs that explain
a) what igt must past of a feature to be considered working
b) add some functions to igt testcases for manually enabling/disabling a
given rpm feature. The tests already know how to do that, and most have
nice explanations about what all should be changed on top to make things
work.
-Daniel
>
>
> > Your review of this patchset is intended to contribute to its full
> > maturity so that when we reach the PowerTOP development all pieces
> > would
> > be ready for commit.
> >
> > Thank you in advance,
> >
> > Alexandra Yates (5):
> > drm/i915: Add sys PSR toggle interface
> > drm/i915: Add sys FBC toggle interface
> > drm/i915: Add sys RC6 toggle interface
> > drm/i915: Add sys drrs toggle interface
> > drm-i915: Add sys IPS toggle interface
> >
> > drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> > drivers/gpu/drm/i915/i915_drv.c | 5 +-
> > drivers/gpu/drm/i915/i915_drv.h | 12 ++
> > drivers/gpu/drm/i915/i915_sysfs.c | 362
> > ++++++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_color.c | 12 +-
> > drivers/gpu/drm/i915/intel_ddi.c | 15 +-
> > drivers/gpu/drm/i915/intel_display.c | 55 ++++--
> > drivers/gpu/drm/i915/intel_dp.c | 48 +++--
> > drivers/gpu/drm/i915/intel_drv.h | 21 +-
> > drivers/gpu/drm/i915/intel_fbc.c | 29 ++-
> > drivers/gpu/drm/i915/intel_pm.c | 53 ++++-
> > drivers/gpu/drm/i915/intel_psr.c | 36 +++-
> > 12 files changed, 588 insertions(+), 65 deletions(-)
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-13 14:50 ` Daniel Vetter
@ 2016-04-13 20:38 ` Vivi, Rodrigo
2016-04-13 20:46 ` Daniel Vetter
0 siblings, 1 reply; 23+ messages in thread
From: Vivi, Rodrigo @ 2016-04-13 20:38 UTC (permalink / raw)
To: daniel@ffwll.ch, Zanoni, Paulo R
Cc: intel-gfx@lists.freedesktop.org, Konno, Joe,
Swaminathan, Nivedita
On Wed, 2016-04-13 at 16:50 +0200, Daniel Vetter wrote:
> On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R wrote:
> > Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> > > This project is explained in detail on the HAS
> > > https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnhD1Te
> > > s3f0
> > > 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
> > >
> > > Summary:
> > > Permits the user to identify and toggle values for PSR, FBC, RC6,
> > > DRRS, and IPS
> > > under /sys/class/drm/card0/power/. By enabling these features
> > > I'm
> > > looking to
> > > empower our customers, such as, power team, chrome OS, and
> > > platform
> > > integration teams
> > > to debug graphics power management features.
> > >
> > > From the current patchset PSR, FBC, RC6, and, DRRS are complete
> > > and
> > > past BAT, modset,
> > > and suspend/resume tests. For IPS I'm looking for feedback since
> > > IPS
> > > seems to change
> > > during runtime dynamically. Additionally, as a future project
> > > IPS
> > > would be better
> > > served if it is implemented by using the atomic check, pre
> > > -commit,
> > > commit, post-commit,
> > > a good example of this is the PSR enable/disable implementation.
> > >
> > > For this project to be completed needs to have in place the
> > > following
> > > components:
> > > (Need to be developed)
> > > * IPS toggle interface flesh-out
> > > * Tests added to intel-gpu-tools repo
> > > * Documentation for all sysfs added interfaces
> > > * PowerTOP component named: GFX-TOP. With the following
> > > requirements:
> > > * It would be available only to developers
> > > * It would allow the developers to toggle the interfaces from
> > > the PowerTOP user interface.
> > > * It would show the Power Consumption impact of toggling on and
> > > off
> > > these settings.
> > >
> >
> > A small suggestion:
> >
> > In the past, I've seen people testing i915.ko power saving features
> > just by "toggling" the features through the i915 module parameters
> > without doing anything else. In most of these cases, the machine
> > was
> > not properly configured for power savings, so the conclusion was
> > often
> > that the feature didn't save any power. While this was true
> > (toggling
> > the feature didn't save any power), these people would have reached
> > different conclusions if they had, for example, changed their disk
> > power management policies. Do we have any sort of plan to try to
> > educate the powertop/gfxtop users regarding these things?
But the toggling the module parameter doesn't change anything because
it depends on the next modeset to have any effect o nmnost of pm
features.
The idea is that we don't need this with the sysfs and the toggle
should represent an immediate change on the feature behaviour. Although
it is a fact that the features also depends on other conditions like
PSR depends on a fully idle screen for few frames at least.
> >
> > Maybe the powertop/gfxtop interface could expose some very-easy-to-
> > reach text explaining these things.
Good idea.
>
> I think trying to extract/reuse those from the igt testcases might be
> really useful. I.e. add docs that explain
> a) what igt must past of a feature to be considered working
> b) add some functions to igt testcases for manually
> enabling/disabling a
> given rpm feature. The tests already know how to do that, and most
> have
> nice explanations about what all should be changed on top to make
> things
> work.
Actually I had the opposite direction in mind. I was expecting we could
make igt tests to start using this sysfs toggles.
The problem I see with current igt tests way is that we need to change
the parameter and depend on the next full modeset happening. Well it is
true that we need to have the modeset on the tests anyway, but I
believe we could reduce that restriction and also powertop wouldn't do
any modeset.
> -Daniel
>
> >
> >
> > > Your review of this patchset is intended to contribute to its
> > > full
> > > maturity so that when we reach the PowerTOP development all
> > > pieces
> > > would
> > > be ready for commit.
> > >
> > > Thank you in advance,
> > >
> > > Alexandra Yates (5):
> > > drm/i915: Add sys PSR toggle interface
> > > drm/i915: Add sys FBC toggle interface
> > > drm/i915: Add sys RC6 toggle interface
> > > drm/i915: Add sys drrs toggle interface
> > > drm-i915: Add sys IPS toggle interface
> > >
> > > drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> > > drivers/gpu/drm/i915/i915_drv.c | 5 +-
> > > drivers/gpu/drm/i915/i915_drv.h | 12 ++
> > > drivers/gpu/drm/i915/i915_sysfs.c | 362
> > > ++++++++++++++++++++++++++++++++++-
> > > drivers/gpu/drm/i915/intel_color.c | 12 +-
> > > drivers/gpu/drm/i915/intel_ddi.c | 15 +-
> > > drivers/gpu/drm/i915/intel_display.c | 55 ++++--
> > > drivers/gpu/drm/i915/intel_dp.c | 48 +++--
> > > drivers/gpu/drm/i915/intel_drv.h | 21 +-
> > > drivers/gpu/drm/i915/intel_fbc.c | 29 ++-
> > > drivers/gpu/drm/i915/intel_pm.c | 53 ++++-
> > > drivers/gpu/drm/i915/intel_psr.c | 36 +++-
> > > 12 files changed, 588 insertions(+), 65 deletions(-)
> > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-13 20:38 ` Vivi, Rodrigo
@ 2016-04-13 20:46 ` Daniel Vetter
2016-04-13 20:49 ` Daniel Vetter
2016-04-13 22:06 ` Vivi, Rodrigo
0 siblings, 2 replies; 23+ messages in thread
From: Daniel Vetter @ 2016-04-13 20:46 UTC (permalink / raw)
To: Vivi, Rodrigo
Cc: Swaminathan, Nivedita, intel-gfx@lists.freedesktop.org,
Konno, Joe, Zanoni, Paulo R
On Wed, Apr 13, 2016 at 10:38 PM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
> On Wed, 2016-04-13 at 16:50 +0200, Daniel Vetter wrote:
>> On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R wrote:
>> > Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
>> > > This project is explained in detail on the HAS
>> > > https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnhD1Te
>> > > s3f0
>> > > 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
>> > >
>> > > Summary:
>> > > Permits the user to identify and toggle values for PSR, FBC, RC6,
>> > > DRRS, and IPS
>> > > under /sys/class/drm/card0/power/. By enabling these features
>> > > I'm
>> > > looking to
>> > > empower our customers, such as, power team, chrome OS, and
>> > > platform
>> > > integration teams
>> > > to debug graphics power management features.
>> > >
>> > > From the current patchset PSR, FBC, RC6, and, DRRS are complete
>> > > and
>> > > past BAT, modset,
>> > > and suspend/resume tests. For IPS I'm looking for feedback since
>> > > IPS
>> > > seems to change
>> > > during runtime dynamically. Additionally, as a future project
>> > > IPS
>> > > would be better
>> > > served if it is implemented by using the atomic check, pre
>> > > -commit,
>> > > commit, post-commit,
>> > > a good example of this is the PSR enable/disable implementation.
>> > >
>> > > For this project to be completed needs to have in place the
>> > > following
>> > > components:
>> > > (Need to be developed)
>> > > * IPS toggle interface flesh-out
>> > > * Tests added to intel-gpu-tools repo
>> > > * Documentation for all sysfs added interfaces
>> > > * PowerTOP component named: GFX-TOP. With the following
>> > > requirements:
>> > > * It would be available only to developers
>> > > * It would allow the developers to toggle the interfaces from
>> > > the PowerTOP user interface.
>> > > * It would show the Power Consumption impact of toggling on and
>> > > off
>> > > these settings.
>> > >
>> >
>> > A small suggestion:
>> >
>> > In the past, I've seen people testing i915.ko power saving features
>> > just by "toggling" the features through the i915 module parameters
>> > without doing anything else. In most of these cases, the machine
>> > was
>> > not properly configured for power savings, so the conclusion was
>> > often
>> > that the feature didn't save any power. While this was true
>> > (toggling
>> > the feature didn't save any power), these people would have reached
>> > different conclusions if they had, for example, changed their disk
>> > power management policies. Do we have any sort of plan to try to
>> > educate the powertop/gfxtop users regarding these things?
>
> But the toggling the module parameter doesn't change anything because
> it depends on the next modeset to have any effect o nmnost of pm
> features.
> The idea is that we don't need this with the sysfs and the toggle
> should represent an immediate change on the feature behaviour. Although
> it is a fact that the features also depends on other conditions like
> PSR depends on a fully idle screen for few frames at least.
Well that part where the knob needs to kick the system into working
correctly (what if we need an expensive w/a that's hard to reset at
runtime) is what scares me.
>> > Maybe the powertop/gfxtop interface could expose some very-easy-to-
>> > reach text explaining these things.
>
> Good idea.
>
>>
>> I think trying to extract/reuse those from the igt testcases might be
>> really useful. I.e. add docs that explain
>> a) what igt must past of a feature to be considered working
>> b) add some functions to igt testcases for manually
>> enabling/disabling a
>> given rpm feature. The tests already know how to do that, and most
>> have
>> nice explanations about what all should be changed on top to make
>> things
>> work.
>
> Actually I had the opposite direction in mind. I was expecting we could
> make igt tests to start using this sysfs toggles.
> The problem I see with current igt tests way is that we need to change
> the parameter and depend on the next full modeset happening. Well it is
> true that we need to have the modeset on the tests anyway, but I
> believe we could reduce that restriction and also powertop wouldn't do
> any modeset.
So taking more steps backwards: The goal here is to help debug runtime
pm issues. The proposed solutions is that you can toggel stuff in
powertop.
Is this really how the experts debug runtime pm issues?
At least when I hack around my first questions are:
- do we get residenency/how much, and maybe what exactly is blocking
it. I think we discussed this at the meeting and agreed that
more/officially exposed residency counters.
- once I have a suspect I keep digging into that direction, using
debugfs/unsafe module options, debugfs information, whatever.
I fully agree that we can try to do a better job on documenting this
stuff, and maybe exposing more information through stable interfaces.
But adding new abi to enable/disable stuff sounds like jumping into
the middle of the use-case story, ignoring everything else. And I
think that was roughly what we agreed at that meeting - we can add
knobs when it makes sense, but only when it makes sense. I'm still not
sold on the "debug by powertop" solution.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-13 20:46 ` Daniel Vetter
@ 2016-04-13 20:49 ` Daniel Vetter
2016-04-13 22:06 ` Vivi, Rodrigo
1 sibling, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2016-04-13 20:49 UTC (permalink / raw)
To: Vivi, Rodrigo
Cc: Swaminathan, Nivedita, intel-gfx@lists.freedesktop.org,
Konno, Joe, Zanoni, Paulo R
On Wed, Apr 13, 2016 at 10:46 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> I fully agree that we can try to do a better job on documenting this
> stuff, and maybe exposing more information through stable interfaces.
>
> But adding new abi to enable/disable stuff sounds like jumping into
> the middle of the use-case story, ignoring everything else. And I
> think that was roughly what we agreed at that meeting - we can add
> knobs when it makes sense, but only when it makes sense. I'm still not
> sold on the "debug by powertop" solution.
Maybe over the top: Exposing a list of marketing features ( why are
e.g. watermark levels not in there - it seems completely arbitrary)
and then debug issues by randomly whacking those until insight hits
you isn't systematic debugging. And I don't want to enshrine that kind
of random whacking into ABI as the recommended way to debug i915.ko
issues.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-13 20:46 ` Daniel Vetter
2016-04-13 20:49 ` Daniel Vetter
@ 2016-04-13 22:06 ` Vivi, Rodrigo
2016-04-14 9:48 ` Daniel Vetter
1 sibling, 1 reply; 23+ messages in thread
From: Vivi, Rodrigo @ 2016-04-13 22:06 UTC (permalink / raw)
To: daniel@ffwll.ch
Cc: Swaminathan, Nivedita, intel-gfx@lists.freedesktop.org,
Konno, Joe, Zanoni, Paulo R
On Wed, 2016-04-13 at 22:46 +0200, Daniel Vetter wrote:
> On Wed, Apr 13, 2016 at 10:38 PM, Vivi, Rodrigo <
> rodrigo.vivi@intel.com> wrote:
> > On Wed, 2016-04-13 at 16:50 +0200, Daniel Vetter wrote:
> > > On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R wrote:
> > > > Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> > > > > This project is explained in detail on the HAS
> > > > > https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnh
> > > > > D1Te
> > > > > s3f0
> > > > > 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
> > > > >
> > > > > Summary:
> > > > > Permits the user to identify and toggle values for PSR, FBC,
> > > > > RC6,
> > > > > DRRS, and IPS
> > > > > under /sys/class/drm/card0/power/. By enabling these
> > > > > features
> > > > > I'm
> > > > > looking to
> > > > > empower our customers, such as, power team, chrome OS, and
> > > > > platform
> > > > > integration teams
> > > > > to debug graphics power management features.
> > > > >
> > > > > From the current patchset PSR, FBC, RC6, and, DRRS are
> > > > > complete
> > > > > and
> > > > > past BAT, modset,
> > > > > and suspend/resume tests. For IPS I'm looking for feedback
> > > > > since
> > > > > IPS
> > > > > seems to change
> > > > > during runtime dynamically. Additionally, as a future
> > > > > project
> > > > > IPS
> > > > > would be better
> > > > > served if it is implemented by using the atomic check, pre
> > > > > -commit,
> > > > > commit, post-commit,
> > > > > a good example of this is the PSR enable/disable
> > > > > implementation.
> > > > >
> > > > > For this project to be completed needs to have in place the
> > > > > following
> > > > > components:
> > > > > (Need to be developed)
> > > > > * IPS toggle interface flesh-out
> > > > > * Tests added to intel-gpu-tools repo
> > > > > * Documentation for all sysfs added interfaces
> > > > > * PowerTOP component named: GFX-TOP. With the following
> > > > > requirements:
> > > > > * It would be available only to developers
> > > > > * It would allow the developers to toggle the interfaces
> > > > > from
> > > > > the PowerTOP user interface.
> > > > > * It would show the Power Consumption impact of toggling on
> > > > > and
> > > > > off
> > > > > these settings.
> > > > >
> > > >
> > > > A small suggestion:
> > > >
> > > > In the past, I've seen people testing i915.ko power saving
> > > > features
> > > > just by "toggling" the features through the i915 module
> > > > parameters
> > > > without doing anything else. In most of these cases, the
> > > > machine
> > > > was
> > > > not properly configured for power savings, so the conclusion
> > > > was
> > > > often
> > > > that the feature didn't save any power. While this was true
> > > > (toggling
> > > > the feature didn't save any power), these people would have
> > > > reached
> > > > different conclusions if they had, for example, changed their
> > > > disk
> > > > power management policies. Do we have any sort of plan to try
> > > > to
> > > > educate the powertop/gfxtop users regarding these things?
> >
> > But the toggling the module parameter doesn't change anything
> > because
> > it depends on the next modeset to have any effect o nmnost of pm
> > features.
> > The idea is that we don't need this with the sysfs and the toggle
> > should represent an immediate change on the feature behaviour.
> > Although
> > it is a fact that the features also depends on other conditions
> > like
> > PSR depends on a fully idle screen for few frames at least.
>
> Well that part where the knob needs to kick the system into working
> correctly (what if we need an expensive w/a that's hard to reset at
> runtime) is what scares me.
>
> > > > Maybe the powertop/gfxtop interface could expose some very-easy
> > > > -to-
> > > > reach text explaining these things.
> >
> > Good idea.
> >
> > >
> > > I think trying to extract/reuse those from the igt testcases
> > > might be
> > > really useful. I.e. add docs that explain
> > > a) what igt must past of a feature to be considered working
> > > b) add some functions to igt testcases for manually
> > > enabling/disabling a
> > > given rpm feature. The tests already know how to do that, and
> > > most
> > > have
> > > nice explanations about what all should be changed on top to make
> > > things
> > > work.
> >
> > Actually I had the opposite direction in mind. I was expecting we
> > could
> > make igt tests to start using this sysfs toggles.
> > The problem I see with current igt tests way is that we need to
> > change
> > the parameter and depend on the next full modeset happening. Well
> > it is
> > true that we need to have the modeset on the tests anyway, but I
> > believe we could reduce that restriction and also powertop wouldn't
> > do
> > any modeset.
>
> So taking more steps backwards: The goal here is to help debug
> runtime
> pm issues. The proposed solutions is that you can toggel stuff in
> powertop.
>
> Is this really how the experts debug runtime pm issues?
I've seen many experts (non-gfx devs) around using the powertop for
debuging PM related issues and checking PC states, etc.
One very good case to have at least the informative tab there is
exactly help this use case: One developer in the power lab cannot see
deep PC state residencies with screen on and look to the gfx tab and
see if DMC is enabled or not and PSR is enabled or not. And the extra
is if is not enabled but possible it would just toggle directly from
that interface.
Or at least the informative with everything together they would know if
i915 is somehow the culprit for blocking PC states without need to hunt
different debugfs interfaces they are not familiar with.
> At least when I hack around my first questions are:
> - do we get residenency/how much, and maybe what exactly is blocking
> it. I think we discussed this at the meeting and agreed that
> more/officially exposed residency counters.
The problem is that we don't have this counters when power wells are
down. So we lost all DC counters and PSR counter when DC entry.
So no residencies. But I still like the idea of the status.
>
> - once I have a suspect I keep digging into that direction, using
> debugfs/unsafe module options, debugfs information, whatever.
>
> I fully agree that we can try to do a better job on documenting this
> stuff, and maybe exposing more information through stable interfaces.
>
> But adding new abi to enable/disable stuff sounds like jumping into
> the middle of the use-case story, ignoring everything else. And I
> think that was roughly what we agreed at that meeting - we can add
> knobs when it makes sense, but only when it makes sense. I'm still
> not
> sold on the "debug by powertop" solution.
what about at least informative for now?
Watermark is my bad. I forgot to include it and also to toggle it we
would need to add support for disabling it what we don't have at all
but I believe we should have anyways.
> -Daniel
Thanks,
Rodrigo.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-13 22:06 ` Vivi, Rodrigo
@ 2016-04-14 9:48 ` Daniel Vetter
2016-04-14 17:17 ` Alexandra Yates
2016-04-15 18:12 ` Vivi, Rodrigo
0 siblings, 2 replies; 23+ messages in thread
From: Daniel Vetter @ 2016-04-14 9:48 UTC (permalink / raw)
To: Vivi, Rodrigo
Cc: Swaminathan, Nivedita, intel-gfx@lists.freedesktop.org,
Konno, Joe, Zanoni, Paulo R
On Wed, Apr 13, 2016 at 10:06:57PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2016-04-13 at 22:46 +0200, Daniel Vetter wrote:
> > On Wed, Apr 13, 2016 at 10:38 PM, Vivi, Rodrigo <
> > rodrigo.vivi@intel.com> wrote:
> > > On Wed, 2016-04-13 at 16:50 +0200, Daniel Vetter wrote:
> > > > On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R wrote:
> > > > > Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> > > > > > This project is explained in detail on the HAS
> > > > > > https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnh
> > > > > > D1Te
> > > > > > s3f0
> > > > > > 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
> > > > > >
> > > > > > Summary:
> > > > > > Permits the user to identify and toggle values for PSR, FBC,
> > > > > > RC6,
> > > > > > DRRS, and IPS
> > > > > > under /sys/class/drm/card0/power/. By enabling these
> > > > > > features
> > > > > > I'm
> > > > > > looking to
> > > > > > empower our customers, such as, power team, chrome OS, and
> > > > > > platform
> > > > > > integration teams
> > > > > > to debug graphics power management features.
> > > > > >
> > > > > > From the current patchset PSR, FBC, RC6, and, DRRS are
> > > > > > complete
> > > > > > and
> > > > > > past BAT, modset,
> > > > > > and suspend/resume tests. For IPS I'm looking for feedback
> > > > > > since
> > > > > > IPS
> > > > > > seems to change
> > > > > > during runtime dynamically. Additionally, as a future
> > > > > > project
> > > > > > IPS
> > > > > > would be better
> > > > > > served if it is implemented by using the atomic check, pre
> > > > > > -commit,
> > > > > > commit, post-commit,
> > > > > > a good example of this is the PSR enable/disable
> > > > > > implementation.
> > > > > >
> > > > > > For this project to be completed needs to have in place the
> > > > > > following
> > > > > > components:
> > > > > > (Need to be developed)
> > > > > > * IPS toggle interface flesh-out
> > > > > > * Tests added to intel-gpu-tools repo
> > > > > > * Documentation for all sysfs added interfaces
> > > > > > * PowerTOP component named: GFX-TOP. With the following
> > > > > > requirements:
> > > > > > * It would be available only to developers
> > > > > > * It would allow the developers to toggle the interfaces
> > > > > > from
> > > > > > the PowerTOP user interface.
> > > > > > * It would show the Power Consumption impact of toggling on
> > > > > > and
> > > > > > off
> > > > > > these settings.
> > > > > >
> > > > >
> > > > > A small suggestion:
> > > > >
> > > > > In the past, I've seen people testing i915.ko power saving
> > > > > features
> > > > > just by "toggling" the features through the i915 module
> > > > > parameters
> > > > > without doing anything else. In most of these cases, the
> > > > > machine
> > > > > was
> > > > > not properly configured for power savings, so the conclusion
> > > > > was
> > > > > often
> > > > > that the feature didn't save any power. While this was true
> > > > > (toggling
> > > > > the feature didn't save any power), these people would have
> > > > > reached
> > > > > different conclusions if they had, for example, changed their
> > > > > disk
> > > > > power management policies. Do we have any sort of plan to try
> > > > > to
> > > > > educate the powertop/gfxtop users regarding these things?
> > >
> > > But the toggling the module parameter doesn't change anything
> > > because
> > > it depends on the next modeset to have any effect o nmnost of pm
> > > features.
> > > The idea is that we don't need this with the sysfs and the toggle
> > > should represent an immediate change on the feature behaviour.
> > > Although
> > > it is a fact that the features also depends on other conditions
> > > like
> > > PSR depends on a fully idle screen for few frames at least.
> >
> > Well that part where the knob needs to kick the system into working
> > correctly (what if we need an expensive w/a that's hard to reset at
> > runtime) is what scares me.
> >
> > > > > Maybe the powertop/gfxtop interface could expose some very-easy
> > > > > -to-
> > > > > reach text explaining these things.
> > >
> > > Good idea.
> > >
> > > >
> > > > I think trying to extract/reuse those from the igt testcases
> > > > might be
> > > > really useful. I.e. add docs that explain
> > > > a) what igt must past of a feature to be considered working
> > > > b) add some functions to igt testcases for manually
> > > > enabling/disabling a
> > > > given rpm feature. The tests already know how to do that, and
> > > > most
> > > > have
> > > > nice explanations about what all should be changed on top to make
> > > > things
> > > > work.
> > >
> > > Actually I had the opposite direction in mind. I was expecting we
> > > could
> > > make igt tests to start using this sysfs toggles.
> > > The problem I see with current igt tests way is that we need to
> > > change
> > > the parameter and depend on the next full modeset happening. Well
> > > it is
> > > true that we need to have the modeset on the tests anyway, but I
> > > believe we could reduce that restriction and also powertop wouldn't
> > > do
> > > any modeset.
> >
> > So taking more steps backwards: The goal here is to help debug
> > runtime
> > pm issues. The proposed solutions is that you can toggel stuff in
> > powertop.
> >
> > Is this really how the experts debug runtime pm issues?
>
> I've seen many experts (non-gfx devs) around using the powertop for
> debuging PM related issues and checking PC states, etc.
>
> One very good case to have at least the informative tab there is
> exactly help this use case: One developer in the power lab cannot see
> deep PC state residencies with screen on and look to the gfx tab and
> see if DMC is enabled or not and PSR is enabled or not. And the extra
> is if is not enabled but possible it would just toggle directly from
> that interface.
>
> Or at least the informative with everything together they would know if
> i915 is somehow the culprit for blocking PC states without need to hunt
> different debugfs interfaces they are not familiar with.
I'm fully on board with you on the "figure out that i915 is the culprit".
I don't get why we need to be able to toggle stuff from within powertop to
do that. Information-only should be enough. Maybe even some of the hints
Paulo suggested (like "you haven't loaded DMC, pls install").
Adding toggles into powertop - I have no idea how that helps with
debugging, beyond the modparams we have already. This is also because I'm
strongly opposed as "powertop the solution to enable rpm stuff by
default", and that's seems to be the reason a lot of folks want these
knobs.
Imo if something doesn't work that should, the next step is to file a bug
report with the i915 experts to fix this. Not knobs and hacks in powertop
...
/me fighting a bit a holy war
> > At least when I hack around my first questions are:
> > - do we get residenency/how much, and maybe what exactly is blocking
> > it. I think we discussed this at the meeting and agreed that
> > more/officially exposed residency counters.
>
> The problem is that we don't have this counters when power wells are
> down. So we lost all DC counters and PSR counter when DC entry.
> So no residencies. But I still like the idea of the status.
And I guess no way to at least somewhat fake those in software?
> > - once I have a suspect I keep digging into that direction, using
> > debugfs/unsafe module options, debugfs information, whatever.
> >
> > I fully agree that we can try to do a better job on documenting this
> > stuff, and maybe exposing more information through stable interfaces.
> >
> > But adding new abi to enable/disable stuff sounds like jumping into
> > the middle of the use-case story, ignoring everything else. And I
> > think that was roughly what we agreed at that meeting - we can add
> > knobs when it makes sense, but only when it makes sense. I'm still
> > not
> > sold on the "debug by powertop" solution.
>
> what about at least informative for now?
Useful information sounds good. I'm freaking out about the maintenance of
adding knobs and stuff that's supposed to work.
> Watermark is my bad. I forgot to include it and also to toggle it we
> would need to add support for disabling it what we don't have at all
> but I believe we should have anyways.
Watermarks was a bit a trick example, since imo adding code to
fake-disable low-power watermark levels is nonsense. It's only going to
help with very specific bugs, and if you don't understand the wm
programming model for your platforms it's going to be totally useless
information. Misconfiguring the driver to make it crap doesn't really tell
you a lot ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-14 9:48 ` Daniel Vetter
@ 2016-04-14 17:17 ` Alexandra Yates
2016-04-15 18:12 ` Vivi, Rodrigo
1 sibling, 0 replies; 23+ messages in thread
From: Alexandra Yates @ 2016-04-14 17:17 UTC (permalink / raw)
To: Daniel Vetter, Vivi, Rodrigo
Cc: Swaminathan, Nivedita, intel-gfx@lists.freedesktop.org,
Konno, Joe, Zanoni, Paulo R
On 04/14/2016 02:48 AM, Daniel Vetter wrote:
> On Wed, Apr 13, 2016 at 10:06:57PM +0000, Vivi, Rodrigo wrote:
>> On Wed, 2016-04-13 at 22:46 +0200, Daniel Vetter wrote:
>>> On Wed, Apr 13, 2016 at 10:38 PM, Vivi, Rodrigo <
>>> rodrigo.vivi@intel.com> wrote:
>>>> On Wed, 2016-04-13 at 16:50 +0200, Daniel Vetter wrote:
>>>>> On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R wrote:
>>>>>> Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
>>>>>>> This project is explained in detail on the HAS
>>>>>>> https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnh
>>>>>>> D1Te
>>>>>>> s3f0
>>>>>>> 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
>>>>>>>
>>>>>>> Summary:
>>>>>>> Permits the user to identify and toggle values for PSR, FBC,
>>>>>>> RC6,
>>>>>>> DRRS, and IPS
>>>>>>> under /sys/class/drm/card0/power/. By enabling these
>>>>>>> features
>>>>>>> I'm
>>>>>>> looking to
>>>>>>> empower our customers, such as, power team, chrome OS, and
>>>>>>> platform
>>>>>>> integration teams
>>>>>>> to debug graphics power management features.
>>>>>>>
>>>>>>> From the current patchset PSR, FBC, RC6, and, DRRS are
>>>>>>> complete
>>>>>>> and
>>>>>>> past BAT, modset,
>>>>>>> and suspend/resume tests. For IPS I'm looking for feedback
>>>>>>> since
>>>>>>> IPS
>>>>>>> seems to change
>>>>>>> during runtime dynamically. Additionally, as a future
>>>>>>> project
>>>>>>> IPS
>>>>>>> would be better
>>>>>>> served if it is implemented by using the atomic check, pre
>>>>>>> -commit,
>>>>>>> commit, post-commit,
>>>>>>> a good example of this is the PSR enable/disable
>>>>>>> implementation.
>>>>>>>
>>>>>>> For this project to be completed needs to have in place the
>>>>>>> following
>>>>>>> components:
>>>>>>> (Need to be developed)
>>>>>>> * IPS toggle interface flesh-out
>>>>>>> * Tests added to intel-gpu-tools repo
>>>>>>> * Documentation for all sysfs added interfaces
>>>>>>> * PowerTOP component named: GFX-TOP. With the following
>>>>>>> requirements:
>>>>>>> * It would be available only to developers
>>>>>>> * It would allow the developers to toggle the interfaces
>>>>>>> from
>>>>>>> the PowerTOP user interface.
>>>>>>> * It would show the Power Consumption impact of toggling on
>>>>>>> and
>>>>>>> off
>>>>>>> these settings.
>>>>>>>
>>>>>>
>>>>>> A small suggestion:
>>>>>>
>>>>>> In the past, I've seen people testing i915.ko power saving
>>>>>> features
>>>>>> just by "toggling" the features through the i915 module
>>>>>> parameters
>>>>>> without doing anything else. In most of these cases, the
>>>>>> machine
>>>>>> was
>>>>>> not properly configured for power savings, so the conclusion
>>>>>> was
>>>>>> often
>>>>>> that the feature didn't save any power. While this was true
>>>>>> (toggling
>>>>>> the feature didn't save any power), these people would have
>>>>>> reached
>>>>>> different conclusions if they had, for example, changed their
>>>>>> disk
>>>>>> power management policies. Do we have any sort of plan to try
>>>>>> to
>>>>>> educate the powertop/gfxtop users regarding these things?
>>>>
>>>> But the toggling the module parameter doesn't change anything
>>>> because
>>>> it depends on the next modeset to have any effect o nmnost of pm
>>>> features.
>>>> The idea is that we don't need this with the sysfs and the toggle
>>>> should represent an immediate change on the feature behaviour.
>>>> Although
>>>> it is a fact that the features also depends on other conditions
>>>> like
>>>> PSR depends on a fully idle screen for few frames at least.
>>>
>>> Well that part where the knob needs to kick the system into working
>>> correctly (what if we need an expensive w/a that's hard to reset at
>>> runtime) is what scares me.
>>>
>>>>>> Maybe the powertop/gfxtop interface could expose some very-easy
>>>>>> -to-
>>>>>> reach text explaining these things.
>>>>
>>>> Good idea.
>>>>
>>>>>
>>>>> I think trying to extract/reuse those from the igt testcases
>>>>> might be
>>>>> really useful. I.e. add docs that explain
>>>>> a) what igt must past of a feature to be considered working
>>>>> b) add some functions to igt testcases for manually
>>>>> enabling/disabling a
>>>>> given rpm feature. The tests already know how to do that, and
>>>>> most
>>>>> have
>>>>> nice explanations about what all should be changed on top to make
>>>>> things
>>>>> work.
>>>>
>>>> Actually I had the opposite direction in mind. I was expecting we
>>>> could
>>>> make igt tests to start using this sysfs toggles.
>>>> The problem I see with current igt tests way is that we need to
>>>> change
>>>> the parameter and depend on the next full modeset happening. Well
>>>> it is
>>>> true that we need to have the modeset on the tests anyway, but I
>>>> believe we could reduce that restriction and also powertop wouldn't
>>>> do
>>>> any modeset.
>>>
>>> So taking more steps backwards: The goal here is to help debug
>>> runtime
>>> pm issues. The proposed solutions is that you can toggel stuff in
>>> powertop.
>>>
>>> Is this really how the experts debug runtime pm issues?
>>
>> I've seen many experts (non-gfx devs) around using the powertop for
>> debuging PM related issues and checking PC states, etc.
>>
>> One very good case to have at least the informative tab there is
>> exactly help this use case: One developer in the power lab cannot see
>> deep PC state residencies with screen on and look to the gfx tab and
>> see if DMC is enabled or not and PSR is enabled or not. And the extra
>> is if is not enabled but possible it would just toggle directly from
>> that interface.
>>
>> Or at least the informative with everything together they would know if
>> i915 is somehow the culprit for blocking PC states without need to hunt
>> different debugfs interfaces they are not familiar with.
>
> I'm fully on board with you on the "figure out that i915 is the culprit".
> I don't get why we need to be able to toggle stuff from within powertop to
> do that. Information-only should be enough. Maybe even some of the hints
> Paulo suggested (like "you haven't loaded DMC, pls install").
>
> Adding toggles into powertop - I have no idea how that helps with
> debugging, beyond the modparams we have already. This is also because I'm
> strongly opposed as "powertop the solution to enable rpm stuff by
> default", and that's seems to be the reason a lot of folks want these
> knobs.
>
> Imo if something doesn't work that should, the next step is to file a bug
> report with the i915 experts to fix this. Not knobs and hacks in powertop
> ...
>
> /me fighting a bit a holy war
>
>>> At least when I hack around my first questions are:
>>> - do we get residenency/how much, and maybe what exactly is blocking
>>> it. I think we discussed this at the meeting and agreed that
>>> more/officially exposed residency counters.
>>
>> The problem is that we don't have this counters when power wells are
>> down. So we lost all DC counters and PSR counter when DC entry.
>> So no residencies. But I still like the idea of the status.
>
> And I guess no way to at least somewhat fake those in software?
>
>>> - once I have a suspect I keep digging into that direction, using
>>> debugfs/unsafe module options, debugfs information, whatever.
>>>
>>> I fully agree that we can try to do a better job on documenting this
>>> stuff, and maybe exposing more information through stable interfaces.
>>>
>>> But adding new abi to enable/disable stuff sounds like jumping into
>>> the middle of the use-case story, ignoring everything else. And I
>>> think that was roughly what we agreed at that meeting - we can add
>>> knobs when it makes sense, but only when it makes sense. I'm still
>>> not
>>> sold on the "debug by powertop" solution.
>>
>> what about at least informative for now?
>
> Useful information sounds good. I'm freaking out about the maintenance of
> adding knobs and stuff that's supposed to work.
>
>> Watermark is my bad. I forgot to include it and also to toggle it we
>> would need to add support for disabling it what we don't have at all
>> but I believe we should have anyways.
>
> Watermarks was a bit a trick example, since imo adding code to
> fake-disable low-power watermark levels is nonsense. It's only going to
> help with very specific bugs, and if you don't understand the wm
> programming model for your platforms it's going to be totally useless
> information. Misconfiguring the driver to make it crap doesn't really tell
> you a lot ...
> -Daniel
>
Thank you for all the reviews and the constructive feedback. I'm
pleased the patches got your attention.
I'll spin today the patches to show only the state of the features on
sysfs. I still think there is some value on toggling the interfaces,
however the stability of the driver for the end user seems to win that
battle for now :)
I see the benefit on adding more documentation around each feature and
how they impact RPM. I think the platform enabling engineer would find
this kind of information interesting when debugging the many GFX bugs
that often are the culprit when reaching deep C states.
--
Thank you,
<Alexandra>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-14 9:48 ` Daniel Vetter
2016-04-14 17:17 ` Alexandra Yates
@ 2016-04-15 18:12 ` Vivi, Rodrigo
2016-04-20 12:44 ` Daniel Vetter
1 sibling, 1 reply; 23+ messages in thread
From: Vivi, Rodrigo @ 2016-04-15 18:12 UTC (permalink / raw)
To: daniel@ffwll.ch
Cc: Swaminathan, Nivedita, intel-gfx@lists.freedesktop.org,
Konno, Joe, Zanoni, Paulo R
On Thu, 2016-04-14 at 11:48 +0200, Daniel Vetter wrote:
> On Wed, Apr 13, 2016 at 10:06:57PM +0000, Vivi, Rodrigo wrote:
> > On Wed, 2016-04-13 at 22:46 +0200, Daniel Vetter wrote:
> > > On Wed, Apr 13, 2016 at 10:38 PM, Vivi, Rodrigo <
> > > rodrigo.vivi@intel.com> wrote:
> > > > On Wed, 2016-04-13 at 16:50 +0200, Daniel Vetter wrote:
> > > > > On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R
> > > > > wrote:
> > > > > > Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates
> > > > > > escreveu:
> > > > > > > This project is explained in detail on the HAS
> > > > > > > https://docs.google.com/a/intel.com/document/d/1E-en_xqfH
> > > > > > > gCnh
> > > > > > > D1Te
> > > > > > > s3f0
> > > > > > > 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
> > > > > > >
> > > > > > > Summary:
> > > > > > > Permits the user to identify and toggle values for PSR,
> > > > > > > FBC,
> > > > > > > RC6,
> > > > > > > DRRS, and IPS
> > > > > > > under /sys/class/drm/card0/power/. By enabling these
> > > > > > > features
> > > > > > > I'm
> > > > > > > looking to
> > > > > > > empower our customers, such as, power team, chrome OS,
> > > > > > > and
> > > > > > > platform
> > > > > > > integration teams
> > > > > > > to debug graphics power management features.
> > > > > > >
> > > > > > > From the current patchset PSR, FBC, RC6, and, DRRS are
> > > > > > > complete
> > > > > > > and
> > > > > > > past BAT, modset,
> > > > > > > and suspend/resume tests. For IPS I'm looking for
> > > > > > > feedback
> > > > > > > since
> > > > > > > IPS
> > > > > > > seems to change
> > > > > > > during runtime dynamically. Additionally, as a future
> > > > > > > project
> > > > > > > IPS
> > > > > > > would be better
> > > > > > > served if it is implemented by using the atomic check,
> > > > > > > pre
> > > > > > > -commit,
> > > > > > > commit, post-commit,
> > > > > > > a good example of this is the PSR enable/disable
> > > > > > > implementation.
> > > > > > >
> > > > > > > For this project to be completed needs to have in place
> > > > > > > the
> > > > > > > following
> > > > > > > components:
> > > > > > > (Need to be developed)
> > > > > > > * IPS toggle interface flesh-out
> > > > > > > * Tests added to intel-gpu-tools repo
> > > > > > > * Documentation for all sysfs added interfaces
> > > > > > > * PowerTOP component named: GFX-TOP. With the following
> > > > > > > requirements:
> > > > > > > * It would be available only to developers
> > > > > > > * It would allow the developers to toggle the
> > > > > > > interfaces
> > > > > > > from
> > > > > > > the PowerTOP user interface.
> > > > > > > * It would show the Power Consumption impact of
> > > > > > > toggling on
> > > > > > > and
> > > > > > > off
> > > > > > > these settings.
> > > > > > >
> > > > > >
> > > > > > A small suggestion:
> > > > > >
> > > > > > In the past, I've seen people testing i915.ko power saving
> > > > > > features
> > > > > > just by "toggling" the features through the i915 module
> > > > > > parameters
> > > > > > without doing anything else. In most of these cases, the
> > > > > > machine
> > > > > > was
> > > > > > not properly configured for power savings, so the
> > > > > > conclusion
> > > > > > was
> > > > > > often
> > > > > > that the feature didn't save any power. While this was true
> > > > > > (toggling
> > > > > > the feature didn't save any power), these people would have
> > > > > > reached
> > > > > > different conclusions if they had, for example, changed
> > > > > > their
> > > > > > disk
> > > > > > power management policies. Do we have any sort of plan to
> > > > > > try
> > > > > > to
> > > > > > educate the powertop/gfxtop users regarding these things?
> > > >
> > > > But the toggling the module parameter doesn't change anything
> > > > because
> > > > it depends on the next modeset to have any effect o nmnost of
> > > > pm
> > > > features.
> > > > The idea is that we don't need this with the sysfs and the
> > > > toggle
> > > > should represent an immediate change on the feature behaviour.
> > > > Although
> > > > it is a fact that the features also depends on other conditions
> > > > like
> > > > PSR depends on a fully idle screen for few frames at least.
> > >
> > > Well that part where the knob needs to kick the system into
> > > working
> > > correctly (what if we need an expensive w/a that's hard to reset
> > > at
> > > runtime) is what scares me.
> > >
> > > > > > Maybe the powertop/gfxtop interface could expose some very
> > > > > > -easy
> > > > > > -to-
> > > > > > reach text explaining these things.
> > > >
> > > > Good idea.
> > > >
> > > > >
> > > > > I think trying to extract/reuse those from the igt testcases
> > > > > might be
> > > > > really useful. I.e. add docs that explain
> > > > > a) what igt must past of a feature to be considered working
> > > > > b) add some functions to igt testcases for manually
> > > > > enabling/disabling a
> > > > > given rpm feature. The tests already know how to do that, and
> > > > > most
> > > > > have
> > > > > nice explanations about what all should be changed on top to
> > > > > make
> > > > > things
> > > > > work.
> > > >
> > > > Actually I had the opposite direction in mind. I was expecting
> > > > we
> > > > could
> > > > make igt tests to start using this sysfs toggles.
> > > > The problem I see with current igt tests way is that we need to
> > > > change
> > > > the parameter and depend on the next full modeset happening.
> > > > Well
> > > > it is
> > > > true that we need to have the modeset on the tests anyway, but
> > > > I
> > > > believe we could reduce that restriction and also powertop
> > > > wouldn't
> > > > do
> > > > any modeset.
> > >
> > > So taking more steps backwards: The goal here is to help debug
> > > runtime
> > > pm issues. The proposed solutions is that you can toggel stuff in
> > > powertop.
> > >
> > > Is this really how the experts debug runtime pm issues?
> >
> > I've seen many experts (non-gfx devs) around using the powertop for
> > debuging PM related issues and checking PC states, etc.
> >
> > One very good case to have at least the informative tab there is
> > exactly help this use case: One developer in the power lab cannot
> > see
> > deep PC state residencies with screen on and look to the gfx tab
> > and
> > see if DMC is enabled or not and PSR is enabled or not. And the
> > extra
> > is if is not enabled but possible it would just toggle directly
> > from
> > that interface.
> >
> > Or at least the informative with everything together they would
> > know if
> > i915 is somehow the culprit for blocking PC states without need to
> > hunt
> > different debugfs interfaces they are not familiar with.
>
> I'm fully on board with you on the "figure out that i915 is the
> culprit".
> I don't get why we need to be able to toggle stuff from within
> powertop to
> do that. Information-only should be enough. Maybe even some of the
> hints
> Paulo suggested (like "you haven't loaded DMC, pls install").
>
> Adding toggles into powertop - I have no idea how that helps with
> debugging, beyond the modparams we have already. This is also because
> I'm
> strongly opposed as "powertop the solution to enable rpm stuff by
> default", and that's seems to be the reason a lot of folks want these
> knobs.
I was talking to PM engineers today on their Summit and they are really
interrested in a way to toggle features on/off easily.
They also told if debugfs is the only way that powertop should have to
deal with it.
So I see two ways here:
1. Stable informative only sysfs interface with powertop always
displaying it. No toggles
2. Stable informative only sysfs interface with powertop always displaying it. Toggle on debugfs only not implemented on powertop.
3. Powertop showing status via debugfs, but not toggling it. Plus Toggle on debugfs only not implemented on powertop.
4. Powertop informative and toggle using debugfs. Probably forcing
powertop to create a flag --debug-only with a warning that it is
unsafe.
What do you think: 1, or 2, or 3, or 4, or none?
>
> Imo if something doesn't work that should, the next step is to file a
> bug
> report with the i915 experts to fix this. Not knobs and hacks in
> powertop
> ...
>
> /me fighting a bit a holy war
>
> > > At least when I hack around my first questions are:
> > > - do we get residenency/how much, and maybe what exactly is
> > > blocking
> > > it. I think we discussed this at the meeting and agreed that
> > > more/officially exposed residency counters.
> >
> > The problem is that we don't have this counters when power wells
> > are
> > down. So we lost all DC counters and PSR counter when DC entry.
> > So no residencies. But I still like the idea of the status.
>
> And I guess no way to at least somewhat fake those in software?
>
> > > - once I have a suspect I keep digging into that direction, using
> > > debugfs/unsafe module options, debugfs information, whatever.
> > >
> > > I fully agree that we can try to do a better job on documenting
> > > this
> > > stuff, and maybe exposing more information through stable
> > > interfaces.
> > >
> > > But adding new abi to enable/disable stuff sounds like jumping
> > > into
> > > the middle of the use-case story, ignoring everything else. And I
> > > think that was roughly what we agreed at that meeting - we can
> > > add
> > > knobs when it makes sense, but only when it makes sense. I'm
> > > still
> > > not
> > > sold on the "debug by powertop" solution.
> >
> > what about at least informative for now?
>
> Useful information sounds good. I'm freaking out about the
> maintenance of
> adding knobs and stuff that's supposed to work.
>
> > Watermark is my bad. I forgot to include it and also to toggle it
> > we
> > would need to add support for disabling it what we don't have at
> > all
> > but I believe we should have anyways.
>
> Watermarks was a bit a trick example, since imo adding code to
> fake-disable low-power watermark levels is nonsense. It's only going
> to
> help with very specific bugs, and if you don't understand the wm
> programming model for your platforms it's going to be totally useless
> information. Misconfiguring the driver to make it crap doesn't really
> tell
> you a lot ...
> -Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] PowerManagement Toggle for PowerTOP
2016-04-15 18:12 ` Vivi, Rodrigo
@ 2016-04-20 12:44 ` Daniel Vetter
0 siblings, 0 replies; 23+ messages in thread
From: Daniel Vetter @ 2016-04-20 12:44 UTC (permalink / raw)
To: Vivi, Rodrigo
Cc: Swaminathan, Nivedita, intel-gfx@lists.freedesktop.org,
Konno, Joe, Zanoni, Paulo R
On Fri, Apr 15, 2016 at 06:12:32PM +0000, Vivi, Rodrigo wrote:
> I was talking to PM engineers today on their Summit and they are really
> interrested in a way to toggle features on/off easily.
>
> They also told if debugfs is the only way that powertop should have to
> deal with it.
>
> So I see two ways here:
>
> 1. Stable informative only sysfs interface with powertop always
> displaying it. No toggles
>
> 2. Stable informative only sysfs interface with powertop always displaying it. Toggle on debugfs only not implemented on powertop.
>
> 3. Powertop showing status via debugfs, but not toggling it. Plus Toggle on debugfs only not implemented on powertop.
>
> 4. Powertop informative and toggle using debugfs. Probably forcing
> powertop to create a flag --debug-only with a warning that it is
> unsafe.
>
> What do you think: 1, or 2, or 3, or 4, or none?
I still don't get the use-case for toggling random power features. What
are people trying to accomplish with that? All the scenarios I can come up
with are imo well-served with the current module options we have.
Now maybe we should document how to best debug power issues better, but
imo what's really needed for that is proper residency counters. And
automatically displaying what exactly is blocking a deeper sleep state.
Both things only the magic PM firmware knows, and currently doesn't tell
us :(
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread