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

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

[Mika Kahola]

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
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4..6eb97fa 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_s
 		return ret;
 
 	do {
-		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
-
 		if (drm_dp_dpcd_readb(&intel_dp->aux,
 				      DP_TEST_SINK_MISC, &buf) < 0) {
 			ret = -EIO;
 			goto stop;
 		}
+
+		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
+
 		count = buf & DP_TEST_COUNT_MASK;
 
 	} while (--attempts && count == 0);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Wait for vblank after register read

[Patchwork]

== Series Details ==

Series: drm/i915: Wait for vblank after register read
URL   : https://patchwork.freedesktop.org/series/41877/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4066 -> Patchwork_8731 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41877/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8731 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-cfl-s3:          PASS -> FAIL (fdo#103481)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (33 -> 31) ==

  Additional (2): fi-glk-j4005 fi-bxt-dsi 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-glk-1 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8731

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8731: e03bb4d9e64828ac5797f6750752437e0a683383 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

e03bb4d9e648 drm/i915: Wait for vblank after register read

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8731/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915: Wait for vblank after register read

[Patchwork]

== Series Details ==

Series: drm/i915: Wait for vblank after register read
URL   : https://patchwork.freedesktop.org/series/41877/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4066_full -> Patchwork_8731_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_8731_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8731_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41877/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8731_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_flip@blocking-absolute-wf_vblank-interruptible:
      shard-apl:          PASS -> FAIL

    
    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-dirty-render:
      shard-kbl:          SKIP -> PASS +1

    
== Known issues ==

  Here are the changes found in Patchwork_8731_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_setmode@basic:
      shard-hsw:          PASS -> FAIL (fdo#99912)

    igt@kms_vblank@pipe-b-accuracy-idle:
      shard-hsw:          PASS -> FAIL (fdo#102583)

    igt@pm_rpm@universal-planes:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602)

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-blocking-wf-vblank:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_sysfs_edid_timing:
      shard-apl:          WARN (fdo#100047) -> PASS

    igt@prime_self_import@basic-with_fd_dup:
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (9 -> 4) ==

  Missing    (5): shard-glk8 shard-glk6 shard-glk7 shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4066 -> Patchwork_8731

  CI_DRM_4066: e1fbca4821d0700551df233285a5c28db09fd0f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4441: 83ba5b7d3bde48b383df41792fc9c955a5a23bdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8731: e03bb4d9e64828ac5797f6750752437e0a683383 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4441: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8731/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lofstedt, Marta]

For the PW results: 
https://patchwork.freedesktop.org/series/41877/

it didn't fix the CRC mismatch on:
 https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8731/shard-snb6/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html

but that test has always failed on SNB: 
http://gfx-ci.fi.intel.com/cibuglog/?action_failures_history=-1&failures_test=igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw&failures_machine=shard-snb

so I don't think you brake anything with this patch.

Mika, do you know if waiting for the extra vblank was done with some purpose?


> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Mika Kahola
> Sent: Wednesday, April 18, 2018 10:57 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH] drm/i915: Wait for vblank after register read
> 
> 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
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index 62f82c4..6eb97fa 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_s
>  		return ret;
> 
>  	do {
> -		intel_wait_for_vblank(dev_priv, intel_crtc-
> >pipe);
> -
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> 
> DP_TEST_SINK_MISC, &buf) < 0) {
>  			ret = -EIO;
>  			goto stop;
>  		}
> +
> +		intel_wait_for_vblank(dev_priv, intel_crtc-
> >pipe);
> +
>  		count = buf & DP_TEST_COUNT_MASK;
> 
>  	} while (--attempts && count == 0);
> --
> 2.7.4
> 
> _______________________________________________
> 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

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

[Mika Kahola]

On Thu, 2018-04-19 at 09:11 +0300, Lofstedt, Marta wrote:
> For the PW results: 
> https://patchwork.freedesktop.org/series/41877/
> 
> it didn't fix the CRC mismatch on:
>  https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8731/shard-
> snb6/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
This was expected that removing one vblank doesn't fix have an effect
on crc mismatches. Theres something more in this issue.

> 
> but that test has always failed on SNB: 
> http://gfx-ci.fi.intel.com/cibuglog/?action_failures_history=-1&failu
> res_test=igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-
> multidraw&failures_machine=shard-snb
> 
> so I don't think you brake anything with this patch.
> 
> Mika, do you know if waiting for the extra vblank was done with some
> purpose?
That I don't know if it was intentional. I'll try to find out why it
was needed.

