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

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Wait for vblank after register read
Date: Mon, 23 Apr 2018 12:34:54 -0700	[thread overview]
Message-ID: <20180423193454.GU1310@intel.com> (raw)
In-Reply-To: <1524511299.24522.21.camel@dk-H97M-D3H>

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@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.

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 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