Re: [Intel-gfx] [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 17.02.2022 15:41, Andi Shyti wrote:
> Now that we have tiles we want each of them to have its own
> interface. A directory "gt/" is created under "cardN/" that will
> contain as many diroctories as the tiles.
>
> In the coming patches tile related interfaces will be added. For
> now the sysfs gt structure simply has an id interface related
> to the current tile count.
>
> The directory structure will follow this scheme:
>
>      /sys/.../card0
>               └── gt
>                   ├── gt0
>                   │   └── id
>                   :
> 		 :
> 		 └─- gtN
>                       └── id
>
> This new set of interfaces will be a basic tool for system
> managers and administrators when using i915.
>
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile            |   1 +
>   drivers/gpu/drm/i915/gt/intel_gt.c       |   2 +
>   drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 118 +++++++++++++++++++++++
>   drivers/gpu/drm/i915/gt/intel_gt_sysfs.h |  34 +++++++
>   drivers/gpu/drm/i915/i915_drv.h          |   2 +
>   drivers/gpu/drm/i915/i915_sysfs.c        |  12 ++-
>   drivers/gpu/drm/i915/i915_sysfs.h        |   3 +
>   7 files changed, 171 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
>   create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 9d588d936e3d..277064b00afd 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -105,6 +105,7 @@ gt-y += \
>   	gt/intel_gt_pm_debugfs.o \
>   	gt/intel_gt_pm_irq.o \
>   	gt/intel_gt_requests.o \
> +	gt/intel_gt_sysfs.o \
>   	gt/intel_gtt.o \
>   	gt/intel_llc.o \
>   	gt/intel_lrc.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 8c64b81e9ec9..0f080bbad043 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -26,6 +26,7 @@
>   #include "intel_rc6.h"
>   #include "intel_renderstate.h"
>   #include "intel_rps.h"
> +#include "intel_gt_sysfs.h"
>   #include "intel_uncore.h"
>   #include "shmem_utils.h"
>   
> @@ -458,6 +459,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
>   	intel_rps_driver_register(&gt->rps);
>   
>   	intel_gt_debugfs_register(gt);
> +	intel_gt_sysfs_register(gt);
>   }
>   
>   static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> new file mode 100644
> index 000000000000..0206e9aa4867
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include <drm/drm_device.h>
> +#include <linux/device.h>
> +#include <linux/kobject.h>
> +#include <linux/printk.h>
> +#include <linux/sysfs.h>
> +
> +#include "i915_drv.h"
> +#include "i915_sysfs.h"
> +#include "intel_gt.h"
> +#include "intel_gt_sysfs.h"
> +#include "intel_gt_types.h"
> +#include "intel_rc6.h"
> +
> +bool is_object_gt(struct kobject *kobj)
> +{
> +	return !strncmp(kobj->name, "gt", 2);
> +}

It looks quite fragile, at the moment I do not have better idea:) maybe 
after reviewing the rest of the patches.