> 
> 
> > 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> > Behalf
> > Of Mika Kahola
> > Sent: Wednesday, April 18, 2018 10:57 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH] drm/i915: Wait for vblank after
> > register read
> > 
> > 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
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index 62f82c4..6eb97fa 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp
> > *intel_dp,
> > struct intel_crtc_state *crtc_s
> >  		return ret;
> > 
> >  	do {
> > -		intel_wait_for_vblank(dev_priv, intel_crtc-
> > > 
> > > pipe);
> > -
> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > 
> > DP_TEST_SINK_MISC, &buf) < 0) {
> >  			ret = -EIO;
> >  			goto stop;
> >  		}
> > +
> > +		intel_wait_for_vblank(dev_priv, intel_crtc-
> > > 
> > > pipe);
> > +
> >  		count = buf & DP_TEST_COUNT_MASK;
> > 
> >  	} while (--attempts && count == 0);
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Thu, Apr 19, 2018 at 10:03:05AM +0300, Mika Kahola wrote:
> On Thu, 2018-04-19 at 09:11 +0300, Lofstedt, Marta wrote:
> > For the PW results: 
> > https://patchwork.freedesktop.org/series/41877/
> > 
> > it didn't fix the CRC mismatch on:
> >  https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8731/shard-
> > snb6/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
> This was expected that removing one vblank doesn't fix have an effect
> on crc mismatches. Theres something more in this issue.
> 
> > 
> > but that test has always failed on SNB: 
> > http://gfx-ci.fi.intel.com/cibuglog/?action_failures_history=-1&failu
> > res_test=igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-
> > multidraw&failures_machine=shard-snb
> > 
> > so I don't think you brake anything with this patch.
> > 
> > Mika, do you know if waiting for the extra vblank was done with some
> > purpose?
> That I don't know if it was intentional. I'll try to find out why it
> was needed.

sink CRC calculation starts on the next active frame and takes a full
frame to finish the calculation. So 2 vblanks before the result is ready.

I don't believe we should move it for after reading it.

But it is a fact that this sink crc was always only a headache.
If we manage to move PSR tests to use the interrupts and status bits only
than we will be able to kill this sink crc code entirely....

> 
> > 
> > 
> > > 
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> > > Behalf
> > > Of Mika Kahola
> > > Sent: Wednesday, April 18, 2018 10:57 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [Intel-gfx] [PATCH] drm/i915: Wait for vblank after
> > > register read
> > > 
> > > 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
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c index 62f82c4..6eb97fa 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp
> > > *intel_dp,
> > > struct intel_crtc_state *crtc_s
> > >  		return ret;
> > > 
> > >  	do {
> > > -		intel_wait_for_vblank(dev_priv, intel_crtc-
> > > > 
> > > > pipe);
> > > -
> > >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > 
> > > DP_TEST_SINK_MISC, &buf) < 0) {
> > >  			ret = -EIO;
> > >  			goto stop;
> > >  		}
> > > +
> > > +		intel_wait_for_vblank(dev_priv, intel_crtc-
> > > > 
> > > > pipe);
> > > +
> > >  		count = buf & DP_TEST_COUNT_MASK;
> > > 
> > >  	} while (--attempts && count == 0);
> > > --
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -- 
> Mika Kahola - Intel OTC
> 
> _______________________________________________
> 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

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

[Dhinakaran Pandiyan]

On Fri, 2018-04-20 at 11:15 -0700, Rodrigo Vivi wrote:
> On Thu, Apr 19, 2018 at 10:03:05AM +0300, Mika Kahola wrote:
> > On Thu, 2018-04-19 at 09:11 +0300, Lofstedt, Marta wrote:
> > > For the PW results: 
> > > https://patchwork.freedesktop.org/series/41877/
> > > 
> > > it didn't fix the CRC mismatch on:
> > >  https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8731/shard-
> > > snb6/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
> > This was expected that removing one vblank doesn't fix have an effect
> > on crc mismatches. Theres something more in this issue.
> > 
> > > 
> > > but that test has always failed on SNB: 
> > > http://gfx-ci.fi.intel.com/cibuglog/?action_failures_history=-1&failu
> > > res_test=igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-
> > > multidraw&failures_machine=shard-snb
> > > 
> > > so I don't think you brake anything with this patch.
> > > 
> > > Mika, do you know if waiting for the extra vblank was done with some
> > > purpose?
> > That I don't know if it was intentional. I'll try to find out why it
> > was needed.
> 
> sink CRC calculation starts on the next active frame and takes a full
> frame to finish the calculation. So 2 vblanks before the result is ready.
> 
> I don't believe we should move it for after reading it.
> 
> But it is a fact that this sink crc was always only a headache.
> If we manage to move PSR tests to use the interrupts and status bits only
> than we will be able to kill this sink crc code entirely....
Yup.

Patch has nothing to do with the bug that it is trying to fix. The test
doesn't read sink-crc's, so I don't think even a References tag is
appropriate here.



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mika Kahola]

