On Tuesday, March 17, 2015 08:20:23 AM Kristian Høgsberg wrote: > On Tue, Mar 17, 2015 at 2:22 AM, Kenneth Graunke <[email protected]> > wrote: > > On Monday, March 16, 2015 10:21:31 PM Kristian Høgsberg wrote: > >> On Wed, Mar 11, 2015 at 11:53 AM, Jordan Justen > >> <[email protected]> wrote: [snip] > >> > @@ -691,8 +702,23 @@ void brw_upload_render_state(struct brw_context > >> > *brw) > >> > fprintf(stderr, "\n"); > >> > } > >> > } > >> > + > >> > + /* Save all dirty state into the other pipelines */ > >> > + for (int i = 0; i < BRW_NUM_PIPELINES; i++) { > >> > + if (i != pipeline) { > >> > + brw->state.pipelines[i].mesa |= state->mesa; > >> > + brw->state.pipelines[i].brw |= state->brw; > >> > + } > >> > + } > >> > >> When we merge this in at this point, state->mesa/brw includes dirty > >> bits from the active pipelines pipeline flags. For example, suppose we > >> do a bunch of render pipeline stuff, which queues up a lot of dirty > >> flags for the compute pipeline. Then when we go to emit state for the > >> compute pipeline we first merge all these dirty flags into > >> brw->state.dirty, then merge that into the render pipeline flags. We > >> only need to merge the new dirty bits, that is, the initial value of > >> brw->state.dirty into the render pipeline flags. If we don't modify > >> brw->state.dirty in this function and merge the flags in clear as > >> described above, this problem is easy to fix. > >> > >> Kristian
OK, I see what you mean now - very good catch.
1. We do a render operation. Any new dirty bits get saved in
brw->state.pipelines[COMPUTE], so they'll happen when we
do the next compute workload.
This completes successfully (ignore retries).
2. We do a compute operation.
brw->state.pipelines[COMPUTE] gets merged into brw->state.dirty.
This includes bits that happened during #1 (and were missed on
the compute pipe).
This all works fine. At the end, we save brw->state.dirty into
brw->state.pipelines[RENDER].
3. We do another render operation.
brw->state.pipelines[RENDER] gets merged into brw->state.dirty.
We now have bits from #1 again, which are not necessary.
So yeah. That's bad. Sorry, I thought this was to handle the
hypothetical case of pipe-switch-during-retries. But it's just
switching pipelines at all!
If I understand your suggestion correctly, you mean that
a) We should eliminate brw->state.dirty.
b) brw_upload_pipeline_state() should do:
struct brw_state_flags *pipeline_state =
&brw->state.pipelines[pipeline];
/* Create a local copy of the dirty state */
struct brw_state_flags dirty;
dirty.mesa = brw->NewGLState | pipeline_state->mesa;
dirty.brw = ctx->NewDriverState | pipeline_state->brw;
Use this for uploads.
c) The existing brw_clear_dirty_bits() should do:
/* Save the dirty flags into the other pipelines */
for (int i = 0; i < BRW_NUM_PIPELINES; i++) {
if (i != pipeline) {
brw->state.pipelines[i].mesa |= brw->NewGLState;
brw->state.pipelines[i].brw |= ctx->NewDriverState;
}
}
/* We've handled any bits passed to us by other pipeline uploads */
brw->state.pipelines[pipe] = 0;
/* We've handled the flags passed to us by Mesa */
brw->NewGLState = 0;
ctx->NewDriverState = 0;
Does that sound like a correct interpretation? This does seem to solve
the problem, and I think it's a bit cleaner too.
Re-evaluating my example to verify that this approach works...
1. We do a render operation.
The local dirty flags are <bits from Mesa> | <saved bits>.
Nothing's saved yet, so just <bits from Mesa>
When done, we save <bits from Mesa> to pipelines[COMPUTE].
2. We do a compute operation.
The local dirty flags are <new bits from Mesa> | <saved bits
from op #1>. We use those for upload.
When done, brw_clear_dirty_bits() saves only <new bits from
Mesa> to pipelines[COMPUTE]. Notably, the bits from op #1
are /not/ saved.
3. We do a render operation.
We get any new bits from Mesa, plus the bits during #2.
No bits from #1.
So...you're right - there's definitely a problem, and your suggestion
totally fixes it. I just had to work through an example to see what
you meant. Thanks again!
Jordan, does this make sense to you?
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
