| 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 |
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
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
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
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?
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
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.
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 --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. */
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(-)