| Message ID | 20170609170258.28201-1-antonio.argenziano@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6/9/2017 10:02 AM, Antonio Argenziano wrote: > Test expects pending batches to be discarded after a reset. That is no > longer the case. Fixed to expect a normal execution. You could expand this to say: after commit 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests"), that is no longer the case. > > Cc: Michel Thierry <michel.thierry@intel.com> > > Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> If we want the test to pass, then it's ok. Someone else may say we need further subtests. On the basis this brings existing tests to the current expectation, Reviewed-by: Michel Thierry <michel.thierry@intel.com> (with the updated commit msg). -Michel > --- > tests/gem_reset_stats.c | 24 +++++++----------------- > 1 file changed, 7 insertions(+), 17 deletions(-) > > diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c > index c4ce4ac2..73afeeb2 100644 > --- a/tests/gem_reset_stats.c > +++ b/tests/gem_reset_stats.c > @@ -239,7 +239,7 @@ static void test_rs(const struct intel_execution_engine *e, > if (i == hang_index) > assert_reset_status(i, fd[i], 0, RS_BATCH_ACTIVE); > if (i > hang_index) > - assert_reset_status(i, fd[i], 0, RS_BATCH_PENDING); > + assert_reset_status(i, fd[i], 0, RS_NO_ERROR); > } > > igt_assert(igt_seconds_elapsed(&ts_injected) <= 30); > @@ -312,10 +312,10 @@ static void test_rs_ctx(const struct intel_execution_engine *e, > RS_BATCH_ACTIVE); > if (i == hang_index && j > hang_context) > assert_reset_status(i, fd[i], ctx[i][j], > - RS_BATCH_PENDING); > + RS_NO_ERROR); > if (i > hang_index) > assert_reset_status(i, fd[i], ctx[i][j], > - RS_BATCH_PENDING); > + RS_NO_ERROR); > } > } > > @@ -330,7 +330,7 @@ static void test_ban(const struct intel_execution_engine *e) > struct local_drm_i915_reset_stats rs_bad, rs_good; > int fd_bad, fd_good; > int ban, retry = 10; > - int active_count = 0, pending_count = 0; > + int active_count = 0; > > fd_bad = drm_open_driver(DRIVER_INTEL); > fd_good = drm_open_driver(DRIVER_INTEL); > @@ -350,9 +350,6 @@ static void test_ban(const struct intel_execution_engine *e) > noop(fd_good, 0, e); > noop(fd_good, 0, e); > > - /* The second hang will count as pending and be discarded */ > - active_count--; > - pending_count += 1; /* inject hang does 1 real exec + 1 dummy */ > while (retry--) { > inject_hang(fd_bad, 0, e, BAN); > active_count++; > @@ -373,12 +370,10 @@ static void test_ban(const struct intel_execution_engine *e) > assert_reset_status(fd_bad, fd_bad, 0, RS_BATCH_ACTIVE); > igt_assert_eq(gem_reset_stats(fd_bad, 0, &rs_bad), 0); > igt_assert_eq(rs_bad.batch_active, active_count); > - igt_assert_eq(rs_bad.batch_pending, pending_count); > > - assert_reset_status(fd_good, fd_good, 0, RS_BATCH_PENDING); > + assert_reset_status(fd_good, fd_good, 0, RS_NO_ERROR); > igt_assert_eq(gem_reset_stats(fd_good, 0, &rs_good), 0); > igt_assert_eq(rs_good.batch_active, 0); > - igt_assert_eq(rs_good.batch_pending, 2); > > close(fd_bad); > close(fd_good); > @@ -389,7 +384,7 @@ static void test_ban_ctx(const struct intel_execution_engine *e) > struct local_drm_i915_reset_stats rs_bad, rs_good; > int fd, ban, retry = 10; > uint32_t ctx_good, ctx_bad; > - int active_count = 0, pending_count = 0; > + int active_count = 0; > > fd = drm_open_driver(DRIVER_INTEL); > > @@ -414,9 +409,6 @@ static void test_ban_ctx(const struct intel_execution_engine *e) > noop(fd, ctx_good, e); > noop(fd, ctx_good, e); > > - /* This second hang will count as pending and be discarded */ > - active_count--; > - pending_count++; > while (retry--) { > inject_hang(fd, ctx_bad, e, BAN); > active_count++; > @@ -437,12 +429,10 @@ static void test_ban_ctx(const struct intel_execution_engine *e) > assert_reset_status(fd, fd, ctx_bad, RS_BATCH_ACTIVE); > igt_assert_eq(gem_reset_stats(fd, ctx_bad, &rs_bad), 0); > igt_assert_eq(rs_bad.batch_active, active_count); > - igt_assert_eq(rs_bad.batch_pending, pending_count); > > - assert_reset_status(fd, fd, ctx_good, RS_BATCH_PENDING); > + assert_reset_status(fd, fd, ctx_good, RS_NO_ERROR); > igt_assert_eq(gem_reset_stats(fd, ctx_good, &rs_good), 0); > igt_assert_eq(rs_good.batch_active, 0); > - igt_assert_eq(rs_good.batch_pending, 2); > > close(fd); > } >
On Fri, Jun 09, 2017 at 10:54:13AM -0700, Michel Thierry wrote: > On 6/9/2017 10:02 AM, Antonio Argenziano wrote: > > Test expects pending batches to be discarded after a reset. That is no > > longer the case. Fixed to expect a normal execution. > > You could expand this to say: > after commit 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete > requests"), that is no longer the case. > > > > > Cc: Michel Thierry <michel.thierry@intel.com> > > > > Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> > > If we want the test to pass, then it's ok. Someone else may say we need > further subtests. > > On the basis this brings existing tests to the current expectation, > Reviewed-by: Michel Thierry <michel.thierry@intel.com> > > (with the updated commit msg). > > -Michel Hey, What is the plan regarding this patch? It has been stale for a while. We can push it with the updated commit message if Antonio is okay with the change. On conditional reviews, it is a good practice, if the original author follows up with the "fixed" commit as a reply or gives us assent on IRC to do the change on our side. Also, from CONTRIBUTING: ----------------------------------------------------------------------------- Please use --subject-prefix="PATCH i-g-t" so that i-g-t patches are easily identified in the massive amount mails on intel-gfx. To ensure this is always done, autogen.sh will run: git config format.subjectprefix "PATCH i-g-t" on its first invocation. ----------------------------------------------------------------------------- Lack of proper prefix breaks filtering / patchwork and makes changes harder to track. The autogen.sh thing is a recent addition. Thanks!
On 20/06/17 04:45, Arkadiusz Hiler wrote: > On Fri, Jun 09, 2017 at 10:54:13AM -0700, Michel Thierry wrote: >> On 6/9/2017 10:02 AM, Antonio Argenziano wrote: >>> Test expects pending batches to be discarded after a reset. That is no >>> longer the case. Fixed to expect a normal execution. >> >> You could expand this to say: >> after commit 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete >> requests"), that is no longer the case. >> >>> >>> Cc: Michel Thierry <michel.thierry@intel.com> >>> >>> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> >> >> If we want the test to pass, then it's ok. Someone else may say we need >> further subtests. >> >> On the basis this brings existing tests to the current expectation, >> Reviewed-by: Michel Thierry <michel.thierry@intel.com> >> >> (with the updated commit msg). >> >> -Michel > > Hey, > > What is the plan regarding this patch? It has been stale for a while. > > We can push it with the updated commit message if Antonio is okay with > the change. > > On conditional reviews, it is a good practice, if the original author > follows up with the "fixed" commit as a reply or gives us assent on IRC > to do the change on our side. Hi Arek, I have resent the patch with the corrected commit message. Michel RB'd it again. Just needs to be merged ;) > > > > Also, from CONTRIBUTING: > ----------------------------------------------------------------------------- > Please use --subject-prefix="PATCH i-g-t" so that i-g-t patches are easily > identified in the massive amount mails on intel-gfx. To ensure this is always > done, autogen.sh will run: > > git config format.subjectprefix "PATCH i-g-t" > > on its first invocation. > ----------------------------------------------------------------------------- Sorry I forgot the prefix, I agree it makes things more readable. Also the new patch doesn't have it. Do you want me to re-push it with the prefix? > > Lack of proper prefix breaks filtering / patchwork and makes changes > harder to track. > > The autogen.sh thing is a recent addition. > > Thanks! >
On Tue, Jun 20, 2017 at 08:01:37AM -0700, Antonio Argenziano wrote: > > > On 20/06/17 04:45, Arkadiusz Hiler wrote: > > On Fri, Jun 09, 2017 at 10:54:13AM -0700, Michel Thierry wrote: > > > On 6/9/2017 10:02 AM, Antonio Argenziano wrote: > > > > Test expects pending batches to be discarded after a reset. That is no > > > > longer the case. Fixed to expect a normal execution. > > > > > > You could expand this to say: > > > after commit 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete > > > requests"), that is no longer the case. > > > > > > > > > > > Cc: Michel Thierry <michel.thierry@intel.com> > > > > > > > > Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> > > > > > > If we want the test to pass, then it's ok. Someone else may say we need > > > further subtests. > > > > > > On the basis this brings existing tests to the current expectation, > > > Reviewed-by: Michel Thierry <michel.thierry@intel.com> > > > > > > (with the updated commit msg). > > > > > > -Michel > > > > Hey, > > > > What is the plan regarding this patch? It has been stale for a while. > > > > We can push it with the updated commit message if Antonio is okay with > > the change. > > > > On conditional reviews, it is a good practice, if the original author > > follows up with the "fixed" commit as a reply or gives us assent on IRC > > to do the change on our side. > > Hi Arek, I have resent the patch with the corrected commit message. Michel > RB'd it again. Just needs to be merged ;) > > Also, from CONTRIBUTING: > > ----------------------------------------------------------------------------- > > Please use --subject-prefix="PATCH i-g-t" so that i-g-t patches are easily > > identified in the massive amount mails on intel-gfx. To ensure this is always > > done, autogen.sh will run: > > > > git config format.subjectprefix "PATCH i-g-t" > > > > on its first invocation. > > ----------------------------------------------------------------------------- > > Sorry I forgot the prefix, I agree it makes things more readable. Also the > new patch doesn't have it. Do you want me to re-push it with the prefix? That is why I have missed it. No need for resending, I'll push it in a minute - just try not forgetting it next time :-) Thanks!
diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c index c4ce4ac2..73afeeb2 100644 --- a/tests/gem_reset_stats.c +++ b/tests/gem_reset_stats.c @@ -239,7 +239,7 @@ static void test_rs(const struct intel_execution_engine *e, if (i == hang_index) assert_reset_status(i, fd[i], 0, RS_BATCH_ACTIVE); if (i > hang_index) - assert_reset_status(i, fd[i], 0, RS_BATCH_PENDING); + assert_reset_status(i, fd[i], 0, RS_NO_ERROR); } igt_assert(igt_seconds_elapsed(&ts_injected) <= 30); @@ -312,10 +312,10 @@ static void test_rs_ctx(const struct intel_execution_engine *e, RS_BATCH_ACTIVE); if (i == hang_index && j > hang_context) assert_reset_status(i, fd[i], ctx[i][j], - RS_BATCH_PENDING); + RS_NO_ERROR); if (i > hang_index) assert_reset_status(i, fd[i], ctx[i][j], - RS_BATCH_PENDING); + RS_NO_ERROR); } } @@ -330,7 +330,7 @@ static void test_ban(const struct intel_execution_engine *e) struct local_drm_i915_reset_stats rs_bad, rs_good; int fd_bad, fd_good; int ban, retry = 10; - int active_count = 0, pending_count = 0; + int active_count = 0; fd_bad = drm_open_driver(DRIVER_INTEL); fd_good = drm_open_driver(DRIVER_INTEL); @@ -350,9 +350,6 @@ static void test_ban(const struct intel_execution_engine *e) noop(fd_good, 0, e); noop(fd_good, 0, e); - /* The second hang will count as pending and be discarded */ - active_count--; - pending_count += 1; /* inject hang does 1 real exec + 1 dummy */ while (retry--) { inject_hang(fd_bad, 0, e, BAN); active_count++; @@ -373,12 +370,10 @@ static void test_ban(const struct intel_execution_engine *e) assert_reset_status(fd_bad, fd_bad, 0, RS_BATCH_ACTIVE); igt_assert_eq(gem_reset_stats(fd_bad, 0, &rs_bad), 0); igt_assert_eq(rs_bad.batch_active, active_count); - igt_assert_eq(rs_bad.batch_pending, pending_count); - assert_reset_status(fd_good, fd_good, 0, RS_BATCH_PENDING); + assert_reset_status(fd_good, fd_good, 0, RS_NO_ERROR); igt_assert_eq(gem_reset_stats(fd_good, 0, &rs_good), 0); igt_assert_eq(rs_good.batch_active, 0); - igt_assert_eq(rs_good.batch_pending, 2); close(fd_bad); close(fd_good); @@ -389,7 +384,7 @@ static void test_ban_ctx(const struct intel_execution_engine *e) struct local_drm_i915_reset_stats rs_bad, rs_good; int fd, ban, retry = 10; uint32_t ctx_good, ctx_bad; - int active_count = 0, pending_count = 0; + int active_count = 0; fd = drm_open_driver(DRIVER_INTEL); @@ -414,9 +409,6 @@ static void test_ban_ctx(const struct intel_execution_engine *e) noop(fd, ctx_good, e); noop(fd, ctx_good, e); - /* This second hang will count as pending and be discarded */ - active_count--; - pending_count++; while (retry--) { inject_hang(fd, ctx_bad, e, BAN); active_count++; @@ -437,12 +429,10 @@ static void test_ban_ctx(const struct intel_execution_engine *e) assert_reset_status(fd, fd, ctx_bad, RS_BATCH_ACTIVE); igt_assert_eq(gem_reset_stats(fd, ctx_bad, &rs_bad), 0); igt_assert_eq(rs_bad.batch_active, active_count); - igt_assert_eq(rs_bad.batch_pending, pending_count); - assert_reset_status(fd, fd, ctx_good, RS_BATCH_PENDING); + assert_reset_status(fd, fd, ctx_good, RS_NO_ERROR); igt_assert_eq(gem_reset_stats(fd, ctx_good, &rs_good), 0); igt_assert_eq(rs_good.batch_active, 0); - igt_assert_eq(rs_good.batch_pending, 2); close(fd); }
Test expects pending batches to be discarded after a reset. That is no longer the case. Fixed to expect a normal execution. Cc: Michel Thierry <michel.thierry@intel.com> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> --- tests/gem_reset_stats.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)