On Fri, 2018-04-20 at 13:56 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-04-20 at 11:15 -0700, Rodrigo Vivi wrote:
> > 
> > On Thu, Apr 19, 2018 at 10:03:05AM +0300, Mika Kahola wrote:
> > > 
> > > On Thu, 2018-04-19 at 09:11 +0300, Lofstedt, Marta wrote:
> > > > 
> > > > For the PW results: 
> > > > https://patchwork.freedesktop.org/series/41877/
> > > > 
> > > > it didn't fix the CRC mismatch on:
> > > >  https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8731/shard-
> > > > snb6/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.ht
> > > > ml
> > > This was expected that removing one vblank doesn't fix have an
> > > effect
> > > on crc mismatches. Theres something more in this issue.
> > > 
> > > > 
> > > > 
> > > > but that test has always failed on SNB: 
> > > > http://gfx-ci.fi.intel.com/cibuglog/?action_failures_history=-1
> > > > &failu
> > > > res_test=igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-
> > > > multidraw&failures_machine=shard-snb
> > > > 
> > > > so I don't think you brake anything with this patch.
> > > > 
> > > > Mika, do you know if waiting for the extra vblank was done with
> > > > some
> > > > purpose?
> > > That I don't know if it was intentional. I'll try to find out why
> > > it
> > > was needed.
> > sink CRC calculation starts on the next active frame and takes a
> > full
> > frame to finish the calculation. So 2 vblanks before the result is
> > ready.
> > 
> > I don't believe we should move it for after reading it.
> > 
> > But it is a fact that this sink crc was always only a headache.
> > If we manage to move PSR tests to use the interrupts and status
> > bits only
> > than we will be able to kill this sink crc code entirely....
> Yup.
> 
> Patch has nothing to do with the bug that it is trying to fix. The
> test
> doesn't read sink-crc's, so I don't think even a References tag is
> appropriate here.
Ok. I don't mind dropping this patch if you guys feel that this has no
relevance. As I said earlier, this is something that caught my eye when
looking into this CRC bug that we have.

> 
> 
> 
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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?

BR,
Jani.


> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4..6eb97fa 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_s
>  		return ret;
>  
>  	do {
> -		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> -
>  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>  				      DP_TEST_SINK_MISC, &buf) < 0) {
>  			ret = -EIO;
>  			goto stop;
>  		}
> +
> +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> +
>  		count = buf & DP_TEST_COUNT_MASK;
>  
>  	} while (--attempts && count == 0);

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

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

[Mika Kahola]

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. 

Was there a reason why we need to wait two vblanks here before running
the loop?

> BR,
> Jani.
> 
> 
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 62f82c4..6eb97fa 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp
> > *intel_dp, struct intel_crtc_state *crtc_s
> >  		return ret;
> >  
> >  	do {
> > -		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > -
> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> >  				      DP_TEST_SINK_MISC, &buf) <
> > 0) {
> >  			ret = -EIO;
> >  			goto stop;
> >  		}
> > +
> > +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > +
> >  		count = buf & DP_TEST_COUNT_MASK;
> >  
> >  	} while (--attempts && count == 0);
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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>

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

BR,
Jani.


>
>> BR,
>> Jani.
>> 
>> 
>> > 
>> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > b/drivers/gpu/drm/i915/intel_dp.c
>> > index 62f82c4..6eb97fa 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp
>> > *intel_dp, struct intel_crtc_state *crtc_s
>> >  		return ret;
>> >  
>> >  	do {
>> > -		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
>> > -
>> >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
>> >  				      DP_TEST_SINK_MISC, &buf) <
>> > 0) {
>> >  			ret = -EIO;
>> >  			goto stop;
>> >  		}
>> > +
>> > +		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
>> > +
>> >  		count = buf & DP_TEST_COUNT_MASK;
>> >  
>> >  	} while (--attempts && count == 0);

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

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

[Mika Kahola]

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?

Cheers,
Mika

> 
> BR,
> Jani.
> 
> 
> > 
> > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > > 
> > > > 
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 62f82c4..6eb97fa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3972,13 +3972,14 @@ int intel_dp_sink_crc(struct intel_dp
> > > > *intel_dp, struct intel_crtc_state *crtc_s
> > > >  		return ret;
> > > >  
> > > >  	do {
> > > > -		intel_wait_for_vblank(dev_priv, intel_crtc-
> > > > >pipe);
> > > > -
> > > >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > > >  				      DP_TEST_SINK_MISC, &buf)
> > > > <
> > > > 0) {
> > > >  			ret = -EIO;
> > > >  			goto stop;
> > > >  		}
> > > > +
> > > > +		intel_wait_for_vblank(dev_priv, intel_crtc-
> > > > >pipe);
> > > > +
> > > >  		count = buf & DP_TEST_COUNT_MASK;
> > > >  
> > > >  	} while (--attempts && count == 0);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

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

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

[Rodrigo Vivi]

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

Re: [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@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.

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

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

[Rodrigo Vivi]

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

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

[Jani Nikula]

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