> +
> +static struct intel_gt *kobj_to_gt(struct kobject *kobj)
> +{
> +	return container_of(kobj, struct kobj_gt, base)->gt;
> +}
> +
> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> +					    const char *name)
> +{
> +	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 (!is_object_gt(kobj)) {
> +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> +
> +		pr_devel_ratelimited(DEPRECATED
> +			"%s (pid %d) is accessing deprecated %s "
> +			"sysfs control, please use gt/gt<n>/%s instead\n",
> +			current->comm, task_pid_nr(current), name, name);
> +		return to_gt(i915);
> +	}
> +
> +	return kobj_to_gt(kobj);

It took some time for me to understand what is going on here.
We have dev argument which sometimes can point to "struct device", 
sometimes to "struct kobj_gt", but it's type suggests differently, quite 
ugly.
I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as 
in case of intel_engines_add_sysfs. This way abstractions would look 
better, hopefully.

> +}
> +
> +static ssize_t id_show(struct device *dev,
> +		       struct device_attribute *attr,
> +		       char *buf)
> +{
> +	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> +
> +	return sysfs_emit(buf, "%u\n", gt->info.id);
> +}
> +static DEVICE_ATTR_RO(id);
> +
> +static struct attribute *id_attrs[] = {
> +	&dev_attr_id.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(id);
> +
> +static void kobj_gt_release(struct kobject *kobj)
> +{
> +	kfree(kobj);
> +}
> +
> +static struct kobj_type kobj_gt_type = {
> +	.release = kobj_gt_release,
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_groups = id_groups,
> +};
> +
> +struct kobject *
> +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name)
> +{
> +	struct kobj_gt *kg;
> +
> +	kg = kzalloc(sizeof(*kg), GFP_KERNEL);
> +	if (!kg)
> +		return NULL;
> +
> +	kobject_init(&kg->base, &kobj_gt_type);
> +	kg->gt = gt;
> +
> +	/* xfer ownership to sysfs tree */
> +	if (kobject_add(&kg->base, dir, "%s", name)) {
> +		kobject_put(&kg->base);
> +		return NULL;
> +	}
> +
> +	return &kg->base; /* borrowed ref */
> +}
> +
> +void intel_gt_sysfs_register(struct intel_gt *gt)
> +{
> +	struct kobject *dir;
> +	char name[80];
> +
> +	snprintf(name, sizeof(name), "gt%d", gt->info.id);
> +
> +	dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
> +	if (!dir) {
> +		drm_warn(&gt->i915->drm,
> +			 "failed to initialize %s sysfs root\n", name);
> +		return;
> +	}
> +}

Squashing intel_gt_create_kobj into intel_gt_sysfs_register would 
simplify code and allows drop snprintf to local array.

> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> new file mode 100644
> index 000000000000..9471b26752cf
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#ifndef __SYSFS_GT_H__
> +#define __SYSFS_GT_H__
> +
> +#include <linux/ctype.h>
> +#include <linux/kobject.h>
> +
> +#include "i915_gem.h" /* GEM_BUG_ON() */
> +
> +struct intel_gt;
> +
> +struct kobj_gt {
> +	struct kobject base;
> +	struct intel_gt *gt;
> +};
> +
> +bool is_object_gt(struct kobject *kobj);
> +
> +struct drm_i915_private *kobj_to_i915(struct kobject *kobj);
> +
> +struct kobject *
> +intel_gt_create_kobj(struct intel_gt *gt,
> +		     struct kobject *dir,
> +		     const char *name);
> +
> +void intel_gt_sysfs_register(struct intel_gt *gt);
> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> +					    const char *name);
> +
> +#endif /* SYSFS_GT_H */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 88a83cd81ddd..e8c729f2ae00 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -811,6 +811,8 @@ struct drm_i915_private {
>   #define I915_MAX_GT 4
>   	struct intel_gt *gt[I915_MAX_GT];
>   
> +	struct kobject *sysfs_gt;
> +
>   	struct {
>   		struct i915_gem_contexts {
>   			spinlock_t lock; /* locks list */
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index a4d1759375b9..3643efde52ca 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -39,7 +39,7 @@
>   #include "i915_sysfs.h"
>   #include "intel_pm.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);
> @@ -487,6 +487,11 @@ static void i915_setup_error_capture(struct device *kdev) {}
>   static void i915_teardown_error_capture(struct device *kdev) {}
>   #endif
>   
> +static struct kobject *i915_setup_gt_sysfs(struct kobject *parent)
> +{
> +	return kobject_create_and_add("gt", parent);
> +}
> +
>   void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>   {
>   	struct device *kdev = dev_priv->drm.primary->kdev;
> @@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>   	if (ret)
>   		drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
>   
> +	dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj);

Why not directly kobject_create_and_add("gt", parent) ? up to you.

Regards
Andrzej

> +	if (!dev_priv->sysfs_gt)
> +		drm_warn(&dev_priv->drm,
> +			 "failed to register GT sysfs directory\n");
> +
>   	i915_setup_error_capture(kdev);
>   
>   	intel_engines_add_sysfs(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h
> index 41afd4366416..243a17741e3f 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__
>   
> +struct device;
>   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);
>