Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Hi Sujaritha,
[...]
> > +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> > +{
> > +}
>
> Is there a reason for this function to not be populated ?
yes, there is, indeed, something missing here. There has been a
fix bout this floating around from Chris about sysfs_gt kobjects.
I will check if we can add the fix at the next verion.
Thanks,
Andi
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
On 10/13/2021 5:08 PM, Andi Shyti wrote:
From: Andi Shyti
The GT has its own properties and in sysfs they should be grouped
in the 'gt/' directory.
Create a 'gt/' directory in sysfs which will contain gt0...gtN
directories related to each tile configured in the GPU. Move the
power management files inside those directories.
The previous power management files are kept in their original
root directory to avoid breaking the ABI. They point to the tile
'0' and a warning message is printed whenever accessed to. The
deprecated interface needs for the CONFIG_SYSFS_DEPRECATED_V2
flag in order to be generated.
The new sysfs structure will have a similar layout for the 4 tile
case:
/sys/.../card0
├── gt
│ ├── gt0
│ │ ├── id
│ │ ├── rc6_enable
│ │ ├── rc6_residency_ms
│ │ ├── rps_act_freq_mhz
│ │ ├── rps_boost_freq_mhz
│ │ ├── rps_cur_freq_mhz
│ │ ├── rps_max_freq_mhz
│ │ ├── rps_min_freq_mhz
│ │ ├── rps_RP0_freq_mhz
│ │ ├── rps_RP1_freq_mhz
│ │ └── rps_RPn_freq_mhz
. .
. .
. .
│ └── gt3
│ ├── id
│ ├── rc6_enable
│ ├── rc6_residency_ms
│ ├── rps_act_freq_mhz
│ ├── rps_boost_freq_mhz
│ ├── rps_cur_freq_mhz
│ ├── rps_max_freq_mhz
│ ├── rps_min_freq_mhz
│ ├── rps_RP0_freq_mhz
│ ├── rps_RP1_freq_mhz
│ └── rps_RPn_freq_mhz
├── gt_act_freq_mhz -+
├── gt_boost_freq_mhz |
├── gt_cur_freq_mhz|Original interface
├── gt_max_freq_mhz+─-> kept as existing ABI;
├── gt_min_freq_mhz|it points to gt0/
├── gt_RP0_freq_mhz|
└── gt_RP1_freq_mhz|
└── gt_RPn_freq_mhz -+
Signed-off-by: Andi Shyti
Signed-off-by: Lucas De Marchi
Cc: Matt Roper
Cc: Sujaritha Sundaresan
Cc: Tvrtko Ursulin
---
Hi,
this patch needs to go on top of Matt Roper's multitile series:
https://patchwork.freedesktop.org/series/95631/
because it requires multitile support.
Matt if you want and it's not much hassle for you, you can take
this patch along with your series.
Thanks,
Andi
drivers/gpu/drm/i915/Makefile | 4 +-
drivers/gpu/drm/i915/gt/intel_gt.c| 3 +
drivers/gpu/drm/i915/gt/sysfs_gt.c| 130 +
drivers/gpu/drm/i915/gt/sysfs_gt.h| 45 +++
drivers/gpu/drm/i915/gt/sysfs_gt_pm.c | 394 ++
drivers/gpu/drm/i915/gt/sysfs_gt_pm.h | 16 ++
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/i915_sysfs.c | 328 +
drivers/gpu/drm/i915/i915_sysfs.h | 3 +
10 files changed, 607 insertions(+), 319 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 21b05ed0e4e8c..f39e00a0d584f 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -120,7 +120,9 @@ gt-y += \
gt/intel_timeline.o \
gt/intel_workarounds.o \
gt/shmem_utils.o \
- gt/sysfs_engines.o
+ gt/sysfs_engines.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 0879e30ace7cc..748a21ab717d2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -21,6 +21,7 @@
#include "intel_rps.h"
#include "intel_uncore.h"
#include "shmem_utils.h"
+#include "sysfs_gt.h"
#include "pxp/intel_pxp.h"
static void
@@ -448,6 +449,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
intel_rps_driver_register(>rps);
intel_gt_debugfs_register(gt);
+ intel_gt_sysfs_register(gt);
}
static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
@@ -764,6 +766,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
{
intel_wakeref_t wakeref;
+ intel_gt_sysfs_unregister(gt);
intel_rps_driver_unregister(>rps);
intel_pxp_fini(>pxp);
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 0..0d1398b2d61ce
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "i915_drv.h"
+#include "i915_sysfs.h"
+#include
[Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
From: Andi Shyti
The GT has its own properties and in sysfs they should be grouped
in the 'gt/' directory.
Create a 'gt/' directory in sysfs which will contain gt0...gtN
directories related to each tile configured in the GPU. Move the
power management files inside those directories.
The previous power management files are kept in their original
root directory to avoid breaking the ABI. They point to the tile
'0' and a warning message is printed whenever accessed to. The
deprecated interface needs for the CONFIG_SYSFS_DEPRECATED_V2
flag in order to be generated.
The new sysfs structure will have a similar layout for the 4 tile
case:
/sys/.../card0
├── gt
│ ├── gt0
│ │ ├── id
│ │ ├── rc6_enable
│ │ ├── rc6_residency_ms
│ │ ├── rps_act_freq_mhz
│ │ ├── rps_boost_freq_mhz
│ │ ├── rps_cur_freq_mhz
│ │ ├── rps_max_freq_mhz
│ │ ├── rps_min_freq_mhz
│ │ ├── rps_RP0_freq_mhz
│ │ ├── rps_RP1_freq_mhz
│ │ └── rps_RPn_freq_mhz
. .
. .
. .
│ └── gt3
│ ├── id
│ ├── rc6_enable
│ ├── rc6_residency_ms
│ ├── rps_act_freq_mhz
│ ├── rps_boost_freq_mhz
│ ├── rps_cur_freq_mhz
│ ├── rps_max_freq_mhz
│ ├── rps_min_freq_mhz
│ ├── rps_RP0_freq_mhz
│ ├── rps_RP1_freq_mhz
│ └── rps_RPn_freq_mhz
├── gt_act_freq_mhz -+
├── gt_boost_freq_mhz |
├── gt_cur_freq_mhz|Original interface
├── gt_max_freq_mhz+─-> kept as existing ABI;
├── gt_min_freq_mhz|it points to gt0/
├── gt_RP0_freq_mhz|
└── gt_RP1_freq_mhz|
└── gt_RPn_freq_mhz -+
Signed-off-by: Andi Shyti
Signed-off-by: Lucas De Marchi
Cc: Matt Roper
Cc: Sujaritha Sundaresan
Cc: Tvrtko Ursulin
---
Hi,
this patch needs to go on top of Matt Roper's multitile series:
https://patchwork.freedesktop.org/series/95631/
because it requires multitile support.
Matt if you want and it's not much hassle for you, you can take
this patch along with your series.
Thanks,
Andi
drivers/gpu/drm/i915/Makefile | 4 +-
drivers/gpu/drm/i915/gt/intel_gt.c| 3 +
drivers/gpu/drm/i915/gt/sysfs_gt.c| 130 +
drivers/gpu/drm/i915/gt/sysfs_gt.h| 45 +++
drivers/gpu/drm/i915/gt/sysfs_gt_pm.c | 394 ++
drivers/gpu/drm/i915/gt/sysfs_gt_pm.h | 16 ++
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/i915_sysfs.c | 328 +
drivers/gpu/drm/i915/i915_sysfs.h | 3 +
10 files changed, 607 insertions(+), 319 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 21b05ed0e4e8c..f39e00a0d584f 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -120,7 +120,9 @@ gt-y += \
gt/intel_timeline.o \
gt/intel_workarounds.o \
gt/shmem_utils.o \
- gt/sysfs_engines.o
+ gt/sysfs_engines.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 0879e30ace7cc..748a21ab717d2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -21,6 +21,7 @@
#include "intel_rps.h"
#include "intel_uncore.h"
#include "shmem_utils.h"
+#include "sysfs_gt.h"
#include "pxp/intel_pxp.h"
static void
@@ -448,6 +449,7 @@ void intel_gt_driver_register(struct intel_gt *gt)
intel_rps_driver_register(>rps);
intel_gt_debugfs_register(gt);
+ intel_gt_sysfs_register(gt);
}
static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
@@ -764,6 +766,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
{
intel_wakeref_t wakeref;
+ intel_gt_sysfs_unregister(gt);
intel_rps_driver_unregister(>rps);
intel_pxp_fini(>pxp);
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 0..0d1398b2d61ce
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "i915_drv.h"
+#include "i915_sysfs.h"
+#include "intel_gt.h"
+#include "intel_gt_types.h"
+#include "intel_rc6.h"
+
+#include "sysfs_gt.h"
+#include
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Hi Tvrtko,
> > > > + }
> > > > +
> > > > + intel_gt_sysfs_pm_remove(gt, root);
> > > > + kobject_put(root);
> > >
> > > Maybe stick to the same terminology regarding root and parent.
> >
> > yes.
> >
> > > Get/put on the parent looks unbalanced. Both register and unregister take
> > > a
> > > reference and only unregister releases it. But do you even need a
> > > reference?
> >
> > why? I take it here:
> >
> > static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
> > {
> > return kobject_get(>i915->drm.primary->kdev->kobj);
> > }
> >
> > at the beginning (when the driver is loaded) and I release it at
> > the end (when the driver is unloaded). Am I not seeing something?
>
> Gt_to_parent_obj at the top of intel_gt_sysfs_register balances out with the
> put at the end of the same function. What balances out gt_to_parent_obj from
> intel_gt_sysfs_register?
And... you are right!
either nothing or too many :)
> > > I am also tempted by the _once alternative, but then it makes less sense
> > > to
> > > include name & pid.
> >
> > It's true, it can be an unrelenting message, and I thought of it,
> > but if the user is resilient at reading out from the wrong
> > directory, why shouldn't I :)
>
> Because we always try to avoid emitting spammy logs when they can be easily
> triggered by userspace. Can we do rate limit? I think that could work well
> with logging the process name & pid.
yes, if two people suggested the same thing, most probably that's
the right thing to do.
> Also, we need an entry in Documentation/ABI/obsolete/.
I was waiting this patch to get in shape before adding the
interface to obsolete.
I will include it in the next patch.
Thanks a lot for the review,
Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
On 14/02/2020 13:18, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-02-14 12:54:35)
On 14/02/2020 11:03, Andi Shyti wrote:
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
+{
+ struct kobject *kobj = >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);
+
+ drm_warn(>drm, "the interface is obsolete, use gt/\n");
Can you log current->name & pid?
I am also thinking is a level down from warn would be better. Notice
sounds intuitively correct to me.
git grep -e 'pr.*obsolete' | grep warn | wc -l
21
git grep -e 'pr.*obsolete' | grep notice | wc -l
1
git grep -e 'pr.*obsolete' | grep info | wc -l
4
Looks like warn's back on the menu, boys.
Majority is just wrong. :P
I am also tempted by the _once alternative, but then it makes less sense
to include name & pid.
I'm more afraid that there are users out there that frequently poke
these files.
Agreed, I think best option is to go with ratelimited and name & pid
logged. And more verbosity about what has been access and what should be
accessed instead.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
On 14/02/2020 13:16, Andi Shyti wrote:
Hi Tvrtko,
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.
Can you paste the new and legacy paths in the commit message?
sure!
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h
b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index 96890dd12b5f..552a27cc0622 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 kobj;
sysfs_root or something like would perhaps be more descriptive?
it's a kobj, but yes, I can call it that.
+static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
+{
+ return kobject_get(>i915->drm.primary->kdev->kobj);
It's a bit surprising X_to_Y helper get a reference as well, no?
gt_get_parent_obj perhaps? But where is this released?
sure!
the kobject put is handled down, for all the cases, have I missed
any?
+}
+
+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 void sysfs_gt_kobj_release(struct kobject *kobj)
+{
+ struct intel_gt *gt = kobj_to_gt(kobj);
+
+ drm_info(>i915->drm, "releasing interface\n");
Debugging remnants.
I wanted to fill this function with a goodbye message :)
+void intel_gt_sysfs_register(struct intel_gt *gt)
+{
+ struct kobject *kparent = gt_to_parent_obj(gt);
+ int ret;
+
+ ret = kobject_init_and_add(>kobj, _gt_ktype, kparent, "gt");
+ if (ret) {
+ drm_err(>i915->drm, "failed to initialize sysfs file\n");
+ kobject_put(>kobj);
So you want gt->kobj to be embedded struct and you want to then override the
release vfunc so it is not freed, but what is the specific reason you want
it embedded?
it looked to me like the cleanest way.
There is no real "struct device" that is containing the object I
am creating, sot that the set_drvdata() was producing some
unwanted effects. Embedding it in the gt, I can always get
easily to the gt structure containign the kobject.
Got it.
+void intel_gt_sysfs_unregister(struct intel_gt *gt)
+{
+ struct kobject *root = gt_to_parent_obj(gt);
+
+ if (>kobj) {
This is always true.
remannt from a vim replace command :)
+ sysfs_remove_file(>kobj, _attr_gt_info.attr);
+ intel_gt_sysfs_pm_remove(gt, >kobj);
+ kobject_put(>kobj);
I think kobject_put is enough to tear down the whole hierarchy so you could
simplify this.
Uh! forgot that kobject was cleaning up everythign. Thanks!
+ }
+
+ intel_gt_sysfs_pm_remove(gt, root);
+ kobject_put(root);
Maybe stick to the same terminology regarding root and parent.
yes.
Get/put on the parent looks unbalanced. Both register and unregister take a
reference and only unregister releases it. But do you even need a reference?
why? I take it here:
static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
{
return kobject_get(>i915->drm.primary->kdev->kobj);
}
at the beginning (when the driver is loaded) and I release it at
the end (when the driver is unloaded). Am I not seeing something?
Gt_to_parent_obj at the top of intel_gt_sysfs_register balances out with
the put at the end of the same function. What balances out
gt_to_parent_obj from intel_gt_sysfs_register?
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
+{
+ struct kobject *kobj = >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);
+
+ drm_warn(>drm, "the interface is obsolete, use gt/\n");
Can you log current->name & pid?
I am also thinking is a level down from warn would be better. Notice sounds
intuitively correct to me.
I swear, I thought hard to come up with a meaningful message, but
that's the best I came up with.
At least we need to mention it is about sysfs, it needs to be helpful
for the userspace developer/user to know what is being access and from
where.
I suggested to google for this. This is what I came up with as an example:
[ 775.385966] batman_adv: [Deprecated]:
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Quoting Tvrtko Ursulin (2020-02-14 12:54:35)
>
> On 14/02/2020 11:03, Andi Shyti wrote:
> > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> > +{
> > + struct kobject *kobj = >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);
> > +
> > + drm_warn(>drm, "the interface is obsolete, use gt/\n");
>
> Can you log current->name & pid?
>
> I am also thinking is a level down from warn would be better. Notice
> sounds intuitively correct to me.
git grep -e 'pr.*obsolete' | grep warn | wc -l
21
git grep -e 'pr.*obsolete' | grep notice | wc -l
1
git grep -e 'pr.*obsolete' | grep info | wc -l
4
Looks like warn's back on the menu, boys.
> I am also tempted by the _once alternative, but then it makes less sense
> to include name & pid.
I'm more afraid that there are users out there that frequently poke
these files.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Hi Tvrtko,
> > 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.
>
> Can you paste the new and legacy paths in the commit message?
sure!
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index 96890dd12b5f..552a27cc0622 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 kobj;
>
> sysfs_root or something like would perhaps be more descriptive?
it's a kobj, but yes, I can call it that.
> > +static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
> > +{
> > + return kobject_get(>i915->drm.primary->kdev->kobj);
>
> It's a bit surprising X_to_Y helper get a reference as well, no?
> gt_get_parent_obj perhaps? But where is this released?
sure!
the kobject put is handled down, for all the cases, have I missed
any?
> > +}
> > +
> > +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 void sysfs_gt_kobj_release(struct kobject *kobj)
> > +{
> > + struct intel_gt *gt = kobj_to_gt(kobj);
> > +
> > + drm_info(>i915->drm, "releasing interface\n");
>
> Debugging remnants.
I wanted to fill this function with a goodbye message :)
> > +void intel_gt_sysfs_register(struct intel_gt *gt)
> > +{
> > + struct kobject *kparent = gt_to_parent_obj(gt);
> > + int ret;
> > +
> > + ret = kobject_init_and_add(>kobj, _gt_ktype, kparent, "gt");
> > + if (ret) {
> > + drm_err(>i915->drm, "failed to initialize sysfs file\n");
> > + kobject_put(>kobj);
>
> So you want gt->kobj to be embedded struct and you want to then override the
> release vfunc so it is not freed, but what is the specific reason you want
> it embedded?
it looked to me like the cleanest way.
There is no real "struct device" that is containing the object I
am creating, sot that the set_drvdata() was producing some
unwanted effects. Embedding it in the gt, I can always get
easily to the gt structure containign the kobject.
> > +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> > +{
> > + struct kobject *root = gt_to_parent_obj(gt);
> > +
> > + if (>kobj) {
>
> This is always true.
remannt from a vim replace command :)
> > + sysfs_remove_file(>kobj, _attr_gt_info.attr);
> > + intel_gt_sysfs_pm_remove(gt, >kobj);
> > + kobject_put(>kobj);
>
> I think kobject_put is enough to tear down the whole hierarchy so you could
> simplify this.
Uh! forgot that kobject was cleaning up everythign. Thanks!
> > + }
> > +
> > + intel_gt_sysfs_pm_remove(gt, root);
> > + kobject_put(root);
>
> Maybe stick to the same terminology regarding root and parent.
yes.
> Get/put on the parent looks unbalanced. Both register and unregister take a
> reference and only unregister releases it. But do you even need a reference?
why? I take it here:
static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
{
return kobject_get(>i915->drm.primary->kdev->kobj);
}
at the beginning (when the driver is loaded) and I release it at
the end (when the driver is unloaded). Am I not seeing something?
> > +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> > +{
> > + struct kobject *kobj = >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);
> > +
> > + drm_warn(>drm, "the interface is obsolete, use gt/\n");
>
> Can you log current->name & pid?
>
> I am also thinking is a level down from warn would be better. Notice sounds
> intuitively correct to me.
I swear, I thought hard to come up with a meaningful message, but
that's the best I came up with.
> I am also tempted by the _once alternative, but then it makes less sense to
> include name & pid.
It's true, it can be an unrelenting message, and I thought of it,
but if the user is resilient at reading out from the wrong
directory, why shouldn't I :)
Andi
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Quoting Andi Shyti (2020-02-14 11:03:08)
> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev)
> +{
> + struct kobject *kobj = >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);
> +
> + drm_warn(>drm, "the interface is obsolete, use gt/\n");
> + return >gt;
I guess it's not possible to flesh this out with path? And we do need it
to be warn_once else the user will be flooded.
One final request, can we also put the old entries under
CONFIG_DRM_I915_SYSFS_OBSOLETE_GT (or somesuch)
As far as the naming goes, the compromise isn't horrendous.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
On 14/02/2020 11:03, Andi Shyti 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.
Can you paste the new and legacy paths in the commit message?
Signed-off-by: Andi Shyti
---
Hi,
this version has some more substantial differences, nothing that
changes the wanted behavior, though.
Thanks Chris, Jani and Tvrtko for the reviews,
Andi
Changelog:
==
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 | 85 +
drivers/gpu/drm/i915/gt/sysfs_gt.h | 22 ++
drivers/gpu/drm/i915/gt/sysfs_gt_pm.c| 446 +++
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, 581 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 49eed50ef0a4..3b81c8a96c06 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -106,7 +106,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(>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(>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..552a27cc0622 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 kobj;
sysfs_root or something like would perhaps be more descriptive?
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 ..4ca140fc215f
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: MIT
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include
+#include
+#include
+#include
+
+#include "../i915_drv.h"
+#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_to_parent_obj(struct intel_gt *gt)
+{
+ return kobject_get(>i915->drm.primary->kdev->kobj);
It's a bit surprising X_to_Y helper get a reference as well, no?
gt_get_parent_obj perhaps? But where is this released?
+}
+
+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 void sysfs_gt_kobj_release(struct kobject *kobj)
+{
+ struct intel_gt *gt = kobj_to_gt(kobj);
+
+ drm_info(>i915->drm, "releasing interface\n");
Debugging remnants.
+}
+
+static struct kobj_type sysfs_gt_ktype = {
+ .release = sysfs_gt_kobj_release,
+ .sysfs_ops = _sysfs_ops,
+};
+
+void intel_gt_sysfs_register(struct intel_gt *gt)
+{
+ struct kobject *kparent = gt_to_parent_obj(gt);
+ int ret;
+
+ ret = kobject_init_and_add(>kobj, _gt_ktype, kparent, "gt");
+ if (ret) {
+
[Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
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.
Signed-off-by: Andi Shyti
---
Hi,
this version has some more substantial differences, nothing that
changes the wanted behavior, though.
Thanks Chris, Jani and Tvrtko for the reviews,
Andi
Changelog:
==
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 | 85 +
drivers/gpu/drm/i915/gt/sysfs_gt.h | 22 ++
drivers/gpu/drm/i915/gt/sysfs_gt_pm.c| 446 +++
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, 581 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 49eed50ef0a4..3b81c8a96c06 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -106,7 +106,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(>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(>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..552a27cc0622 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 kobj;
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 ..4ca140fc215f
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: MIT
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include
+#include
+#include
+#include
+
+#include "../i915_drv.h"
+#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_to_parent_obj(struct intel_gt *gt)
+{
+ return kobject_get(>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 void sysfs_gt_kobj_release(struct kobject *kobj)
+{
+ struct intel_gt *gt = kobj_to_gt(kobj);
+
+ drm_info(>i915->drm, "releasing interface\n");
+}
+
+static struct kobj_type sysfs_gt_ktype = {
+ .release = sysfs_gt_kobj_release,
+ .sysfs_ops = _sysfs_ops,
+};
+
+void intel_gt_sysfs_register(struct intel_gt *gt)
+{
+ struct kobject *kparent = gt_to_parent_obj(gt);
+ int ret;
+
+ ret = kobject_init_and_add(>kobj, _gt_ktype, kparent, "gt");
+ if (ret) {
+ drm_err(>i915->drm, "failed to initialize sysfs file\n");
+ kobject_put(>kobj);
+ goto parent_files;
+ }
+
+ ret = sysfs_create_file(>kobj, _attr_gt_info.attr);
+ if (ret)
+ drm_err(>i915->drm, "failed to create sysfs gt info
files\n");
+
+ intel_gt_sysfs_pm_init(gt, >kobj);
+
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Hi Jani,
> > 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(>rps);
> >
> > debugfs_gt_register(gt);
>
> Yikes, when did this happen? Not good. We don't own the debugfs
> "namespace" prefix.
>
> > + sysfs_gt_register(gt);
>
> Ditto for the sysfs namespace prefix.
>
> I guess it's not likely debugfs or sysfs would add functions named like
> that, but if they did, they'd be right to call these names a violation
> of their namespace.
>
> I have been promoting the idea of naming functions somewhat based on the
> name of the file they reside in... so do we want to name the files like
> this either?
Yes, you're right here. I will rename the "debugfs" function and
fix the sysfs in my next patch.
Thanks,
Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
On Sat, 08 Feb 2020, Andi Shyti wrote:
> From: Andi Shyti
>
> 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.
>
> Signed-off-by: Andi Shyti
> ---
> 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 | 66
> drivers/gpu/drm/i915/gt/sysfs_gt.h | 15 +
> drivers/gpu/drm/i915/gt/sysfs_gt_pm.c| 431 +++
> drivers/gpu/drm/i915/gt/sysfs_gt_pm.h| 17 +
> drivers/gpu/drm/i915/i915_sysfs.c| 373
> 8 files changed, 536 insertions(+), 374 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 49eed50ef0a4..3b81c8a96c06 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -106,7 +106,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..8c360db14320 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(>rps);
>
> debugfs_gt_register(gt);
Yikes, when did this happen? Not good. We don't own the debugfs
"namespace" prefix.
> + sysfs_gt_register(gt);
Ditto for the sysfs namespace prefix.
I guess it's not likely debugfs or sysfs would add functions named like
that, but if they did, they'd be right to call these names a violation
of their namespace.
I have been promoting the idea of naming functions somewhat based on the
name of the file they reside in... so do we want to name the files like
this either?
BR,
Jani.
> }
>
> 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)
> {
> + sysfs_gt_unregister(gt);
> intel_rps_driver_unregister(>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..cdf659a7c74f 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 *kobj;
>
> 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 ..4eda2ae144a0
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: MIT
> +
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "../i915_drv.h"
> +#include "intel_gt.h"
> +#include "intel_gt_types.h"
> +#include "intel_rc6.h"
> +
> +#include "sysfs_gt_pm.h"
> +
> +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 attribute *gt_attrs[] = {
> + _attr_gt_info.attr,
> + NULL
> +};
> +
> +static const struct attribute_group gt_attribute_group = {
> + .attrs = gt_attrs,
> +};
> +
> +void sysfs_gt_register(struct intel_gt *gt)
> +{
> + struct device *dev;
> + int ret;
> +
> + gt->kobj = kobject_create_and_add("gt",
> + kobject_get(>i915->drm.primary->kdev->kobj));
> + if (!gt->kobj) {
> + pr_err("failed to initialize sysfs file\n");
> + return;
> + }
> +
> + dev = kobj_to_dev(gt->kobj);
> + dev_set_drvdata(dev, gt);
> +
> + ret = sysfs_create_group(gt->kobj, _attribute_group);
> + if (ret)
> + pr_err("failed to create sysfs gt info files\n");
> +
> + intel_sysfs_pm_init(gt, gt->kobj);
> +}
> +
> +void sysfs_gt_unregister(struct
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
> > > > > > 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. > > > > > > > > > > As shown by BAT, we have a conundrum; do we have to preserve the old > > > > > names forevermore? Or do we just userspace that they have to adapt? > > > > > > > > With this, I guess you are suggesting to change igt accordingly, > > > > because I'm pretty sure this interface is subject to change, > > > > sooner or later. > > > > > > Right; but what about powertop? And the bajillion wikis? > > > > mmhhh... right! > > > > > Just feels clumsy, so I'd like to spend a moment to see if we can think > > > of any options before dropping them. No matter how silly, if userspace > > > breaks, it's a regression :( > > > > > > Maybe we could do something like keep the old ones and put a deprecation > > > warning in? > > > > how about linking and declare the old interface obsolete? > > Is a possibility, but I'm not keen on having more and more dangling > symlinks. > > 1. Do nothing; have redundant files. > 2. Remove them, expect complaints. > 3. Mark them as deprecated, remove in 5.9? > 4. Symlinks forevermore (not clear if we can symlink and emit a warning) my symlinks suggestions was a mix of the above and it would be: 1. add symlinks (or redundant files with warning) and mark the interfaces as deprecated. 2. in 5.9 remove the symlinks (or redundant files, I like the idea of warning userspace). 3. expect anyway complaints :-P Let's give it a few days, perhaps after the weekend someone might have an opinion or better recommendation. Andi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Hi Chris, > > > > 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. > > > > > > As shown by BAT, we have a conundrum; do we have to preserve the old > > > names forevermore? Or do we just userspace that they have to adapt? > > > > With this, I guess you are suggesting to change igt accordingly, > > because I'm pretty sure this interface is subject to change, > > sooner or later. > > Right; but what about powertop? And the bajillion wikis? mmhhh... right! > Just feels clumsy, so I'd like to spend a moment to see if we can think > of any options before dropping them. No matter how silly, if userspace > breaks, it's a regression :( > > Maybe we could do something like keep the old ones and put a deprecation > warning in? how about linking and declare the old interface obsolete? Andi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
On Sat, Feb 08, 2020 at 04:26:18PM +, Chris Wilson wrote: > Quoting Andi Shyti (2020-02-08 12:27:59) > > From: Andi Shyti > > > > 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. > > As shown by BAT, we have a conundrum; do we have to preserve the old > names forevermore? Or do we just userspace that they have to adapt? With this, I guess you are suggesting to change igt accordingly, because I'm pretty sure this interface is subject to change, sooner or later. Andi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Quoting Andi Shyti (2020-02-08 17:01:53) > Hi Chris, > > > > > > 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. > > > > > > > > As shown by BAT, we have a conundrum; do we have to preserve the old > > > > names forevermore? Or do we just userspace that they have to adapt? > > > > > > With this, I guess you are suggesting to change igt accordingly, > > > because I'm pretty sure this interface is subject to change, > > > sooner or later. > > > > Right; but what about powertop? And the bajillion wikis? > > mmhhh... right! > > > Just feels clumsy, so I'd like to spend a moment to see if we can think > > of any options before dropping them. No matter how silly, if userspace > > breaks, it's a regression :( > > > > Maybe we could do something like keep the old ones and put a deprecation > > warning in? > > how about linking and declare the old interface obsolete? Is a possibility, but I'm not keen on having more and more dangling symlinks. 1. Do nothing; have redundant files. 2. Remove them, expect complaints. 3. Mark them as deprecated, remove in 5.9? 4. Symlinks forevermore (not clear if we can symlink and emit a warning) 5. Profit? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Quoting Andi Shyti (2020-02-08 16:51:39) > On Sat, Feb 08, 2020 at 04:26:18PM +, Chris Wilson wrote: > > Quoting Andi Shyti (2020-02-08 12:27:59) > > > From: Andi Shyti > > > > > > 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. > > > > As shown by BAT, we have a conundrum; do we have to preserve the old > > names forevermore? Or do we just userspace that they have to adapt? > > With this, I guess you are suggesting to change igt accordingly, > because I'm pretty sure this interface is subject to change, > sooner or later. Right; but what about powertop? And the bajillion wikis? Just feels clumsy, so I'd like to spend a moment to see if we can think of any options before dropping them. No matter how silly, if userspace breaks, it's a regression :( Maybe we could do something like keep the old ones and put a deprecation warning in? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
Quoting Andi Shyti (2020-02-08 12:27:59) > From: Andi Shyti > > 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. As shown by BAT, we have a conundrum; do we have to preserve the old names forevermore? Or do we just userspace that they have to adapt? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files
From: Andi Shyti
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.
Signed-off-by: Andi Shyti
---
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 | 66
drivers/gpu/drm/i915/gt/sysfs_gt.h | 15 +
drivers/gpu/drm/i915/gt/sysfs_gt_pm.c| 431 +++
drivers/gpu/drm/i915/gt/sysfs_gt_pm.h| 17 +
drivers/gpu/drm/i915/i915_sysfs.c| 373
8 files changed, 536 insertions(+), 374 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 49eed50ef0a4..3b81c8a96c06 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -106,7 +106,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..8c360db14320 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(>rps);
debugfs_gt_register(gt);
+ sysfs_gt_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)
{
+ sysfs_gt_unregister(gt);
intel_rps_driver_unregister(>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..cdf659a7c74f 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 *kobj;
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 ..4eda2ae144a0
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: MIT
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#include
+#include
+#include
+#include
+
+#include "../i915_drv.h"
+#include "intel_gt.h"
+#include "intel_gt_types.h"
+#include "intel_rc6.h"
+
+#include "sysfs_gt_pm.h"
+
+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 attribute *gt_attrs[] = {
+ _attr_gt_info.attr,
+ NULL
+};
+
+static const struct attribute_group gt_attribute_group = {
+ .attrs = gt_attrs,
+};
+
+void sysfs_gt_register(struct intel_gt *gt)
+{
+ struct device *dev;
+ int ret;
+
+ gt->kobj = kobject_create_and_add("gt",
+ kobject_get(>i915->drm.primary->kdev->kobj));
+ if (!gt->kobj) {
+ pr_err("failed to initialize sysfs file\n");
+ return;
+ }
+
+ dev = kobj_to_dev(gt->kobj);
+ dev_set_drvdata(dev, gt);
+
+ ret = sysfs_create_group(gt->kobj, _attribute_group);
+ if (ret)
+ pr_err("failed to create sysfs gt info files\n");
+
+ intel_sysfs_pm_init(gt, gt->kobj);
+}
+
+void sysfs_gt_unregister(struct intel_gt *gt)
+{
+ if (!gt->kobj)
+ return;
+
+ intel_sysfs_pm_remove(gt, gt->kobj);
+ sysfs_remove_group(gt->kobj, _attribute_group);
+}
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 ..07638dde6e28
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef SYSFS_GT_H
+#define SYSFS_GT_H
+
+struct intel_gt;
+
+void sysfs_gt_register(struct intel_gt *gt);
+void sysfs_gt_unregister(struct intel_gt *gt);
+
+#endif /* SYSFS_GT_H */
diff --git a/drivers/gpu/drm/i915/gt/sysfs_gt_pm.c
