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

2020-02-25 Thread Andi Shyti 
> > > > > > +void intel_gt_sysfs_register(struct intel_gt *gt)
> > > > > > +{
> > > > > > +   struct kobject *parent = kobject_get(gt_get_parent_obj(gt));
> > > > > > +   int ret;
> > > > > > +
> > 
> > and if I need to call kobject_put at the end. If for some reason
> > the files have failed to be initialized, I would have an
> > unbalanced put and a warning would be printed.
> > 
> > I'll summarize in pseudo code:
> > 
> > intel_gt_sysfs_register()
> > {
> > kobject_init_and_add(sysfs_root...); /* which calls kobject_get() 
> > inside */
> > if (fails)
> > kobject_put(sysfs_root); /* reference goes to '0' */
> > }
> > 
> > intel_gt_sysfs_unregister()
> > {
> > option1: I don't call kobject_put(), I have an unbalanced
> >   situation as you reviewed in patch 1.
> > 
> >  option2: I call kobject_put(), if it did fail during init
> >   there is an unbalanced situation, which is
> >   handled but an annoying WARN() is issued.
> > 
> > option3: I check if "state_initialized" which I suppose
> >   has been properly initialised during declaration
> >   (maybe too paranoic?) and call _put()
> >   accordingly
> > }
> 
> Yes you are right, I confused the two parents again. :I

this little things are good for jambling the brain up :)

> Okay then, is the extra kobject_get/put on the parent
> (kobject_get(gt_get_parent_obj(gt) - this one) needed?

I do not see any strong reason for calling kobject_get, I do it
only because I am creating files in there and I don't want anyone
to free those without my permission. Otherwise, the creation and
the destruction of the son object would take care of the
refcount.

Will it ever happen that parent will be destroyed before I have
time to release the files? I don't think so, but I do it more for
the form than for the use. In the sense that "I have stuff in
there and I declare I have stuff in there and I declare when I
won't need them anymore".

Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2020-02-25 Thread Tvrtko Ursulin 



On 25/02/2020 01:35, Andi Shyti wrote:

+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(>sysfs_root);


Slightly nicer would be looking at  kobj->state_initialized for this check I
think. Or even kref_get_unless_zero on kobj->kref? Ugliness there is double
put on sucess which makes me ask whether holding a reference on parent is
even needed? It can't go away so perhaps it isn't.


I'd rather use the state_initialized, even though I don't trust
its value if the kobject has failed to initialise earlier, I
trust it only if it's '1', maybe I'm paranoic.


But is the reference even needed?


yes, because I _get it here (i.e. above, during initialization):


+void intel_gt_sysfs_register(struct intel_gt *gt)
+{
+   struct kobject *parent = kobject_get(gt_get_parent_obj(gt));
+   int ret;
+


and if I need to call kobject_put at the end. If for some reason
the files have failed to be initialized, I would have an
unbalanced put and a warning would be printed.

I'll summarize in pseudo code:

intel_gt_sysfs_register()
{
kobject_init_and_add(sysfs_root...); /* which calls kobject_get() 
inside */
if (fails)
kobject_put(sysfs_root); /* reference goes to '0' */
}

intel_gt_sysfs_unregister()
{
option1: I don't call kobject_put(), I have an unbalanced
  situation as you reviewed in patch 1.

 option2: I call kobject_put(), if it did fail during init
  there is an unbalanced situation, which is
  handled but an annoying WARN() is issued.

option3: I check if "state_initialized" which I suppose
  has been properly initialised during declaration
  (maybe too paranoic?) and call _put()
  accordingly
}


Yes you are right, I confused the two parents again. :I

Okay then, is the extra kobject_get/put on the parent 
(kobject_get(gt_get_parent_obj(gt) - this one) needed?


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2020-02-24 Thread Andi Shyti 
> > > > +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(>sysfs_root);
> > > 
> > > Slightly nicer would be looking at  kobj->state_initialized for this 
> > > check I
> > > think. Or even kref_get_unless_zero on kobj->kref? Ugliness there is 
> > > double
> > > put on sucess which makes me ask whether holding a reference on parent is
> > > even needed? It can't go away so perhaps it isn't.
> > 
> > I'd rather use the state_initialized, even though I don't trust
> > its value if the kobject has failed to initialise earlier, I
> > trust it only if it's '1', maybe I'm paranoic.
> 
> But is the reference even needed?

yes, because I _get it here (i.e. above, during initialization):

> > > > +void intel_gt_sysfs_register(struct intel_gt *gt)
> > > > +{
> > > > +   struct kobject *parent = kobject_get(gt_get_parent_obj(gt));
> > > > +   int ret;
> > > > +

and if I need to call kobject_put at the end. If for some reason
the files have failed to be initialized, I would have an
unbalanced put and a warning would be printed.

I'll summarize in pseudo code:

intel_gt_sysfs_register()
{
kobject_init_and_add(sysfs_root...); /* which calls kobject_get() 
inside */
if (fails)
kobject_put(sysfs_root); /* reference goes to '0' */
}

intel_gt_sysfs_unregister()
{
option1: I don't call kobject_put(), I have an unbalanced
 situation as you reviewed in patch 1.

option2: I call kobject_put(), if it did fail during init
 there is an unbalanced situation, which is
 handled but an annoying WARN() is issued.

option3: I check if "state_initialized" which I suppose
 has been properly initialised during declaration
 (maybe too paranoic?) and call _put()
 accordingly
}

Thanks,
Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2020-02-24 Thread Tvrtko Ursulin 



On 24/02/2020 16:30, Andi Shyti wrote:

+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(>sysfs_root,
+  _gt_ktype,
+  parent, "gt");
+   if (ret) {
+   drm_err(>i915->drm, "failed to initialize sysfs file\n");


I'd perhaps pin point the failure more by s/file/GT sysfs root/.


OK


+   kobject_put(>sysfs_root);


Is the reference needed for the registration steps? I am thinking if you
could kobject_get only once everything worked to simplify.


I haven't really understood what you mean here. Are you saying
that kobject_put not needed? in the lib/kobject.c it says as
comment to kobject_init_and_add():

"
  * If this function returns an error, kobject_put() must be called to
  * properly clean up the memory associated with the object.  This is the
  * same type of error handling after a call to kobject_add() and kobject
  * lifetime rules are the same here.
  */
"


My mistake, I confused the two objects.


+   ret = sysfs_create_file(>sysfs_root, _attr_gt_info.attr);
+   if (ret)
+   drm_err(>i915->drm, "failed to create sysfs gt info 
files\n");
+
+   intel_gt_sysfs_pm_init(gt, >sysfs_root);


If you put this first you can avoid the goto I think which makes the
function smaller.


True!


+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(>sysfs_root);


Slightly nicer would be looking at  kobj->state_initialized for this check I
think. Or even kref_get_unless_zero on kobj->kref? Ugliness there is double
put on sucess which makes me ask whether holding a reference on parent is
even needed? It can't go away so perhaps it isn't.


I'd rather use the state_initialized, even though I don't trust
its value if the kobject has failed to initialise earlier, I
trust it only if it's '1', maybe I'm paranoic.


But is the reference even needed?

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2020-02-24 Thread Andi Shyti 
> > +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(>sysfs_root,
> > +  _gt_ktype,
> > +  parent, "gt");
> > +   if (ret) {
> > +   drm_err(>i915->drm, "failed to initialize sysfs file\n");
> 
> I'd perhaps pin point the failure more by s/file/GT sysfs root/.

OK

> > +   kobject_put(>sysfs_root);
> 
> Is the reference needed for the registration steps? I am thinking if you
> could kobject_get only once everything worked to simplify.

I haven't really understood what you mean here. Are you saying
that kobject_put not needed? in the lib/kobject.c it says as
comment to kobject_init_and_add():

"
 * If this function returns an error, kobject_put() must be called to
 * properly clean up the memory associated with the object.  This is the
 * same type of error handling after a call to kobject_add() and kobject
 * lifetime rules are the same here.
 */
"

> > +   ret = sysfs_create_file(>sysfs_root, _attr_gt_info.attr);
> > +   if (ret)
> > +   drm_err(>i915->drm, "failed to create sysfs gt info 
> > files\n");
> > +
> > +   intel_gt_sysfs_pm_init(gt, >sysfs_root);
> 
> If you put this first you can avoid the goto I think which makes the
> function smaller.

True!

> > +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(>sysfs_root);
> 
> Slightly nicer would be looking at  kobj->state_initialized for this check I
> think. Or even kref_get_unless_zero on kobj->kref? Ugliness there is double
> put on sucess which makes me ask whether holding a reference on parent is
> even needed? It can't go away so perhaps it isn't.

I'd rather use the state_initialized, even though I don't trust
its value if the kobject has failed to initialise earlier, I
trust it only if it's '1', maybe I'm paranoic.

> > +   /*
> > +* 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",
> 
> Saying interface two times sounds a bit suboptimal but I leave this to
> native speakers to improve.
> 
> Can you get to the name of the sysfs control here?

sure.

> "(%s, %d) is trying to access deprecated '%s' sysfs control. Please use
> 'gt/%s' instead.". Something like that?

yes... it's not always easy to write logs when you have to stay
within the 80 characters

Thanks 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 v5] drm/i915/gt: make a gt sysfs group and move power management files

2020-02-24 Thread Tvrtko Ursulin 


On 19/02/2020 19:30, 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.

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 
---
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(>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..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 ..9335a92d5248
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,79 @@
+// 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_get_parent_obj(struct intel_gt *gt)
+{
+   return >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 = _sysfs_ops,
+};
+
+void intel_gt_sysfs_register(struct intel_gt *gt)
+{
+   struct kobject *parent = kobject_get(gt_get_parent_obj(gt));
+   int ret;
+
+   ret =

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

2020-02-20 Thread Jani Nikula 
On Wed, 19 Feb 2020, 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.
>
> 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 
> ---
> 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(>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..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 ..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 
> +#include 
> +#include 
> +#include 
> +
> +#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 >i915->drm.primary->kdev->kobj;
> +}
> +
> +static ssize_t gt_info_show(struct device *dev,
> + struct device_attribute *attr,
> +

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

2020-02-19 Thread 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.

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 
---
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(>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..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 ..9335a92d5248
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/sysfs_gt.c
@@ -0,0 +1,79 @@
+// 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_get_parent_obj(struct intel_gt *gt)
+{
+   return >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 = _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(>sysfs_root,
+  _gt_ktype,
+