Re: [Intel-gfx] [PATCH 5/5] drm/i915: Estimate and update missed vblanks.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel