[Intel-gfx] [PATCH] drm/i915: Do not export RC6p and RC6pp if they don't exist

[Daniel Vetter]

On Tue, Sep 30, 2014 at 08:00:33AM -0700, Rodrigo Vivi wrote:
> Avoid to expose RC6 and RC6pp to the platforms that doesn't support it.
> Although this doesn't really change powertop behaviour as described on the
> request.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=84524
> Cc: Josh Triplett <josh.triplett at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  3 +++
>  drivers/gpu/drm/i915/i915_sysfs.c | 39 ++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_pm.c   | 12 ++++++++----
>  3 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d10b417..54b2aac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2185,6 +2185,9 @@ struct drm_i915_cmd_table {
>  #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
>  #define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev) || \
>  				 IS_BROADWELL(dev) || IS_VALLEYVIEW(dev))
> +#define HAS_RC6(dev)		(INTEL_INFO(dev)->gen >= 6)
> +#define HAS_RC6p(dev)		(IS_GEN6(dev) || IS_IVYBRIDGE(dev))
> +#define HAS_RC6pp(dev)		IS_GEN6(dev)
>  
>  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 503847f..879e889 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -139,8 +139,6 @@ static DEVICE_ATTR(rc6pp_residency_ms, S_IRUGO, show_rc6pp_ms, NULL);
>  static struct attribute *rc6_attrs[] = {
>  	&dev_attr_rc6_enable.attr,
>  	&dev_attr_rc6_residency_ms.attr,
> -	&dev_attr_rc6p_residency_ms.attr,
> -	&dev_attr_rc6pp_residency_ms.attr,
>  	NULL
>  };
>  
> @@ -148,6 +146,26 @@ static struct attribute_group rc6_attr_group = {
>  	.name = power_group_name,
>  	.attrs =  rc6_attrs
>  };
> +
> +static struct attribute *rc6p_attrs[] = {
> +	&dev_attr_rc6p_residency_ms.attr,
> +	NULL
> +};
> +
> +static struct attribute_group rc6p_attr_group = {
> +	.name = power_group_name,
> +	.attrs =  rc6p_attrs
> +};
> +
> +static struct attribute *rc6pp_attrs[] = {
> +	&dev_attr_rc6pp_residency_ms.attr,
> +	NULL
> +};
> +
> +static struct attribute_group rc6pp_attr_group = {
> +	.name = power_group_name,
> +	.attrs =  rc6pp_attrs
> +};
>  #endif
>  
>  static int l3_access_valid(struct drm_device *dev, loff_t offset)
> @@ -595,12 +613,25 @@ void i915_setup_sysfs(struct drm_device *dev)
>  	int ret;
>  
>  #ifdef CONFIG_PM
> -	if (INTEL_INFO(dev)->gen >= 6) {
> +	if (HAS_RC6(dev)) {
>  		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
>  					&rc6_attr_group);
>  		if (ret)
>  			DRM_ERROR("RC6 residency sysfs setup failed\n");
>  	}
> +	if (HAS_RC6p(dev)) {
> +		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
> +					&rc6p_attr_group);
> +		if (ret)
> +			DRM_ERROR("RC6p residency sysfs setup failed\n");
> +	}
> +
> +	if (HAS_RC6pp(dev)) {
> +		ret = sysfs_merge_group(&dev->primary->kdev->kobj,
> +					&rc6pp_attr_group);
> +		if (ret)
> +			DRM_ERROR("RC6pp residency sysfs setup failed\n");
> +	}
>  #endif
>  	if (HAS_L3_DPF(dev)) {
>  		ret = device_create_bin_file(dev->primary->kdev, &dpf_attrs);
> @@ -640,5 +671,7 @@ void i915_teardown_sysfs(struct drm_device *dev)
>  	device_remove_bin_file(dev->primary->kdev,  &dpf_attrs);
>  #ifdef CONFIG_PM
>  	sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6_attr_group);
> +	sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6p_attr_group);
> +	sysfs_unmerge_group(&dev->primary->kdev->kobj, &rc6pp_attr_group);

I think we could do just one additional group here for both rc6p and
rc6pp, simplifies the code a bit.
-Daniel

>  #endif
>  }
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7553e47..fa1227e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3632,10 +3632,14 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
>  		else
>  			mode = 0;
>  	}
> -	DRM_DEBUG_KMS("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> -		      (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> -		      (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> -		      (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
> +	DRM_DEBUG_KMS("Enabling RC6 states: RC6 %s\n",
> +		      (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off");
> +	if (HAS_RC6p(dev))
> +		DRM_DEBUG_KMS("Enabling RC6 states: RC6p %s\n",
> +			      (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off");
> +	if (HAS_RC6pp(dev))
> +		DRM_DEBUG_KMS("Enabling RC6 states: RC6pp %s\n",
> +			      (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
>  }
>  
>  static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch