[PATCH v5 0/3] Move pm_runtime accounted time to raw nsec

[PATCH v5 0/3] Move pm_runtime accounted time to raw nsec

[Vincent Guittot]

Move pm_runtime accounted time to raw nsec. The subject of the patchset
has changed as the 1st patch of the previous version has been queued by
Rafael.

Patch 1 adds a new pm_runtime interface to get accounted suspended time

Patch 2 moves drm/i915 driver on the new interface and removes access to
internal fields.

Patch 3 moves time accounting on raw ns. This patch initially used
ktime instead of raw ns but it was easier to move i915 driver on raw ns
than on ktime.

Changes since v4:
-Update commit message

Changes since v3:
- Rebase on v4.20-rc7 without patch that has been queued by Rafael
- Simplify the new interface pm_runtime_suspended_time()

Changes since v2:
- remove patch1 that has been queued by rafael
- add new interface in pm_runtime to get accounted time
- reorder patchset to prevent compilation error

Changes since v1:
- updated commit message of patch 1
- Added patches 2 & 3 to move runtime_pm accounting on raw ns
  
Thara Gopinath (1):
  PM/runtime:Replace jiffies based accounting with ktime based
    accounting

Vincent Guittot (2):
  PM/runtime: Add a new interface to get accounted time
  drm/i915: Move on the new pm runtime interface

 drivers/base/power/runtime.c    | 27 ++++++++++++++++++++++-----
 drivers/base/power/sysfs.c      | 11 ++++++++---
 drivers/gpu/drm/i915/i915_pmu.c | 16 ++++++----------
 drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
 include/linux/pm.h              |  6 +++---
 include/linux/pm_runtime.h      |  2 ++
 6 files changed, 43 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH v5 0/3] Move pm_runtime accounted time to raw nsec

[Vincent Guittot]

Move pm_runtime accounted time to raw nsec. The subject of the patchset
has changed as the 1st patch of the previous version has been queued by
Rafael.

Patch 1 adds a new pm_runtime interface to get accounted suspended time

Patch 2 moves drm/i915 driver on the new interface and removes access to
internal fields.

Patch 3 moves time accounting on raw ns. This patch initially used
ktime instead of raw ns but it was easier to move i915 driver on raw ns
than on ktime.

Changes since v4:
-Update commit message

Changes since v3:
- Rebase on v4.20-rc7 without patch that has been queued by Rafael
- Simplify the new interface pm_runtime_suspended_time()

Changes since v2:
- remove patch1 that has been queued by rafael
- add new interface in pm_runtime to get accounted time
- reorder patchset to prevent compilation error

Changes since v1:
- updated commit message of patch 1
- Added patches 2 & 3 to move runtime_pm accounting on raw ns
  
Thara Gopinath (1):
  PM/runtime:Replace jiffies based accounting with ktime based
    accounting

Vincent Guittot (2):
  PM/runtime: Add a new interface to get accounted time
  drm/i915: Move on the new pm runtime interface

 drivers/base/power/runtime.c    | 27 ++++++++++++++++++++++-----
 drivers/base/power/sysfs.c      | 11 ++++++++---
 drivers/gpu/drm/i915/i915_pmu.c | 16 ++++++----------
 drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
 include/linux/pm.h              |  6 +++---
 include/linux/pm_runtime.h      |  2 ++
 6 files changed, 43 insertions(+), 23 deletions(-)

-- 
2.7.4

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

[PATCH v5 1/3] PM/runtime: Add a new interface to get accounted time

[Vincent Guittot]

Some drivers (like i915/drm) needs to get the accounted suspended time.
pm_runtime_suspended_time() will return the suspended accounted time
in ns unit.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/base/power/runtime.c | 16 ++++++++++++++++
 include/linux/pm_runtime.h   |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index beb85c3..e695544 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -86,6 +86,22 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
 	dev->power.runtime_status = status;
 }
 
+u64 pm_runtime_suspended_time(struct device *dev)
+{
+	unsigned long flags, time;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+
+	update_pm_runtime_accounting(dev);
+
+	time = dev->power.suspended_jiffies;
+
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+
+	return jiffies_to_nsecs(time);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
+
 /**
  * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
  * @dev: Device to handle.
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index f0fc470..d479707 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
 	return dev->power.irq_safe;
 }
 
+extern u64 pm_runtime_suspended_time(struct device *dev);
+
 #else /* !CONFIG_PM */
 
 static inline bool queue_pm_work(struct work_struct *work) { return false; }
-- 
2.7.4

[PATCH v5 1/3] PM/runtime: Add a new interface to get accounted time

[Vincent Guittot]

Some drivers (like i915/drm) needs to get the accounted suspended time.
pm_runtime_suspended_time() will return the suspended accounted time
in ns unit.

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/base/power/runtime.c | 16 ++++++++++++++++
 include/linux/pm_runtime.h   |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index beb85c3..e695544 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -86,6 +86,22 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
 	dev->power.runtime_status = status;
 }
 
+u64 pm_runtime_suspended_time(struct device *dev)
+{
+	unsigned long flags, time;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+
+	update_pm_runtime_accounting(dev);
+
+	time = dev->power.suspended_jiffies;
+
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+
+	return jiffies_to_nsecs(time);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
+
 /**
  * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
  * @dev: Device to handle.
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index f0fc470..d479707 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -113,6 +113,8 @@ static inline bool pm_runtime_is_irq_safe(struct device *dev)
 	return dev->power.irq_safe;
 }
 
+extern u64 pm_runtime_suspended_time(struct device *dev);
+
 #else /* !CONFIG_PM */
 
 static inline bool queue_pm_work(struct work_struct *work) { return false; }
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Vincent Guittot]

Use the new pm runtime interface to get the accounted suspended time:
pm_runtime_suspended_time().
This new interface helps to simplify and cleanup the code that computes
__I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
PM runtime.

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

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..3f76f60 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/irq.h>
+#include <linux/pm_runtime.h>
 #include "i915_pmu.h"
 #include "intel_ringbuffer.h"
 #include "i915_drv.h"
@@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
 		 * counter value.
 		 */
 		spin_lock_irqsave(&i915->pmu.lock, flags);
-		spin_lock(&kdev->power.lock);
 
 		/*
 		 * After the above branch intel_runtime_pm_get_if_in_use failed
@@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
 		 * suspended and if not we cannot do better than report the last
 		 * known RC6 value.
 		 */
-		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;
+		if (pm_runtime_status_suspended(kdev)) {
+			val = pm_runtime_suspended_time(kdev);
 
-			val = kdev->power.suspended_jiffies -
-			      i915->pmu.suspended_jiffies_last;
-			val += jiffies - kdev->power.accounting_timestamp;
+			if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
+				i915->pmu.suspended_time_last = val;
 
-			val = jiffies_to_nsecs(val);
+			val -= i915->pmu.suspended_time_last;
 			val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
 
 			i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
@@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
 			val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
 		}
 
-		spin_unlock(&kdev->power.lock);
 		spin_unlock_irqrestore(&i915->pmu.lock, flags);
 	}
 
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

[PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Vincent Guittot]

Use the new pm runtime interface to get the accounted suspended time:
pm_runtime_suspended_time().
This new interface helps to simplify and cleanup the code that computes
__I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
PM runtime.

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

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index d6c8f8f..3f76f60 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/irq.h>
+#include <linux/pm_runtime.h>
 #include "i915_pmu.h"
 #include "intel_ringbuffer.h"
 #include "i915_drv.h"
@@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
 		 * counter value.
 		 */
 		spin_lock_irqsave(&i915->pmu.lock, flags);
-		spin_lock(&kdev->power.lock);
 
 		/*
 		 * After the above branch intel_runtime_pm_get_if_in_use failed
@@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
 		 * suspended and if not we cannot do better than report the last
 		 * known RC6 value.
 		 */
-		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;
+		if (pm_runtime_status_suspended(kdev)) {
+			val = pm_runtime_suspended_time(kdev);
 
-			val = kdev->power.suspended_jiffies -
-			      i915->pmu.suspended_jiffies_last;
-			val += jiffies - kdev->power.accounting_timestamp;
+			if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
+				i915->pmu.suspended_time_last = val;
 
-			val = jiffies_to_nsecs(val);
+			val -= i915->pmu.suspended_time_last;
 			val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
 
 			i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
@@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
 			val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
 		}
 
-		spin_unlock(&kdev->power.lock);
 		spin_unlock_irqrestore(&i915->pmu.lock, flags);
 	}
 
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

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

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Tvrtko Ursulin]

Hi,

On 21/12/2018 10:33, Vincent Guittot wrote:
> Use the new pm runtime interface to get the accounted suspended time:
> pm_runtime_suspended_time().
> This new interface helps to simplify and cleanup the code that computes
> __I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
> PM runtime.
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>   drivers/gpu/drm/i915/i915_pmu.c | 16 ++++++----------
>   drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
>   2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index d6c8f8f..3f76f60 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -5,6 +5,7 @@
>    */
>   
>   #include <linux/irq.h>
> +#include <linux/pm_runtime.h>
>   #include "i915_pmu.h"
>   #include "intel_ringbuffer.h"
>   #include "i915_drv.h"
> @@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
>   		 * counter value.
>   		 */
>   		spin_lock_irqsave(&i915->pmu.lock, flags);
> -		spin_lock(&kdev->power.lock);
>   
>   		/*
>   		 * After the above branch intel_runtime_pm_get_if_in_use failed
> @@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
>   		 * suspended and if not we cannot do better than report the last
>   		 * known RC6 value.
>   		 */
> -		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;
> +		if (pm_runtime_status_suspended(kdev)) {
> +			val = pm_runtime_suspended_time(kdev);

There is a race condition between the status check and timestamp access 
which the existing code solves by holding the power.lock over it. But I 
don't exactly remember how this issue was manifesting. Is 
kdev->power.suspended_jiffies perhaps reset on exit from runtime 
suspend, which was then underflowing the val, not sure.

Anyways, is the new way of doing this safe with regards to this race? In 
other words is the value pm_runtime_suspended_time always monotonic, 
even when not suspended? If not we have to handle the race somehow.

If it is always monotonic, then worst case we report one wrong sample, 
which I guess is still not ideal since someone could be querying the PMU 
with quite low frequency.

There are tests which probably can hit this, but to run them 
automatically your patches would need to be rebased on drm-tip and maybe 
sent to our trybot. I can do that after the holiday break if you are 
okay with having the series waiting until then.

Regards,

Tvrtko

>   
> -			val = kdev->power.suspended_jiffies -
> -			      i915->pmu.suspended_jiffies_last;
> -			val += jiffies - kdev->power.accounting_timestamp;
> +			if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> +				i915->pmu.suspended_time_last = val;
>   
> -			val = jiffies_to_nsecs(val);
> +			val -= i915->pmu.suspended_time_last;
>   			val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
>   
>   			i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> @@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
>   			val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
>   		}
>   
> -		spin_unlock(&kdev->power.lock);
>   		spin_unlock_irqrestore(&i915->pmu.lock, flags);
>   	}
>   
> 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.
>   	 */
> 

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Vincent Guittot]

On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> Hi,
>
> On 21/12/2018 10:33, Vincent Guittot wrote:
> > Use the new pm runtime interface to get the accounted suspended time:
> > pm_runtime_suspended_time().
> > This new interface helps to simplify and cleanup the code that computes
> > __I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
> > PM runtime.
> >
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 16 ++++++----------
> >   drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> >   2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index d6c8f8f..3f76f60 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -5,6 +5,7 @@
> >    */
> >
> >   #include <linux/irq.h>
> > +#include <linux/pm_runtime.h>
> >   #include "i915_pmu.h"
> >   #include "intel_ringbuffer.h"
> >   #include "i915_drv.h"
> > @@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >                * counter value.
> >                */
> >               spin_lock_irqsave(&i915->pmu.lock, flags);
> > -             spin_lock(&kdev->power.lock);
> >
> >               /*
> >                * After the above branch intel_runtime_pm_get_if_in_use failed
> > @@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >                * suspended and if not we cannot do better than report the last
> >                * known RC6 value.
> >                */
> > -             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;
> > +             if (pm_runtime_status_suspended(kdev)) {
> > +                     val = pm_runtime_suspended_time(kdev);
>
> There is a race condition between the status check and timestamp access
> which the existing code solves by holding the power.lock over it. But I
> don't exactly remember how this issue was manifesting. Is
> kdev->power.suspended_jiffies perhaps reset on exit from runtime
> suspend, which was then underflowing the val, not sure.
>
> Anyways, is the new way of doing this safe with regards to this race? In

AFAICT it is safe.
The current version does:
1-take lock,
2-test if dev is suspended
3-read some internals field to computed an up-to-date suspended time
4-update __I915_SAMPLE_RC6_ESTIMATED
5-release lock

The new version does:
1-test if dev is suspended
2-get an up-to-date  suspended time with pm_runtime_suspended_time.
This is atomic and monotonic
3-update __I915_SAMPLE_RC6_ESTIMATED

A change from suspended to another states that happens just before
step 1 is ok for both as we will run the else if
No change of the state can happen after step 1 in current code and the
estimated suspended time will be the time up to step2. In parallel,
Any state change will have to wait step5 to continue
If a change from suspended to another state happens after step 1 in
new code, the suspended time return by PM core will be the time up to
this change. So I would say you don't delay state transition and you
get a more accurate estimated suspended time (even if the difference
should be small).
If a change from suspended to another state happens after step 2 in
new code, the suspended time return by PM core will be the time up to
step 2 so there is no changes


> other words is the value pm_runtime_suspended_time always monotonic,
> even when not suspended? If not we have to handle the race somehow.

Yes pm_runtime_suspended_time is monotonic and stays unchanged when
not suspended

>
> If it is always monotonic, then worst case we report one wrong sample,
> which I guess is still not ideal since someone could be querying the PMU
> with quite low frequency.
>
> There are tests which probably can hit this, but to run them
> automatically your patches would need to be rebased on drm-tip and maybe
> sent to our trybot. I can do that after the holiday break if you are
> okay with having the series waiting until then.

yes looks good to me

Thanks,
Vincent

>
> Regards,
>
> Tvrtko
>
> >
> > -                     val = kdev->power.suspended_jiffies -
> > -                           i915->pmu.suspended_jiffies_last;
> > -                     val += jiffies - kdev->power.accounting_timestamp;
> > +                     if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > +                             i915->pmu.suspended_time_last = val;
> >
> > -                     val = jiffies_to_nsecs(val);
> > +                     val -= i915->pmu.suspended_time_last;
> >                       val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >
> >                       i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> > @@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >                       val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >               }
> >
> > -             spin_unlock(&kdev->power.lock);
> >               spin_unlock_irqrestore(&i915->pmu.lock, flags);
> >       }
> >
> > 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.
> >        */
> >

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Vincent Guittot]

On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> Hi,
>
> On 21/12/2018 10:33, Vincent Guittot wrote:
> > Use the new pm runtime interface to get the accounted suspended time:
> > pm_runtime_suspended_time().
> > This new interface helps to simplify and cleanup the code that computes
> > __I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
> > PM runtime.
> >
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 16 ++++++----------
> >   drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
> >   2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index d6c8f8f..3f76f60 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -5,6 +5,7 @@
> >    */
> >
> >   #include <linux/irq.h>
> > +#include <linux/pm_runtime.h>
> >   #include "i915_pmu.h"
> >   #include "intel_ringbuffer.h"
> >   #include "i915_drv.h"
> > @@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >                * counter value.
> >                */
> >               spin_lock_irqsave(&i915->pmu.lock, flags);
> > -             spin_lock(&kdev->power.lock);
> >
> >               /*
> >                * After the above branch intel_runtime_pm_get_if_in_use failed
> > @@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >                * suspended and if not we cannot do better than report the last
> >                * known RC6 value.
> >                */
> > -             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;
> > +             if (pm_runtime_status_suspended(kdev)) {
> > +                     val = pm_runtime_suspended_time(kdev);
>
> There is a race condition between the status check and timestamp access
> which the existing code solves by holding the power.lock over it. But I
> don't exactly remember how this issue was manifesting. Is
> kdev->power.suspended_jiffies perhaps reset on exit from runtime
> suspend, which was then underflowing the val, not sure.
>
> Anyways, is the new way of doing this safe with regards to this race? In

AFAICT it is safe.
The current version does:
1-take lock,
2-test if dev is suspended
3-read some internals field to computed an up-to-date suspended time
4-update __I915_SAMPLE_RC6_ESTIMATED
5-release lock

The new version does:
1-test if dev is suspended
2-get an up-to-date  suspended time with pm_runtime_suspended_time.
This is atomic and monotonic
3-update __I915_SAMPLE_RC6_ESTIMATED

A change from suspended to another states that happens just before
step 1 is ok for both as we will run the else if
No change of the state can happen after step 1 in current code and the
estimated suspended time will be the time up to step2. In parallel,
Any state change will have to wait step5 to continue
If a change from suspended to another state happens after step 1 in
new code, the suspended time return by PM core will be the time up to
this change. So I would say you don't delay state transition and you
get a more accurate estimated suspended time (even if the difference
should be small).
If a change from suspended to another state happens after step 2 in
new code, the suspended time return by PM core will be the time up to
step 2 so there is no changes


> other words is the value pm_runtime_suspended_time always monotonic,
> even when not suspended? If not we have to handle the race somehow.

Yes pm_runtime_suspended_time is monotonic and stays unchanged when
not suspended

>
> If it is always monotonic, then worst case we report one wrong sample,
> which I guess is still not ideal since someone could be querying the PMU
> with quite low frequency.
>
> There are tests which probably can hit this, but to run them
> automatically your patches would need to be rebased on drm-tip and maybe
> sent to our trybot. I can do that after the holiday break if you are
> okay with having the series waiting until then.

yes looks good to me

Thanks,
Vincent

>
> Regards,
>
> Tvrtko
>
> >
> > -                     val = kdev->power.suspended_jiffies -
> > -                           i915->pmu.suspended_jiffies_last;
> > -                     val += jiffies - kdev->power.accounting_timestamp;
> > +                     if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > +                             i915->pmu.suspended_time_last = val;
> >
> > -                     val = jiffies_to_nsecs(val);
> > +                     val -= i915->pmu.suspended_time_last;
> >                       val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >
> >                       i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> > @@ -510,7 +507,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
> >                       val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >               }
> >
> > -             spin_unlock(&kdev->power.lock);
> >               spin_unlock_irqrestore(&i915->pmu.lock, flags);
> >       }
> >
> > 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.
> >        */
> >

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Tvrtko Ursulin]

On 21/12/2018 13:26, Vincent Guittot wrote:
> On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> Hi,
>>
>> On 21/12/2018 10:33, Vincent Guittot wrote:
>>> Use the new pm runtime interface to get the accounted suspended time:
>>> pm_runtime_suspended_time().
>>> This new interface helps to simplify and cleanup the code that computes
>>> __I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
>>> PM runtime.
>>>
>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>> ---
>>>    drivers/gpu/drm/i915/i915_pmu.c | 16 ++++++----------
>>>    drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
>>>    2 files changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index d6c8f8f..3f76f60 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -5,6 +5,7 @@
>>>     */
>>>
>>>    #include <linux/irq.h>
>>> +#include <linux/pm_runtime.h>
>>>    #include "i915_pmu.h"
>>>    #include "intel_ringbuffer.h"
>>>    #include "i915_drv.h"
>>> @@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
>>>                 * counter value.
>>>                 */
>>>                spin_lock_irqsave(&i915->pmu.lock, flags);
>>> -             spin_lock(&kdev->power.lock);
>>>
>>>                /*
>>>                 * After the above branch intel_runtime_pm_get_if_in_use failed
>>> @@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
>>>                 * suspended and if not we cannot do better than report the last
>>>                 * known RC6 value.
>>>                 */
>>> -             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;
>>> +             if (pm_runtime_status_suspended(kdev)) {
>>> +                     val = pm_runtime_suspended_time(kdev);
>>
>> There is a race condition between the status check and timestamp access
>> which the existing code solves by holding the power.lock over it. But I
>> don't exactly remember how this issue was manifesting. Is
>> kdev->power.suspended_jiffies perhaps reset on exit from runtime
>> suspend, which was then underflowing the val, not sure.
>>
>> Anyways, is the new way of doing this safe with regards to this race? In
> 
> AFAICT it is safe.
> The current version does:
> 1-take lock,
> 2-test if dev is suspended
> 3-read some internals field to computed an up-to-date suspended time
> 4-update __I915_SAMPLE_RC6_ESTIMATED
> 5-release lock
> 
> The new version does:
> 1-test if dev is suspended
> 2-get an up-to-date  suspended time with pm_runtime_suspended_time.
> This is atomic and monotonic
> 3-update __I915_SAMPLE_RC6_ESTIMATED
> 
> A change from suspended to another states that happens just before
> step 1 is ok for both as we will run the else if
> No change of the state can happen after step 1 in current code and the
> estimated suspended time will be the time up to step2. In parallel,
> Any state change will have to wait step5 to continue
> If a change from suspended to another state happens after step 1 in
> new code, the suspended time return by PM core will be the time up to
> this change. So I would say you don't delay state transition and you
> get a more accurate estimated suspended time (even if the difference
> should be small).
> If a change from suspended to another state happens after step 2 in
> new code, the suspended time return by PM core will be the time up to
> step 2 so there is no changes
> 
> 
>> other words is the value pm_runtime_suspended_time always monotonic,
>> even when not suspended? If not we have to handle the race somehow.
> 
> Yes pm_runtime_suspended_time is monotonic and stays unchanged when
> not suspended
> 
>>
>> If it is always monotonic, then worst case we report one wrong sample,
>> which I guess is still not ideal since someone could be querying the PMU
>> with quite low frequency.
>>
>> There are tests which probably can hit this, but to run them
>> automatically your patches would need to be rebased on drm-tip and maybe
>> sent to our trybot. I can do that after the holiday break if you are
>> okay with having the series waiting until then.
> 
> yes looks good to me

Looks good to me as well. And our CI agrees with it. So:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I assume you will take the patch through some power related tree and we 
will eventually pull it back to drm-tip.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Tvrtko Ursulin]

On 21/12/2018 13:26, Vincent Guittot wrote:
> On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> Hi,
>>
>> On 21/12/2018 10:33, Vincent Guittot wrote:
>>> Use the new pm runtime interface to get the accounted suspended time:
>>> pm_runtime_suspended_time().
>>> This new interface helps to simplify and cleanup the code that computes
>>> __I915_SAMPLE_RC6_ESTIMATED and to remove direct access to internals of
>>> PM runtime.
>>>
>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>> ---
>>>    drivers/gpu/drm/i915/i915_pmu.c | 16 ++++++----------
>>>    drivers/gpu/drm/i915/i915_pmu.h |  4 ++--
>>>    2 files changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index d6c8f8f..3f76f60 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -5,6 +5,7 @@
>>>     */
>>>
>>>    #include <linux/irq.h>
>>> +#include <linux/pm_runtime.h>
>>>    #include "i915_pmu.h"
>>>    #include "intel_ringbuffer.h"
>>>    #include "i915_drv.h"
>>> @@ -478,7 +479,6 @@ static u64 get_rc6(struct drm_i915_private *i915)
>>>                 * counter value.
>>>                 */
>>>                spin_lock_irqsave(&i915->pmu.lock, flags);
>>> -             spin_lock(&kdev->power.lock);
>>>
>>>                /*
>>>                 * After the above branch intel_runtime_pm_get_if_in_use failed
>>> @@ -491,16 +491,13 @@ static u64 get_rc6(struct drm_i915_private *i915)
>>>                 * suspended and if not we cannot do better than report the last
>>>                 * known RC6 value.
>>>                 */
>>> -             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;
>>> +             if (pm_runtime_status_suspended(kdev)) {
>>> +                     val = pm_runtime_suspended_time(kdev);
>>
>> There is a race condition between the status check and timestamp access
>> which the existing code solves by holding the power.lock over it. But I
>> don't exactly remember how this issue was manifesting. Is
>> kdev->power.suspended_jiffies perhaps reset on exit from runtime
>> suspend, which was then underflowing the val, not sure.
>>
>> Anyways, is the new way of doing this safe with regards to this race? In
> 
> AFAICT it is safe.
> The current version does:
> 1-take lock,
> 2-test if dev is suspended
> 3-read some internals field to computed an up-to-date suspended time
> 4-update __I915_SAMPLE_RC6_ESTIMATED
> 5-release lock
> 
> The new version does:
> 1-test if dev is suspended
> 2-get an up-to-date  suspended time with pm_runtime_suspended_time.
> This is atomic and monotonic
> 3-update __I915_SAMPLE_RC6_ESTIMATED
> 
> A change from suspended to another states that happens just before
> step 1 is ok for both as we will run the else if
> No change of the state can happen after step 1 in current code and the
> estimated suspended time will be the time up to step2. In parallel,
> Any state change will have to wait step5 to continue
> If a change from suspended to another state happens after step 1 in
> new code, the suspended time return by PM core will be the time up to
> this change. So I would say you don't delay state transition and you
> get a more accurate estimated suspended time (even if the difference
> should be small).
> If a change from suspended to another state happens after step 2 in
> new code, the suspended time return by PM core will be the time up to
> step 2 so there is no changes
> 
> 
>> other words is the value pm_runtime_suspended_time always monotonic,
>> even when not suspended? If not we have to handle the race somehow.
> 
> Yes pm_runtime_suspended_time is monotonic and stays unchanged when
> not suspended
> 
>>
>> If it is always monotonic, then worst case we report one wrong sample,
>> which I guess is still not ideal since someone could be querying the PMU
>> with quite low frequency.
>>
>> There are tests which probably can hit this, but to run them
>> automatically your patches would need to be rebased on drm-tip and maybe
>> sent to our trybot. I can do that after the holiday break if you are
>> okay with having the series waiting until then.
> 
> yes looks good to me

Looks good to me as well. And our CI agrees with it. So:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I assume you will take the patch through some power related tree and we 
will eventually pull it back to drm-tip.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Vincent Guittot]

Hi Tvrtko,

On Mon, 31 Dec 2018 at 13:32, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 21/12/2018 13:26, Vincent Guittot wrote:
> > On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin

[snip]

> >>
> >> If it is always monotonic, then worst case we report one wrong sample,
> >> which I guess is still not ideal since someone could be querying the PMU
> >> with quite low frequency.
> >>
> >> There are tests which probably can hit this, but to run them
> >> automatically your patches would need to be rebased on drm-tip and maybe
> >> sent to our trybot. I can do that after the holiday break if you are
> >> okay with having the series waiting until then.
> >
> > yes looks good to me
>
> Looks good to me as well. And our CI agrees with it. So:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for the review and the test

>
> I assume you will take the patch through some power related tree and we
> will eventually pull it back to drm-tip.

Rafael,
How do you want to proceed with this patch and the 2 others of the serie ?

Regards,
Vincent

>
> Regards,
>
> Tvrtko

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Vincent Guittot]

Hi Tvrtko,

On Mon, 31 Dec 2018 at 13:32, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 21/12/2018 13:26, Vincent Guittot wrote:
> > On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin

