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

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

On Mon, 2015-11-16 at 16:57 -0200, Paulo Zanoni wrote:
> 2015-11-13 22:56 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > The ultimate goal here is to remove the dependency we
> > currently have on audio driver power to get PSR working.
> > 
> > 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.
> > 
> > When we introduced PSR we let LPSP masked allowing us
> > to get PSR independtly from the audio runtime PM. However
> > in one of the attempts to get PSR enabled by default one
> > user reported one specific case where he would miss
> > screen updates if scrolling the firefox in a Gnome
> > environment when i915 runtime pm was enabled. So for
> > this specific case that (I could never create an i-g-t
> > test case) we decided to remove the LPSP mask and
> > let HW tracking taking care of this case. So, we
> > started depending on audio driver again.
> 
> Thanks for the better commit message, but I still think there's a 
> huge
> gap between the paragraph above and the paragraph below. What is 
> still
> not clear to me is: what is the relationship between the LPSP mask
> problem mentioned above and the automatic page flip tracking 
> mentioned
> below? In other words: why not relying on the automatic HW tracking
> solves the bug?

So, or we solve by fully relying on SW tracking or we solve by fully
relying on HW tracking. The main disadvantage on the HW tracking is
that we end up depending on audio driver. Since the SW tracking is more
mature and covering all our use cases on Linux is better to rely more
on the SW and implement the function as expected (i.e. flush =
inactivate + flush.)

> 
> Also, did you do any measurements on how this patch affects PC state
> residency on the affected platforms? I expect to see some delta here.

No, I didn't. Yes, it can affect the PC residency, but it depends a lot
on the use case, the environment, etc. 
Although on the idle scenario that is the one that targets maximum PC
residency we should see no difference.



> 
> The patch itself looks fine. And we can always look into re-adding 
> the
> HW piece after we get the current bugs sorted.
> 
> With those two explained in the commit message:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I will prepare another v.
Thanks!

> 
> > 
> > An alternative solution that makes us indepent and also
> > solve this case is to fully rely on our frontbuffer
> > tracking that is really mature right now.
> > 
> > From the frontbuffer tracking perspective the flush means
> > invalidate and flush. But 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, that is exactly the case with
> > firefox scrolling on gnome.
> > 
> > So let's rely more on frontbuffer tracking and do the
> > invalidation regardless the origin as expected and for
> > all platforms.
> > 
> > v2: Improve commit message as suggested by Paulo.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 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,
> > --
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx