Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
Hello,
I don't know how we usually do in such cases.
Could I just restore back the 'review-by' tags here due to trivial changes
or this patch should pass review process once again?
Thanks,
Andrii.
On Mon, Nov 5, 2018 at 10:42 AM Jordan Justen
wrote:
> On 2018-11-05 00:11:52, andrey simiklit wrote:
> > Hello,
> >
> > I tested this patch few times and have the following results:
> >
> https://mesa-ci.01.org/global_logic/builds/38/group/63a9f0ea7bb98050796b649e85481845
> >
> https://mesa-ci.01.org/global_logic/builds/41/group/63a9f0ea7bb98050796b649e85481845
> >
> https://mesa-ci.01.org/global_logic/builds/43/group/63a9f0ea7bb98050796b649e85481845
> >
> > All test runs are finished with the following issue on G965:
> >
> > https://mesa-ci.01.org/global_logic/builds/38/results/46496330
> > ext_framebuffer_multisample-accuracy all_samples srgb small depthstencil
> > linear -auto -fbo
> >
> > ERROR: This test passed when it expected failure
> >
> > Note: Also I checked latest mesa master few times and there was no
> > regression:
> >
> >
> https://mesa-ci.01.org/global_logic/builds/42/group/63a9f0ea7bb98050796b649e85481845
> >
>
> It also looks like the tex3d-maxsize test is back to failing after a
> few seconds on g965, rather than running forever. (So, #108630 is
> fixed by this version.)
>
> https://mesa-ci.01.org/global_logic/builds/38/results/46492597
>
> -Jordan
>
> >
> > On Mon, Nov 5, 2018 at 9:48 AM wrote:
> >
> > > From: Andrii Simiklit
> > >
> > > There's no point reverting to the last saved point if that save point
> is
> > > the empty batch, we will just repeat ourselves.
> > >
> > > v2: Merge with new commits, changes was minimized, added the 'fixes'
> tag
> > > v3: Added in to patch series
> > > v4: Fixed the regression which was introduced by this patch
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
> > > Reported-by: Mark Janes
> > > The solution provided by: Jordan Justen >
> > >
> > > CC: Chris Wilson
> > > Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
> > > the batchbuffer state."
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
> > > Signed-off-by: Andrii Simiklit
> > > ---
> > > src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-
> > > src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-
> > > src/mesa/drivers/dri/i965/genX_blorp_exec.c | 1 +
> > > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> > > src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> > > 5 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> > > b/src/mesa/drivers/dri/i965/brw_compute.c
> > > index de08fc3ac1..5c8e3a5d4d 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_compute.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_compute.c
> > > @@ -167,7 +167,7 @@ static void
> > > brw_dispatch_compute_common(struct gl_context *ctx)
> > > {
> > > struct brw_context *brw = brw_context(ctx);
> > > - bool fail_next = false;
> > > + bool fail_next;
> > >
> > > if (!_mesa_check_conditional_render(ctx))
> > >return;
> > > @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
> > > intel_batchbuffer_require_space(brw, 600);
> > > brw_require_statebuffer_space(brw, 2500);
> > > intel_batchbuffer_save_state(brw);
> > > + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> > >
> > > retry:
> > > brw->batch.no_wrap = true;
> > > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> > > b/src/mesa/drivers/dri/i965/brw_draw.c
> > > index 8536c04010..19ee3962d7 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > > @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> > > {
> > > struct brw_context *brw = brw_context(ctx);
> > > const struct gen_device_info *devinfo = >screen->devinfo;
> > > - bool fail_next = false;
> > > + bool fail_next;
> > >
> > > /* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have
> > > * atoms that happen on every draw call.
> > > @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> > > intel_batchbuffer_require_space(brw, 1500);
> > > brw_require_statebuffer_space(brw, 2400);
> > > intel_batchbuffer_save_state(brw);
> > > + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> > >
> > > if (brw->num_instances != prim->num_instances ||
> > > brw->basevertex != prim->basevertex ||
> > > diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > index 34bfcad03e..a62b88e166 100644
> > > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > @@ -309,6 +309,7 @@ retry:
> > > intel_batchbuffer_require_space(brw, 1400);
> > > brw_require_statebuffer_space(brw, 600);
> > >
Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
Hello,
Good news, thanks :-)
One more thanks for clarification about Intel CI workflow.
Thanks,
Andrii.
On Mon, Nov 5, 2018 at 6:31 PM Mark Janes wrote:
> andrey simiklit writes:
>
> > Hello,
> >
> > I tested this patch few times and have the following results:
> >
> https://mesa-ci.01.org/global_logic/builds/38/group/63a9f0ea7bb98050796b649e85481845
> >
> https://mesa-ci.01.org/global_logic/builds/41/group/63a9f0ea7bb98050796b649e85481845
> >
> https://mesa-ci.01.org/global_logic/builds/43/group/63a9f0ea7bb98050796b649e85481845
> >
> > All test runs are finished with the following issue on G965:
> >
> > https://mesa-ci.01.org/global_logic/builds/38/results/46496330
> > ext_framebuffer_multisample-accuracy all_samples srgb small depthstencil
> > linear -auto -fbo
> >
> > ERROR: This test passed when it expected failure
>
> If you test the exact same revision twice in i965 CI, it will simply
> publish the results from the previous run. Often, a single machine
> fails in a run, and we send the build a second time to re-test only that
> machine (all other results are re-used for faster results). We re-use
> results based the commits of the source projects.
>
> Instead of alternating two commits through the CI, you should trivially
> amend the commit message of the patch you want to test, so it gets a
> different sha. This will cause a complete re-test of the patch.
>
> For this test, it may be that you are looking at an intermittent result.
> However, it is never a problem to unexpectedly fix a test.
>
> > Note: Also I checked latest mesa master few times and there was no
> > regression:
> >
> >
> https://mesa-ci.01.org/global_logic/builds/42/group/63a9f0ea7bb98050796b649e85481845
> >
> > Regards,
> > Andrii.
> >
> > On Mon, Nov 5, 2018 at 9:48 AM wrote:
> >
> >> From: Andrii Simiklit
> >>
> >> There's no point reverting to the last saved point if that save point is
> >> the empty batch, we will just repeat ourselves.
> >>
> >> v2: Merge with new commits, changes was minimized, added the 'fixes' tag
> >> v3: Added in to patch series
> >> v4: Fixed the regression which was introduced by this patch
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
> >> Reported-by: Mark Janes
> >> The solution provided by: Jordan Justen
> >>
> >> CC: Chris Wilson
> >> Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
> >> the batchbuffer state."
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
> >> Signed-off-by: Andrii Simiklit
> >> ---
> >> src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-
> >> src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-
> >> src/mesa/drivers/dri/i965/genX_blorp_exec.c | 1 +
> >> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> >> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> >> 5 files changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> >> b/src/mesa/drivers/dri/i965/brw_compute.c
> >> index de08fc3ac1..5c8e3a5d4d 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_compute.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_compute.c
> >> @@ -167,7 +167,7 @@ static void
> >> brw_dispatch_compute_common(struct gl_context *ctx)
> >> {
> >> struct brw_context *brw = brw_context(ctx);
> >> - bool fail_next = false;
> >> + bool fail_next;
> >>
> >> if (!_mesa_check_conditional_render(ctx))
> >>return;
> >> @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
> >> intel_batchbuffer_require_space(brw, 600);
> >> brw_require_statebuffer_space(brw, 2500);
> >> intel_batchbuffer_save_state(brw);
> >> + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> >>
> >> retry:
> >> brw->batch.no_wrap = true;
> >> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> >> b/src/mesa/drivers/dri/i965/brw_draw.c
> >> index 8536c04010..19ee3962d7 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> >> @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> >> {
> >> struct brw_context *brw = brw_context(ctx);
> >> const struct gen_device_info *devinfo = >screen->devinfo;
> >> - bool fail_next = false;
> >> + bool fail_next;
> >>
> >> /* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have
> >> * atoms that happen on every draw call.
> >> @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> >> intel_batchbuffer_require_space(brw, 1500);
> >> brw_require_statebuffer_space(brw, 2400);
> >> intel_batchbuffer_save_state(brw);
> >> + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> >>
> >> if (brw->num_instances != prim->num_instances ||
> >> brw->basevertex != prim->basevertex ||
> >> diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> >> b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> >> index 34bfcad03e..a62b88e166 100644
> >> ---
Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
andrey simiklit writes:
> Hello,
>
> I tested this patch few times and have the following results:
> https://mesa-ci.01.org/global_logic/builds/38/group/63a9f0ea7bb98050796b649e85481845
> https://mesa-ci.01.org/global_logic/builds/41/group/63a9f0ea7bb98050796b649e85481845
> https://mesa-ci.01.org/global_logic/builds/43/group/63a9f0ea7bb98050796b649e85481845
>
> All test runs are finished with the following issue on G965:
>
> https://mesa-ci.01.org/global_logic/builds/38/results/46496330
> ext_framebuffer_multisample-accuracy all_samples srgb small depthstencil
> linear -auto -fbo
>
> ERROR: This test passed when it expected failure
If you test the exact same revision twice in i965 CI, it will simply
publish the results from the previous run. Often, a single machine
fails in a run, and we send the build a second time to re-test only that
machine (all other results are re-used for faster results). We re-use
results based the commits of the source projects.
Instead of alternating two commits through the CI, you should trivially
amend the commit message of the patch you want to test, so it gets a
different sha. This will cause a complete re-test of the patch.
For this test, it may be that you are looking at an intermittent result.
However, it is never a problem to unexpectedly fix a test.
> Note: Also I checked latest mesa master few times and there was no
> regression:
>
> https://mesa-ci.01.org/global_logic/builds/42/group/63a9f0ea7bb98050796b649e85481845
>
> Regards,
> Andrii.
>
> On Mon, Nov 5, 2018 at 9:48 AM wrote:
>
>> From: Andrii Simiklit
>>
>> There's no point reverting to the last saved point if that save point is
>> the empty batch, we will just repeat ourselves.
>>
>> v2: Merge with new commits, changes was minimized, added the 'fixes' tag
>> v3: Added in to patch series
>> v4: Fixed the regression which was introduced by this patch
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
>> Reported-by: Mark Janes
>> The solution provided by: Jordan Justen
>>
>> CC: Chris Wilson
>> Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
>> the batchbuffer state."
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
>> Signed-off-by: Andrii Simiklit
>> ---
>> src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-
>> src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-
>> src/mesa/drivers/dri/i965/genX_blorp_exec.c | 1 +
>> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
>> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
>> 5 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
>> b/src/mesa/drivers/dri/i965/brw_compute.c
>> index de08fc3ac1..5c8e3a5d4d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_compute.c
>> +++ b/src/mesa/drivers/dri/i965/brw_compute.c
>> @@ -167,7 +167,7 @@ static void
>> brw_dispatch_compute_common(struct gl_context *ctx)
>> {
>> struct brw_context *brw = brw_context(ctx);
>> - bool fail_next = false;
>> + bool fail_next;
>>
>> if (!_mesa_check_conditional_render(ctx))
>>return;
>> @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
>> intel_batchbuffer_require_space(brw, 600);
>> brw_require_statebuffer_space(brw, 2500);
>> intel_batchbuffer_save_state(brw);
>> + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
>>
>> retry:
>> brw->batch.no_wrap = true;
>> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
>> b/src/mesa/drivers/dri/i965/brw_draw.c
>> index 8536c04010..19ee3962d7 100644
>> --- a/src/mesa/drivers/dri/i965/brw_draw.c
>> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
>> @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
>> {
>> struct brw_context *brw = brw_context(ctx);
>> const struct gen_device_info *devinfo = >screen->devinfo;
>> - bool fail_next = false;
>> + bool fail_next;
>>
>> /* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have
>> * atoms that happen on every draw call.
>> @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
>> intel_batchbuffer_require_space(brw, 1500);
>> brw_require_statebuffer_space(brw, 2400);
>> intel_batchbuffer_save_state(brw);
>> + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
>>
>> if (brw->num_instances != prim->num_instances ||
>> brw->basevertex != prim->basevertex ||
>> diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
>> b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
>> index 34bfcad03e..a62b88e166 100644
>> --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
>> +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
>> @@ -309,6 +309,7 @@ retry:
>> intel_batchbuffer_require_space(brw, 1400);
>> brw_require_statebuffer_space(brw, 600);
>> intel_batchbuffer_save_state(brw);
>> + check_aperture_failed_once |=
>> intel_batchbuffer_saved_state_is_empty(brw);
>> brw->batch.no_wrap = true;
>>
Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
Hello,
Thanks for your quick reply.
Is it acceptable that this patch somehow fixes one more test for G965
platform:
ext_framebuffer_multisample-accuracy
or we need to fix this to push it?
Regards,
Andrii.
On Mon, Nov 5, 2018 at 10:42 AM Jordan Justen
wrote:
> On 2018-11-05 00:11:52, andrey simiklit wrote:
> > Hello,
> >
> > I tested this patch few times and have the following results:
> >
> https://mesa-ci.01.org/global_logic/builds/38/group/63a9f0ea7bb98050796b649e85481845
> >
> https://mesa-ci.01.org/global_logic/builds/41/group/63a9f0ea7bb98050796b649e85481845
> >
> https://mesa-ci.01.org/global_logic/builds/43/group/63a9f0ea7bb98050796b649e85481845
> >
> > All test runs are finished with the following issue on G965:
> >
> > https://mesa-ci.01.org/global_logic/builds/38/results/46496330
> > ext_framebuffer_multisample-accuracy all_samples srgb small depthstencil
> > linear -auto -fbo
> >
> > ERROR: This test passed when it expected failure
> >
> > Note: Also I checked latest mesa master few times and there was no
> > regression:
> >
> >
> https://mesa-ci.01.org/global_logic/builds/42/group/63a9f0ea7bb98050796b649e85481845
> >
>
> It also looks like the tex3d-maxsize test is back to failing after a
> few seconds on g965, rather than running forever. (So, #108630 is
> fixed by this version.)
>
> https://mesa-ci.01.org/global_logic/builds/38/results/46492597
>
> -Jordan
> >
> > On Mon, Nov 5, 2018 at 9:48 AM wrote:
> >
> > > From: Andrii Simiklit
> > >
> > > There's no point reverting to the last saved point if that save point
> is
> > > the empty batch, we will just repeat ourselves.
> > >
> > > v2: Merge with new commits, changes was minimized, added the 'fixes'
> tag
> > > v3: Added in to patch series
> > > v4: Fixed the regression which was introduced by this patch
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
> > > Reported-by: Mark Janes
> > > The solution provided by: Jordan Justen >
> > >
> > > CC: Chris Wilson
> > > Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
> > > the batchbuffer state."
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
> > > Signed-off-by: Andrii Simiklit
> > > ---
> > > src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-
> > > src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-
> > > src/mesa/drivers/dri/i965/genX_blorp_exec.c | 1 +
> > > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> > > src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> > > 5 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> > > b/src/mesa/drivers/dri/i965/brw_compute.c
> > > index de08fc3ac1..5c8e3a5d4d 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_compute.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_compute.c
> > > @@ -167,7 +167,7 @@ static void
> > > brw_dispatch_compute_common(struct gl_context *ctx)
> > > {
> > > struct brw_context *brw = brw_context(ctx);
> > > - bool fail_next = false;
> > > + bool fail_next;
> > >
> > > if (!_mesa_check_conditional_render(ctx))
> > >return;
> > > @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
> > > intel_batchbuffer_require_space(brw, 600);
> > > brw_require_statebuffer_space(brw, 2500);
> > > intel_batchbuffer_save_state(brw);
> > > + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> > >
> > > retry:
> > > brw->batch.no_wrap = true;
> > > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> > > b/src/mesa/drivers/dri/i965/brw_draw.c
> > > index 8536c04010..19ee3962d7 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > > @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> > > {
> > > struct brw_context *brw = brw_context(ctx);
> > > const struct gen_device_info *devinfo = >screen->devinfo;
> > > - bool fail_next = false;
> > > + bool fail_next;
> > >
> > > /* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have
> > > * atoms that happen on every draw call.
> > > @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> > > intel_batchbuffer_require_space(brw, 1500);
> > > brw_require_statebuffer_space(brw, 2400);
> > > intel_batchbuffer_save_state(brw);
> > > + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> > >
> > > if (brw->num_instances != prim->num_instances ||
> > > brw->basevertex != prim->basevertex ||
> > > diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > index 34bfcad03e..a62b88e166 100644
> > > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > > @@ -309,6 +309,7 @@ retry:
> > > intel_batchbuffer_require_space(brw, 1400);
> > > brw_require_statebuffer_space(brw, 600);
> > >
Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
On 2018-11-05 00:11:52, andrey simiklit wrote:
> Hello,
>
> I tested this patch few times and have the following results:
> https://mesa-ci.01.org/global_logic/builds/38/group/63a9f0ea7bb98050796b649e85481845
> https://mesa-ci.01.org/global_logic/builds/41/group/63a9f0ea7bb98050796b649e85481845
> https://mesa-ci.01.org/global_logic/builds/43/group/63a9f0ea7bb98050796b649e85481845
>
> All test runs are finished with the following issue on G965:
>
> https://mesa-ci.01.org/global_logic/builds/38/results/46496330
> ext_framebuffer_multisample-accuracy all_samples srgb small depthstencil
> linear -auto -fbo
>
> ERROR: This test passed when it expected failure
>
> Note: Also I checked latest mesa master few times and there was no
> regression:
>
> https://mesa-ci.01.org/global_logic/builds/42/group/63a9f0ea7bb98050796b649e85481845
>
It also looks like the tex3d-maxsize test is back to failing after a
few seconds on g965, rather than running forever. (So, #108630 is
fixed by this version.)
https://mesa-ci.01.org/global_logic/builds/38/results/46492597
-Jordan
>
> On Mon, Nov 5, 2018 at 9:48 AM wrote:
>
> > From: Andrii Simiklit
> >
> > There's no point reverting to the last saved point if that save point is
> > the empty batch, we will just repeat ourselves.
> >
> > v2: Merge with new commits, changes was minimized, added the 'fixes' tag
> > v3: Added in to patch series
> > v4: Fixed the regression which was introduced by this patch
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
> > Reported-by: Mark Janes
> > The solution provided by: Jordan Justen
> >
> > CC: Chris Wilson
> > Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
> > the batchbuffer state."
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
> > Signed-off-by: Andrii Simiklit
> > ---
> > src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-
> > src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-
> > src/mesa/drivers/dri/i965/genX_blorp_exec.c | 1 +
> > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> > src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> > 5 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> > b/src/mesa/drivers/dri/i965/brw_compute.c
> > index de08fc3ac1..5c8e3a5d4d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_compute.c
> > +++ b/src/mesa/drivers/dri/i965/brw_compute.c
> > @@ -167,7 +167,7 @@ static void
> > brw_dispatch_compute_common(struct gl_context *ctx)
> > {
> > struct brw_context *brw = brw_context(ctx);
> > - bool fail_next = false;
> > + bool fail_next;
> >
> > if (!_mesa_check_conditional_render(ctx))
> >return;
> > @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
> > intel_batchbuffer_require_space(brw, 600);
> > brw_require_statebuffer_space(brw, 2500);
> > intel_batchbuffer_save_state(brw);
> > + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> >
> > retry:
> > brw->batch.no_wrap = true;
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> > b/src/mesa/drivers/dri/i965/brw_draw.c
> > index 8536c04010..19ee3962d7 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> > {
> > struct brw_context *brw = brw_context(ctx);
> > const struct gen_device_info *devinfo = >screen->devinfo;
> > - bool fail_next = false;
> > + bool fail_next;
> >
> > /* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have
> > * atoms that happen on every draw call.
> > @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> > intel_batchbuffer_require_space(brw, 1500);
> > brw_require_statebuffer_space(brw, 2400);
> > intel_batchbuffer_save_state(brw);
> > + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
> >
> > if (brw->num_instances != prim->num_instances ||
> > brw->basevertex != prim->basevertex ||
> > diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > index 34bfcad03e..a62b88e166 100644
> > --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> > @@ -309,6 +309,7 @@ retry:
> > intel_batchbuffer_require_space(brw, 1400);
> > brw_require_statebuffer_space(brw, 600);
> > intel_batchbuffer_save_state(brw);
> > + check_aperture_failed_once |=
> > intel_batchbuffer_saved_state_is_empty(brw);
> > brw->batch.no_wrap = true;
> >
> > #if GEN_GEN == 6
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > index 8b769eaf53..6207de5a06 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -301,6 +301,13 @@
Re: [Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
Hello,
I tested this patch few times and have the following results:
https://mesa-ci.01.org/global_logic/builds/38/group/63a9f0ea7bb98050796b649e85481845
https://mesa-ci.01.org/global_logic/builds/41/group/63a9f0ea7bb98050796b649e85481845
https://mesa-ci.01.org/global_logic/builds/43/group/63a9f0ea7bb98050796b649e85481845
All test runs are finished with the following issue on G965:
https://mesa-ci.01.org/global_logic/builds/38/results/46496330
ext_framebuffer_multisample-accuracy all_samples srgb small depthstencil
linear -auto -fbo
ERROR: This test passed when it expected failure
Note: Also I checked latest mesa master few times and there was no
regression:
https://mesa-ci.01.org/global_logic/builds/42/group/63a9f0ea7bb98050796b649e85481845
Regards,
Andrii.
On Mon, Nov 5, 2018 at 9:48 AM wrote:
> From: Andrii Simiklit
>
> There's no point reverting to the last saved point if that save point is
> the empty batch, we will just repeat ourselves.
>
> v2: Merge with new commits, changes was minimized, added the 'fixes' tag
> v3: Added in to patch series
> v4: Fixed the regression which was introduced by this patch
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
> Reported-by: Mark Janes
> The solution provided by: Jordan Justen
>
> CC: Chris Wilson
> Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
> the batchbuffer state."
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
> Signed-off-by: Andrii Simiklit
> ---
> src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-
> src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-
> src/mesa/drivers/dri/i965/genX_blorp_exec.c | 1 +
> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
> 5 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
> b/src/mesa/drivers/dri/i965/brw_compute.c
> index de08fc3ac1..5c8e3a5d4d 100644
> --- a/src/mesa/drivers/dri/i965/brw_compute.c
> +++ b/src/mesa/drivers/dri/i965/brw_compute.c
> @@ -167,7 +167,7 @@ static void
> brw_dispatch_compute_common(struct gl_context *ctx)
> {
> struct brw_context *brw = brw_context(ctx);
> - bool fail_next = false;
> + bool fail_next;
>
> if (!_mesa_check_conditional_render(ctx))
>return;
> @@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
> intel_batchbuffer_require_space(brw, 600);
> brw_require_statebuffer_space(brw, 2500);
> intel_batchbuffer_save_state(brw);
> + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
>
> retry:
> brw->batch.no_wrap = true;
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index 8536c04010..19ee3962d7 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> {
> struct brw_context *brw = brw_context(ctx);
> const struct gen_device_info *devinfo = >screen->devinfo;
> - bool fail_next = false;
> + bool fail_next;
>
> /* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have
> * atoms that happen on every draw call.
> @@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
> intel_batchbuffer_require_space(brw, 1500);
> brw_require_statebuffer_space(brw, 2400);
> intel_batchbuffer_save_state(brw);
> + fail_next = intel_batchbuffer_saved_state_is_empty(brw);
>
> if (brw->num_instances != prim->num_instances ||
> brw->basevertex != prim->basevertex ||
> diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> index 34bfcad03e..a62b88e166 100644
> --- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> +++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
> @@ -309,6 +309,7 @@ retry:
> intel_batchbuffer_require_space(brw, 1400);
> brw_require_statebuffer_space(brw, 600);
> intel_batchbuffer_save_state(brw);
> + check_aperture_failed_once |=
> intel_batchbuffer_saved_state_is_empty(brw);
> brw->batch.no_wrap = true;
>
> #if GEN_GEN == 6
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 8b769eaf53..6207de5a06 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -301,6 +301,13 @@ intel_batchbuffer_save_state(struct brw_context *brw)
> brw->batch.saved.exec_count = brw->batch.exec_count;
> }
>
> +bool
> +intel_batchbuffer_saved_state_is_empty(struct brw_context *brw)
> +{
> + struct intel_batchbuffer *batch = >batch;
> + return (batch->saved.map_next == batch->batch.map);
> +}
> +
> void
> intel_batchbuffer_reset_to_saved(struct brw_context *brw)
> {
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> index
[Mesa-dev] [PATCH v4 1/2] i965/batch: avoid reverting batch buffer if saved state is an empty
From: Andrii Simiklit
There's no point reverting to the last saved point if that save point is
the empty batch, we will just repeat ourselves.
v2: Merge with new commits, changes was minimized, added the 'fixes' tag
v3: Added in to patch series
v4: Fixed the regression which was introduced by this patch
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108630
Reported-by: Mark Janes
The solution provided by: Jordan Justen
CC: Chris Wilson
Fixes: 3faf56ffbdeb "intel: Add an interface for saving/restoring
the batchbuffer state."
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107626
Signed-off-by: Andrii Simiklit
---
src/mesa/drivers/dri/i965/brw_compute.c | 3 ++-
src/mesa/drivers/dri/i965/brw_draw.c | 3 ++-
src/mesa/drivers/dri/i965/genX_blorp_exec.c | 1 +
src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 +++
src/mesa/drivers/dri/i965/intel_batchbuffer.h | 1 +
5 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_compute.c
b/src/mesa/drivers/dri/i965/brw_compute.c
index de08fc3ac1..5c8e3a5d4d 100644
--- a/src/mesa/drivers/dri/i965/brw_compute.c
+++ b/src/mesa/drivers/dri/i965/brw_compute.c
@@ -167,7 +167,7 @@ static void
brw_dispatch_compute_common(struct gl_context *ctx)
{
struct brw_context *brw = brw_context(ctx);
- bool fail_next = false;
+ bool fail_next;
if (!_mesa_check_conditional_render(ctx))
return;
@@ -185,6 +185,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
intel_batchbuffer_require_space(brw, 600);
brw_require_statebuffer_space(brw, 2500);
intel_batchbuffer_save_state(brw);
+ fail_next = intel_batchbuffer_saved_state_is_empty(brw);
retry:
brw->batch.no_wrap = true;
diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
b/src/mesa/drivers/dri/i965/brw_draw.c
index 8536c04010..19ee3962d7 100644
--- a/src/mesa/drivers/dri/i965/brw_draw.c
+++ b/src/mesa/drivers/dri/i965/brw_draw.c
@@ -885,7 +885,7 @@ brw_draw_single_prim(struct gl_context *ctx,
{
struct brw_context *brw = brw_context(ctx);
const struct gen_device_info *devinfo = >screen->devinfo;
- bool fail_next = false;
+ bool fail_next;
/* Flag BRW_NEW_DRAW_CALL on every draw. This allows us to have
* atoms that happen on every draw call.
@@ -898,6 +898,7 @@ brw_draw_single_prim(struct gl_context *ctx,
intel_batchbuffer_require_space(brw, 1500);
brw_require_statebuffer_space(brw, 2400);
intel_batchbuffer_save_state(brw);
+ fail_next = intel_batchbuffer_saved_state_is_empty(brw);
if (brw->num_instances != prim->num_instances ||
brw->basevertex != prim->basevertex ||
diff --git a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
index 34bfcad03e..a62b88e166 100644
--- a/src/mesa/drivers/dri/i965/genX_blorp_exec.c
+++ b/src/mesa/drivers/dri/i965/genX_blorp_exec.c
@@ -309,6 +309,7 @@ retry:
intel_batchbuffer_require_space(brw, 1400);
brw_require_statebuffer_space(brw, 600);
intel_batchbuffer_save_state(brw);
+ check_aperture_failed_once |= intel_batchbuffer_saved_state_is_empty(brw);
brw->batch.no_wrap = true;
#if GEN_GEN == 6
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index 8b769eaf53..6207de5a06 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -301,6 +301,13 @@ intel_batchbuffer_save_state(struct brw_context *brw)
brw->batch.saved.exec_count = brw->batch.exec_count;
}
+bool
+intel_batchbuffer_saved_state_is_empty(struct brw_context *brw)
+{
+ struct intel_batchbuffer *batch = >batch;
+ return (batch->saved.map_next == batch->batch.map);
+}
+
void
intel_batchbuffer_reset_to_saved(struct brw_context *brw)
{
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
index 0632142cd3..91720dad5b 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
@@ -24,6 +24,7 @@ struct intel_batchbuffer;
void intel_batchbuffer_init(struct brw_context *brw);
void intel_batchbuffer_free(struct intel_batchbuffer *batch);
void intel_batchbuffer_save_state(struct brw_context *brw);
+bool intel_batchbuffer_saved_state_is_empty(struct brw_context *brw);
void intel_batchbuffer_reset_to_saved(struct brw_context *brw);
void intel_batchbuffer_require_space(struct brw_context *brw, GLuint sz);
int _intel_batchbuffer_flush_fence(struct brw_context *brw,
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
