On Sat, Feb 10, 2018 at 09:43:59AM +0000, Chris Wilson wrote:
> Quoting Andy Lutomirski (2018-02-09 17:55:38)
> > On Fri, Feb 9, 2018 at 7:39 AM, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> > > Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> writes:
> > > So, I move the hacked scheduled to 10s and forced psr_activate 10ms
> > > after that and the result is this:
> > >
> > >
> > > [ 11.757909] [drm:intel_psr_enable [i915]] *ERROR* I915-DEBUG:
> > > Scheduling 10s
> > > [ 11.778586] [drm:intel_psr_flush [i915]] *ERROR* I915-DEBUG: Work busy!
> > > ...
> > > a bunch more of Work busy
> > > ...
> > > [ 21.980643] [drm:intel_psr_flush [i915]] *ERROR* I915-DEBUG: Work busy!
> >
> > This like ("Work busy!") is intel_psr_flush() noticing that
> > work_busy() returns true (which it does indeed do when the work is
> > scheduled for the future). But this means that intel_psr_flush()
> > wants to keep PSR off for a little bit (10s with your patch applied)
> > because the screen isn't idle.
> >
> > > [ 21.983060] [drm:intel_psr_work [i915]] *ERROR* I915-DEBUG: Work running
> >
> > And here's intel_psr_work() turning PSR back on 3ms later.
> >
> > So I think you're seeing exactly the bug I described.
Well, not in the way you described though.
As we can see on the log work running get executed only 10 seconds after
intel_psr_enable wanted. None of the thousand flushes happening there will
trigger psr activate before the wanted 10s.
So the hack indeed work as expected.
Given that description I proved that it works as expected and move away.
But since you insisted I step back and re-read your patch like if you had
never mentioned the hack on intel_psr_enable ever, but only focusing
on the regular calls from intel_psr_flush and you are right,
we do have a bug that would affect VLV/CHV here.
In the current code the psr_activate can happen any moment from 0ms to 100ms
after the flush.
This 100ms would just reduce the amount of fast exit-activate cases,
but not eliminate the possibility of enabling before 100ms.
On core platforms it shouldn't be any problem because HW tracking would
take care of waiting the extra idle frames.
This could be a particular problem for VLV/CHV platforms where I believe
we should wait few frames before really activating it back.
So, probably we can go ahead with your patch so we get this covered for
VLV/CHV, but I'd like a version without mentioning the intel_psr_enable
case that is already proven work as expected.
But meanwhile I will check back on the hack and see if we can already
remove that since many things got fixed and improved around eDP link training
and psr itself.
Also I will discuss with DK and check the possibility of a immediate activate
on platforms with HW tracking. Specially now that we are moving towards more
use of HW tracking.
In this case we would reduce this scheduled/timer based solution only for VLV/CHV.
And then on VLV/CHV front there are even more issues to sync with VBLANK etc in
a platform that probably went to the market without any single unit with PSR panels
that was to expensive when those platforms got released.
I even consider totally removing VLV/CHV code completely.
>
> It's also evident in the tests that PSR isn't being disabled as
> often/quick as we need for frontbuffer updates to be seen.
Yeap, it is evident we have many bugs there, but I'm afraid this is not
the answer for the lag.
DK found some promising ones...
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
