| Message ID | 20161007135032.12825-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 07, 2016 at 02:50:32PM +0100, Chris Wilson wrote: > Whilst the vblank is configured to send an interrupt everytime, we need > to keep the device awake to process those interrupts. If we can enable vblanks the pipe will be active, and thus we can't runtime suspend anyway. Also might_sleep() would be a problem. > > Reported-by: Anssi Hannula <anssi.hannula@bitwise.fi> > References: https://bugs.freedesktop.org/show_bug.cgi?id=97985 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index bd6c8b0eeaef..bda7a27615a6 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2723,6 +2723,8 @@ static int i915_enable_vblank(struct drm_device *dev, unsigned int pipe) > struct drm_i915_private *dev_priv = to_i915(dev); > unsigned long irqflags; > > + intel_runtime_pm_get(dev_priv); > + > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > if (INTEL_INFO(dev)->gen >= 4) > i915_enable_pipestat(dev_priv, pipe, > @@ -2742,6 +2744,8 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) > uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) : > DE_PIPE_VBLANK(pipe); > > + intel_runtime_pm_get(dev_priv); > + > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > ilk_enable_display_irq(dev_priv, bit); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > @@ -2754,6 +2758,8 @@ static int valleyview_enable_vblank(struct drm_device *dev, unsigned int pipe) > struct drm_i915_private *dev_priv = to_i915(dev); > unsigned long irqflags; > > + intel_runtime_pm_get(dev_priv); > + > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > i915_enable_pipestat(dev_priv, pipe, > PIPE_START_VBLANK_INTERRUPT_STATUS); > @@ -2767,6 +2773,8 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > struct drm_i915_private *dev_priv = to_i915(dev); > unsigned long irqflags; > > + intel_runtime_pm_get(dev_priv); > + > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > @@ -2787,6 +2795,8 @@ static void i915_disable_vblank(struct drm_device *dev, unsigned int pipe) > PIPE_VBLANK_INTERRUPT_STATUS | > PIPE_START_VBLANK_INTERRUPT_STATUS); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > + > + intel_runtime_pm_put(dev_priv); > } > > static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe) > @@ -2799,6 +2809,8 @@ static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe) > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > ilk_disable_display_irq(dev_priv, bit); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > + > + intel_runtime_pm_put(dev_priv); > } > > static void valleyview_disable_vblank(struct drm_device *dev, unsigned int pipe) > @@ -2810,6 +2822,8 @@ static void valleyview_disable_vblank(struct drm_device *dev, unsigned int pipe) > i915_disable_pipestat(dev_priv, pipe, > PIPE_START_VBLANK_INTERRUPT_STATUS); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > + > + intel_runtime_pm_put(dev_priv); > } > > static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) > @@ -2820,6 +2834,8 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > bdw_disable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > + > + intel_runtime_pm_put(dev_priv); > } > > static bool > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 07, 2016 at 09:06:07PM +0300, Ville Syrjälä wrote: > On Fri, Oct 07, 2016 at 02:50:32PM +0100, Chris Wilson wrote: > > Whilst the vblank is configured to send an interrupt everytime, we need > > to keep the device awake to process those interrupts. > > If we can enable vblanks the pipe will be active, and thus we can't > runtime suspend anyway. Also might_sleep() would be a problem. Where is the pipe made active? Is it just having any mode is sufficient to disable rpm? Will that always be the case? Just tracing from the drm_wait_vblank ioctl to the mmio access it didn't occur to me that there was a crtc wakeref. Choice is either using something like intel_runtime_pm_get_noresume() or /* CRTC must be kept alive whilst we enable vblanks */ assert_rpm...() And possibly even using the seq check. -Chris
On Fri, Oct 07, 2016 at 07:33:00PM +0100, Chris Wilson wrote: > On Fri, Oct 07, 2016 at 09:06:07PM +0300, Ville Syrjälä wrote: > > On Fri, Oct 07, 2016 at 02:50:32PM +0100, Chris Wilson wrote: > > > Whilst the vblank is configured to send an interrupt everytime, we need > > > to keep the device awake to process those interrupts. > > > > If we can enable vblanks the pipe will be active, and thus we can't > > runtime suspend anyway. Also might_sleep() would be a problem. > > Where is the pipe made active? Is it just having any mode is sufficient > to disable rpm? Will that always be the case? Active pipe holds a power domain reference, which implies an rpm reference as well. > > Just tracing from the drm_wait_vblank ioctl to the mmio access it didn't > occur to me that there was a crtc wakeref. > > Choice is either using something like intel_runtime_pm_get_noresume() or > /* CRTC must be kept alive whilst we enable vblanks */ > assert_rpm...() > > And possibly even using the seq check. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Oct 07, 2016 at 09:44:53PM +0300, Ville Syrjälä wrote: > On Fri, Oct 07, 2016 at 07:33:00PM +0100, Chris Wilson wrote: > > On Fri, Oct 07, 2016 at 09:06:07PM +0300, Ville Syrjälä wrote: > > > On Fri, Oct 07, 2016 at 02:50:32PM +0100, Chris Wilson wrote: > > > > Whilst the vblank is configured to send an interrupt everytime, we need > > > > to keep the device awake to process those interrupts. > > > > > > If we can enable vblanks the pipe will be active, and thus we can't > > > runtime suspend anyway. Also might_sleep() would be a problem. > > > > Where is the pipe made active? Is it just having any mode is sufficient > > to disable rpm? Will that always be the case? > > Active pipe holds a power domain reference, which implies an rpm > reference as well. What's the best way to write /* vblank IRQ requires the powerwell to stay awake */ WARN_ON(!to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe])->active); ?
On Fri, Oct 07, 2016 at 07:57:21PM +0100, Chris Wilson wrote: > On Fri, Oct 07, 2016 at 09:44:53PM +0300, Ville Syrjälä wrote: > > On Fri, Oct 07, 2016 at 07:33:00PM +0100, Chris Wilson wrote: > > > On Fri, Oct 07, 2016 at 09:06:07PM +0300, Ville Syrjälä wrote: > > > > On Fri, Oct 07, 2016 at 02:50:32PM +0100, Chris Wilson wrote: > > > > > Whilst the vblank is configured to send an interrupt everytime, we need > > > > > to keep the device awake to process those interrupts. > > > > > > > > If we can enable vblanks the pipe will be active, and thus we can't > > > > runtime suspend anyway. Also might_sleep() would be a problem. > > > > > > Where is the pipe made active? Is it just having any mode is sufficient > > > to disable rpm? Will that always be the case? > > > > Active pipe holds a power domain reference, which implies an rpm > > reference as well. > > What's the best way to write > > /* vblank IRQ requires the powerwell to stay awake */ > WARN_ON(!to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe])->active); Also, any reason why we shouldn't merge valleyview_enable_vblank() with i915_enable_vblank()? -Chris
On Fri, Oct 07, 2016 at 08:00:35PM +0100, Chris Wilson wrote: > On Fri, Oct 07, 2016 at 07:57:21PM +0100, Chris Wilson wrote: > > On Fri, Oct 07, 2016 at 09:44:53PM +0300, Ville Syrjälä wrote: > > > On Fri, Oct 07, 2016 at 07:33:00PM +0100, Chris Wilson wrote: > > > > On Fri, Oct 07, 2016 at 09:06:07PM +0300, Ville Syrjälä wrote: > > > > > On Fri, Oct 07, 2016 at 02:50:32PM +0100, Chris Wilson wrote: > > > > > > Whilst the vblank is configured to send an interrupt everytime, we need > > > > > > to keep the device awake to process those interrupts. > > > > > > > > > > If we can enable vblanks the pipe will be active, and thus we can't > > > > > runtime suspend anyway. Also might_sleep() would be a problem. > > > > > > > > Where is the pipe made active? Is it just having any mode is sufficient > > > > to disable rpm? Will that always be the case? > > > > > > Active pipe holds a power domain reference, which implies an rpm > > > reference as well. > > > > What's the best way to write > > > > /* vblank IRQ requires the powerwell to stay awake */ > > WARN_ON(!to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe])->active); Probably just that. We have some kind of function to hide the details of the array a bit though, I just never rememer what it's called. And I think I have patches somewhere to convert all of that over to storing intel_crtc * instead of drm_crtc * since that's what everyone wants. > > Also, any reason why we shouldn't merge valleyview_enable_vblank() with > i915_enable_vblank()? No reason AFAICS. Same for disable. Though maybe we should add the gen check to disable to make it symmetric, just to avoid some head scratching. Hmm. Or actually, maybe just split to i915_*_vblank() and i965_*_vblank() instead. Not much point in having the if in there when you can just assign the right function pointer to begin with.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index bd6c8b0eeaef..bda7a27615a6 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2723,6 +2723,8 @@ static int i915_enable_vblank(struct drm_device *dev, unsigned int pipe) struct drm_i915_private *dev_priv = to_i915(dev); unsigned long irqflags; + intel_runtime_pm_get(dev_priv); + spin_lock_irqsave(&dev_priv->irq_lock, irqflags); if (INTEL_INFO(dev)->gen >= 4) i915_enable_pipestat(dev_priv, pipe, @@ -2742,6 +2744,8 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe); + intel_runtime_pm_get(dev_priv); + spin_lock_irqsave(&dev_priv->irq_lock, irqflags); ilk_enable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); @@ -2754,6 +2758,8 @@ static int valleyview_enable_vblank(struct drm_device *dev, unsigned int pipe) struct drm_i915_private *dev_priv = to_i915(dev); unsigned long irqflags; + intel_runtime_pm_get(dev_priv); + spin_lock_irqsave(&dev_priv->irq_lock, irqflags); i915_enable_pipestat(dev_priv, pipe, PIPE_START_VBLANK_INTERRUPT_STATUS); @@ -2767,6 +2773,8 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) struct drm_i915_private *dev_priv = to_i915(dev); unsigned long irqflags; + intel_runtime_pm_get(dev_priv); + spin_lock_irqsave(&dev_priv->irq_lock, irqflags); bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); @@ -2787,6 +2795,8 @@ static void i915_disable_vblank(struct drm_device *dev, unsigned int pipe) PIPE_VBLANK_INTERRUPT_STATUS | PIPE_START_VBLANK_INTERRUPT_STATUS); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + + intel_runtime_pm_put(dev_priv); } static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe) @@ -2799,6 +2809,8 @@ static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe) spin_lock_irqsave(&dev_priv->irq_lock, irqflags); ilk_disable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + + intel_runtime_pm_put(dev_priv); } static void valleyview_disable_vblank(struct drm_device *dev, unsigned int pipe) @@ -2810,6 +2822,8 @@ static void valleyview_disable_vblank(struct drm_device *dev, unsigned int pipe) i915_disable_pipestat(dev_priv, pipe, PIPE_START_VBLANK_INTERRUPT_STATUS); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + + intel_runtime_pm_put(dev_priv); } static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) @@ -2820,6 +2834,8 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) spin_lock_irqsave(&dev_priv->irq_lock, irqflags); bdw_disable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + + intel_runtime_pm_put(dev_priv); } static bool
Whilst the vblank is configured to send an interrupt everytime, we need to keep the device awake to process those interrupts. Reported-by: Anssi Hannula <anssi.hannula@bitwise.fi> References: https://bugs.freedesktop.org/show_bug.cgi?id=97985 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)