[Intel-gfx] [PATCH 13/13] drm/i915: Allow Dynamically GT3 Slice Shutdown.

[Chris Wilson]

On Mon, Sep 23, 2013 at 05:33:30PM -0300, Rodrigo Vivi wrote:
> Slice shutdown is a power savings feature whereby parts of HW i.e. slice is
> shut off on boot or dynamically to save power.
> 
> This patch introduces a sysfs interface to easily allow dynamically switch
> between full and half GT3 slices.
> 
> v2: remove unused variables and fix identation

gt3_policy is a misnomer, what you have is more of a "slice config".

I'm not convinced by the boot parameter and want to see how this
interacts with the automatic configuration before declaring any
interface sign.

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c | 48 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h  |  4 +++-
>  drivers/gpu/drm/i915/intel_pm.c   | 31 +++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 7659dd4..40f23a8 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -97,6 +97,48 @@ static struct attribute_group rc6_attr_group = {
>  	.name = power_group_name,
>  	.attrs =  rc6_attrs
>  };
> +
> +static ssize_t gt3_policy_show(struct device *kdev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> +	struct drm_device *dev = minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	return sprintf(buf, "%s\n", I915_READ(MI_PREDICATE_RESULT_2) ==
> +		       LOWER_SLICE_ENABLED ? "full" : "half");
> +}
> +
> +static ssize_t gt3_policy_store(struct device *kdev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> +	struct drm_device *dev = minor->dev;
> +
> +	if (!strncmp(buf, "full", sizeof("full") - 1))
> +		intel_set_gt3_full(dev);

Be strict and use strcmp(buf, "full") == 0

> +	else if (!strncmp(buf, "half", sizeof("half") - 1))
> +		intel_set_gt3_half(dev);
> +	else
> +		return -EINVAL;

This probably wants to be synchronous and not return before the change
has taken effect.

> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(gt3_policy, S_IRUGO | S_IWUSR,
> +		   gt3_policy_show, gt3_policy_store);
> +
> +static struct attribute *gt3_policy_attrs[] = {
> +	&dev_attr_gt3_policy.attr,
> +	NULL
> +};
> +
> +static struct attribute_group gt3_policy_attr_group = {
> +	.name = power_group_name,
> +	.attrs =  gt3_policy_attrs
> +};
> +
>  #endif
>  
>  static int l3_access_valid(struct drm_device *dev, loff_t offset)
> @@ -528,6 +570,12 @@ void i915_setup_sysfs(struct drm_device *dev)
>  		if (ret)
>  			DRM_ERROR("RC6 residency sysfs setup failed\n");
>  	}
> +	if (IS_HSW_GT3(dev)) {
> +		ret = sysfs_merge_group(&dev->primary->kdev.kobj,
> +					&gt3_policy_attr_group);
> +		if (ret)
> +			DRM_ERROR("GT3 policy sysfs setup failed\n");

Shouldn't there be a remove as well?

> +	}
>  #endif
>  	if (HAS_L3_DPF(dev)) {
>  		ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f2ef5cb..2d28289 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -756,7 +756,9 @@ extern void intel_update_fbc(struct drm_device *dev);
>  /* IPS */
>  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
> -
> +/* Slice Shutdown */
> +extern void intel_set_gt3_full(struct drm_device *dev);
> +extern void intel_set_gt3_half(struct drm_device *dev);
>  /* Power well */
>  extern int i915_init_power_well(struct drm_device *dev);
>  extern void i915_remove_power_well(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c03566e..e7a987f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3712,6 +3712,37 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	gen6_gt_force_wake_put(dev_priv);
>  }
>  
> +void intel_set_gt3_full(struct drm_device *dev)

Just one function will do,

intel_set_slice_config(dev, enum slice_config config);

(and if the boot parameter remains it should call into it)

I don't think you should be changing MI_PREDICATE whilst the (a) the
slice configuration has not taken effect, (b) whilst the GPU is active,
(c) those are indeed the same.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre