[Intel-gfx] [PATCH 3/7] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Vivi, Rodrigo]

On Mon, 2015-08-24 at 14:29 +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> > Many reasons here:
> > 
> > - Hardware tracking also has hidden corner cases
> 
> Can you please elaborate more on that? I really really really really
> really think we should try as hard as possible to cook some IGT cases
> if something is affecting us :)

Nothing much to be elaborated... Just that case I mentioned with
unrecoverable frozen screen after dpms off that I couldn't reproduce
with igt.

> 
> > - Frontbuffer tracking is mature and reliable now
> > - Our sw exit by unseting bit 31 is really fast and reliable.
> 
> But doesn't it trigger an automatic link retraining?

afaik nothing different from what hw needs to do when entering PSR
already. But if this is not true we need to stop trying SW tracking at
all for all core platforms but also let userspace to control when to
enable disable PSR otherwise it would broken half of the world.


> 
> > 
> > Also frontbuffer tracking flush means invalidate and flush.
> 
> I don't know what are the implications of this in the current 
> context.
> 
> > 
> > So, let's rely more and do the proper meaning of flush for
> > all cases without any workaround.
> 
> I'm really in favor of the idea that if the HW can properly handle 
> the
> flips, we should rely on it, since in a lot of modern desktop
> environments we basically do one flip per frame. Did we study how 
> this
> patch affects the PSR residency on the different cases we care about?

I believe what affects the PSR residency for good is to remove the
100ms delay on re-enable.


> (yes, I know FBC is not relying on the HW for flips, but this is on 
> the
> "optimization" TODO list after we finally merge the bug fixes)
> 
> Due to the benefits of relying on the HW tracking, I think you'll 
> have
> to bring some good arguments to sell this patch to me. But a
> "Testcase:" tag would totally do it :)

I'm not convinced without this patch we can mask again the LPSP,
otherwise that issue Mathew Garret faced with strange scroll on firefox
on Gnome will be seen, but with LPSP we have a higher risk of
unrecoverable screens.

There is no test case for visual feeling. I could reproduce what Mathew
had seen when opened firefox in a gnome and scroolled down and up a big
page... I'm not able to create an IGT that gets this kind of feeling,
sorry...

> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
> >  1 file changed, 3 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 92e2b467..63bbab2 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -719,25 +719,9 @@ void intel_psr_flush(struct drm_device *dev,
> >  	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> >  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> >  
> > -	if (HAS_DDI(dev)) {
> > -		/*
> > -		 * By definition every flush should mean 
> > invalidate 
> > + flush,
> > -		 * however on core platforms let's minimize the
> > -		 * disable/re-enable so we can avoid the 
> > invalidate 
> > when flip
> > -		 * originated the flush.
> > -		 */
> > -		if (frontbuffer_bits && origin != ORIGIN_FLIP)
> > -			intel_psr_exit(dev);
> > -	} else {
> > -		/*
> > -		 * On Valleyview and Cherryview we don't use 
> > hardware tracking
> > -		 * so any plane updates or cursor moves don't 
> > result 
> > in a PSR
> > -		 * invalidating. Which means we need to manually 
> > fake this in
> > -		 * software for all flushes.
> > -		 */
> > -		if (frontbuffer_bits)
> > -			intel_psr_exit(dev);
> > -	}
> > +	/* By definition flush = invalidate + flush */
> > +	if (frontbuffer_bits)
> > +		intel_psr_exit(dev);
> >  
> >  	if (!dev_priv->psr.active && !dev_priv
> > ->psr.busy_frontbuffer_bits)