[snip]

> >>
> >> If it is always monotonic, then worst case we report one wrong sample,
> >> which I guess is still not ideal since someone could be querying the PMU
> >> with quite low frequency.
> >>
> >> There are tests which probably can hit this, but to run them
> >> automatically your patches would need to be rebased on drm-tip and maybe
> >> sent to our trybot. I can do that after the holiday break if you are
> >> okay with having the series waiting until then.
> >
> > yes looks good to me
>
> Looks good to me as well. And our CI agrees with it. So:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for the review and the test

>
> I assume you will take the patch through some power related tree and we
> will eventually pull it back to drm-tip.

Rafael,
How do you want to proceed with this patch and the 2 others of the serie ?

Regards,
Vincent

>
> Regards,
>
> Tvrtko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Rafael J. Wysocki]

On Mon, Jan 7, 2019 at 3:04 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Hi Tvrtko,
>
> On Mon, 31 Dec 2018 at 13:32, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
> >
> >
> > On 21/12/2018 13:26, Vincent Guittot wrote:
> > > On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
>
> [snip]
>
> > >>
> > >> If it is always monotonic, then worst case we report one wrong sample,
> > >> which I guess is still not ideal since someone could be querying the PMU
> > >> with quite low frequency.
> > >>
> > >> There are tests which probably can hit this, but to run them
> > >> automatically your patches would need to be rebased on drm-tip and maybe
> > >> sent to our trybot. I can do that after the holiday break if you are
> > >> okay with having the series waiting until then.
> > >
> > > yes looks good to me
> >
> > Looks good to me as well. And our CI agrees with it. So:
> >
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Thanks for the review and the test
>
> >
> > I assume you will take the patch through some power related tree and we
> > will eventually pull it back to drm-tip.
>
> Rafael,
> How do you want to proceed with this patch and the 2 others of the serie ?

I'm going to queue up the whole series for 5.1.

Thanks!

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Rafael J. Wysocki]

On Mon, Jan 7, 2019 at 3:04 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Hi Tvrtko,
>
> On Mon, 31 Dec 2018 at 13:32, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
> >
> >
> > On 21/12/2018 13:26, Vincent Guittot wrote:
> > > On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
>
> [snip]
>
> > >>
> > >> If it is always monotonic, then worst case we report one wrong sample,
> > >> which I guess is still not ideal since someone could be querying the PMU
> > >> with quite low frequency.
> > >>
> > >> There are tests which probably can hit this, but to run them
> > >> automatically your patches would need to be rebased on drm-tip and maybe
> > >> sent to our trybot. I can do that after the holiday break if you are
> > >> okay with having the series waiting until then.
> > >
> > > yes looks good to me
> >
> > Looks good to me as well. And our CI agrees with it. So:
> >
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Thanks for the review and the test
>
> >
> > I assume you will take the patch through some power related tree and we
> > will eventually pull it back to drm-tip.
>
> Rafael,
> How do you want to proceed with this patch and the 2 others of the serie ?

I'm going to queue up the whole series for 5.1.

Thanks!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Rafael J. Wysocki]

On Monday, January 7, 2019 3:21:49 PM CET Rafael J. Wysocki wrote:
> On Mon, Jan 7, 2019 at 3:04 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Hi Tvrtko,
> >
> > On Mon, 31 Dec 2018 at 13:32, Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> > >
> > >
> > > On 21/12/2018 13:26, Vincent Guittot wrote:
> > > > On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
> >
> > [snip]
> >
> > > >>
> > > >> If it is always monotonic, then worst case we report one wrong sample,
> > > >> which I guess is still not ideal since someone could be querying the PMU
> > > >> with quite low frequency.
> > > >>
> > > >> There are tests which probably can hit this, but to run them
> > > >> automatically your patches would need to be rebased on drm-tip and maybe
> > > >> sent to our trybot. I can do that after the holiday break if you are
> > > >> okay with having the series waiting until then.
> > > >
> > > > yes looks good to me
> > >
> > > Looks good to me as well. And our CI agrees with it. So:
> > >
> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > Thanks for the review and the test
> >
> > >
> > > I assume you will take the patch through some power related tree and we
> > > will eventually pull it back to drm-tip.
> >
> > Rafael,
> > How do you want to proceed with this patch and the 2 others of the serie ?
> 
> I'm going to queue up the whole series for 5.1.

And it has been queued up now, thanks!

Re: [Intel-gfx] [PATCH v5 2/3] drm/i915: Move on the new pm runtime interface

[Rafael J. Wysocki]

On Monday, January 7, 2019 3:21:49 PM CET Rafael J. Wysocki wrote:
> On Mon, Jan 7, 2019 at 3:04 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Hi Tvrtko,
> >
> > On Mon, 31 Dec 2018 at 13:32, Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> > >
> > >
> > > On 21/12/2018 13:26, Vincent Guittot wrote:
> > > > On Fri, 21 Dec 2018 at 12:33, Tvrtko Ursulin
> >
> > [snip]
> >
> > > >>
> > > >> If it is always monotonic, then worst case we report one wrong sample,
> > > >> which I guess is still not ideal since someone could be querying the PMU
> > > >> with quite low frequency.
> > > >>
> > > >> There are tests which probably can hit this, but to run them
> > > >> automatically your patches would need to be rebased on drm-tip and maybe
> > > >> sent to our trybot. I can do that after the holiday break if you are
> > > >> okay with having the series waiting until then.
> > > >
> > > > yes looks good to me
> > >
> > > Looks good to me as well. And our CI agrees with it. So:
> > >
> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > Thanks for the review and the test
> >
> > >
> > > I assume you will take the patch through some power related tree and we
> > > will eventually pull it back to drm-tip.
> >
> > Rafael,
> > How do you want to proceed with this patch and the 2 others of the serie ?
> 
> I'm going to queue up the whole series for 5.1.

And it has been queued up now, thanks!

[PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

From: Thara Gopinath <thara.gopinath@linaro.org>

This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.

timekeeping is initialized before pm_runtime_init() so ktime_get() will
be ready before first call. In fact, timekeeping_init() is called early
in start_kernel() which is way before driver_init() (and that's when
devices can start to be initialized) called from rest_init() via
kernel_init_freeable() and do_basic_setup().

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
[move from ktime to raw nsec]
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/base/power/runtime.c | 17 +++++++++--------
 drivers/base/power/sysfs.c   | 11 ++++++++---
 include/linux/pm.h           |  6 +++---
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index e695544..f700524 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
  */
 void update_pm_runtime_accounting(struct device *dev)
 {
-	unsigned long now = jiffies;
-	unsigned long delta;
+	u64 now = ktime_to_ns(ktime_get());
+	u64 delta;
 
 	delta = now - dev->power.accounting_timestamp;
 
@@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
 		return;
 
 	if (dev->power.runtime_status == RPM_SUSPENDED)
-		dev->power.suspended_jiffies += delta;
+		dev->power.suspended_time += delta;
 	else
-		dev->power.active_jiffies += delta;
+		dev->power.active_time += delta;
 }
 
 static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
 
 u64 pm_runtime_suspended_time(struct device *dev)
 {
-	unsigned long flags, time;
+	u64 time;
+	unsigned long flags;
 
 	spin_lock_irqsave(&dev->power.lock, flags);
 
 	update_pm_runtime_accounting(dev);
 
-	time = dev->power.suspended_jiffies;
+	time = dev->power.suspended_time;
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
-	return jiffies_to_nsecs(time);
+	return time;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
 
@@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
 	dev->power.request_pending = false;
 	dev->power.request = RPM_REQ_NONE;
 	dev->power.deferred_resume = false;
-	dev->power.accounting_timestamp = jiffies;
+	dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
 	INIT_WORK(&dev->power.work, pm_runtime_work);
 
 	dev->power.timer_expires = 0;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int ret;
+	u64 tmp;
 	spin_lock_irq(&dev->power.lock);
 	update_pm_runtime_accounting(dev);
-	ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+	tmp = dev->power.active_time;
+	do_div(tmp, NSEC_PER_MSEC);
+	ret = sprintf(buf, "%llu\n", tmp);
 	spin_unlock_irq(&dev->power.lock);
 	return ret;
 }
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int ret;
+	u64 tmp;
 	spin_lock_irq(&dev->power.lock);
 	update_pm_runtime_accounting(dev);
-	ret = sprintf(buf, "%i\n",
-		jiffies_to_msecs(dev->power.suspended_jiffies));
+	tmp = dev->power.suspended_time;
+	do_div(tmp, NSEC_PER_MSEC);
+	ret = sprintf(buf, "%llu\n", tmp);
 	spin_unlock_irq(&dev->power.lock);
 	return ret;
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78..e5a34e2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -632,9 +632,9 @@ struct dev_pm_info {
 	int			runtime_error;
 	int			autosuspend_delay;
 	unsigned long		last_busy;
-	unsigned long		active_jiffies;
-	unsigned long		suspended_jiffies;
-	unsigned long		accounting_timestamp;
+	u64			active_time;
+	u64			suspended_time;
+	u64			accounting_timestamp;
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	void (*set_latency_tolerance)(struct device *, s32);
-- 
2.7.4

[PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

From: Thara Gopinath <thara.gopinath@linaro.org>

This patch replaces jiffies based accounting for runtime_active_time
and runtime_suspended_time with ktime base accounting. This makes the
runtime debug counters inline with genpd and other pm subsytems which
uses ktime based accounting.

timekeeping is initialized before pm_runtime_init() so ktime_get() will
be ready before first call. In fact, timekeeping_init() is called early
in start_kernel() which is way before driver_init() (and that's when
devices can start to be initialized) called from rest_init() via
kernel_init_freeable() and do_basic_setup().

Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
[move from ktime to raw nsec]
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 drivers/base/power/runtime.c | 17 +++++++++--------
 drivers/base/power/sysfs.c   | 11 ++++++++---
 include/linux/pm.h           |  6 +++---
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index e695544..f700524 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
  */
 void update_pm_runtime_accounting(struct device *dev)
 {
-	unsigned long now = jiffies;
-	unsigned long delta;
+	u64 now = ktime_to_ns(ktime_get());
+	u64 delta;
 
 	delta = now - dev->power.accounting_timestamp;
 
@@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
 		return;
 
 	if (dev->power.runtime_status == RPM_SUSPENDED)
-		dev->power.suspended_jiffies += delta;
+		dev->power.suspended_time += delta;
 	else
-		dev->power.active_jiffies += delta;
+		dev->power.active_time += delta;
 }
 
 static void __update_runtime_status(struct device *dev, enum rpm_status status)
@@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
 
 u64 pm_runtime_suspended_time(struct device *dev)
 {
-	unsigned long flags, time;
+	u64 time;
+	unsigned long flags;
 
 	spin_lock_irqsave(&dev->power.lock, flags);
 
 	update_pm_runtime_accounting(dev);
 
-	time = dev->power.suspended_jiffies;
+	time = dev->power.suspended_time;
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
-	return jiffies_to_nsecs(time);
+	return time;
 }
 EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
 
@@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
 	dev->power.request_pending = false;
 	dev->power.request = RPM_REQ_NONE;
 	dev->power.deferred_resume = false;
-	dev->power.accounting_timestamp = jiffies;
+	dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
 	INIT_WORK(&dev->power.work, pm_runtime_work);
 
 	dev->power.timer_expires = 0;
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738..96c8a22 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int ret;
+	u64 tmp;
 	spin_lock_irq(&dev->power.lock);
 	update_pm_runtime_accounting(dev);
-	ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
+	tmp = dev->power.active_time;
+	do_div(tmp, NSEC_PER_MSEC);
+	ret = sprintf(buf, "%llu\n", tmp);
 	spin_unlock_irq(&dev->power.lock);
 	return ret;
 }
@@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int ret;
+	u64 tmp;
 	spin_lock_irq(&dev->power.lock);
 	update_pm_runtime_accounting(dev);
-	ret = sprintf(buf, "%i\n",
-		jiffies_to_msecs(dev->power.suspended_jiffies));
+	tmp = dev->power.suspended_time;
+	do_div(tmp, NSEC_PER_MSEC);
+	ret = sprintf(buf, "%llu\n", tmp);
 	spin_unlock_irq(&dev->power.lock);
 	return ret;
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78..e5a34e2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -632,9 +632,9 @@ struct dev_pm_info {
 	int			runtime_error;
 	int			autosuspend_delay;
 	unsigned long		last_busy;
-	unsigned long		active_jiffies;
-	unsigned long		suspended_jiffies;
-	unsigned long		accounting_timestamp;
+	u64			active_time;
+	u64			suspended_time;
+	u64			accounting_timestamp;
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	void (*set_latency_tolerance)(struct device *, s32);
-- 
2.7.4

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

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Ulf Hansson]

On Fri, 21 Dec 2018 at 11:34, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> From: Thara Gopinath <thara.gopinath@linaro.org>
>
> This patch replaces jiffies based accounting for runtime_active_time
> and runtime_suspended_time with ktime base accounting. This makes the
> runtime debug counters inline with genpd and other pm subsytems which
> uses ktime based accounting.
>
> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> be ready before first call. In fact, timekeeping_init() is called early
> in start_kernel() which is way before driver_init() (and that's when
> devices can start to be initialized) called from rest_init() via
> kernel_init_freeable() and do_basic_setup().
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> [move from ktime to raw nsec]
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/runtime.c | 17 +++++++++--------
>  drivers/base/power/sysfs.c   | 11 ++++++++---
>  include/linux/pm.h           |  6 +++---
>  3 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index e695544..f700524 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
>   */
>  void update_pm_runtime_accounting(struct device *dev)
>  {
> -       unsigned long now = jiffies;
> -       unsigned long delta;
> +       u64 now = ktime_to_ns(ktime_get());
> +       u64 delta;
>
>         delta = now - dev->power.accounting_timestamp;
>
> @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
>                 return;
>
>         if (dev->power.runtime_status == RPM_SUSPENDED)
> -               dev->power.suspended_jiffies += delta;
> +               dev->power.suspended_time += delta;
>         else
> -               dev->power.active_jiffies += delta;
> +               dev->power.active_time += delta;
>  }
>
>  static void __update_runtime_status(struct device *dev, enum rpm_status status)
> @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
>
>  u64 pm_runtime_suspended_time(struct device *dev)
>  {
> -       unsigned long flags, time;
> +       u64 time;
> +       unsigned long flags;
>
>         spin_lock_irqsave(&dev->power.lock, flags);
>
>         update_pm_runtime_accounting(dev);
>
> -       time = dev->power.suspended_jiffies;
> +       time = dev->power.suspended_time;
>
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>
> -       return jiffies_to_nsecs(time);
> +       return time;
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
>
> @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
>         dev->power.request_pending = false;
>         dev->power.request = RPM_REQ_NONE;
>         dev->power.deferred_resume = false;
> -       dev->power.accounting_timestamp = jiffies;
> +       dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
>         INIT_WORK(&dev->power.work, pm_runtime_work);
>
>         dev->power.timer_expires = 0;
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index d713738..96c8a22 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
>                                 struct device_attribute *attr, char *buf)
>  {
>         int ret;
> +       u64 tmp;
>         spin_lock_irq(&dev->power.lock);
>         update_pm_runtime_accounting(dev);
> -       ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
> +       tmp = dev->power.active_time;
> +       do_div(tmp, NSEC_PER_MSEC);
> +       ret = sprintf(buf, "%llu\n", tmp);
>         spin_unlock_irq(&dev->power.lock);
>         return ret;
>  }
> @@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev,
>                                 struct device_attribute *attr, char *buf)
>  {
>         int ret;
> +       u64 tmp;
>         spin_lock_irq(&dev->power.lock);
>         update_pm_runtime_accounting(dev);
> -       ret = sprintf(buf, "%i\n",
> -               jiffies_to_msecs(dev->power.suspended_jiffies));
> +       tmp = dev->power.suspended_time;
> +       do_div(tmp, NSEC_PER_MSEC);
> +       ret = sprintf(buf, "%llu\n", tmp);
>         spin_unlock_irq(&dev->power.lock);
>         return ret;
>  }
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78..e5a34e2 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -632,9 +632,9 @@ struct dev_pm_info {
>         int                     runtime_error;
>         int                     autosuspend_delay;
>         unsigned long           last_busy;
> -       unsigned long           active_jiffies;
> -       unsigned long           suspended_jiffies;
> -       unsigned long           accounting_timestamp;
> +       u64                     active_time;
> +       u64                     suspended_time;
> +       u64                     accounting_timestamp;
>  #endif
>         struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */
>         void (*set_latency_tolerance)(struct device *, s32);
> --
> 2.7.4
>

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Ulf Hansson]

On Fri, 21 Dec 2018 at 11:34, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> From: Thara Gopinath <thara.gopinath@linaro.org>
>
> This patch replaces jiffies based accounting for runtime_active_time
> and runtime_suspended_time with ktime base accounting. This makes the
> runtime debug counters inline with genpd and other pm subsytems which
> uses ktime based accounting.
>
> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> be ready before first call. In fact, timekeeping_init() is called early
> in start_kernel() which is way before driver_init() (and that's when
> devices can start to be initialized) called from rest_init() via
> kernel_init_freeable() and do_basic_setup().
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> [move from ktime to raw nsec]
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/runtime.c | 17 +++++++++--------
>  drivers/base/power/sysfs.c   | 11 ++++++++---
>  include/linux/pm.h           |  6 +++---
>  3 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index e695544..f700524 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags);
>   */
>  void update_pm_runtime_accounting(struct device *dev)
>  {
> -       unsigned long now = jiffies;
> -       unsigned long delta;
> +       u64 now = ktime_to_ns(ktime_get());
> +       u64 delta;
>
>         delta = now - dev->power.accounting_timestamp;
>
> @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev)
>                 return;
>
>         if (dev->power.runtime_status == RPM_SUSPENDED)
> -               dev->power.suspended_jiffies += delta;
> +               dev->power.suspended_time += delta;
>         else
> -               dev->power.active_jiffies += delta;
> +               dev->power.active_time += delta;
>  }
>
>  static void __update_runtime_status(struct device *dev, enum rpm_status status)
> @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
>
>  u64 pm_runtime_suspended_time(struct device *dev)
>  {
> -       unsigned long flags, time;
> +       u64 time;
> +       unsigned long flags;
>
>         spin_lock_irqsave(&dev->power.lock, flags);
>
>         update_pm_runtime_accounting(dev);
>
> -       time = dev->power.suspended_jiffies;
> +       time = dev->power.suspended_time;
>
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>
> -       return jiffies_to_nsecs(time);
> +       return time;
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
>
> @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev)
>         dev->power.request_pending = false;
>         dev->power.request = RPM_REQ_NONE;
>         dev->power.deferred_resume = false;
> -       dev->power.accounting_timestamp = jiffies;
> +       dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
>         INIT_WORK(&dev->power.work, pm_runtime_work);
>
>         dev->power.timer_expires = 0;
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index d713738..96c8a22 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev,
>                                 struct device_attribute *attr, char *buf)
>  {
>         int ret;
> +       u64 tmp;
>         spin_lock_irq(&dev->power.lock);
>         update_pm_runtime_accounting(dev);
> -       ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies));
> +       tmp = dev->power.active_time;
> +       do_div(tmp, NSEC_PER_MSEC);
> +       ret = sprintf(buf, "%llu\n", tmp);
>         spin_unlock_irq(&dev->power.lock);
>         return ret;
>  }
> @@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev,
>                                 struct device_attribute *attr, char *buf)
>  {
>         int ret;
> +       u64 tmp;
>         spin_lock_irq(&dev->power.lock);
>         update_pm_runtime_accounting(dev);
> -       ret = sprintf(buf, "%i\n",
> -               jiffies_to_msecs(dev->power.suspended_jiffies));
> +       tmp = dev->power.suspended_time;
> +       do_div(tmp, NSEC_PER_MSEC);
> +       ret = sprintf(buf, "%llu\n", tmp);
>         spin_unlock_irq(&dev->power.lock);
>         return ret;
>  }
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78..e5a34e2 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -632,9 +632,9 @@ struct dev_pm_info {
>         int                     runtime_error;
>         int                     autosuspend_delay;
>         unsigned long           last_busy;
> -       unsigned long           active_jiffies;
> -       unsigned long           suspended_jiffies;
> -       unsigned long           accounting_timestamp;
> +       u64                     active_time;
> +       u64                     suspended_time;
> +       u64                     accounting_timestamp;
>  #endif
>         struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */
>         void (*set_latency_tolerance)(struct device *, s32);
> --
> 2.7.4
>

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> From: Thara Gopinath <thara.gopinath@linaro.org>
> 
> This patch replaces jiffies based accounting for runtime_active_time
> and runtime_suspended_time with ktime base accounting. This makes the
> runtime debug counters inline with genpd and other pm subsytems which
> uses ktime based accounting.
> 
> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> be ready before first call. In fact, timekeeping_init() is called early
> in start_kernel() which is way before driver_init() (and that's when
> devices can start to be initialized) called from rest_init() via
> kernel_init_freeable() and do_basic_setup().
> 
This is not (always) correct. My qemu "collie" boot test fails with this
patch applied. Reverting the patch fixes the problem. Bisect log attached.

With some added debugging:

...
IRQS: 16, nr_irqs: 65, preallocated irqs: 65
irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
gpio gpiochip0: ############### pm_runtime_init() ############
irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
############## timekeeping_init() ####################
sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 582542222200ns^M
...

This is with:

void pm_runtime_init(struct device *dev)
{
+       dev_info(dev, "############### pm_runtime_init() ############\n");
+
...
@@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
	struct clocksource *clock;
	unsigned long flags;
		 
+       pr_info("############## timekeeping_init() ####################\n");
+

Guenter

---
# bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files for 20190117
# good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
git bisect start 'HEAD' 'v5.0-rc2'
# bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 'pm/linux-next'
git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
# good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 'omap/for-next'
git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
# good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 'fuse/for-next'
git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
# good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 'i2c/i2c/for-next'
git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
# good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for h.264 chroma qp index offset
git bisect good 3943f059823b6e15884387f31618b84826e924b3
# good: [44970bbbbb5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 'v4l-dvb/master'
git bisect good 44970bbbbb5079ee100875b06e45db5d6e91a16e
# bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into linux-next
git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
# good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into linux-next
git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
# good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' and 'acpi-apei' into linux-next
git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
# good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm runtime interface
git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
# bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing clk_prepare() return value check
git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
# bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
# first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> From: Thara Gopinath <thara.gopinath@linaro.org>
> 
> This patch replaces jiffies based accounting for runtime_active_time
> and runtime_suspended_time with ktime base accounting. This makes the
> runtime debug counters inline with genpd and other pm subsytems which
> uses ktime based accounting.
> 
> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> be ready before first call. In fact, timekeeping_init() is called early
> in start_kernel() which is way before driver_init() (and that's when
> devices can start to be initialized) called from rest_init() via
> kernel_init_freeable() and do_basic_setup().
> 
This is not (always) correct. My qemu "collie" boot test fails with this
patch applied. Reverting the patch fixes the problem. Bisect log attached.

With some added debugging:

...
IRQS: 16, nr_irqs: 65, preallocated irqs: 65
irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
gpio gpiochip0: ############### pm_runtime_init() ############
irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
############## timekeeping_init() ####################
sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 582542222200ns^M
...

This is with:

void pm_runtime_init(struct device *dev)
{
+       dev_info(dev, "############### pm_runtime_init() ############\n");
+
...
@@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
	struct clocksource *clock;
	unsigned long flags;
		 
+       pr_info("############## timekeeping_init() ####################\n");
+

Guenter

---
# bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files for 20190117
# good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
git bisect start 'HEAD' 'v5.0-rc2'
# bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 'pm/linux-next'
git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
# good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 'omap/for-next'
git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
# good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 'fuse/for-next'
git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
# good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 'i2c/i2c/for-next'
git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
# good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for h.264 chroma qp index offset
git bisect good 3943f059823b6e15884387f31618b84826e924b3
# good: [44970bbbbb5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 'v4l-dvb/master'
git bisect good 44970bbbbb5079ee100875b06e45db5d6e91a16e
# bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into linux-next
git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
# good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into linux-next
git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
# good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' and 'acpi-apei' into linux-next
git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
# good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm runtime interface
git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
# bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing clk_prepare() return value check
git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
# bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
# first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

Hi Guenter,

Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > From: Thara Gopinath <thara.gopinath@linaro.org>
> > 
> > This patch replaces jiffies based accounting for runtime_active_time
> > and runtime_suspended_time with ktime base accounting. This makes the
> > runtime debug counters inline with genpd and other pm subsytems which
> > uses ktime based accounting.
> > 
> > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > be ready before first call. In fact, timekeeping_init() is called early
> > in start_kernel() which is way before driver_init() (and that's when
> > devices can start to be initialized) called from rest_init() via
> > kernel_init_freeable() and do_basic_setup().
> > 
> This is not (always) correct. My qemu "collie" boot test fails with this
> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> 

Can you try the patch below ?
ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
it can be used at early_init.

---
 drivers/base/power/runtime.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ae1c728..118c7f6 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
  */
 void update_pm_runtime_accounting(struct device *dev)
 {
-	u64 now = ktime_to_ns(ktime_get());
+	u64 now = ktime_get_mono_fast_ns();
 	u64 delta;
 
 	delta = now - dev->power.accounting_timestamp;
@@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
 	dev->power.request_pending = false;
 	dev->power.request = RPM_REQ_NONE;
 	dev->power.deferred_resume = false;
-	dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
+	dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
 	INIT_WORK(&dev->power.work, pm_runtime_work);
 
 	dev->power.timer_expires = 0;
-- 
2.7.4


> With some added debugging:
> 
> ...
> IRQS: 16, nr_irqs: 65, preallocated irqs: 65
> irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
> gpio gpiochip0: ############### pm_runtime_init() ############
> irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
> ############## timekeeping_init() ####################
> sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 582542222200ns^M
> ...
> 
> This is with:
> 
> void pm_runtime_init(struct device *dev)
> {
> +       dev_info(dev, "############### pm_runtime_init() ############\n");
> +
> ...
> @@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
> 	struct clocksource *clock;
> 	unsigned long flags;
> 		 
> +       pr_info("############## timekeeping_init() ####################\n");
> +
> 
> Guenter
> 
> ---
> # bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files for 20190117
> # good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
> git bisect start 'HEAD' 'v5.0-rc2'
> # bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 'pm/linux-next'
> git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
> # good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 'omap/for-next'
> git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
> # good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 'fuse/for-next'
> git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
> # good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 'i2c/i2c/for-next'
> git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
> # good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for h.264 chroma qp index offset
> git bisect good 3943f059823b6e15884387f31618b84826e924b3
> # good: [44970bbbbb5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 'v4l-dvb/master'
> git bisect good 44970bbbbb5079ee100875b06e45db5d6e91a16e
> # bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into linux-next
> git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
> # good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into linux-next
> git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
> # good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' and 'acpi-apei' into linux-next
> git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
> # good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm runtime interface
> git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
> # bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing clk_prepare() return value check
> git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
> # bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
> git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
> # first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

Hi Guenter,

Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > From: Thara Gopinath <thara.gopinath@linaro.org>
> > 
> > This patch replaces jiffies based accounting for runtime_active_time
> > and runtime_suspended_time with ktime base accounting. This makes the
> > runtime debug counters inline with genpd and other pm subsytems which
> > uses ktime based accounting.
> > 
> > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > be ready before first call. In fact, timekeeping_init() is called early
> > in start_kernel() which is way before driver_init() (and that's when
> > devices can start to be initialized) called from rest_init() via
> > kernel_init_freeable() and do_basic_setup().
> > 
> This is not (always) correct. My qemu "collie" boot test fails with this
> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> 

Can you try the patch below ?
ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
it can be used at early_init.

---
 drivers/base/power/runtime.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index ae1c728..118c7f6 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
  */
 void update_pm_runtime_accounting(struct device *dev)
 {
-	u64 now = ktime_to_ns(ktime_get());
+	u64 now = ktime_get_mono_fast_ns();
 	u64 delta;
 
 	delta = now - dev->power.accounting_timestamp;
@@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
 	dev->power.request_pending = false;
 	dev->power.request = RPM_REQ_NONE;
 	dev->power.deferred_resume = false;
-	dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
+	dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
 	INIT_WORK(&dev->power.work, pm_runtime_work);
 
 	dev->power.timer_expires = 0;
-- 
2.7.4


> With some added debugging:
> 
> ...
> IRQS: 16, nr_irqs: 65, preallocated irqs: 65
> irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
> gpio gpiochip0: ############### pm_runtime_init() ############
> irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
> ############## timekeeping_init() ####################
> sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 582542222200ns^M
> ...
> 
> This is with:
> 
> void pm_runtime_init(struct device *dev)
> {
> +       dev_info(dev, "############### pm_runtime_init() ############\n");
> +
> ...
> @@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
> 	struct clocksource *clock;
> 	unsigned long flags;
> 		 
> +       pr_info("############## timekeeping_init() ####################\n");
> +
> 
> Guenter
> 
> ---
> # bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files for 20190117
> # good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
> git bisect start 'HEAD' 'v5.0-rc2'
> # bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 'pm/linux-next'
> git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
> # good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 'omap/for-next'
> git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
> # good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 'fuse/for-next'
> git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
> # good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 'i2c/i2c/for-next'
> git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
> # good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for h.264 chroma qp index offset
> git bisect good 3943f059823b6e15884387f31618b84826e924b3
> # good: [44970bbbbb5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 'v4l-dvb/master'
> git bisect good 44970bbbbb5079ee100875b06e45db5d6e91a16e
> # bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into linux-next
> git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
> # good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into linux-next
> git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
> # good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' and 'acpi-apei' into linux-next
> git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
> # good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm runtime interface
> git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
> # bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing clk_prepare() return value check
> git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
> # bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
> git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
> # first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Hi Guenter,
>
> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > > From: Thara Gopinath <thara.gopinath@linaro.org>
> > >
> > > This patch replaces jiffies based accounting for runtime_active_time
> > > and runtime_suspended_time with ktime base accounting. This makes the
> > > runtime debug counters inline with genpd and other pm subsytems which
> > > uses ktime based accounting.
> > >
> > > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > > be ready before first call. In fact, timekeeping_init() is called early
> > > in start_kernel() which is way before driver_init() (and that's when
> > > devices can start to be initialized) called from rest_init() via
> > > kernel_init_freeable() and do_basic_setup().
> > >
> > This is not (always) correct. My qemu "collie" boot test fails with this
> > patch applied. Reverting the patch fixes the problem. Bisect log attached.
> >
>
> Can you try the patch below ?
> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> it can be used at early_init.

Another possibility would be delay the init of the gpiochip

>
> ---
>  drivers/base/power/runtime.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index ae1c728..118c7f6 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
>   */
>  void update_pm_runtime_accounting(struct device *dev)
>  {
> -       u64 now = ktime_to_ns(ktime_get());
> +       u64 now = ktime_get_mono_fast_ns();
>         u64 delta;
>
>         delta = now - dev->power.accounting_timestamp;
> @@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
>         dev->power.request_pending = false;
>         dev->power.request = RPM_REQ_NONE;
>         dev->power.deferred_resume = false;
> -       dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
> +       dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
>         INIT_WORK(&dev->power.work, pm_runtime_work);
>
>         dev->power.timer_expires = 0;
> --
> 2.7.4
>
>
> > With some added debugging:
> >
> > ...
> > IRQS: 16, nr_irqs: 65, preallocated irqs: 65
> > irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
> > gpio gpiochip0: ############### pm_runtime_init() ############
> > irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
> > ############## timekeeping_init() ####################
> > sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 582542222200ns^M
> > ...
> >
> > This is with:
> >
> > void pm_runtime_init(struct device *dev)
> > {
> > +       dev_info(dev, "############### pm_runtime_init() ############\n");
> > +
> > ...
> > @@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
> >       struct clocksource *clock;
> >       unsigned long flags;
> >
> > +       pr_info("############## timekeeping_init() ####################\n");
> > +
> >
> > Guenter
> >
> > ---
> > # bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files for 20190117
> > # good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
> > git bisect start 'HEAD' 'v5.0-rc2'
> > # bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 'pm/linux-next'
> > git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
> > # good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 'omap/for-next'
> > git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
> > # good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 'fuse/for-next'
> > git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
> > # good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 'i2c/i2c/for-next'
> > git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
> > # good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for h.264 chroma qp index offset
> > git bisect good 3943f059823b6e15884387f31618b84826e924b3
> > # good: [44970bbbbb5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 'v4l-dvb/master'
> > git bisect good 44970bbbbb5079ee100875b06e45db5d6e91a16e
> > # bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into linux-next
> > git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
> > # good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into linux-next
> > git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
> > # good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' and 'acpi-apei' into linux-next
> > git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
> > # good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm runtime interface
> > git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
> > # bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing clk_prepare() return value check
> > git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
> > # bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
> > git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
> > # first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> Hi Guenter,
>
> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > > From: Thara Gopinath <thara.gopinath@linaro.org>
> > >
> > > This patch replaces jiffies based accounting for runtime_active_time
> > > and runtime_suspended_time with ktime base accounting. This makes the
> > > runtime debug counters inline with genpd and other pm subsytems which
> > > uses ktime based accounting.
> > >
> > > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > > be ready before first call. In fact, timekeeping_init() is called early
> > > in start_kernel() which is way before driver_init() (and that's when
> > > devices can start to be initialized) called from rest_init() via
> > > kernel_init_freeable() and do_basic_setup().
> > >
> > This is not (always) correct. My qemu "collie" boot test fails with this
> > patch applied. Reverting the patch fixes the problem. Bisect log attached.
> >
>
> Can you try the patch below ?
> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> it can be used at early_init.

Another possibility would be delay the init of the gpiochip

>
> ---
>  drivers/base/power/runtime.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index ae1c728..118c7f6 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
>   */
>  void update_pm_runtime_accounting(struct device *dev)
>  {
> -       u64 now = ktime_to_ns(ktime_get());
> +       u64 now = ktime_get_mono_fast_ns();
>         u64 delta;
>
>         delta = now - dev->power.accounting_timestamp;
> @@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
>         dev->power.request_pending = false;
>         dev->power.request = RPM_REQ_NONE;
>         dev->power.deferred_resume = false;
> -       dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
> +       dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
>         INIT_WORK(&dev->power.work, pm_runtime_work);
>
>         dev->power.timer_expires = 0;
> --
> 2.7.4
>
>
> > With some added debugging:
> >
> > ...
> > IRQS: 16, nr_irqs: 65, preallocated irqs: 65
> > irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
> > gpio gpiochip0: ############### pm_runtime_init() ############
> > irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
> > ############## timekeeping_init() ####################
> > sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 582542222200ns^M
> > ...
> >
> > This is with:
> >
> > void pm_runtime_init(struct device *dev)
> > {
> > +       dev_info(dev, "############### pm_runtime_init() ############\n");
> > +
> > ...
> > @@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
> >       struct clocksource *clock;
> >       unsigned long flags;
> >
> > +       pr_info("############## timekeeping_init() ####################\n");
> > +
> >
> > Guenter
> >
> > ---
> > # bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files for 20190117
> > # good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
> > git bisect start 'HEAD' 'v5.0-rc2'
> > # bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 'pm/linux-next'
> > git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
> > # good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 'omap/for-next'
> > git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
> > # good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 'fuse/for-next'
> > git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
> > # good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 'i2c/i2c/for-next'
> > git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
> > # good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for h.264 chroma qp index offset
> > git bisect good 3943f059823b6e15884387f31618b84826e924b3
> > # good: [44970bbbbb5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 'v4l-dvb/master'
> > git bisect good 44970bbbbb5079ee100875b06e45db5d6e91a16e
> > # bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into linux-next
> > git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
> > # good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into linux-next
> > git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
> > # good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' and 'acpi-apei' into linux-next
> > git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
> > # good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm runtime interface
> > git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
> > # bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing clk_prepare() return value check
> > git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
> > # bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
> > git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
> > # first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Rafael J. Wysocki]

On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Hi Guenter,
> >
> > Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > > On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > > > From: Thara Gopinath <thara.gopinath@linaro.org>
> > > >
> > > > This patch replaces jiffies based accounting for runtime_active_time
> > > > and runtime_suspended_time with ktime base accounting. This makes the
> > > > runtime debug counters inline with genpd and other pm subsytems which
> > > > uses ktime based accounting.
> > > >
> > > > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > > > be ready before first call. In fact, timekeeping_init() is called early
> > > > in start_kernel() which is way before driver_init() (and that's when
> > > > devices can start to be initialized) called from rest_init() via
> > > > kernel_init_freeable() and do_basic_setup().
> > > >
> > > This is not (always) correct. My qemu "collie" boot test fails with this
> > > patch applied. Reverting the patch fixes the problem. Bisect log attached.
> > >
> >
> > Can you try the patch below ?
> > ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> > it can be used at early_init.
>
> Another possibility would be delay the init of the gpiochip

Well, right.

Initializing devices before timekeeping doesn't feel particularly
robust from the design perspective.

How exactly does that happen?

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Rafael J. Wysocki]

On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > Hi Guenter,
> >
> > Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > > On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > > > From: Thara Gopinath <thara.gopinath@linaro.org>
> > > >
> > > > This patch replaces jiffies based accounting for runtime_active_time
> > > > and runtime_suspended_time with ktime base accounting. This makes the
> > > > runtime debug counters inline with genpd and other pm subsytems which
> > > > uses ktime based accounting.
> > > >
> > > > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > > > be ready before first call. In fact, timekeeping_init() is called early
> > > > in start_kernel() which is way before driver_init() (and that's when
> > > > devices can start to be initialized) called from rest_init() via
> > > > kernel_init_freeable() and do_basic_setup().
> > > >
> > > This is not (always) correct. My qemu "collie" boot test fails with this
> > > patch applied. Reverting the patch fixes the problem. Bisect log attached.
> > >
> >
> > Can you try the patch below ?
> > ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> > it can be used at early_init.
>
> Another possibility would be delay the init of the gpiochip

Well, right.

Initializing devices before timekeeping doesn't feel particularly
robust from the design perspective.

How exactly does that happen?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
>>
>> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
>> <vincent.guittot@linaro.org> wrote:
>>>
>>> Hi Guenter,
>>>
>>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
>>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
>>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
>>>>>
>>>>> This patch replaces jiffies based accounting for runtime_active_time
>>>>> and runtime_suspended_time with ktime base accounting. This makes the
>>>>> runtime debug counters inline with genpd and other pm subsytems which
>>>>> uses ktime based accounting.
>>>>>
>>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
>>>>> be ready before first call. In fact, timekeeping_init() is called early
>>>>> in start_kernel() which is way before driver_init() (and that's when
>>>>> devices can start to be initialized) called from rest_init() via
>>>>> kernel_init_freeable() and do_basic_setup().
>>>>>
>>>> This is not (always) correct. My qemu "collie" boot test fails with this
>>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
>>>>
>>>
>>> Can you try the patch below ?
>>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
>>> it can be used at early_init.
>>
>> Another possibility would be delay the init of the gpiochip
> 
> Well, right.
> 
> Initializing devices before timekeeping doesn't feel particularly
> robust from the design perspective.
> 
> How exactly does that happen?
> 

With an added 'initialized' flag and backtrace into the timekeeping code,
with the change suggested earlier applied:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
Timekeeping not initialized
CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
Hardware name: Sharp-Collie
Backtrace:
[<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
  r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
[<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
[<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
[<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
  r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
[<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
  r3:00000000 r2:c065bad8
  r5:00000000 r4:df407b08
[<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
  r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
[<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
  r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
[<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
  r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
[<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
  r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
  r4:c06fca34
[<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
  r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
[<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
  r5:c0713300 r4:00000000
[<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
[<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
  r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
  r4:c06a7330
---[ end trace 91e1bd00dd7cce32 ]---

Guenter

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
>>
>> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
>> <vincent.guittot@linaro.org> wrote:
>>>
>>> Hi Guenter,
>>>
>>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
>>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
>>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
>>>>>
>>>>> This patch replaces jiffies based accounting for runtime_active_time
>>>>> and runtime_suspended_time with ktime base accounting. This makes the
>>>>> runtime debug counters inline with genpd and other pm subsytems which
>>>>> uses ktime based accounting.
>>>>>
>>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
>>>>> be ready before first call. In fact, timekeeping_init() is called early
>>>>> in start_kernel() which is way before driver_init() (and that's when
>>>>> devices can start to be initialized) called from rest_init() via
>>>>> kernel_init_freeable() and do_basic_setup().
>>>>>
>>>> This is not (always) correct. My qemu "collie" boot test fails with this
>>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
>>>>
>>>
>>> Can you try the patch below ?
>>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
>>> it can be used at early_init.
>>
>> Another possibility would be delay the init of the gpiochip
> 
> Well, right.
> 
> Initializing devices before timekeeping doesn't feel particularly
> robust from the design perspective.
> 
> How exactly does that happen?
> 

With an added 'initialized' flag and backtrace into the timekeeping code,
with the change suggested earlier applied:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
Timekeeping not initialized
CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
Hardware name: Sharp-Collie
Backtrace:
[<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
  r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
[<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
[<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
[<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
  r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
[<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
  r3:00000000 r2:c065bad8
  r5:00000000 r4:df407b08
[<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
  r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
[<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
  r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
[<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
  r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
[<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
  r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
  r4:c06fca34
[<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
  r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
[<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
  r5:c0713300 r4:00000000
[<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
[<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
  r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
  r4:c06a7330
---[ end trace 91e1bd00dd7cce32 ]---

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

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> >>
> >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> >> <vincent.guittot@linaro.org> wrote:
> >>>
> >>> Hi Guenter,
> >>>
> >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> >>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
> >>>>>
> >>>>> This patch replaces jiffies based accounting for runtime_active_time
> >>>>> and runtime_suspended_time with ktime base accounting. This makes the
> >>>>> runtime debug counters inline with genpd and other pm subsytems which
> >>>>> uses ktime based accounting.
> >>>>>
> >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> >>>>> be ready before first call. In fact, timekeeping_init() is called early
> >>>>> in start_kernel() which is way before driver_init() (and that's when
> >>>>> devices can start to be initialized) called from rest_init() via
> >>>>> kernel_init_freeable() and do_basic_setup().
> >>>>>
> >>>> This is not (always) correct. My qemu "collie" boot test fails with this
> >>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> >>>>
> >>>
> >>> Can you try the patch below ?
> >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> >>> it can be used at early_init.
> >>
> >> Another possibility would be delay the init of the gpiochip
> >
> > Well, right.
> >
> > Initializing devices before timekeeping doesn't feel particularly
> > robust from the design perspective.
> >
> > How exactly does that happen?
> >
>
> With an added 'initialized' flag and backtrace into the timekeeping code,
> with the change suggested earlier applied:
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
> Timekeeping not initialized
> CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
> Hardware name: Sharp-Collie
> Backtrace:
> [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
>   r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
> [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
> [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
> [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
>   r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
> [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
>   r3:00000000 r2:c065bad8
>   r5:00000000 r4:df407b08
> [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
>   r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
>   r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
>   r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
> [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
>   r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
>   r4:c06fca34
> [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
>   r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
> [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
>   r5:c0713300 r4:00000000
> [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
> [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
>   r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
>   r4:c06a7330
> ---[ end trace 91e1bd00dd7cce32 ]---

Does it means that only the pm_runtime_init is done before
timekeeping_init() but no update_pm_runtime_accounting() ?
In this case, we can keep using ktimeçget in
update_pm_runtime_accounting() and find a solution to deal with
early_call of pm_runtime_init()

Vincent
>
> Guenter

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> >>
> >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> >> <vincent.guittot@linaro.org> wrote:
> >>>
> >>> Hi Guenter,
> >>>
> >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> >>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
> >>>>>
> >>>>> This patch replaces jiffies based accounting for runtime_active_time
> >>>>> and runtime_suspended_time with ktime base accounting. This makes the
> >>>>> runtime debug counters inline with genpd and other pm subsytems which
> >>>>> uses ktime based accounting.
> >>>>>
> >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> >>>>> be ready before first call. In fact, timekeeping_init() is called early
> >>>>> in start_kernel() which is way before driver_init() (and that's when
> >>>>> devices can start to be initialized) called from rest_init() via
> >>>>> kernel_init_freeable() and do_basic_setup().
> >>>>>
> >>>> This is not (always) correct. My qemu "collie" boot test fails with this
> >>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> >>>>
> >>>
> >>> Can you try the patch below ?
> >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> >>> it can be used at early_init.
> >>
> >> Another possibility would be delay the init of the gpiochip
> >
> > Well, right.
> >
> > Initializing devices before timekeeping doesn't feel particularly
> > robust from the design perspective.
> >
> > How exactly does that happen?
> >
>
> With an added 'initialized' flag and backtrace into the timekeeping code,
> with the change suggested earlier applied:
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
> Timekeeping not initialized
> CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
> Hardware name: Sharp-Collie
> Backtrace:
> [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
>   r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
> [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
> [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
> [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
>   r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
> [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
>   r3:00000000 r2:c065bad8
>   r5:00000000 r4:df407b08
> [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
>   r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
>   r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
>   r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
> [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
>   r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
>   r4:c06fca34
> [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
>   r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
> [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
>   r5:c0713300 r4:00000000
> [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
> [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
>   r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
>   r4:c06a7330
> ---[ end trace 91e1bd00dd7cce32 ]---

Does it means that only the pm_runtime_init is done before
timekeeping_init() but no update_pm_runtime_accounting() ?
In this case, we can keep using ktimeçget in
update_pm_runtime_accounting() and find a solution to deal with
early_call of pm_runtime_init()

Vincent
>
> Guenter

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/21/19 7:17 AM, Vincent Guittot wrote:
> On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
>>> On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
>>> <vincent.guittot@linaro.org> wrote:
>>>>
>>>> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
>>>> <vincent.guittot@linaro.org> wrote:
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
>>>>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
>>>>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
>>>>>>>
>>>>>>> This patch replaces jiffies based accounting for runtime_active_time
>>>>>>> and runtime_suspended_time with ktime base accounting. This makes the
>>>>>>> runtime debug counters inline with genpd and other pm subsytems which
>>>>>>> uses ktime based accounting.
>>>>>>>
>>>>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
>>>>>>> be ready before first call. In fact, timekeeping_init() is called early
>>>>>>> in start_kernel() which is way before driver_init() (and that's when
>>>>>>> devices can start to be initialized) called from rest_init() via
>>>>>>> kernel_init_freeable() and do_basic_setup().
>>>>>>>
>>>>>> This is not (always) correct. My qemu "collie" boot test fails with this
>>>>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
>>>>>>
>>>>>
>>>>> Can you try the patch below ?
>>>>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
>>>>> it can be used at early_init.
>>>>
>>>> Another possibility would be delay the init of the gpiochip
>>>
>>> Well, right.
>>>
>>> Initializing devices before timekeeping doesn't feel particularly
>>> robust from the design perspective.
>>>
>>> How exactly does that happen?
>>>
>>
>> With an added 'initialized' flag and backtrace into the timekeeping code,
>> with the change suggested earlier applied:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
>> Timekeeping not initialized
>> CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
>> Hardware name: Sharp-Collie
>> Backtrace:
>> [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
>>    r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
>> [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
>> [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
>> [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
>>    r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
>> [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
>>    r3:00000000 r2:c065bad8
>>    r5:00000000 r4:df407b08
>> [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
>>    r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
>> [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
>>    r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
>> [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
>>    r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
>> [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
>>    r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
>>    r4:c06fca34
>> [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
>>    r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
>> [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
>>    r5:c0713300 r4:00000000
>> [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
>> [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
>>    r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
>>    r4:c06a7330
>> ---[ end trace 91e1bd00dd7cce32 ]---
> 
> Does it means that only the pm_runtime_init is done before
> timekeeping_init() but no update_pm_runtime_accounting() ?
> In this case, we can keep using ktimeçget in
> update_pm_runtime_accounting() and find a solution to deal with
> early_call of pm_runtime_init()
> 

For this platform that is correct. I can't answer for the generic case.

Guenter

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/21/19 7:17 AM, Vincent Guittot wrote:
> On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
>>> On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
>>> <vincent.guittot@linaro.org> wrote:
>>>>
>>>> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
>>>> <vincent.guittot@linaro.org> wrote:
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
>>>>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
>>>>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
>>>>>>>
>>>>>>> This patch replaces jiffies based accounting for runtime_active_time
>>>>>>> and runtime_suspended_time with ktime base accounting. This makes the
>>>>>>> runtime debug counters inline with genpd and other pm subsytems which
>>>>>>> uses ktime based accounting.
>>>>>>>
>>>>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
>>>>>>> be ready before first call. In fact, timekeeping_init() is called early
>>>>>>> in start_kernel() which is way before driver_init() (and that's when
>>>>>>> devices can start to be initialized) called from rest_init() via
>>>>>>> kernel_init_freeable() and do_basic_setup().
>>>>>>>
>>>>>> This is not (always) correct. My qemu "collie" boot test fails with this
>>>>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
>>>>>>
>>>>>
>>>>> Can you try the patch below ?
>>>>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
>>>>> it can be used at early_init.
>>>>
>>>> Another possibility would be delay the init of the gpiochip
>>>
>>> Well, right.
>>>
>>> Initializing devices before timekeeping doesn't feel particularly
>>> robust from the design perspective.
>>>
>>> How exactly does that happen?
>>>
>>
>> With an added 'initialized' flag and backtrace into the timekeeping code,
>> with the change suggested earlier applied:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
>> Timekeeping not initialized
>> CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
>> Hardware name: Sharp-Collie
>> Backtrace:
>> [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
>>    r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
>> [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
>> [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
>> [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
>>    r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
>> [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
>>    r3:00000000 r2:c065bad8
>>    r5:00000000 r4:df407b08
>> [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
>>    r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
>> [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
>>    r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
>> [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
>>    r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
>> [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
>>    r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
>>    r4:c06fca34
>> [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
>>    r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
>> [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
>>    r5:c0713300 r4:00000000
>> [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
>> [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
>>    r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
>>    r4:c06a7330
>> ---[ end trace 91e1bd00dd7cce32 ]---
> 
> Does it means that only the pm_runtime_init is done before
> timekeeping_init() but no update_pm_runtime_accounting() ?
> In this case, we can keep using ktimeçget in
> update_pm_runtime_accounting() and find a solution to deal with
> early_call of pm_runtime_init()
> 

For this platform that is correct. I can't answer for the generic case.

Guenter

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Rafael J. Wysocki]

On Mon, Jan 21, 2019 at 4:17 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> > > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > >>
> > >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> > >> <vincent.guittot@linaro.org> wrote:
> > >>>
> > >>> Hi Guenter,
> > >>>
> > >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > >>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
> > >>>>>
> > >>>>> This patch replaces jiffies based accounting for runtime_active_time
> > >>>>> and runtime_suspended_time with ktime base accounting. This makes the
> > >>>>> runtime debug counters inline with genpd and other pm subsytems which
> > >>>>> uses ktime based accounting.
> > >>>>>
> > >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > >>>>> be ready before first call. In fact, timekeeping_init() is called early
> > >>>>> in start_kernel() which is way before driver_init() (and that's when
> > >>>>> devices can start to be initialized) called from rest_init() via
> > >>>>> kernel_init_freeable() and do_basic_setup().
> > >>>>>
> > >>>> This is not (always) correct. My qemu "collie" boot test fails with this
> > >>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> > >>>>
> > >>>
> > >>> Can you try the patch below ?
> > >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> > >>> it can be used at early_init.
> > >>
> > >> Another possibility would be delay the init of the gpiochip
> > >
> > > Well, right.
> > >
> > > Initializing devices before timekeeping doesn't feel particularly
> > > robust from the design perspective.
> > >
> > > How exactly does that happen?
> > >
> >
> > With an added 'initialized' flag and backtrace into the timekeeping code,
> > with the change suggested earlier applied:
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
> > Timekeeping not initialized
> > CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
> > Hardware name: Sharp-Collie
> > Backtrace:
> > [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
> >   r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
> > [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
> > [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
> > [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
> >   r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
> > [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
> >   r3:00000000 r2:c065bad8
> >   r5:00000000 r4:df407b08
> > [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
> >   r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
> >   r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
> >   r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
> > [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
> >   r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
> >   r4:c06fca34
> > [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
> >   r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
> > [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
> >   r5:c0713300 r4:00000000
> > [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
> > [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
> >   r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
> >   r4:c06a7330
> > ---[ end trace 91e1bd00dd7cce32 ]---
>
> Does it means that only the pm_runtime_init is done before
> timekeeping_init() but no update_pm_runtime_accounting() ?

This platform calls device_initialize(), via sa1100_init_irq(), from
init_IRQ() which is in the start_kernel() code path before
timekeeping_init().  That's the initialization of structure fields
alone.

Runtime PM really cannot be used legitimately before driver_init(),
because it needs bus types to be there at least.

> In this case, we can keep using ktimeçget in
> update_pm_runtime_accounting() and find a solution to deal with
> early_call of pm_runtime_init()

Given the above, I think that initializing accounting_timestamp in
pm_runtime_init() to anything different from 0 is a mistake.

Note that update_pm_runtime_accounting() ignores the delta value if
power.disable_depth is not zero anyway, so it really should be
sufficient to update accounting_timestamp when enabling runtime PM -
and I'm not sure why it is not updated in pm_runtime_enable() for that
matter (that looks like a bug to me).

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Rafael J. Wysocki]

On Mon, Jan 21, 2019 at 4:17 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> > > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > >>
> > >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> > >> <vincent.guittot@linaro.org> wrote:
> > >>>
> > >>> Hi Guenter,
> > >>>
> > >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > >>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
> > >>>>>
> > >>>>> This patch replaces jiffies based accounting for runtime_active_time
> > >>>>> and runtime_suspended_time with ktime base accounting. This makes the
> > >>>>> runtime debug counters inline with genpd and other pm subsytems which
> > >>>>> uses ktime based accounting.
> > >>>>>
> > >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > >>>>> be ready before first call. In fact, timekeeping_init() is called early
> > >>>>> in start_kernel() which is way before driver_init() (and that's when
> > >>>>> devices can start to be initialized) called from rest_init() via
> > >>>>> kernel_init_freeable() and do_basic_setup().
> > >>>>>
> > >>>> This is not (always) correct. My qemu "collie" boot test fails with this
> > >>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> > >>>>
> > >>>
> > >>> Can you try the patch below ?
> > >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> > >>> it can be used at early_init.
> > >>
> > >> Another possibility would be delay the init of the gpiochip
> > >
> > > Well, right.
> > >
> > > Initializing devices before timekeeping doesn't feel particularly
> > > robust from the design perspective.
> > >
> > > How exactly does that happen?
> > >
> >
> > With an added 'initialized' flag and backtrace into the timekeeping code,
> > with the change suggested earlier applied:
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
> > Timekeeping not initialized
> > CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
> > Hardware name: Sharp-Collie
> > Backtrace:
> > [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
> >   r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
> > [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
> > [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
> > [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
> >   r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
> > [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
> >   r3:00000000 r2:c065bad8
> >   r5:00000000 r4:df407b08
> > [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
> >   r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
> >   r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
> >   r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
> > [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
> >   r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
> >   r4:c06fca34
> > [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
> >   r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
> > [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
> >   r5:c0713300 r4:00000000
> > [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
> > [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
> >   r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
> >   r4:c06a7330
> > ---[ end trace 91e1bd00dd7cce32 ]---
>
> Does it means that only the pm_runtime_init is done before
> timekeeping_init() but no update_pm_runtime_accounting() ?

This platform calls device_initialize(), via sa1100_init_irq(), from
init_IRQ() which is in the start_kernel() code path before
timekeeping_init().  That's the initialization of structure fields
alone.

Runtime PM really cannot be used legitimately before driver_init(),
because it needs bus types to be there at least.

> In this case, we can keep using ktimeçget in
> update_pm_runtime_accounting() and find a solution to deal with
> early_call of pm_runtime_init()

Given the above, I think that initializing accounting_timestamp in
pm_runtime_init() to anything different from 0 is a mistake.

Note that update_pm_runtime_accounting() ignores the delta value if
power.disable_depth is not zero anyway, so it really should be
sufficient to update accounting_timestamp when enabling runtime PM -
and I'm not sure why it is not updated in pm_runtime_enable() for that
matter (that looks like a bug to me).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

On Mon, 21 Jan 2019 at 23:53, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jan 21, 2019 at 4:17 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> > > > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > >>
> > > >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> > > >> <vincent.guittot@linaro.org> wrote:
> > > >>>
> > > >>> Hi Guenter,
> > > >>>
> > > >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > > >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > > >>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
> > > >>>>>
> > > >>>>> This patch replaces jiffies based accounting for runtime_active_time
> > > >>>>> and runtime_suspended_time with ktime base accounting. This makes the
> > > >>>>> runtime debug counters inline with genpd and other pm subsytems which
> > > >>>>> uses ktime based accounting.
> > > >>>>>
> > > >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > > >>>>> be ready before first call. In fact, timekeeping_init() is called early
> > > >>>>> in start_kernel() which is way before driver_init() (and that's when
> > > >>>>> devices can start to be initialized) called from rest_init() via
> > > >>>>> kernel_init_freeable() and do_basic_setup().
> > > >>>>>
> > > >>>> This is not (always) correct. My qemu "collie" boot test fails with this
> > > >>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> > > >>>>
> > > >>>
> > > >>> Can you try the patch below ?
> > > >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> > > >>> it can be used at early_init.
> > > >>
> > > >> Another possibility would be delay the init of the gpiochip
> > > >
> > > > Well, right.
> > > >
> > > > Initializing devices before timekeeping doesn't feel particularly
> > > > robust from the design perspective.
> > > >
> > > > How exactly does that happen?
> > > >
> > >
> > > With an added 'initialized' flag and backtrace into the timekeeping code,
> > > with the change suggested earlier applied:
> > >
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
> > > Timekeeping not initialized
> > > CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
> > > Hardware name: Sharp-Collie
> > > Backtrace:
> > > [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
> > >   r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
> > > [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
> > > [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
> > > [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
> > >   r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
> > > [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
> > >   r3:00000000 r2:c065bad8
> > >   r5:00000000 r4:df407b08
> > > [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
> > >   r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > > [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
> > >   r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > > [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
> > >   r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
> > > [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
> > >   r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
> > >   r4:c06fca34
> > > [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
> > >   r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
> > > [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
> > >   r5:c0713300 r4:00000000
> > > [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
> > > [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
> > >   r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
> > >   r4:c06a7330
> > > ---[ end trace 91e1bd00dd7cce32 ]---
> >
> > Does it means that only the pm_runtime_init is done before
> > timekeeping_init() but no update_pm_runtime_accounting() ?
>
> This platform calls device_initialize(), via sa1100_init_irq(), from
> init_IRQ() which is in the start_kernel() code path before
> timekeeping_init().  That's the initialization of structure fields
> alone.
>
> Runtime PM really cannot be used legitimately before driver_init(),
> because it needs bus types to be there at least.
>
> > In this case, we can keep using ktimeçget in
> > update_pm_runtime_accounting() and find a solution to deal with
> > early_call of pm_runtime_init()
>
> Given the above, I think that initializing accounting_timestamp in
> pm_runtime_init() to anything different from 0 is a mistake.

I agree

>
> Note that update_pm_runtime_accounting() ignores the delta value if
> power.disable_depth is not zero anyway, so it really should be
> sufficient to update accounting_timestamp when enabling runtime PM -
> and I'm not sure why it is not updated in pm_runtime_enable() for that
> matter (that looks like a bug to me).

That's sounds reasonable to me. I'm going to add a patch to add this
changes before moving to ktime_get

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Vincent Guittot]

On Mon, 21 Jan 2019 at 23:53, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jan 21, 2019 at 4:17 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Fri, 18 Jan 2019 at 13:08, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On 1/18/19 3:05 AM, Rafael J. Wysocki wrote:
> > > > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > >>
> > > >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot
> > > >> <vincent.guittot@linaro.org> wrote:
> > > >>>
> > > >>> Hi Guenter,
> > > >>>
> > > >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
> > > >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > > >>>>> From: Thara Gopinath <thara.gopinath@linaro.org>
> > > >>>>>
> > > >>>>> This patch replaces jiffies based accounting for runtime_active_time
> > > >>>>> and runtime_suspended_time with ktime base accounting. This makes the
> > > >>>>> runtime debug counters inline with genpd and other pm subsytems which
> > > >>>>> uses ktime based accounting.
> > > >>>>>
> > > >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > > >>>>> be ready before first call. In fact, timekeeping_init() is called early
> > > >>>>> in start_kernel() which is way before driver_init() (and that's when
> > > >>>>> devices can start to be initialized) called from rest_init() via
> > > >>>>> kernel_init_freeable() and do_basic_setup().
> > > >>>>>
> > > >>>> This is not (always) correct. My qemu "collie" boot test fails with this
> > > >>>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
> > > >>>>
> > > >>>
> > > >>> Can you try the patch below ?
> > > >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> > > >>> it can be used at early_init.
> > > >>
> > > >> Another possibility would be delay the init of the gpiochip
> > > >
> > > > Well, right.
> > > >
> > > > Initializing devices before timekeeping doesn't feel particularly
> > > > robust from the design perspective.
> > > >
> > > > How exactly does that happen?
> > > >
> > >
> > > With an added 'initialized' flag and backtrace into the timekeeping code,
> > > with the change suggested earlier applied:
> > >
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 ktime_get_mono_fast_ns+0x114/0x12c
> > > Timekeeping not initialized
> > > CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2
> > > Hardware name: Sharp-Collie
> > > Backtrace:
> > > [<c000dce8>] (dump_backtrace) from [<c000df78>] (show_stack+0x18/0x1c)
> > >   r7:00000009 r6:00000000 r5:c065ba90 r4:c06d3e54
> > > [<c000df60>] (show_stack) from [<c0588930>] (dump_stack+0x20/0x28)
> > > [<c0588910>] (dump_stack) from [<c0018ae8>] (__warn+0xcc/0xf4)
> > > [<c0018a1c>] (__warn) from [<c0018b5c>] (warn_slowpath_fmt+0x4c/0x6c)
> > >   r8:df407b08 r7:00000000 r6:c0c01550 r5:c065bad8 r4:c06dd028
> > > [<c0018b14>] (warn_slowpath_fmt) from [<c0069e2c>] (ktime_get_mono_fast_ns+0x114/0x12c)
> > >   r3:00000000 r2:c065bad8
> > >   r5:00000000 r4:df407b08
> > > [<c0069d18>] (ktime_get_mono_fast_ns) from [<c03c7810>] (pm_runtime_init+0x38/0xb8)
> > >   r9:c06c9a5c r8:df407b08 r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > > [<c03c77d8>] (pm_runtime_init) from [<c03b6a34>] (device_initialize+0xb0/0xec)
> > >   r7:00000000 r6:c0c01550 r5:00000000 r4:df407b08
> > > [<c03b6984>] (device_initialize) from [<c0366d30>] (gpiochip_add_data_with_key+0x9c/0x884)
> > >   r7:00000000 r6:c06fca34 r5:00000000 r4:00000000
> > > [<c0366c94>] (gpiochip_add_data_with_key) from [<c06b9708>] (sa1100_init_gpio+0x40/0x98)
> > >   r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6:ffffffff r5:00000000
> > >   r4:c06fca34
> > > [<c06b96c8>] (sa1100_init_gpio) from [<c06ae58c>] (sa1100_init_irq+0x2c/0x3c)
> > >   r7:c06dd028 r6:ffffffff r5:c0713300 r4:c06e1070
> > > [<c06ae560>] (sa1100_init_irq) from [<c06aab1c>] (init_IRQ+0x20/0x28)
> > >   r5:c0713300 r4:00000000
> > > [<c06aaafc>] (init_IRQ) from [<c06a7cd0>] (start_kernel+0x254/0x4cc)
> > > [<c06a7a7c>] (start_kernel) from [<00000000>] (  (null))
> > >   r10:0000717f r9:6901b119 r8:c0000100 r7:00000092 r6:0000313d r5:00000053
> > >   r4:c06a7330
> > > ---[ end trace 91e1bd00dd7cce32 ]---
> >
> > Does it means that only the pm_runtime_init is done before
> > timekeeping_init() but no update_pm_runtime_accounting() ?
>
> This platform calls device_initialize(), via sa1100_init_irq(), from
> init_IRQ() which is in the start_kernel() code path before
> timekeeping_init().  That's the initialization of structure fields
> alone.
>
> Runtime PM really cannot be used legitimately before driver_init(),
> because it needs bus types to be there at least.
>
> > In this case, we can keep using ktimeçget in
> > update_pm_runtime_accounting() and find a solution to deal with
> > early_call of pm_runtime_init()
>
> Given the above, I think that initializing accounting_timestamp in
> pm_runtime_init() to anything different from 0 is a mistake.

I agree

>
> Note that update_pm_runtime_accounting() ignores the delta value if
> power.disable_depth is not zero anyway, so it really should be
> sufficient to update accounting_timestamp when enabling runtime PM -
> and I'm not sure why it is not updated in pm_runtime_enable() for that
> matter (that looks like a bug to me).

That's sounds reasonable to me. I'm going to add a patch to add this
changes before moving to ktime_get

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Guenter Roeck]

On 1/18/19 2:42 AM, Vincent Guittot wrote:
> Hi Guenter,
> 
> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit :
>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
>>> From: Thara Gopinath <thara.gopinath@linaro.org>
>>>
>>> This patch replaces jiffies based accounting for runtime_active_time
>>> and runtime_suspended_time with ktime base accounting. This makes the
>>> runtime debug counters inline with genpd and other pm subsytems which
>>> uses ktime based accounting.
>>>
>>> timekeeping is initialized before pm_runtime_init() so ktime_get() will
>>> be ready before first call. In fact, timekeeping_init() is called early
>>> in start_kernel() which is way before driver_init() (and that's when
>>> devices can start to be initialized) called from rest_init() via
>>> kernel_init_freeable() and do_basic_setup().
>>>
>> This is not (always) correct. My qemu "collie" boot test fails with this
>> patch applied. Reverting the patch fixes the problem. Bisect log attached.
>>
> 
> Can you try the patch below ?
> ktime_get_mono_fast_ns() has the advantage of being init with dummy clock so
> it can be used at early_init.
> 
Yes, that works.

Guenter

> ---
>   drivers/base/power/runtime.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index ae1c728..118c7f6 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -66,7 +66,7 @@ static int rpm_suspend(struct device *dev, int rpmflags);
>    */
>   void update_pm_runtime_accounting(struct device *dev)
>   {
> -	u64 now = ktime_to_ns(ktime_get());
> +	u64 now = ktime_get_mono_fast_ns();
>   	u64 delta;
>   
>   	delta = now - dev->power.accounting_timestamp;
> @@ -1507,7 +1507,7 @@ void pm_runtime_init(struct device *dev)
>   	dev->power.request_pending = false;
>   	dev->power.request = RPM_REQ_NONE;
>   	dev->power.deferred_resume = false;
> -	dev->power.accounting_timestamp = ktime_to_ns(ktime_get());
> +	dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
>   	INIT_WORK(&dev->power.work, pm_runtime_work);
>   
>   	dev->power.timer_expires = 0;
> 

Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting

[Rafael J. Wysocki]

On Thu, Jan 17, 2019 at 11:16 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote:
> > From: Thara Gopinath <thara.gopinath@linaro.org>
> >
> > This patch replaces jiffies based accounting for runtime_active_time
> > and runtime_suspended_time with ktime base accounting. This makes the
> > runtime debug counters inline with genpd and other pm subsytems which
> > uses ktime based accounting.
> >
> > timekeeping is initialized before pm_runtime_init() so ktime_get() will
> > be ready before first call. In fact, timekeeping_init() is called early
> > in start_kernel() which is way before driver_init() (and that's when
> > devices can start to be initialized) called from rest_init() via
> > kernel_init_freeable() and do_basic_setup().
> >
> This is not (always) correct. My qemu "collie" boot test fails with this
> patch applied. Reverting the patch fixes the problem. Bisect log attached.
>
> With some added debugging:
>
> ...
> IRQS: 16, nr_irqs: 65, preallocated irqs: 65
> irq: Cannot allocate irq_descs @ IRQ1, assuming pre-allocated
> gpio gpiochip0: ############### pm_runtime_init() ############
> irq: Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
> ############## timekeeping_init() ####################
> sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 582542222200ns^M
> ...
>
> This is with:
>
> void pm_runtime_init(struct device *dev)
> {
> +       dev_info(dev, "############### pm_runtime_init() ############\n");
> +
> ...
> @@ -1526,6 +1526,8 @@ void __init timekeeping_init(void)
>         struct clocksource *clock;
>         unsigned long flags;
>
> +       pr_info("############## timekeeping_init() ####################\n");
> +
>
> Guenter
>
> ---
> # bad: [a37d50ca3b837c19a297f349365d11a20c1087d0] Add linux-next specific files for 20190117
> # good: [1c7fc5cbc33980acd13d668f1c8f0313d6ae9fd8] Linux 5.0-rc2
> git bisect start 'HEAD' 'v5.0-rc2'
> # bad: [4edb817d29fdf19fb5d52784bb3c29c40e00f586] Merge remote-tracking branch 'pm/linux-next'
> git bisect bad 4edb817d29fdf19fb5d52784bb3c29c40e00f586
> # good: [6d95886720d306a1914a7c9a8aeb0bcbc7aef018] Merge remote-tracking branch 'omap/for-next'
> git bisect good 6d95886720d306a1914a7c9a8aeb0bcbc7aef018
> # good: [975b5cdd74430bc8a04f832d65a47cdb95b73fec] Merge remote-tracking branch 'fuse/for-next'
> git bisect good 975b5cdd74430bc8a04f832d65a47cdb95b73fec
> # good: [112386d2189fc54b979c3a8bf64b2908df91e123] Merge remote-tracking branch 'i2c/i2c/for-next'
> git bisect good 112386d2189fc54b979c3a8bf64b2908df91e123
> # good: [3943f059823b6e15884387f31618b84826e924b3] media: coda: Add control for h.264 chroma qp index offset
> git bisect good 3943f059823b6e15884387f31618b84826e924b3
> # good: [44970bbbbb5079ee100875b06e45db5d6e91a16e] Merge remote-tracking branch 'v4l-dvb/master'
> git bisect good 44970bbbbb5079ee100875b06e45db5d6e91a16e
> # bad: [599170c2b860aca3364574f833bb9cc801c9668d] Merge branch 'pm-core' into linux-next
> git bisect bad 599170c2b860aca3364574f833bb9cc801c9668d
> # good: [347d570919ca9a3a3653ce9cbb7399c7c0ba8248] Merge branch 'acpi-pci' into linux-next
> git bisect good 347d570919ca9a3a3653ce9cbb7399c7c0ba8248
> # good: [e0a9fde86ba1bc26dd754c733b32e70cd8f1c187] Merge branches 'acpi-tables' and 'acpi-apei' into linux-next
> git bisect good e0a9fde86ba1bc26dd754c733b32e70cd8f1c187
> # good: [3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567] drm/i915: Move on the new pm runtime interface
> git bisect good 3b4ed2e2eb5583774a1ed4aa4596a5a3c55f2567
> # bad: [c75c303933a68c547f3352d1d708843f9449d3f4] PM: clock_ops: fix missing clk_prepare() return value check
> git bisect bad c75c303933a68c547f3352d1d708843f9449d3f4
> # bad: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting
> git bisect bad 3982ab9ce433efc72ca31eb9ddc85d9355966444
> # first bad commit: [3982ab9ce433efc72ca31eb9ddc85d9355966444] PM-runtime: Replace jiffies based accounting with ktime-based accounting

OK, dropping this one for now, we need a working replacement.

Thanks!

✗ Fi.CI.CHECKPATCH: warning for Move pm_runtime accounted time to raw nsec

[Patchwork]

== Series Details ==

Series: Move pm_runtime accounted time to raw nsec
URL   : https://patchwork.freedesktop.org/series/54404/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c92f56748cd1 PM/runtime: Add a new interface to get accounted time
-:48: CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files
#48: FILE: include/linux/pm_runtime.h:116:
+extern u64 pm_runtime_suspended_time(struct device *dev);

total: 0 errors, 0 warnings, 1 checks, 30 lines checked
b055afbb789b drm/i915: Move on the new pm runtime interface
223e162d09bf PM/runtime:Replace jiffies based accounting with ktime based accounting

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

✓ Fi.CI.BAT: success for Move pm_runtime accounted time to raw nsec

[Patchwork]

== Series Details ==

Series: Move pm_runtime accounted time to raw nsec
URL   : https://patchwork.freedesktop.org/series/54404/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5337 -> Patchwork_11144
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_11144 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11144, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/54404/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_11144:

### IGT changes ###

#### Warnings ####

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6770hq:      PASS -> SKIP +36

  
Known issues
------------

  Here are the changes found in Patchwork_11144 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        PASS -> DMESG-FAIL [fdo#108735]

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       PASS -> FAIL [fdo#108767]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  * {igt@runner@aborted}:
    - fi-icl-y:           NOTRUN -> FAIL [fdo#108915]

  
#### Possible fixes ####

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6700hq:      DMESG-WARN [fdo#105998] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          FAIL [fdo#103167] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767
  [fdo#108915]: https://bugs.freedesktop.org/show_bug.cgi?id=108915
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965


Participating hosts (48 -> 41)
------------------------------

  Additional (1): fi-icl-y 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 fi-icl-u3 fi-byt-n2820 


Build changes
-------------

    * Linux: CI_DRM_5337 -> Patchwork_11144

  CI_DRM_5337: 3ac901085a9fae8699716ac44579dab1dec546c3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4752: 321fe77d32fef32ef820f53924045fe6ef0cf6ed @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11144: 223e162d09bfae095c6f988c5c9031560228ada6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

223e162d09bf PM/runtime:Replace jiffies based accounting with ktime based accounting
b055afbb789b drm/i915: Move on the new pm runtime interface
c92f56748cd1 PM/runtime: Add a new interface to get accounted time

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11144/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Move pm_runtime accounted time to raw nsec

[Patchwork]

== Series Details ==

Series: Move pm_runtime accounted time to raw nsec
URL   : https://patchwork.freedesktop.org/series/54404/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5337_full -> Patchwork_11144_full
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_11144_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11144_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_11144_full:

### IGT changes ###

#### Warnings ####

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-snb:          SKIP -> PASS

  
Known issues
------------

  Here are the changes found in Patchwork_11144_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@pi-ringfull-vebox:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103158]

  * igt@gem_ppgtt@blt-vs-render-ctxn:
    - shard-skl:          PASS -> TIMEOUT [fdo#108039]

  * igt@i915_selftest@live_workarounds:
    - shard-iclb:         PASS -> DMESG-FAIL [fdo#108954]

  * igt@i915_suspend@shrink:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#108784]

  * igt@kms_ccs@pipe-a-missing-ccs-buffer:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724] +15

  * igt@kms_cursor_crc@cursor-256x256-onscreen:
    - shard-skl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-offscreen:
    - shard-skl:          NOTRUN -> FAIL [fdo#103232]

  * igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-xtiled:
    - shard-iclb:         PASS -> WARN [fdo#108336] +2

  * igt@kms_draw_crc@draw-method-xrgb8888-pwrite-ytiled:
    - shard-iclb:         NOTRUN -> WARN [fdo#108336]

  * igt@kms_fbcon_fbt@psr:
    - shard-skl:          NOTRUN -> FAIL [fdo#107882]

  * igt@kms_flip@2x-flip-vs-expired-vblank:
    - shard-glk:          PASS -> FAIL [fdo#105363] +1

  * igt@kms_flip@plain-flip-fb-recreate:
    - shard-skl:          PASS -> FAIL [fdo#100368]

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-gtt:
    - shard-iclb:         PASS -> DMESG-FAIL [fdo#107724] +5

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-wc:
    - shard-apl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite:
    - shard-skl:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +4

  * igt@kms_frontbuffer_tracking@fbc-rgb101010-draw-mmap-wc:
    - shard-skl:          PASS -> FAIL [fdo#103167] / [fdo#105682]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-mmap-cpu:
    - shard-iclb:         NOTRUN -> DMESG-FAIL [fdo#107724] +1

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-msflip-blt:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] / [fdo#108336] +9

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724] / [fdo#108336] +3

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#107713]

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - shard-apl:          PASS -> FAIL [fdo#103166] +3
    - shard-iclb:         PASS -> FAIL [fdo#103166] +3

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-glk:          PASS -> FAIL [fdo#108145] +1

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          PASS -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815]

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_setmode@basic:
    - shard-apl:          PASS -> FAIL [fdo#99912]
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@perf_pmu@event-wait-rcs0:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +23

  * igt@pm_rpm@basic-rte:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  
#### Possible fixes ####

  * igt@gem_exec_fence@basic-busy-default:
    - shard-snb:          INCOMPLETE [fdo#105411] -> PASS

  * igt@gem_exec_whisper@normal:
    - shard-skl:          TIMEOUT [fdo#108592] -> PASS

  * igt@kms_color@pipe-c-ctm-max:
    - shard-apl:          FAIL [fdo#108147] -> PASS

  * igt@kms_cursor_crc@cursor-256x85-sliding:
    - shard-apl:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip_tiling@flip-to-x-tiled:
    - shard-iclb:         FAIL [fdo#108134] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-apl:          FAIL [fdo#103167] -> PASS +3

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-pwrite:
    - shard-glk:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-farfromfence:
    - shard-kbl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS +21

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         FAIL [fdo#105683] / [fdo#108040] -> PASS

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +1

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-apl:          FAIL [fdo#108948] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-glk:          FAIL [fdo#103166] -> PASS +3

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
    - shard-iclb:         FAIL [fdo#103166] -> PASS +1

  * igt@kms_plane_scaling@pipe-b-scaler-with-pixel-format:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-glk:          DMESG-FAIL [fdo#105763] / [fdo#106538] -> PASS

  * igt@kms_setmode@basic:
    - shard-hsw:          FAIL [fdo#99912] -> PASS

  * igt@pm_rpm@dpms-mode-unset-lpsp:
    - shard-iclb:         INCOMPLETE [fdo#107713] / [fdo#108840] -> PASS

  * igt@pm_rpm@gem-execbuf-stress:
    - shard-skl:          INCOMPLETE [fdo#107803] / [fdo#107807] -> PASS

  * igt@pm_rpm@universal-planes-dpms:
    - shard-skl:          INCOMPLETE [fdo#107807] -> PASS

  
#### Warnings ####

  * igt@kms_cursor_crc@cursor-128x128-sliding:
    - shard-iclb:         FAIL [fdo#103232] -> DMESG-WARN [fdo#107724] / [fdo#108336]

  * igt@kms_plane_alpha_blend@pipe-c-alpha-transparant-fb:
    - shard-kbl:          DMESG-FAIL [fdo#103558] / [fdo#105602] / [fdo#108145] -> FAIL [fdo#108145] +1

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          DMESG-WARN [fdo#105604] -> DMESG-FAIL [fdo#108950]

  
  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105604]: https://bugs.freedesktop.org/show_bug.cgi?id=105604
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105683]: https://bugs.freedesktop.org/show_bug.cgi?id=105683
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107882]: https://bugs.freedesktop.org/show_bug.cgi?id=107882
  [fdo#108039]: https://bugs.freedesktop.org/show_bug.cgi?id=108039
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108134]: https://bugs.freedesktop.org/show_bug.cgi?id=108134
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
  [fdo#108592]: https://bugs.freedesktop.org/show_bug.cgi?id=108592
  [fdo#108784]: https://bugs.freedesktop.org/show_bug.cgi?id=108784
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#108950]: https://bugs.freedesktop.org/show_bug.cgi?id=108950
  [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


Build changes
-------------

    * Linux: CI_DRM_5337 -> Patchwork_11144

  CI_DRM_5337: 3ac901085a9fae8699716ac44579dab1dec546c3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4752: 321fe77d32fef32ef820f53924045fe6ef0cf6ed @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11144: 223e162d09bfae095c6f988c5c9031560228ada6 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11144/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Move pm_runtime accounted time to raw nsec (rev2)

[Patchwork]

== Series Details ==

Series: Move pm_runtime accounted time to raw nsec (rev2)
URL   : https://patchwork.freedesktop.org/series/54404/
State : failure

== Summary ==

Applying: PM/runtime: Add a new interface to get accounted time
Applying: drm/i915: Move on the new pm runtime interface
Applying: PM/runtime:Replace jiffies based accounting with ktime based accounting
error: sha1 information is lacking or useless (drivers/base/power/runtime.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0003 PM/runtime:Replace jiffies based accounting with ktime based accounting
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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