On Tuesday, March 17, 2015 08:20:23 AM Kristian Høgsberg wrote:
> On Tue, Mar 17, 2015 at 2:22 AM, Kenneth Graunke <kenn...@whitecape.org>
> 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
> >> <jordan.l.jus...@intel.com> 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 mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
