On Mon, 23 Apr 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> wrote: > On Fri, 2018-04-20 at 14:15 +0300, Mika Kahola wrote: >> On Fri, 2018-04-20 at 11:22 +0300, Jani Nikula wrote: >> > On Fri, 20 Apr 2018, Mika Kahola <mika.kahola@xxxxxxxxx> wrote: >> > > >> > > On Thu, 2018-04-19 at 17:09 +0300, Jani Nikula wrote: >> > > > >> > > > On Wed, 18 Apr 2018, Mika Kahola <mika.kahola@xxxxxxxxx> wrote: >> > > > > >> > > > > >> > > > > When reading out CRC's we wait for a vblank on >> > > > > intel_dp_sink_crc_start() >> > > > > function. When we start reading out CRC's in >> > > > > intel_dp_sink_crc() >> > > > > loop we >> > > > > first wait for a vblank yielding that all in all we end up >> > > > > waiting >> > > > > two >> > > > > vblanks on the first iteration round. Therefore, let's move the >> > > > > intel_wait_for_vblank() as the last routine that we do in an >> > > > > iteration loop >> > > > > in intel_dp_sink_crc(). >> > > > > >> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103166 >> > > > Umm, do the CI failures in the bug really use sink crc, or are >> > > > they >> > > > rather about pipe crc? >> > > > >> > > The bug is more on pipe crc. This just caught my attention while I >> > > was >> > > looking into these bugs. >> > I think the practice we've adopted is, >> > >> > Bugzilla: <bug that this patch should fix> >> > >> > References: <bug or something else that this patch is related to> >> Got it :) I try to remember this notation. >> >> > >> > > >> > > Was there a reason why we need to wait two vblanks here before >> > > running >> > > the loop? >> > I can't remember by heart. I'm not sure if it would make more sense >> > to >> > remove the vblank wait from intel_dp_sink_crc_start() instead. Even >> > with >> > your patch, there'll still be an extra vblank wait, you just move it >> > to >> > a different place. >> We could remove vblank wait form intel_dp_sink_crc_start(). Maybe that >> would be more logical place for the removal. As CI runs pointed out >> this patch didn't fix the actual bug so should I drop this change or >> should we still try optimize the code a bit? >> > > I looked at this code in more detail, there is a big problem here. > > The implementation generously uses vblank waits that end up triggering > PSR exits. This in turn means we never read crc's when PSR is active. I > am not surprised anymore the tests were not reliable. We should nuke > this whole thing or use delays in place of vblank waits. This patch is > not what we need. The other day I was looking at some aux code, and bpsec says PSR should be disabled before doing aux transactions. So I also don't understand how this is supposed to work. (+Imre, what was the conclusion on our aux code dealing with PSR being enabled?) BR, Jani. > > There is also the assumption of starting and stopping crc calculation. > Careful reading of the spec shows they are not required for crc > calculation for PSR idle frames. We need to put more thought into fixing > this. > > > -DK > -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
