Re: [PATCH 2/5] drm/i915: Add sys FBC toggle interface

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

Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
> This interface allows an immediate enabling of 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