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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Apr 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 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.

The other day I was looking at some aux code, and bpsec says PSR should
be disabled before doing aux transactions. So I also don't understand
how this is supposed to work. (+Imre, what was the conclusion on our aux
code dealing with PSR being enabled?)

BR,
Jani.




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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx