On Wed, 13 Apr 2016, "Vivi, Rodrigo" <rodrigo.vivi@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
