| Message ID | 1524038216-22142-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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);
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);
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);
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
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.
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
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 >
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 >
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
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 >
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. > > >
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);
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(-)