Rodrigo Vivi <rodrigo.v...@intel.com> writes:
> 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.v...@intel.com>
>> > wrote:
>> > > Rodrigo Vivi <rodrigo.v...@intel.com> 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
> And then on VLV/CHV front there are even more issues to sync with VBLANK etc
> a platform that probably went to the market without any single unit with PSR
> 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...
Ok. I was wrong. This also explains and fixes some cases here like
Tested-by: Rodrigo Vivi <rodrigo.v...@intel.com>
I'd still want a version without mention to the schedule case at
intel_psr_enable to avoid confusion here. But since I'm now
basing my patches on top of this:
Acked-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> Intel-gfx mailing list
Intel-gfx mailing list