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

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

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 :)

> - 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?

> 
> 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?

(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 :)

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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)
>  		schedule_delayed_work(&dev_priv->psr.work,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx