[Intel-gfx] [PATCH] drm/i915: Wait for vblank after register read

[Dhinakaran Pandiyan]

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 at intel.com> wrote:
> > > > > 
> > > > > On Thu, 2018-04-19 at 17:09 +0300, Jani Nikula wrote:
> > > > > > 
> > > > > > On Wed, 18 Apr 2018, Mika Kahola <mika.kahola at intel.com> 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.

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



> > 
> > 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx