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

[Jani Nikula <jani.nikula@linux.intel.com>]

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