Re: [Intel-gfx] [PATCH v5] drm/i915/gt: make a gt sysfs group and move power management files

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

On Wed, 19 Feb 2020, Andi Shyti <andi.shyti@intel.com> wrote:
> The GT has its own properties and in sysfs they should be grouped
> in the 'gt/' directory.
>
> Create the 'gt/' directory in sysfs and move the power management
> related files.
>
> The new interfaces are:
>
> gt/gt_act_freq_mhz
> gt/gt_boost_freq_mhz
> gt/gt_cur_freq_mhz
> gt/gt_info
> gt/gt_max_freq_mhz
> gt/gt_min_freq_mhz
> gt/gt_RP0_freq_mhz
> gt/gt_RP1_freq_mhz
> gt/gt_RPn_freq_mhz
> gt/rc6_enable
> gt/rc6_residency_ms
>
> The once in the root directory will be marked as deprecated, if
> accessed a warning message is printed.
>
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> ---
> v4 -> v5:
>   - removed spurious ghost 'vvv' file that was never meant to be
>     there... sorry for spamming.
> v3 -> v4:
>   - fixed Tvrtko's comments:
>         - some renaming
>         - some clumsy unbalanced kobject_put/get
>         - the warning print is more descriptive and printed with
>           limited rate
>         - TODO: drm_print doesn't have a drm_warn_unlimited, to
>           be added
> v2 -> v3:
>   - fix some cleanups that I forgot in the previous patch
>   - fix reference pointers to the gt structure
>   - and many other small changes here and there.
> v1 -> v2:
>   - keep the existing files as they are
>   - use "intel_gt_*" as prefix instead of "sysfs_*"
>
>  drivers/gpu/drm/i915/Makefile            |   4 +-
>  drivers/gpu/drm/i915/gt/intel_gt.c       |   3 +
>  drivers/gpu/drm/i915/gt/intel_gt_types.h |   1 +
>  drivers/gpu/drm/i915/gt/sysfs_gt.c       |  79 +++++
>  drivers/gpu/drm/i915/gt/sysfs_gt.h       |  22 ++
>  drivers/gpu/drm/i915/gt/sysfs_gt_pm.c    | 432 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/gt/sysfs_gt_pm.h    |  17 +
>  drivers/gpu/drm/i915/i915_sysfs.c        | 375 +-------------------
>  drivers/gpu/drm/i915/i915_sysfs.h        |   3 +
>  9 files changed, 561 insertions(+), 375 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.c
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt.h
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
>  create mode 100644 drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b314d44ded5e..ff9e17c97dc2 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -107,7 +107,9 @@ gt-y += \
>  	gt/intel_rps.o \
>  	gt/intel_sseu.o \
>  	gt/intel_timeline.o \
> -	gt/intel_workarounds.o
> +	gt/intel_workarounds.o \
> +	gt/sysfs_gt.o \
> +	gt/sysfs_gt_pm.o
>  # autogenerated null render state
>  gt-y += \
>  	gt/gen6_renderstate.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index f1f1b306e0af..e794d05d69a1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -15,6 +15,7 @@
>  #include "intel_rps.h"
>  #include "intel_uncore.h"
>  #include "intel_pm.h"
> +#include "sysfs_gt.h"
>  
>  void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915)
>  {
> @@ -321,6 +322,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
>  	intel_rps_driver_register(&gt->rps);
>  
>  	debugfs_gt_register(gt);
> +	intel_gt_sysfs_register(gt);
>  }
>  
>  static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
> @@ -641,6 +643,7 @@ void intel_gt_driver_remove(struct intel_gt *gt)
>  
>  void intel_gt_driver_unregister(struct intel_gt *gt)
>  {
> +	intel_gt_sysfs_unregister(gt);
>  	intel_rps_driver_unregister(&gt->rps);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index 96890dd12b5f..7f0b4f8d9e28 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -32,6 +32,7 @@ struct intel_gt {
>  	struct drm_i915_private *i915;
>  	struct intel_uncore *uncore;
>  	struct i915_ggtt *ggtt;
> +	struct kobject sysfs_root;
>  
>  	struct intel_uc uc;
>  
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.c b/drivers/gpu/drm/i915/gt/sysfs_gt.c
> new file mode 100644
> index 000000000000..9335a92d5248
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: MIT
> +

Superfluous newline.

> +/*
> + * Copyright © 2019 Intel Corporation
> + */

It's 2020 now. ;)

> +
> +#include <linux/sysfs.h>
> +#include <drm/drm_device.h>
> +#include <linux/kobject.h>
> +#include <linux/printk.h>
> +
> +#include "../i915_drv.h"

No  need for "../", it's in include path.

> +#include "intel_gt.h"
> +#include "intel_gt_types.h"
> +#include "intel_rc6.h"
> +
> +#include "sysfs_gt.h"
> +#include "sysfs_gt_pm.h"
> +
> +static inline struct kobject *gt_get_parent_obj(struct intel_gt *gt)

In .c files just drop the inline keyword and let the compiler do what's
best.

> +{
> +	return &gt->i915->drm.primary->kdev->kobj;
> +}
> +
> +static ssize_t gt_info_show(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buff)
> +{
> +	return snprintf(buff, PAGE_SIZE, "0\n");
> +}
> +
> +static DEVICE_ATTR_RO(gt_info);
> +
> +static struct kobj_type sysfs_gt_ktype = {
> +	.sysfs_ops = &kobj_sysfs_ops,
> +};
> +
> +void intel_gt_sysfs_register(struct intel_gt *gt)
> +{
> +	struct kobject *parent = kobject_get(gt_get_parent_obj(gt));
> +	int ret;
> +
> +	ret = kobject_init_and_add(&gt->sysfs_root,
> +				   &sysfs_gt_ktype,
> +				   parent, "gt");
> +	if (ret) {
> +		drm_err(&gt->i915->drm, "failed to initialize sysfs file\n");
> +		kobject_put(&gt->sysfs_root);
> +		goto parent_files;
> +	}
> +
> +	ret = sysfs_create_file(&gt->sysfs_root, &dev_attr_gt_info.attr);
> +	if (ret)
> +		drm_err(&gt->i915->drm, "failed to create sysfs gt info files\n");
> +
> +	intel_gt_sysfs_pm_init(gt, &gt->sysfs_root);
> +
> +parent_files:
> +	/*
> +	 * we need to make things right with the
> +	 * ABI compatibility. The files were originally
> +	 * generated under the parent directory.
> +	 */
> +	intel_gt_sysfs_pm_init(gt, parent);
> +}
> +
> +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> +{
> +	struct kobject *parent = gt_get_parent_obj(gt);
> +
> +	/*
> +	 * the name gt tells us wether sysfs_root
> +	 * object was initialized properly
> +	 */
> +	if (!strcmp(gt->sysfs_root.name, "gt"))
> +		kobject_put(&gt->sysfs_root);
> +
> +	kobject_put(parent);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt.h b/drivers/gpu/drm/i915/gt/sysfs_gt.h
> new file mode 100644
> index 000000000000..2e479aa902e5
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +

Superfluous newline.

> +/*
> + * Copyright © 2019 Intel Corporation
> + */

2020.

> +
> +#ifndef SYSFS_GT_H
> +#define SYSFS_GT_H

Please add some underscores, e.g. __SYSFS_GT_H__.

> +
> +#include "intel_gt_types.h"
> +
> +struct intel_gt;
> +
> +static inline struct intel_gt *kobj_to_gt(struct kobject *kobj)
> +{
> +	return container_of(kobj, struct intel_gt, sysfs_root);
> +}
> +
> +void intel_gt_sysfs_register(struct intel_gt *gt);
> +void intel_gt_sysfs_unregister(struct intel_gt *gt);
> +
> +#endif /* SYSFS_GT_H */
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
> new file mode 100644
> index 000000000000..c548eb851a70
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: MIT
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */

You know the drill. ;)

> +
> +#include <drm/drm_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/printk.h>
> +
> +#include "../i915_drv.h"
> +#include "../i915_sysfs.h"

"../" is unnecessary.

> +#include "intel_gt.h"
> +#include "intel_rc6.h"
> +#include "intel_rps.h"
> +#include "sysfs_gt.h"
> +#include "sysfs_gt_pm.h"
> +
> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> +{
> +	struct kobject *kobj = &dev->kobj;
> +	/*
> +	 * We are interested at knowing from where the interface
> +	 * has been called, whether it's called from gt/ or from
> +	 * the parent directory.
> +	 * From the interface position it depends also the value of
> +	 * the private data.
> +	 * If the interface is called from gt/ then private data is
> +	 * of the "struct intel_gt *" type, otherwise it's * a
> +	 * "struct drm_i915_private *" type.
> +	 */
> +	if (strcmp(dev->kobj.name, "gt")) {
> +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> +
> +		pr_warn_ratelimited(DEPRECATED
> +			"(%s, %d) trying to access deprecated interface, "
> +			"use the corresponding interface in gt/\n",
> +			current->comm, task_pid_nr(current));
> +		return &i915->gt;
> +	}
> +
> +	return kobj_to_gt(kobj);
> +}
> +
> +#ifdef CONFIG_PM
> +static u32 get_residency(struct intel_gt *gt, i915_reg_t reg)
> +{
> +	intel_wakeref_t wakeref;
> +	u64 res = 0;
> +
> +	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> +		res = intel_rc6_residency_us(&gt->rc6, reg);
> +
> +	return DIV_ROUND_CLOSEST_ULL(res, 1000);
> +}
> +
> +static ssize_t rc6_enable_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u8 mask = 0;
> +
> +	if (HAS_RC6(gt->i915))
> +		mask |= BIT(0);
> +	if (HAS_RC6p(gt->i915))
> +		mask |= BIT(1);
> +	if (HAS_RC6pp(gt->i915))
> +		mask |= BIT(2);
> +
> +	return snprintf(buff, PAGE_SIZE, "%x\n", mask);
> +}
> +
> +static ssize_t rc6_residency_ms_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6_residency = get_residency(gt, GEN6_GT_GFX_RC6);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
> +}
> +
> +static ssize_t rc6p_residency_ms_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6p_residency = get_residency(gt, GEN6_GT_GFX_RC6p);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6p_residency);
> +}
> +
> +static ssize_t rc6pp_residency_ms_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6pp_residency = get_residency(gt, GEN6_GT_GFX_RC6pp);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6pp_residency);
> +}
> +
> +static ssize_t media_rc6_residency_ms_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	u32 rc6_residency = get_residency(gt, VLV_GT_MEDIA_RC6);
> +
> +	return snprintf(buff, PAGE_SIZE, "%u\n", rc6_residency);
> +}
> +
> +static DEVICE_ATTR_RO(rc6_enable);
> +static DEVICE_ATTR_RO(rc6_residency_ms);
> +static DEVICE_ATTR_RO(rc6p_residency_ms);
> +static DEVICE_ATTR_RO(rc6pp_residency_ms);
> +static DEVICE_ATTR_RO(media_rc6_residency_ms);
> +
> +static const struct attribute *rc6_attrs[] = {
> +	&dev_attr_rc6_enable.attr,
> +	&dev_attr_rc6_residency_ms.attr,
> +	NULL
> +};
> +
> +static const struct attribute *rc6p_attrs[] = {
> +	&dev_attr_rc6p_residency_ms.attr,
> +	&dev_attr_rc6pp_residency_ms.attr,
> +	NULL
> +};
> +
> +static const struct attribute *media_rc6_attrs[] = {
> +	&dev_attr_media_rc6_residency_ms.attr,
> +	NULL
> +};
> +
> +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	int ret = 0;
> +
> +	if (HAS_RC6(gt->i915)) {
> +		ret = sysfs_create_files(kobj, rc6_attrs);
> +		if (ret)
> +			drm_err(&gt->i915->drm,
> +				"failed to create RC6 sysfs files\n");
> +	}
> +
> +	if (HAS_RC6p(gt->i915)) {
> +		ret = sysfs_create_files(kobj, rc6p_attrs);
> +		if (ret)
> +			drm_err(&gt->i915->drm,
> +				"failed to create RC6p sysfs files\n");
> +	}
> +
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
> +		ret = sysfs_create_files(kobj, media_rc6_attrs);
> +		if (ret)
> +			drm_err(&gt->i915->drm,
> +				"failed to create media RC6 sysfs files\n");
> +	}
> +}
> +#else
> +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static ssize_t gt_act_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_rps_read_actual_frequency(&gt->rps));
> +}
> +
> +static ssize_t gt_cur_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->cur_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->boost_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	bool boost = false;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Validate against (static) hardware limits */
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq || val > rps->max_freq)
> +		return -EINVAL;
> +
> +	mutex_lock(&rps->lock);
> +	if (val != rps->boost_freq) {
> +		rps->boost_freq = val;
> +		boost = atomic_read(&rps->num_waiters);
> +	}
> +	mutex_unlock(&rps->lock);
> +	if (boost)
> +		schedule_work(&rps->work);
> +
> +	return count;
> +}
> +
> +static ssize_t vlv_rpe_freq_mhz_show(struct device *dev,
> +				     struct device_attribute *attr, char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->efficient_freq));
> +}
> +
> +static ssize_t gt_max_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->max_freq_softlimit));
> +}
> +
> +static ssize_t gt_max_freq_mhz_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&rps->lock);
> +
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq ||
> +	    val > rps->max_freq ||
> +	    val < rps->min_freq_softlimit) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (val > rps->rp0_freq)
> +		DRM_DEBUG("User requested overclocking to %d\n",
> +			  intel_gpu_freq(rps, val));
> +
> +	rps->max_freq_softlimit = val;
> +
> +	val = clamp_t(int, rps->cur_freq,
> +		      rps->min_freq_softlimit,
> +		      rps->max_freq_softlimit);
> +
> +	/*
> +	 * We still need *_set_rps to process the new max_delay and
> +	 * update the interrupt limits and PMINTRMSK even though
> +	 * frequency request may be unchanged.
> +	 */
> +	intel_rps_set(rps, val);
> +
> +unlock:
> +	mutex_unlock(&rps->lock);
> +
> +	return ret ?: count;
> +}
> +
> +static ssize_t gt_min_freq_mhz_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n",
> +			intel_gpu_freq(rps, rps->min_freq_softlimit));
> +}
> +
> +static ssize_t gt_min_freq_mhz_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buff, size_t count)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	ssize_t ret;
> +	u32 val;
> +
> +	ret = kstrtou32(buff, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&rps->lock);
> +
> +	val = intel_freq_opcode(rps, val);
> +	if (val < rps->min_freq ||
> +	    val > rps->max_freq ||
> +	    val > rps->max_freq_softlimit) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	rps->min_freq_softlimit = val;
> +
> +	val = clamp_t(int, rps->cur_freq,
> +		      rps->min_freq_softlimit,
> +		      rps->max_freq_softlimit);
> +
> +	/*
> +	 * We still need *_set_rps to process the new min_delay and
> +	 * update the interrupt limits and PMINTRMSK even though
> +	 * frequency request may be unchanged.
> +	 */
> +	intel_rps_set(rps, val);
> +
> +unlock:
> +	mutex_unlock(&rps->lock);
> +
> +	return ret ?: count;
> +}
> +
> +static DEVICE_ATTR_RO(gt_act_freq_mhz);
> +static DEVICE_ATTR_RO(gt_cur_freq_mhz);
> +static DEVICE_ATTR_RW(gt_boost_freq_mhz);
> +static DEVICE_ATTR_RW(gt_max_freq_mhz);
> +static DEVICE_ATTR_RW(gt_min_freq_mhz);
> +
> +static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
> +
> +static ssize_t gt_rp_mhz_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buff);
> +
> +static DEVICE_ATTR(gt_RP0_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_RP1_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_RPn_freq_mhz, 0444, gt_rp_mhz_show, NULL);
> +
> +/* For now we have a static number of RP states */
> +static ssize_t gt_rp_mhz_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buff)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
> +	struct intel_rps *rps = &gt->rps;
> +	u32 val;
> +
> +	if (attr == &dev_attr_gt_RP0_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->rp0_freq);
> +	else if (attr == &dev_attr_gt_RP1_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->rp1_freq);
> +	else if (attr == &dev_attr_gt_RPn_freq_mhz)
> +		val = intel_gpu_freq(rps, rps->min_freq);
> +	else
> +		BUG();
> +
> +	return snprintf(buff, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static const struct attribute * const gen6_attrs[] = {
> +	&dev_attr_gt_act_freq_mhz.attr,
> +	&dev_attr_gt_cur_freq_mhz.attr,
> +	&dev_attr_gt_boost_freq_mhz.attr,
> +	&dev_attr_gt_max_freq_mhz.attr,
> +	&dev_attr_gt_min_freq_mhz.attr,
> +	&dev_attr_gt_RP0_freq_mhz.attr,
> +	&dev_attr_gt_RP1_freq_mhz.attr,
> +	&dev_attr_gt_RPn_freq_mhz.attr,
> +	NULL,
> +};
> +
> +static const struct attribute * const vlv_attrs[] = {
> +	&dev_attr_gt_act_freq_mhz.attr,
> +	&dev_attr_gt_cur_freq_mhz.attr,
> +	&dev_attr_gt_boost_freq_mhz.attr,
> +	&dev_attr_gt_max_freq_mhz.attr,
> +	&dev_attr_gt_min_freq_mhz.attr,
> +	&dev_attr_gt_RP0_freq_mhz.attr,
> +	&dev_attr_gt_RP1_freq_mhz.attr,
> +	&dev_attr_gt_RPn_freq_mhz.attr,
> +	&dev_attr_vlv_rpe_freq_mhz.attr,
> +	NULL,
> +};
> +
> +static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
> +		return sysfs_create_files(kobj, vlv_attrs);
> +
> +	if (INTEL_GEN(gt->i915) >= 6)
> +		return sysfs_create_files(kobj, gen6_attrs);
> +
> +	return 0;
> +}
> +
> +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
> +{
> +	int ret;
> +
> +	intel_sysfs_rc6_init(gt, kobj);
> +
> +	ret = intel_sysfs_rps_init(gt, kobj);
> +	if (ret)
> +		drm_err(&gt->i915->drm, "failed to create RPS sysfs files");
> +}
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
> new file mode 100644
> index 000000000000..758d0c3cb998
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt_pm.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef SYSFS_RC6_H
> +#define SYSFS_RC6_H

All the things mentioned before; newline, year, underscores.

Additionally make the include protection match the file name.

> +
> +#include <linux/kobject.h>
> +
> +#include "intel_gt_types.h"
> +
> +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
> +void intel_gt_sysfs_pm_remove(struct intel_gt *gt, struct kobject *kobj);
> +
> +#endif /* SYSFS_RC6_H */
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index c14d762bd652..1298977fc08b 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -38,113 +38,12 @@
>  #include "intel_pm.h"
>  #include "intel_sideband.h"
>  
> -static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
> +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
>  {
>  	struct drm_minor *minor = dev_get_drvdata(kdev);
>  	return to_i915(minor->dev);
>  }
>  
> -#ifdef CONFIG_PM
> -static u32 calc_residency(struct drm_i915_private *dev_priv,
> -			  i915_reg_t reg)
> -{
> -	intel_wakeref_t wakeref;
> -	u64 res = 0;
> -
> -	with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref)
> -		res = intel_rc6_residency_us(&dev_priv->gt.rc6, reg);
> -
> -	return DIV_ROUND_CLOSEST_ULL(res, 1000);
> -}
> -
> -static ssize_t
> -show_rc6_mask(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	unsigned int mask;
> -
> -	mask = 0;
> -	if (HAS_RC6(dev_priv))
> -		mask |= BIT(0);
> -	if (HAS_RC6p(dev_priv))
> -		mask |= BIT(1);
> -	if (HAS_RC6pp(dev_priv))
> -		mask |= BIT(2);
> -
> -	return snprintf(buf, PAGE_SIZE, "%x\n", mask);
> -}
> -
> -static ssize_t
> -show_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6);
> -	return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
> -}
> -
> -static ssize_t
> -show_rc6p_ms(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p);
> -	return snprintf(buf, PAGE_SIZE, "%u\n", rc6p_residency);
> -}
> -
> -static ssize_t
> -show_rc6pp_ms(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp);
> -	return snprintf(buf, PAGE_SIZE, "%u\n", rc6pp_residency);
> -}
> -
> -static ssize_t
> -show_media_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6);
> -	return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency);
> -}
> -
> -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 *rc6_attrs[] = {
> -	&dev_attr_rc6_enable.attr,
> -	&dev_attr_rc6_residency_ms.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group rc6_attr_group = {
> -	.name = power_group_name,
> -	.attrs =  rc6_attrs
> -};
> -
> -static struct attribute *rc6p_attrs[] = {
> -	&dev_attr_rc6p_residency_ms.attr,
> -	&dev_attr_rc6pp_residency_ms.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group rc6p_attr_group = {
> -	.name = power_group_name,
> -	.attrs =  rc6p_attrs
> -};
> -
> -static struct attribute *media_rc6_attrs[] = {
> -	&dev_attr_media_rc6_residency_ms.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group media_rc6_attr_group = {
> -	.name = power_group_name,
> -	.attrs =  media_rc6_attrs
> -};
> -#endif
> -
>  static int l3_access_valid(struct drm_i915_private *i915, loff_t offset)
>  {
>  	if (!HAS_L3_DPF(i915))
> @@ -256,239 +155,6 @@ static const struct bin_attribute dpf_attrs_1 = {
>  	.private = (void *)1
>  };
>  
> -static ssize_t gt_act_freq_mhz_show(struct device *kdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_rps_read_actual_frequency(rps));
> -}
> -
> -static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> -				    struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->cur_freq));
> -}
> -
> -static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &i915->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->boost_freq));
> -}
> -
> -static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
> -				       struct device_attribute *attr,
> -				       const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	bool boost = false;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	/* Validate against (static) hardware limits */
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq || val > rps->max_freq)
> -		return -EINVAL;
> -
> -	mutex_lock(&rps->lock);
> -	if (val != rps->boost_freq) {
> -		rps->boost_freq = val;
> -		boost = atomic_read(&rps->num_waiters);
> -	}
> -	mutex_unlock(&rps->lock);
> -	if (boost)
> -		schedule_work(&rps->work);
> -
> -	return count;
> -}
> -
> -static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
> -				     struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->efficient_freq));
> -}
> -
> -static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->max_freq_softlimit));
> -}
> -
> -static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	mutex_lock(&rps->lock);
> -
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq ||
> -	    val > rps->max_freq ||
> -	    val < rps->min_freq_softlimit) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	if (val > rps->rp0_freq)
> -		DRM_DEBUG("User requested overclocking to %d\n",
> -			  intel_gpu_freq(rps, val));
> -
> -	rps->max_freq_softlimit = val;
> -
> -	val = clamp_t(int, rps->cur_freq,
> -		      rps->min_freq_softlimit,
> -		      rps->max_freq_softlimit);
> -
> -	/*
> -	 * We still need *_set_rps to process the new max_delay and
> -	 * update the interrupt limits and PMINTRMSK even though
> -	 * frequency request may be unchanged.
> -	 */
> -	intel_rps_set(rps, val);
> -
> -unlock:
> -	mutex_unlock(&rps->lock);
> -
> -	return ret ?: count;
> -}
> -
> -static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			intel_gpu_freq(rps, rps->min_freq_softlimit));
> -}
> -
> -static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> -				     struct device_attribute *attr,
> -				     const char *buf, size_t count)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	ssize_t ret;
> -	u32 val;
> -
> -	ret = kstrtou32(buf, 0, &val);
> -	if (ret)
> -		return ret;
> -
> -	mutex_lock(&rps->lock);
> -
> -	val = intel_freq_opcode(rps, val);
> -	if (val < rps->min_freq ||
> -	    val > rps->max_freq ||
> -	    val > rps->max_freq_softlimit) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> -
> -	rps->min_freq_softlimit = val;
> -
> -	val = clamp_t(int, rps->cur_freq,
> -		      rps->min_freq_softlimit,
> -		      rps->max_freq_softlimit);
> -
> -	/*
> -	 * We still need *_set_rps to process the new min_delay and
> -	 * update the interrupt limits and PMINTRMSK even though
> -	 * frequency request may be unchanged.
> -	 */
> -	intel_rps_set(rps, val);
> -
> -unlock:
> -	mutex_unlock(&rps->lock);
> -
> -	return ret ?: count;
> -}
> -
> -static DEVICE_ATTR_RO(gt_act_freq_mhz);
> -static DEVICE_ATTR_RO(gt_cur_freq_mhz);
> -static DEVICE_ATTR_RW(gt_boost_freq_mhz);
> -static DEVICE_ATTR_RW(gt_max_freq_mhz);
> -static DEVICE_ATTR_RW(gt_min_freq_mhz);
> -
> -static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
> -
> -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf);
> -static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> -
> -/* For now we have a static number of RP states */
> -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> -{
> -	struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
> -	struct intel_rps *rps = &dev_priv->gt.rps;
> -	u32 val;
> -
> -	if (attr == &dev_attr_gt_RP0_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->rp0_freq);
> -	else if (attr == &dev_attr_gt_RP1_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->rp1_freq);
> -	else if (attr == &dev_attr_gt_RPn_freq_mhz)
> -		val = intel_gpu_freq(rps, rps->min_freq);
> -	else
> -		BUG();
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", val);
> -}
> -
> -static const struct attribute * const gen6_attrs[] = {
> -	&dev_attr_gt_act_freq_mhz.attr,
> -	&dev_attr_gt_cur_freq_mhz.attr,
> -	&dev_attr_gt_boost_freq_mhz.attr,
> -	&dev_attr_gt_max_freq_mhz.attr,
> -	&dev_attr_gt_min_freq_mhz.attr,
> -	&dev_attr_gt_RP0_freq_mhz.attr,
> -	&dev_attr_gt_RP1_freq_mhz.attr,
> -	&dev_attr_gt_RPn_freq_mhz.attr,
> -	NULL,
> -};
> -
> -static const struct attribute * const vlv_attrs[] = {
> -	&dev_attr_gt_act_freq_mhz.attr,
> -	&dev_attr_gt_cur_freq_mhz.attr,
> -	&dev_attr_gt_boost_freq_mhz.attr,
> -	&dev_attr_gt_max_freq_mhz.attr,
> -	&dev_attr_gt_min_freq_mhz.attr,
> -	&dev_attr_gt_RP0_freq_mhz.attr,
> -	&dev_attr_gt_RP1_freq_mhz.attr,
> -	&dev_attr_gt_RPn_freq_mhz.attr,
> -	&dev_attr_vlv_rpe_freq_mhz.attr,
> -	NULL,
> -};
> -
>  #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
>  
>  static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
> @@ -559,29 +225,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>  	struct device *kdev = dev_priv->drm.primary->kdev;
>  	int ret;
>  
> -#ifdef CONFIG_PM
> -	if (HAS_RC6(dev_priv)) {
> -		ret = sysfs_merge_group(&kdev->kobj,
> -					&rc6_attr_group);
> -		if (ret)
> -			drm_err(&dev_priv->drm,
> -				"RC6 residency sysfs setup failed\n");
> -	}
> -	if (HAS_RC6p(dev_priv)) {
> -		ret = sysfs_merge_group(&kdev->kobj,
> -					&rc6p_attr_group);
> -		if (ret)
> -			drm_err(&dev_priv->drm,
> -				"RC6p residency sysfs setup failed\n");
> -	}
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		ret = sysfs_merge_group(&kdev->kobj,
> -					&media_rc6_attr_group);
> -		if (ret)
> -			drm_err(&dev_priv->drm,
> -				"Media RC6 residency sysfs setup failed\n");
> -	}
> -#endif
>  	if (HAS_L3_DPF(dev_priv)) {
>  		ret = device_create_bin_file(kdev, &dpf_attrs);
>  		if (ret)
> @@ -597,14 +240,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> -	ret = 0;
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		ret = sysfs_create_files(&kdev->kobj, vlv_attrs);
> -	else if (INTEL_GEN(dev_priv) >= 6)
> -		ret = sysfs_create_files(&kdev->kobj, gen6_attrs);
> -	if (ret)
> -		drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
> -
>  	i915_setup_error_capture(kdev);
>  }
>  
> @@ -614,14 +249,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
>  
>  	i915_teardown_error_capture(kdev);
>  
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		sysfs_remove_files(&kdev->kobj, vlv_attrs);
> -	else
> -		sysfs_remove_files(&kdev->kobj, gen6_attrs);
>  	device_remove_bin_file(kdev,  &dpf_attrs_1);
>  	device_remove_bin_file(kdev,  &dpf_attrs);
> -#ifdef CONFIG_PM
> -	sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group);
> -	sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
> -#endif
>  }
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h
> index 41afd4366416..ad6114de81c9 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.h
> +++ b/drivers/gpu/drm/i915/i915_sysfs.h
> @@ -6,8 +6,11 @@
>  #ifndef __I915_SYSFS_H__
>  #define __I915_SYSFS_H__
>  
> +#include <linux/device.h>

You'll only need a forward declaration.

> +
>  struct drm_i915_private;
>  
> +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev);
>  void i915_setup_sysfs(struct drm_i915_private *i915);
>  void i915_teardown_sysfs(struct drm_i915_private *i915);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx