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

Zanoni, Paulo R
2015-11-18T19:27:50Z

Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu:
> The ultimate goal here is to remove the dependency we
> currently have on audio driver power to get PSR working.
> Since with audio driver runtime PM disabled the Hardware tracking
> believes graphics is fully active and prevent PSR Entry, or
> in other words continuously exit PSR.
>
> So, the idea is to transfer the PSR exit responsability
> from the HW tracking to the SW tracking (frontbuffer tracking),
> who is really mature right now.
>
> However with LPSP masked out there might be cases where we could
> miss exit from HW tracking since it can be relying on this,
> like a specific case reported at our mailing list who
> user reported he would miss screen updates if scrolling firefox
> in a Gnome environment when i915 runtimepm was enabled.
>
> So before masking out LPSP again to make us independent from
> the audio driver we need to make sure that all our cases
> are coverred from the frontbuffer tracking perspective,
> where the flush means invalidate and flush.
>
> Without this patch for HSW, BDW and SKL we just do the
> invalidate part when the flush wasn't originated by a page flip
> because we were trusting the HW tracking for the flip case.
>
> So let's rely more on frontbuffer tracking and do the
> invalidation regardless the origin as expected for all platforms.
>
> v2: Improve commit message as suggested by Paulo.
>
> v3: Another attempt to let commit message more clear.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
s/Cc/Reviewed-by/

> 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 e5b3fce..b0e343c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -711,25 +711,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,