On Mon, Apr 23, 2018 at 01:24:39PM -0700, Dhinakaran Pandiyan wrote: > > > > On Mon, 2018-04-23 at 12:34 -0700, Rodrigo Vivi wrote: > > On Mon, Apr 23, 2018 at 12:21:39PM -0700, Dhinakaran Pandiyan 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. > > > > hmmm... good point... > > > > so we should for now remove all vblank waits there and wait for the TEST_COUNT > > to increase with some "random" sleep timeout... > > > > There is some IPS related code that needs vblanks, git blame didn't tell > if it exactly needs to wait until a vblank interrupt occurs. oh! indeed. luckly IPS is only HSW and BDW. SKL+ is free from that. But from what I rememeber a time based wait there should be ok. Not necessarily an interrupt. But safe if we change the behaviour only when disabling IPS from the sink crc check. > > Second, we'll need a dual implementation, > 1. with crc start and stop programming before entering PSR and after > exit. Read $debugfs/sink_crc_edp > 2. directly read the CRC for static frames when PSR is active. > $debugfs/sink_crc_edp_psr I'm not sure if I followed, but if the idea is to decouple start/stop from the read itself I agree... The problem is just on how to handle the counters since we could end up in situations where what we are reading is not exactly what we want. But anyways I believe we should put the minimal effort there if the idea is to move away from that and also the wish of enabling the CRC feature... > > > > > > > > > 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 > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
