diff mbox series

[v2,3/3] drm/i915: Move to new PM core fields

Message ID 1544797347-20601-4-git-send-email-vincent.guittot@linaro.org (mailing list archive)
State RFC, archived
Headers show
Series PM/pm_runtime: move on hrtimer and nsec | expand

Commit Message

Vincent Guittot Dec. 14, 2018, 2:22 p.m. UTC
With jiffies been replaced by raw ns in PM core accounting, 915 driver is
updated to use this new time infrastructure.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
 drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

Comments

Ulf Hansson Dec. 14, 2018, 2:36 p.m. UTC | #1
On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> updated to use this new time infrastructure.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index d6c8f8f..cf6437d 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
>                  */
>                 if (kdev->power.runtime_status == RPM_SUSPENDED) {
>                         if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> -                               i915->pmu.suspended_jiffies_last =
> -                                                 kdev->power.suspended_jiffies;
> +                               i915->pmu.suspended_time_last =
> +                                       kdev->power.suspended_time;
>

Huh, so patch 2 introduces a complier error because of removing the
old fields. We can't have that.

Ideally the driver shouldn't touch these fields in the first place,
but because the fields are defined in a public header, there is always
a risk that they becomes abused. I would appreciate if we can change
the driver move away from using these fields and make that a patch
preceding patch 2.

> -                       val = kdev->power.suspended_jiffies -
> -                             i915->pmu.suspended_jiffies_last;
> -                       val += jiffies - kdev->power.accounting_timestamp;
> +                       val = kdev->power.suspended_time -
> +                               i915->pmu.suspended_time_last;
> +                       val += ktime_to_ns(ktime_get()) -
> +                               kdev->power.accounting_timestamp;
>
> -                       val = jiffies_to_nsecs(val);
>                         val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
>
>                         i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> index 7f164ca..3dc2a30 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.h
> +++ b/drivers/gpu/drm/i915/i915_pmu.h
> @@ -95,9 +95,9 @@ struct i915_pmu {
>          */
>         struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
>         /**
> -        * @suspended_jiffies_last: Cached suspend time from PM core.
> +        * @suspended_time_last: Cached suspend time from PM core.
>          */
> -       unsigned long suspended_jiffies_last;
> +       u64 suspended_time_last;
>         /**
>          * @i915_attr: Memory block holding device attributes.
>          */
> --
> 2.7.4
>

Kind regards
Uffe
Vincent Guittot Dec. 17, 2018, 2:21 p.m. UTC | #2
On Fri, 14 Dec 2018 at 15:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> > updated to use this new time infrastructure.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
> >  drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index d6c8f8f..cf6437d 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >                  */
> >                 if (kdev->power.runtime_status == RPM_SUSPENDED) {
> >                         if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > -                               i915->pmu.suspended_jiffies_last =
> > -                                                 kdev->power.suspended_jiffies;
> > +                               i915->pmu.suspended_time_last =
> > +                                       kdev->power.suspended_time;
> >
>
> Huh, so patch 2 introduces a complier error because of removing the
> old fields. We can't have that.

I agree
The patch was mainly to raise discussion
>
> Ideally the driver shouldn't touch these fields in the first place,
> but because the fields are defined in a public header, there is always
> a risk that they becomes abused. I would appreciate if we can change
> the driver move away from using these fields and make that a patch
> preceding patch 2.

In fact, the driver doesn't only use some internal fields but also
takes the power.lock
IIUC, the driver wants the current pm runtime suspended time to
estimate a metric when perf events are not accessible
I'm going to send a new version with a runtime pm interface proposal to fix this

Vincent

>
> > -                       val = kdev->power.suspended_jiffies -
> > -                             i915->pmu.suspended_jiffies_last;
> > -                       val += jiffies - kdev->power.accounting_timestamp;
> > +                       val = kdev->power.suspended_time -
> > +                               i915->pmu.suspended_time_last;
> > +                       val += ktime_to_ns(ktime_get()) -
> > +                               kdev->power.accounting_timestamp;
> >
> > -                       val = jiffies_to_nsecs(val);
> >                         val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >
> >                         i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> > index 7f164ca..3dc2a30 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.h
> > +++ b/drivers/gpu/drm/i915/i915_pmu.h
> > @@ -95,9 +95,9 @@ struct i915_pmu {
> >          */
> >         struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
> >         /**
> > -        * @suspended_jiffies_last: Cached suspend time from PM core.
> > +        * @suspended_time_last: Cached suspend time from PM core.
> >          */
> > -       unsigned long suspended_jiffies_last;
> > +       u64 suspended_time_last;
> >         /**
> >          * @i915_attr: Memory block holding device attributes.
> >          */
> > --
> > 2.7.4
> >
>
> Kind regards
> Uffe
Ulf Hansson Dec. 17, 2018, 3:17 p.m. UTC | #3
On Mon, 17 Dec 2018 at 15:22, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Fri, 14 Dec 2018 at 15:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> > > updated to use this new time infrastructure.
> > >
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
> > >  drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > > index d6c8f8f..cf6437d 100644
> > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > >                  */
> > >                 if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > >                         if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > > -                               i915->pmu.suspended_jiffies_last =
> > > -                                                 kdev->power.suspended_jiffies;
> > > +                               i915->pmu.suspended_time_last =
> > > +                                       kdev->power.suspended_time;
> > >
> >
> > Huh, so patch 2 introduces a complier error because of removing the
> > old fields. We can't have that.
>
> I agree
> The patch was mainly to raise discussion
> >
> > Ideally the driver shouldn't touch these fields in the first place,
> > but because the fields are defined in a public header, there is always
> > a risk that they becomes abused. I would appreciate if we can change
> > the driver move away from using these fields and make that a patch
> > preceding patch 2.
>
> In fact, the driver doesn't only use some internal fields but also
> takes the power.lock
> IIUC, the driver wants the current pm runtime suspended time to
> estimate a metric when perf events are not accessible

Huh, so it's even worse. :-(

I had a brief look at the runtime PM deployment for these drivers and
it's really messy. Unfortunate, I have limited knowledge about the
gpu/drm drivers, so I can't tell without a investing in a deeper
analyze, of how to convert to something more generic.

> I'm going to send a new version with a runtime pm interface proposal to fix this

Yeah, please go ahead and try something, I am willing to help review.

> >
> > > -                       val = kdev->power.suspended_jiffies -
> > > -                             i915->pmu.suspended_jiffies_last;
> > > -                       val += jiffies - kdev->power.accounting_timestamp;
> > > +                       val = kdev->power.suspended_time -
> > > +                               i915->pmu.suspended_time_last;
> > > +                       val += ktime_to_ns(ktime_get()) -
> > > +                               kdev->power.accounting_timestamp;
> > >
> > > -                       val = jiffies_to_nsecs(val);
> > >                         val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> > >
> > >                         i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> > > diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
> > > index 7f164ca..3dc2a30 100644
> > > --- a/drivers/gpu/drm/i915/i915_pmu.h
> > > +++ b/drivers/gpu/drm/i915/i915_pmu.h
> > > @@ -95,9 +95,9 @@ struct i915_pmu {
> > >          */
> > >         struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
> > >         /**
> > > -        * @suspended_jiffies_last: Cached suspend time from PM core.
> > > +        * @suspended_time_last: Cached suspend time from PM core.
> > >          */
> > > -       unsigned long suspended_jiffies_last;
> > > +       u64 suspended_time_last;
> > >         /**
> > >          * @i915_attr: Memory block holding device attributes.
> > >          */
> > > --
> > > 2.7.4
> > >

Kind regards
Uffe
Rafael J. Wysocki Dec. 18, 2018, 9:57 a.m. UTC | #4
On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Fri, 14 Dec 2018 at 15:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> > > updated to use this new time infrastructure.
> > >
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
> > >  drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > > index d6c8f8f..cf6437d 100644
> > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > >                  */
> > >                 if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > >                         if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > > -                               i915->pmu.suspended_jiffies_last =
> > > -                                                 kdev->power.suspended_jiffies;
> > > +                               i915->pmu.suspended_time_last =
> > > +                                       kdev->power.suspended_time;
> > >
> >
> > Huh, so patch 2 introduces a complier error because of removing the
> > old fields. We can't have that.
>
> I agree
> The patch was mainly to raise discussion

OK, so patch [1/3] from this series should be applicable regardless, right?
Vincent Guittot Dec. 18, 2018, 9:58 a.m. UTC | #5
On Tue, 18 Dec 2018 at 10:57, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Fri, 14 Dec 2018 at 15:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> > > > updated to use this new time infrastructure.
> > > >
> > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
> > > >  drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> > > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > > > index d6c8f8f..cf6437d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > > >                  */
> > > >                 if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > > >                         if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > > > -                               i915->pmu.suspended_jiffies_last =
> > > > -                                                 kdev->power.suspended_jiffies;
> > > > +                               i915->pmu.suspended_time_last =
> > > > +                                       kdev->power.suspended_time;
> > > >
> > >
> > > Huh, so patch 2 introduces a complier error because of removing the
> > > old fields. We can't have that.
> >
> > I agree
> > The patch was mainly to raise discussion
>
> OK, so patch [1/3] from this series should be applicable regardless, right?

Yes
Rafael J. Wysocki Dec. 18, 2018, 10:03 a.m. UTC | #6
On Tue, Dec 18, 2018 at 10:58 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Tue, 18 Dec 2018 at 10:57, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Fri, 14 Dec 2018 at 15:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> > > > > updated to use this new time infrastructure.
> > > > >
> > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
> > > > >  drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> > > > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > index d6c8f8f..cf6437d 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > > > >                  */
> > > > >                 if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > > > >                         if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > > > > -                               i915->pmu.suspended_jiffies_last =
> > > > > -                                                 kdev->power.suspended_jiffies;
> > > > > +                               i915->pmu.suspended_time_last =
> > > > > +                                       kdev->power.suspended_time;
> > > > >
> > > >
> > > > Huh, so patch 2 introduces a complier error because of removing the
> > > > old fields. We can't have that.
> > >
> > > I agree
> > > The patch was mainly to raise discussion
> >
> > OK, so patch [1/3] from this series should be applicable regardless, right?
>
> Yes

OK, I'll queue it up, then.

Next time you do something like that  please mark patches for
discussion in a series as [RFC] so it is all clear.
Vincent Guittot Dec. 18, 2018, 10:08 a.m. UTC | #7
On Tue, 18 Dec 2018 at 11:03, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Dec 18, 2018 at 10:58 AM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Tue, 18 Dec 2018 at 10:57, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > On Fri, 14 Dec 2018 at 15:36, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot
> > > > > <vincent.guittot@linaro.org> wrote:
> > > > > >
> > > > > > With jiffies been replaced by raw ns in PM core accounting, 915 driver is
> > > > > > updated to use this new time infrastructure.
> > > > > >
> > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++------
> > > > > >  drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> > > > > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > index d6c8f8f..cf6437d 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > > > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915)
> > > > > >                  */
> > > > > >                 if (kdev->power.runtime_status == RPM_SUSPENDED) {
> > > > > >                         if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > > > > > -                               i915->pmu.suspended_jiffies_last =
> > > > > > -                                                 kdev->power.suspended_jiffies;
> > > > > > +                               i915->pmu.suspended_time_last =
> > > > > > +                                       kdev->power.suspended_time;
> > > > > >
> > > > >
> > > > > Huh, so patch 2 introduces a complier error because of removing the
> > > > > old fields. We can't have that.
> > > >
> > > > I agree
> > > > The patch was mainly to raise discussion
> > >
> > > OK, so patch [1/3] from this series should be applicable regardless, right?
> >
> > Yes
>
> OK, I'll queue it up, then.

Thanks

>
> Next time you do something like that  please mark patches for
> discussion in a series as [RFC] so it is all clear.

ok. will do for the next version of the last 2 patches
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..cf6437d 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -493,14 +493,14 @@  static u64 get_rc6(struct drm_i915_private *i915)
 		 */
 		if (kdev->power.runtime_status == RPM_SUSPENDED) {
 			if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
-				i915->pmu.suspended_jiffies_last =
-						  kdev->power.suspended_jiffies;
+				i915->pmu.suspended_time_last =
+					kdev->power.suspended_time;
 
-			val = kdev->power.suspended_jiffies -
-			      i915->pmu.suspended_jiffies_last;
-			val += jiffies - kdev->power.accounting_timestamp;
+			val = kdev->power.suspended_time -
+				i915->pmu.suspended_time_last;
+			val += ktime_to_ns(ktime_get()) -
+				kdev->power.accounting_timestamp;
 
-			val = jiffies_to_nsecs(val);
 			val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
 
 			i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 7f164ca..3dc2a30 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -95,9 +95,9 @@  struct i915_pmu {
 	 */
 	struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
 	/**
-	 * @suspended_jiffies_last: Cached suspend time from PM core.
+	 * @suspended_time_last: Cached suspend time from PM core.
 	 */
-	unsigned long suspended_jiffies_last;
+	u64 suspended_time_last;
 	/**
 	 * @i915_attr: Memory block holding device attributes.
 	 */