* [PATCH] drm/i915: Wait for vblank after register read @ 2018-04-18 7:56 Mika Kahola 2018-04-18 14:32 ` ✓ Fi.CI.BAT: success for " Patchwork ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Mika Kahola @ 2018-04-18 7:56 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Wait for vblank after register read 2018-04-18 7:56 [PATCH] drm/i915: Wait for vblank after register read Mika Kahola @ 2018-04-18 14:32 ` Patchwork 2018-04-18 18:00 ` ✗ Fi.CI.IGT: failure " Patchwork ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Patchwork @ 2018-04-18 14:32 UTC (permalink / raw) To: Mika Kahola; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915: Wait for vblank after register read 2018-04-18 7:56 [PATCH] drm/i915: Wait for vblank after register read Mika Kahola 2018-04-18 14:32 ` ✓ Fi.CI.BAT: success for " Patchwork @ 2018-04-18 18:00 ` Patchwork 2018-04-19 6:11 ` [PATCH] " Lofstedt, Marta 2018-04-19 14:09 ` Jani Nikula 3 siblings, 0 replies; 17+ messages in thread From: Patchwork @ 2018-04-18 18:00 UTC (permalink / raw) To: Mika Kahola; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-18 7:56 [PATCH] drm/i915: Wait for vblank after register read Mika Kahola 2018-04-18 14:32 ` ✓ Fi.CI.BAT: success for " Patchwork 2018-04-18 18:00 ` ✗ Fi.CI.IGT: failure " Patchwork @ 2018-04-19 6:11 ` Lofstedt, Marta 2018-04-19 7:03 ` Mika Kahola 2018-04-19 14:09 ` Jani Nikula 3 siblings, 1 reply; 17+ messages in thread From: Lofstedt, Marta @ 2018-04-19 6:11 UTC (permalink / raw) To: Kahola, Mika, intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-19 6:11 ` [PATCH] " Lofstedt, Marta @ 2018-04-19 7:03 ` Mika Kahola 2018-04-20 18:15 ` Rodrigo Vivi 0 siblings, 1 reply; 17+ messages in thread From: Mika Kahola @ 2018-04-19 7:03 UTC (permalink / raw) To: Lofstedt, Marta, intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-19 7:03 ` Mika Kahola @ 2018-04-20 18:15 ` Rodrigo Vivi 2018-04-20 20:56 ` Dhinakaran Pandiyan 0 siblings, 1 reply; 17+ messages in thread From: Rodrigo Vivi @ 2018-04-20 18:15 UTC (permalink / raw) To: Mika Kahola; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-20 18:15 ` Rodrigo Vivi @ 2018-04-20 20:56 ` Dhinakaran Pandiyan 2018-04-24 8:14 ` Mika Kahola 0 siblings, 1 reply; 17+ messages in thread From: Dhinakaran Pandiyan @ 2018-04-20 20:56 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-20 20:56 ` Dhinakaran Pandiyan @ 2018-04-24 8:14 ` Mika Kahola 0 siblings, 0 replies; 17+ messages in thread From: Mika Kahola @ 2018-04-24 8:14 UTC (permalink / raw) To: dhinakaran.pandiyan, Rodrigo Vivi; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-18 7:56 [PATCH] drm/i915: Wait for vblank after register read Mika Kahola ` (2 preceding siblings ...) 2018-04-19 6:11 ` [PATCH] " Lofstedt, Marta @ 2018-04-19 14:09 ` Jani Nikula 2018-04-20 6:42 ` Mika Kahola 3 siblings, 1 reply; 17+ messages in thread From: Jani Nikula @ 2018-04-19 14:09 UTC (permalink / raw) To: Mika Kahola, intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-19 14:09 ` Jani Nikula @ 2018-04-20 6:42 ` Mika Kahola 2018-04-20 8:22 ` Jani Nikula 0 siblings, 1 reply; 17+ messages in thread From: Mika Kahola @ 2018-04-20 6:42 UTC (permalink / raw) To: Jani Nikula, intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-20 6:42 ` Mika Kahola @ 2018-04-20 8:22 ` Jani Nikula 2018-04-20 11:15 ` Mika Kahola 0 siblings, 1 reply; 17+ messages in thread From: Jani Nikula @ 2018-04-20 8:22 UTC (permalink / raw) To: mika.kahola, intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-20 8:22 ` Jani Nikula @ 2018-04-20 11:15 ` Mika Kahola 2018-04-23 19:21 ` Dhinakaran Pandiyan 0 siblings, 1 reply; 17+ messages in thread From: Mika Kahola @ 2018-04-20 11:15 UTC (permalink / raw) To: Jani Nikula, intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-20 11:15 ` Mika Kahola @ 2018-04-23 19:21 ` Dhinakaran Pandiyan 2018-04-23 19:34 ` Rodrigo Vivi 2018-04-24 7:41 ` Jani Nikula 0 siblings, 2 replies; 17+ messages in thread From: Dhinakaran Pandiyan @ 2018-04-23 19:21 UTC (permalink / raw) To: mika.kahola; +Cc: intel-gfx, Rodrigo Vivi 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-23 19:21 ` Dhinakaran Pandiyan @ 2018-04-23 19:34 ` Rodrigo Vivi 2018-04-23 20:24 ` Dhinakaran Pandiyan 2018-04-24 7:41 ` Jani Nikula 1 sibling, 1 reply; 17+ messages in thread From: Rodrigo Vivi @ 2018-04-23 19:34 UTC (permalink / raw) To: Dhinakaran Pandiyan; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-23 19:34 ` Rodrigo Vivi @ 2018-04-23 20:24 ` Dhinakaran Pandiyan 2018-04-23 20:17 ` Rodrigo Vivi 0 siblings, 1 reply; 17+ messages in thread From: Dhinakaran Pandiyan @ 2018-04-23 20:24 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-23 20:24 ` Dhinakaran Pandiyan @ 2018-04-23 20:17 ` Rodrigo Vivi 0 siblings, 0 replies; 17+ messages in thread From: Rodrigo Vivi @ 2018-04-23 20:17 UTC (permalink / raw) To: Dhinakaran Pandiyan; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Wait for vblank after register read 2018-04-23 19:21 ` Dhinakaran Pandiyan 2018-04-23 19:34 ` Rodrigo Vivi @ 2018-04-24 7:41 ` Jani Nikula 1 sibling, 0 replies; 17+ messages in thread From: Jani Nikula @ 2018-04-24 7:41 UTC (permalink / raw) To: dhinakaran.pandiyan, mika.kahola; +Cc: imre.deak, intel-gfx, Rodrigo Vivi 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-04-24 8:14 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-18 7:56 [PATCH] drm/i915: Wait for vblank after register read Mika Kahola 2018-04-18 14:32 ` ✓ Fi.CI.BAT: success for " Patchwork 2018-04-18 18:00 ` ✗ Fi.CI.IGT: failure " Patchwork 2018-04-19 6:11 ` [PATCH] " Lofstedt, Marta 2018-04-19 7:03 ` Mika Kahola 2018-04-20 18:15 ` Rodrigo Vivi 2018-04-20 20:56 ` Dhinakaran Pandiyan 2018-04-24 8:14 ` Mika Kahola 2018-04-19 14:09 ` Jani Nikula 2018-04-20 6:42 ` Mika Kahola 2018-04-20 8:22 ` Jani Nikula 2018-04-20 11:15 ` Mika Kahola 2018-04-23 19:21 ` Dhinakaran Pandiyan 2018-04-23 19:34 ` Rodrigo Vivi 2018-04-23 20:24 ` Dhinakaran Pandiyan 2018-04-23 20:17 ` Rodrigo Vivi 2018-04-24 7:41 ` Jani Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.