| Message ID | 20180112215707.3084-5-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 12, 2018 at 01:57:07PM -0800, Dhinakaran Pandiyan wrote: > The frame counter may have got reset between disabling and enabling vblank > interrupts due to DMC putting the hardware to DC5/6 state if PSR was > active. The frame counter also could have stalled if PSR is active in cases > where there is no DMC. The frame counter resetting as a user visible impact > of screen freezes. Use drm_vblank_restore() to compute missed vblanks > in the duration for which vblank interrupts are disabled. There's no need > particularly check if PSR was active in the interrupt disabled duration. > > Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it > from going back again as long as the there are pending interrupts. So, we > don't have to explicity disallow DC5/6 after enabling vblank interrupts > to keep the counter running. > > Let's not apply this to CHV for now, as enabling interrupts does not > prevent the hardware from activating PSR and thereby stalling the counter. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Read this series, and I think compared to all the previous attempts to fix this issue this one here seems the cleanest. Not an expert on the vblank code (Michel or Ville are best for that), but on the series: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Daniel > --- > drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 3517c6548e2c..db3466ec6faa 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) > ilk_enable_display_irq(dev_priv, bit); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > + if (HAS_PSR(dev_priv)) > + drm_vblank_restore(dev, pipe); > + > return 0; > } > > @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > + if (HAS_PSR(dev_priv)) > + drm_vblank_restore(dev, pipe); > + > return 0; > } > > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote: > The frame counter may have got reset between disabling and enabling vblank > interrupts due to DMC putting the hardware to DC5/6 state if PSR was > active. The frame counter also could have stalled if PSR is active in cases > where there is no DMC. The frame counter resetting as a user visible impact > of screen freezes. Use drm_vblank_restore() to compute missed vblanks > in the duration for which vblank interrupts are disabled. There's no need > particularly check if PSR was active in the interrupt disabled duration. > > Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it > from going back again as long as the there are pending interrupts. So, we > don't have to explicity disallow DC5/6 after enabling vblank interrupts > to keep the counter running. > > Let's not apply this to CHV for now, as enabling interrupts does not > prevent the hardware from activating PSR and thereby stalling the counter. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 3517c6548e2c..db3466ec6faa 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) > ilk_enable_display_irq(dev_priv, bit); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > + if (HAS_PSR(dev_priv)) > + drm_vblank_restore(dev, pipe); > + I don't believe we need this one here. pre-gen9 platform has psr but not dmc, so the case where we need to restore the counter doesn't exist. > return 0; > } > > @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > + if (HAS_PSR(dev_priv)) HAS_PSR(dev_priv) && HAS_CSR(dev_priv) maybe? So it gets clear that it is not because PSR that we need to restore the counter, but also don't do it when not needed. > + drm_vblank_restore(dev, pipe); > + > return 0; > } > > -- > 2.11.0 >
On Thu, 2018-01-18 at 23:26 -0800, Rodrigo Vivi wrote: > On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote: > > The frame counter may have got reset between disabling and enabling vblank > > interrupts due to DMC putting the hardware to DC5/6 state if PSR was > > active. The frame counter also could have stalled if PSR is active in cases > > where there is no DMC. The frame counter resetting as a user visible impact > > of screen freezes. Use drm_vblank_restore() to compute missed vblanks > > in the duration for which vblank interrupts are disabled. There's no need > > particularly check if PSR was active in the interrupt disabled duration. > > > > Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it > > from going back again as long as the there are pending interrupts. So, we > > don't have to explicity disallow DC5/6 after enabling vblank interrupts > > to keep the counter running. > > > > Let's not apply this to CHV for now, as enabling interrupts does not > > prevent the hardware from activating PSR and thereby stalling the counter. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 3517c6548e2c..db3466ec6faa 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) > > ilk_enable_display_irq(dev_priv, bit); > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > > > + if (HAS_PSR(dev_priv)) > > + drm_vblank_restore(dev, pipe); > > + > > I don't believe we need this one here. > > pre-gen9 platform has psr but not dmc, so the case > where we need to restore the counter doesn't exist. Even without DMC, counter should be stuck when PSR is active as no frames are generated by the pipe. I am using drm_vblank_restore_count() to take care of that. > > > return 0; > > } > > > > @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > > bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > > > + if (HAS_PSR(dev_priv)) > > HAS_PSR(dev_priv) && HAS_CSR(dev_priv) > maybe? > So it gets clear that it is not because PSR that we need to restore > the counter, but also don't do it when not needed. Same reason as above, let me test this again by disabling DMC. > > > + drm_vblank_restore(dev, pipe); > > + > > return 0; > > } > > > > -- > > 2.11.0 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 19, 2018 at 09:42:14PM +0000, Pandiyan, Dhinakaran wrote: > On Thu, 2018-01-18 at 23:26 -0800, Rodrigo Vivi wrote: > > On Fri, Jan 12, 2018 at 09:57:07PM +0000, Dhinakaran Pandiyan wrote: > > > The frame counter may have got reset between disabling and enabling vblank > > > interrupts due to DMC putting the hardware to DC5/6 state if PSR was > > > active. The frame counter also could have stalled if PSR is active in cases > > > where there is no DMC. The frame counter resetting as a user visible impact > > > of screen freezes. Use drm_vblank_restore() to compute missed vblanks > > > in the duration for which vblank interrupts are disabled. There's no need > > > particularly check if PSR was active in the interrupt disabled duration. > > > > > > Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it > > > from going back again as long as the there are pending interrupts. So, we > > > don't have to explicity disallow DC5/6 after enabling vblank interrupts > > > to keep the counter running. > > > > > > Let's not apply this to CHV for now, as enabling interrupts does not > > > prevent the hardware from activating PSR and thereby stalling the counter. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > > index 3517c6548e2c..db3466ec6faa 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) > > > ilk_enable_display_irq(dev_priv, bit); > > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > > > > > + if (HAS_PSR(dev_priv)) > > > + drm_vblank_restore(dev, pipe); > > > + > > > > I don't believe we need this one here. > > > > pre-gen9 platform has psr but not dmc, so the case > > where we need to restore the counter doesn't exist. > > Even without DMC, counter should be stuck when PSR is active as no > frames are generated by the pipe. I am using drm_vblank_restore_count() > to take care of that. Oh oh! Indeed. Now I remember you had told me this here. Can you please add a comment with this info somewhere so I don't ask the same question again ;) anyways: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > return 0; > > > } > > > > > > @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > > > bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); > > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > > > > > + if (HAS_PSR(dev_priv)) > > > > HAS_PSR(dev_priv) && HAS_CSR(dev_priv) > > maybe? > > So it gets clear that it is not because PSR that we need to restore > > the counter, but also don't do it when not needed. > > Same reason as above, let me test this again by disabling DMC. > > > > > > + drm_vblank_restore(dev, pipe); > > > + > > > return 0; > > > } > > > > > > -- > > > 2.11.0 > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 3517c6548e2c..db3466ec6faa 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2956,6 +2956,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) ilk_enable_display_irq(dev_priv, bit); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + if (HAS_PSR(dev_priv)) + drm_vblank_restore(dev, pipe); + return 0; } @@ -2968,6 +2971,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + if (HAS_PSR(dev_priv)) + drm_vblank_restore(dev, pipe); + return 0; }
The frame counter may have got reset between disabling and enabling vblank interrupts due to DMC putting the hardware to DC5/6 state if PSR was active. The frame counter also could have stalled if PSR is active in cases where there is no DMC. The frame counter resetting as a user visible impact of screen freezes. Use drm_vblank_restore() to compute missed vblanks in the duration for which vblank interrupts are disabled. There's no need particularly check if PSR was active in the interrupt disabled duration. Enabling vblank interrupts wakes up the hardware from DC5/6 and prevents it from going back again as long as the there are pending interrupts. So, we don't have to explicity disallow DC5/6 after enabling vblank interrupts to keep the counter running. Let's not apply this to CHV for now, as enabling interrupts does not prevent the hardware from activating PSR and thereby stalling the counter. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ 1 file changed, 6 insertions(+)