[Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
From: Oscar Mateo Or with a spinlock grabbed, because it might sleep, which is not a nice thing to do. Instead, do the runtime_pm get/put together with the create/destroy request, and handle the forcewake get/put directly. Signed-off-by: Oscar Mateo --- drivers/gpu/drm/i915/intel_lrc.c | 26 +- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index d33e622..ea4b358 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -159,6 +159,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, struct drm_i915_private *dev_priv = ring->dev->dev_private; uint64_t temp = 0; uint32_t desc[4]; + unsigned long flags; /* XXX: You must always write both descriptors in the order below. */ if (ctx_obj1) @@ -172,9 +173,17 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, desc[3] = (u32)(temp >> 32); desc[2] = (u32)temp; - /* Set Force Wakeup bit to prevent GT from entering C6 while -* ELSP writes are in progress */ - gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); + /* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes +* are in progress. +* +* The other problem is that we can't just call gen6_gt_force_wake_get() +* because that function calls intel_runtime_pm_get(), which might sleep. +* Instead, we do the runtime_pm_get/put when creating/destroying requests. +*/ + spin_lock_irqsave(&dev_priv->uncore.lock, flags); + if (dev_priv->uncore.forcewake_count++ == 0) + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); I915_WRITE(RING_ELSP(ring), desc[1]); I915_WRITE(RING_ELSP(ring), desc[0]); @@ -185,8 +194,11 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, /* ELSP is a write only register, so this serves as a posting read */ POSTING_READ(RING_EXECLIST_STATUS(ring)); - /* Release Force Wakeup */ - gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); + /* Release Force Wakeup (see the big comment above). */ + spin_lock_irqsave(&dev_priv->uncore.lock, flags); + if (--dev_priv->uncore.forcewake_count == 0) + dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); } static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 tail) @@ -353,6 +365,9 @@ static void execlists_free_request_task(struct work_struct *work) struct intel_ctx_submit_request *req = container_of(work, struct intel_ctx_submit_request, work); struct drm_device *dev = req->ring->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + + intel_runtime_pm_put(dev_priv); mutex_lock(&dev->struct_mutex); i915_gem_context_unreference(req->ctx); @@ -378,6 +393,7 @@ static int execlists_context_queue(struct intel_engine_cs *ring, req->ring = ring; req->tail = tail; INIT_WORK(&req->work, execlists_free_request_task); + intel_runtime_pm_get(dev_priv); spin_lock_irqsave(&ring->execlist_lock, flags); -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com wrote: > From: Oscar Mateo > > Or with a spinlock grabbed, because it might sleep, which is not > a nice thing to do. Instead, do the runtime_pm get/put together > with the create/destroy request, and handle the forcewake get/put > directly. > > Signed-off-by: Oscar Mateo Looks like a fixup that should be squashed into relevant earlier patches. -Daniel > --- > drivers/gpu/drm/i915/intel_lrc.c | 26 +- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index d33e622..ea4b358 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -159,6 +159,7 @@ static void execlists_elsp_write(struct intel_engine_cs > *ring, > struct drm_i915_private *dev_priv = ring->dev->dev_private; > uint64_t temp = 0; > uint32_t desc[4]; > + unsigned long flags; > > /* XXX: You must always write both descriptors in the order below. */ > if (ctx_obj1) > @@ -172,9 +173,17 @@ static void execlists_elsp_write(struct intel_engine_cs > *ring, > desc[3] = (u32)(temp >> 32); > desc[2] = (u32)temp; > > - /* Set Force Wakeup bit to prevent GT from entering C6 while > - * ELSP writes are in progress */ > - gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > + /* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes > + * are in progress. > + * > + * The other problem is that we can't just call gen6_gt_force_wake_get() > + * because that function calls intel_runtime_pm_get(), which might > sleep. > + * Instead, we do the runtime_pm_get/put when creating/destroying > requests. > + */ > + spin_lock_irqsave(&dev_priv->uncore.lock, flags); > + if (dev_priv->uncore.forcewake_count++ == 0) > + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); > + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > > I915_WRITE(RING_ELSP(ring), desc[1]); > I915_WRITE(RING_ELSP(ring), desc[0]); > @@ -185,8 +194,11 @@ static void execlists_elsp_write(struct intel_engine_cs > *ring, > /* ELSP is a write only register, so this serves as a posting read */ > POSTING_READ(RING_EXECLIST_STATUS(ring)); > > - /* Release Force Wakeup */ > - gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > + /* Release Force Wakeup (see the big comment above). */ > + spin_lock_irqsave(&dev_priv->uncore.lock, flags); > + if (--dev_priv->uncore.forcewake_count == 0) > + dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); > + spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > } > > static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 > tail) > @@ -353,6 +365,9 @@ static void execlists_free_request_task(struct > work_struct *work) > struct intel_ctx_submit_request *req = > container_of(work, struct intel_ctx_submit_request, > work); > struct drm_device *dev = req->ring->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + intel_runtime_pm_put(dev_priv); > > mutex_lock(&dev->struct_mutex); > i915_gem_context_unreference(req->ctx); > @@ -378,6 +393,7 @@ static int execlists_context_queue(struct intel_engine_cs > *ring, > req->ring = ring; > req->tail = tail; > INIT_WORK(&req->work, execlists_free_request_task); > + intel_runtime_pm_get(dev_priv); > > spin_lock_irqsave(&ring->execlist_lock, flags); > > -- > 1.9.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote: > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com wrote: > > From: Oscar Mateo > > > > Or with a spinlock grabbed, because it might sleep, which is not > > a nice thing to do. Instead, do the runtime_pm get/put together > > with the create/destroy request, and handle the forcewake get/put > > directly. > > > > Signed-off-by: Oscar Mateo > > Looks like a fixup that should be squashed into relevant earlier patches. The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is broken due to this - we must be able to read registers in atomic context! Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690 -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote: > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote: > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com wrote: > > > From: Oscar Mateo > > > > > > Or with a spinlock grabbed, because it might sleep, which is not > > > a nice thing to do. Instead, do the runtime_pm get/put together > > > with the create/destroy request, and handle the forcewake get/put > > > directly. > > > > > > Signed-off-by: Oscar Mateo > > > > Looks like a fixup that should be squashed into relevant earlier patches. > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is > broken due to this - we must be able to read registers in atomic > context! > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690 force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if you want to read registers from atomic context you have to have a runtime pm reference from someone else. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote: > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote: > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote: > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com wrote: > > > > From: Oscar Mateo > > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not > > > > a nice thing to do. Instead, do the runtime_pm get/put together > > > > with the create/destroy request, and handle the forcewake get/put > > > > directly. > > > > > > > > Signed-off-by: Oscar Mateo > > > > > > Looks like a fixup that should be squashed into relevant earlier patches. > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is > > broken due to this - we must be able to read registers in atomic > > context! > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690 > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if > you want to read registers from atomic context you have to have a runtime > pm reference from someone else. Nope. That cannot work. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote: > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote: > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote: > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote: > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com wrote: > > > > > From: Oscar Mateo > > > > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not > > > > > a nice thing to do. Instead, do the runtime_pm get/put together > > > > > with the create/destroy request, and handle the forcewake get/put > > > > > directly. > > > > > > > > > > Signed-off-by: Oscar Mateo > > > > > > > > Looks like a fixup that should be squashed into relevant earlier > > > > patches. > > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is > > > broken due to this - we must be able to read registers in atomic > > > context! > > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690 > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if > > you want to read registers from atomic context you have to have a runtime > > pm reference from someone else. > > Nope. That cannot work. Well it works currently. So where do you see the problem? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote: > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote: > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote: > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote: > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote: > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com wrote: > > > > > > From: Oscar Mateo > > > > > > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together > > > > > > with the create/destroy request, and handle the forcewake get/put > > > > > > directly. > > > > > > > > > > > > Signed-off-by: Oscar Mateo > > > > > > > > > > Looks like a fixup that should be squashed into relevant earlier > > > > > patches. > > > > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is > > > > broken due to this - we must be able to read registers in atomic > > > > context! > > > > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690 > > > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if > > > you want to read registers from atomic context you have to have a runtime > > > pm reference from someone else. > > > > Nope. That cannot work. > > Well it works currently. So where do you see the problem? Sampling registers from an timer - in particular, we really do not want to disable runtime pm whilst trying to monitor the impact of runtime pm. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com
> > > > > > wrote:
> > > > > > > From: Oscar Mateo
> > > > > > >
> > > > > > > Or with a spinlock grabbed, because it might sleep, which is not
> > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together
> > > > > > > with the create/destroy request, and handle the forcewake get/put
> > > > > > > directly.
> > > > > > >
> > > > > > > Signed-off-by: Oscar Mateo
> > > > > >
> > > > > > Looks like a fixup that should be squashed into relevant earlier
> > > > > > patches.
> > > > >
> > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is
> > > > > broken due to this - we must be able to read registers in atomic
> > > > > context!
> > > > >
> > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > >
> > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So if
> > > > you want to read registers from atomic context you have to have a
> > > > runtime
> > > > pm reference from someone else.
> > >
> > > Nope. That cannot work.
> >
> > Well it works currently. So where do you see the problem?
>
> Sampling registers from an timer - in particular, we really do not want
> to disable runtime pm whilst trying to monitor the impact of runtime pm.
In that case you can grab a runtime pm reference iff the device is powered
on already. Which won't call anything scary, just amounts to an
atomic_add_unless or so, and then drop it again.
Unfortunately there doesn't seem to be such a thing around already, so
need to add it first. Greg, how much would you freak out if we add
something like
/**
* pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
*
* Returns true if an rpm ref has been acquire, false otherwise. Can be
* called from atomic context to e.g. sample perfomance counters (where we
* obviously don't want to disturb system state if everything is off atm).
*/
static inline bool pm_runtime_get_unless_suspended(struct device *dev)
{
return atomic_add_unless(&dev->power.usage_count, 1, 0);
}
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Fri, Aug 08, 2014 at 11:37:01AM +0200, Daniel Vetter wrote:
> On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> > On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com
> > > > > > > wrote:
> > > > > > > > From: Oscar Mateo
> > > > > > > >
> > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not
> > > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together
> > > > > > > > with the create/destroy request, and handle the forcewake
> > > > > > > > get/put
> > > > > > > > directly.
> > > > > > > >
> > > > > > > > Signed-off-by: Oscar Mateo
> > > > > > >
> > > > > > > Looks like a fixup that should be squashed into relevant earlier
> > > > > > > patches.
> > > > > >
> > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is
> > > > > > broken due to this - we must be able to read registers in atomic
> > > > > > context!
> > > > > >
> > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > > >
> > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So
> > > > > if
> > > > > you want to read registers from atomic context you have to have a
> > > > > runtime
> > > > > pm reference from someone else.
> > > >
> > > > Nope. That cannot work.
> > >
> > > Well it works currently. So where do you see the problem?
> >
> > Sampling registers from an timer - in particular, we really do not want
> > to disable runtime pm whilst trying to monitor the impact of runtime pm.
>
> In that case you can grab a runtime pm reference iff the device is powered
> on already. Which won't call anything scary, just amounts to an
> atomic_add_unless or so, and then drop it again.
>
> Unfortunately there doesn't seem to be such a thing around already, so
> need to add it first. Greg, how much would you freak out if we add
> something like
>
> /**
> * pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
> *
> * Returns true if an rpm ref has been acquire, false otherwise. Can be
> * called from atomic context to e.g. sample perfomance counters (where we
> * obviously don't want to disturb system state if everything is off atm).
> */
> static inline bool pm_runtime_get_unless_suspended(struct device *dev)
> {
> return atomic_add_unless(&dev->power.usage_count, 1, 0);
> }
I'd freak out a lot :)
Rafael, isn't there some other better way to resolve this issue without
resorting to something as "horrid" as the above?
thanks,
greg k-h
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Friday, August 08, 2014 11:37:01 AM Daniel Vetter wrote:
> On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> > On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com
> > > > > > > wrote:
> > > > > > > > From: Oscar Mateo
> > > > > > > >
> > > > > > > > Or with a spinlock grabbed, because it might sleep, which is not
> > > > > > > > a nice thing to do. Instead, do the runtime_pm get/put together
> > > > > > > > with the create/destroy request, and handle the forcewake
> > > > > > > > get/put
> > > > > > > > directly.
> > > > > > > >
> > > > > > > > Signed-off-by: Oscar Mateo
> > > > > > >
> > > > > > > Looks like a fixup that should be squashed into relevant earlier
> > > > > > > patches.
> > > > > >
> > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get() is
> > > > > > broken due to this - we must be able to read registers in atomic
> > > > > > context!
> > > > > >
> > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > > >
> > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep. So
> > > > > if
> > > > > you want to read registers from atomic context you have to have a
> > > > > runtime
> > > > > pm reference from someone else.
> > > >
> > > > Nope. That cannot work.
> > >
> > > Well it works currently. So where do you see the problem?
> >
> > Sampling registers from an timer - in particular, we really do not want
> > to disable runtime pm whilst trying to monitor the impact of runtime pm.
>
> In that case you can grab a runtime pm reference iff the device is powered
> on already. Which won't call anything scary, just amounts to an
> atomic_add_unless or so, and then drop it again.
>
> Unfortunately there doesn't seem to be such a thing around already, so
> need to add it first. Greg, how much would you freak out if we add
> something like
>
> /**
> * pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
> *
> * Returns true if an rpm ref has been acquire, false otherwise. Can be
> * called from atomic context to e.g. sample perfomance counters (where we
> * obviously don't want to disturb system state if everything is off atm).
> */
> static inline bool pm_runtime_get_unless_suspended(struct device *dev)
> {
> return atomic_add_unless(&dev->power.usage_count, 1, 0);
> }
I don't think it'll work universally.
That'd need to be synchronized with other stuff done under the spinlock
and in fact, what you're interested in is runtime_status (and that being
RPM_ACTIVE) and not just the usage count.
Rafael
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Friday, August 08, 2014 06:41:10 AM Greg KH wrote:
> On Fri, Aug 08, 2014 at 11:37:01AM +0200, Daniel Vetter wrote:
> > On Fri, Aug 08, 2014 at 10:20:40AM +0100, Chris Wilson wrote:
> > > On Tue, Jul 29, 2014 at 12:26:36PM +0200, Daniel Vetter wrote:
> > > > On Tue, Jul 29, 2014 at 08:37:48AM +0100, Chris Wilson wrote:
> > > > > On Mon, Jul 28, 2014 at 10:54:06AM +0200, Daniel Vetter wrote:
> > > > > > On Sat, Jul 26, 2014 at 11:27:38AM +0100, Chris Wilson wrote:
> > > > > > > On Wed, Jun 18, 2014 at 10:54:13PM +0200, Daniel Vetter wrote:
> > > > > > > > On Fri, Jun 13, 2014 at 04:38:03PM +0100, oscar.ma...@intel.com
> > > > > > > > wrote:
> > > > > > > > > From: Oscar Mateo
> > > > > > > > >
> > > > > > > > > Or with a spinlock grabbed, because it might sleep, which is
> > > > > > > > > not
> > > > > > > > > a nice thing to do. Instead, do the runtime_pm get/put
> > > > > > > > > together
> > > > > > > > > with the create/destroy request, and handle the forcewake
> > > > > > > > > get/put
> > > > > > > > > directly.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Oscar Mateo
> > > > > > > >
> > > > > > > > Looks like a fixup that should be squashed into relevant
> > > > > > > > earlier patches.
> > > > > > >
> > > > > > > The whole gen6_gt_force_wake_get() calling intel_runtime_pm_get()
> > > > > > > is
> > > > > > > broken due to this - we must be able to read registers in atomic
> > > > > > > context!
> > > > > > >
> > > > > > > Please revert c8c8fb33b37766acf6474784b0d5245dab9a1690
> > > > > >
> > > > > > force_wake_get can't call runtime_pm_get becuase pm_get can sleep.
> > > > > > So if
> > > > > > you want to read registers from atomic context you have to have a
> > > > > > runtime
> > > > > > pm reference from someone else.
> > > > >
> > > > > Nope. That cannot work.
> > > >
> > > > Well it works currently. So where do you see the problem?
> > >
> > > Sampling registers from an timer - in particular, we really do not want
> > > to disable runtime pm whilst trying to monitor the impact of runtime pm.
> >
> > In that case you can grab a runtime pm reference iff the device is powered
> > on already. Which won't call anything scary, just amounts to an
> > atomic_add_unless or so, and then drop it again.
> >
> > Unfortunately there doesn't seem to be such a thing around already, so
> > need to add it first. Greg, how much would you freak out if we add
> > something like
> >
> > /**
> > * pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
> > *
> > * Returns true if an rpm ref has been acquire, false otherwise. Can be
> > * called from atomic context to e.g. sample perfomance counters (where we
> > * obviously don't want to disturb system state if everything is off atm).
> > */
> > static inline bool pm_runtime_get_unless_suspended(struct device *dev)
> > {
> > return atomic_add_unless(&dev->power.usage_count, 1, 0);
> > }
>
> I'd freak out a lot :)
>
> Rafael, isn't there some other better way to resolve this issue without
> resorting to something as "horrid" as the above?
I'm not sure how to solve this at all, because the above isn't going to work
in general in my opinion.
Can anyone please try to explain the problem to me without referring to the
i915 internals?
Rafael
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Sat, 9 Aug 2014, Rafael J. Wysocki wrote:
> > > > Well it works currently. So where do you see the problem?
> > >
> > > Sampling registers from an timer - in particular, we really do not want
> > > to disable runtime pm whilst trying to monitor the impact of runtime pm.
> >
> > In that case you can grab a runtime pm reference iff the device is powered
> > on already. Which won't call anything scary, just amounts to an
> > atomic_add_unless or so, and then drop it again.
> >
> > Unfortunately there doesn't seem to be such a thing around already, so
> > need to add it first. Greg, how much would you freak out if we add
> > something like
> >
> > /**
> > * pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
> > *
> > * Returns true if an rpm ref has been acquire, false otherwise. Can be
> > * called from atomic context to e.g. sample perfomance counters (where we
> > * obviously don't want to disturb system state if everything is off atm).
> > */
> > static inline bool pm_runtime_get_unless_suspended(struct device *dev)
> > {
> > return atomic_add_unless(&dev->power.usage_count, 1, 0);
> > }
>
> I don't think it'll work universally.
>
> That'd need to be synchronized with other stuff done under the spinlock
> and in fact, what you're interested in is runtime_status (and that being
> RPM_ACTIVE) and not just the usage count.
That's right. You'd need to acquire the spinlock, test runtime_status,
do the register sampling if the status is RPM_ACTIVE, and then drop the
spinlock.
I suppose wrapper routines for acquiring and releasing the spinlock
could be added to the runtime-PM API. Something like this:
#define pm_runtime_lock(dev, flags) \
spin_lock_irqsave(&(dev)->power.lock, flags)
#define pm_runtime_unlock(dev, flags) \
spin_unlock_irqrestore(&(dev)->power.lock, flags)
It looks a little silly but it would work.
Alan Stern
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Sat, Aug 9, 2014 at 3:21 AM, Alan Stern wrote:
> On Sat, 9 Aug 2014, Rafael J. Wysocki wrote:
>
>> > > > Well it works currently. So where do you see the problem?
>> > >
>> > > Sampling registers from an timer - in particular, we really do not want
>> > > to disable runtime pm whilst trying to monitor the impact of runtime pm.
>> >
>> > In that case you can grab a runtime pm reference iff the device is powered
>> > on already. Which won't call anything scary, just amounts to an
>> > atomic_add_unless or so, and then drop it again.
>> >
>> > Unfortunately there doesn't seem to be such a thing around already, so
>> > need to add it first. Greg, how much would you freak out if we add
>> > something like
>> >
>> > /**
>> > * pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
>> > *
>> > * Returns true if an rpm ref has been acquire, false otherwise. Can be
>> > * called from atomic context to e.g. sample perfomance counters (where we
>> > * obviously don't want to disturb system state if everything is off atm).
>> > */
>> > static inline bool pm_runtime_get_unless_suspended(struct device *dev)
>> > {
>> > return atomic_add_unless(&dev->power.usage_count, 1, 0);
>> > }
>>
>> I don't think it'll work universally.
>>
>> That'd need to be synchronized with other stuff done under the spinlock
>> and in fact, what you're interested in is runtime_status (and that being
>> RPM_ACTIVE) and not just the usage count.
>
> That's right. You'd need to acquire the spinlock, test runtime_status,
> do the register sampling if the status is RPM_ACTIVE, and then drop the
> spinlock.
>
> I suppose wrapper routines for acquiring and releasing the spinlock
> could be added to the runtime-PM API. Something like this:
>
> #define pm_runtime_lock(dev, flags) \
> spin_lock_irqsave(&(dev)->power.lock, flags)
> #define pm_runtime_unlock(dev, flags) \
> spin_unlock_irqrestore(&(dev)->power.lock, flags)
>
> It looks a little silly but it would work.
Oh right, I've totally ignored all the async resuming/suspending
stuff. Anyway what we want to do is sample a perf monitoring unit on
the gpu from an hrtimer and then expose that as a perf pmu. But we
don't want to wake up the gpu for the sampling or hold a special
reference, since that disturbs the sampling and also tends to upset
the gpu.
Note that those registers are just status indicator registers, so no
counters that will get reset when the device is suspended. For the
later counters (we have them too) we're not sure yet how to handle
them, especially since the amount the gpu saves/restores into its own
context storage depends upon the platform.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 45/53] drm/i915/bdw: Do not call intel_runtime_pm_get() in an interrupt
On Saturday, August 09, 2014 10:53:03 AM Daniel Vetter wrote:
> On Sat, Aug 9, 2014 at 3:21 AM, Alan Stern wrote:
> > On Sat, 9 Aug 2014, Rafael J. Wysocki wrote:
> >
> >> > > > Well it works currently. So where do you see the problem?
> >> > >
> >> > > Sampling registers from an timer - in particular, we really do not want
> >> > > to disable runtime pm whilst trying to monitor the impact of runtime
> >> > > pm.
> >> >
> >> > In that case you can grab a runtime pm reference iff the device is
> >> > powered
> >> > on already. Which won't call anything scary, just amounts to an
> >> > atomic_add_unless or so, and then drop it again.
> >> >
> >> > Unfortunately there doesn't seem to be such a thing around already, so
> >> > need to add it first. Greg, how much would you freak out if we add
> >> > something like
> >> >
> >> > /**
> >> > * pm_runtime_get_unless_suspended - grab a rpm ref if the device is on
> >> > *
> >> > * Returns true if an rpm ref has been acquire, false otherwise. Can be
> >> > * called from atomic context to e.g. sample perfomance counters (where
> >> > we
> >> > * obviously don't want to disturb system state if everything is off
> >> > atm).
> >> > */
> >> > static inline bool pm_runtime_get_unless_suspended(struct device *dev)
> >> > {
> >> > return atomic_add_unless(&dev->power.usage_count, 1, 0);
> >> > }
> >>
> >> I don't think it'll work universally.
> >>
> >> That'd need to be synchronized with other stuff done under the spinlock
> >> and in fact, what you're interested in is runtime_status (and that being
> >> RPM_ACTIVE) and not just the usage count.
> >
> > That's right. You'd need to acquire the spinlock, test runtime_status,
> > do the register sampling if the status is RPM_ACTIVE, and then drop the
> > spinlock.
> >
> > I suppose wrapper routines for acquiring and releasing the spinlock
> > could be added to the runtime-PM API. Something like this:
> >
> > #define pm_runtime_lock(dev, flags) \
> > spin_lock_irqsave(&(dev)->power.lock, flags)
> > #define pm_runtime_unlock(dev, flags) \
> > spin_unlock_irqrestore(&(dev)->power.lock, flags)
> >
> > It looks a little silly but it would work.
>
> Oh right, I've totally ignored all the async resuming/suspending
> stuff. Anyway what we want to do is sample a perf monitoring unit on
> the gpu from an hrtimer and then expose that as a perf pmu. But we
> don't want to wake up the gpu for the sampling or hold a special
> reference, since that disturbs the sampling and also tends to upset
> the gpu.
The way to go is as Alan said: take the spinlock, check the runtime status,
do stuff and release the spinlock.
Rafael
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
