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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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