Re: [PATCH] drm/i915: Wait for vblank after register read

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

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

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