On Wed, Mar 11, 2015 at 11:53 AM, Jordan Justen <jordan.l.jus...@intel.com> wrote: > When uploading state for a pipeline, we will save changed state for > the other pipelines. > > Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
In reviewing this again, I realize I'm not completely comfortable with how this is done. The overall approach is great, I'm happy to see the dual pipeline flags contained in brw_state_upload.c, but I think we need to keep brw_upload_pipeline_state() a no-op in case it fails. I can't think of a case where it might bite us, but currently, the expectation is that if it fails, nothing changes and you can call it again after flushing the batch. This patch changes that in two ways: 1) we merge the pipeline brw->state.dirty into state and clear the pipeline state and 2) we copy the dirty state into the other pipeline. Suppose we call with the render pipeline and fail, flush the batch, then call with the compute pipeline and clear the dirty flags [1]. If we then go back and try to emit render state again, there are no longer any dirty bits and we fail to emit render state. This may be far-fetched and, as I said, I don't think we can hit this. But I'd rather not break the contract that brw_upload_pipeline_state() doesn't change state in case of failure - I know I'd hate to debug that. We can just introduce a brw_clear_pipeline_dirty_bits() function that will be called on successful state upload and merge the state bits to the other pipeline and then clear the selected pipeline dirty flags as well as brw->state.dirty. And we need to use a local copy of brw->state.dirty in brw_upload_pipeline_state() for merging in the pipeline dirty flags so we don't modify brw->state.dirty on error. [1] I don't see how we'd get into that, but with fast clear resolve using meta we end up running the render pipeline at unexpected places. > --- > src/mesa/drivers/dri/i965/brw_context.h | 1 + > src/mesa/drivers/dri/i965/brw_state_upload.c | 42 > ++++++++++++++++++++++------ > 2 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h > b/src/mesa/drivers/dri/i965/brw_context.h > index 91b4054..e693f50 100644 > --- a/src/mesa/drivers/dri/i965/brw_context.h > +++ b/src/mesa/drivers/dri/i965/brw_context.h > @@ -1101,6 +1101,7 @@ struct brw_context > GLuint NewGLState; > struct { > struct brw_state_flags dirty; > + struct brw_state_flags pipelines[BRW_NUM_PIPELINES]; > } state; > > struct brw_cache cache; > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c > b/src/mesa/drivers/dri/i965/brw_state_upload.c > index 4f21002..55a9050 100644 > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c > @@ -586,15 +586,16 @@ brw_upload_programs(struct brw_context *brw) > brw_upload_wm_prog(brw); > } > > -/*********************************************************************** > - * Emit all state: > - */ > -void brw_upload_render_state(struct brw_context *brw) > +static inline void > +brw_upload_pipeline_state(struct brw_context *brw, > + enum brw_pipeline pipeline) > { > struct gl_context *ctx = &brw->ctx; > struct brw_state_flags *state = &brw->state.dirty; > int i; > static int dirty_count = 0; > + struct brw_state_flags *pipeline_state = > + &brw->state.pipelines[pipeline]; > > state->mesa |= brw->NewGLState; > brw->NewGLState = 0; > @@ -633,6 +634,12 @@ void brw_upload_render_state(struct brw_context *brw) > brw->state.dirty.brw |= BRW_NEW_NUM_SAMPLES; > } > > + if ((pipeline_state->mesa | pipeline_state->brw) != 0) { > + state->mesa |= pipeline_state->mesa; > + state->brw |= pipeline_state->brw; > + memset(pipeline_state, 0, sizeof(struct brw_state_flags)); > + } > + > if ((state->mesa | state->brw) == 0) > return; > > @@ -642,6 +649,10 @@ void brw_upload_render_state(struct brw_context *brw) > > brw_upload_programs(brw); > > + const struct brw_tracked_state *atoms = > + brw_get_pipeline_atoms(brw, pipeline); > + const int num_atoms = brw->num_atoms[pipeline]; > + > if (unlikely(INTEL_DEBUG)) { > /* Debug version which enforces various sanity checks on the > * state flags which are generated and checked to help ensure > @@ -651,8 +662,8 @@ void brw_upload_render_state(struct brw_context *brw) > memset(&examined, 0, sizeof(examined)); > prev = *state; > > - for (i = 0; i < brw->num_atoms[BRW_RENDER_PIPELINE]; i++) { > - const struct brw_tracked_state *atom = &brw->render_atoms[i]; > + for (i = 0; i < num_atoms; i++) { > + const struct brw_tracked_state *atom = &atoms[i]; > struct brw_state_flags generated; > > if (check_state(state, &atom->dirty)) { > @@ -671,8 +682,8 @@ void brw_upload_render_state(struct brw_context *brw) > } > } > else { > - for (i = 0; i < brw->num_atoms[BRW_RENDER_PIPELINE]; i++) { > - const struct brw_tracked_state *atom = &brw->render_atoms[i]; > + for (i = 0; i < num_atoms; i++) { > + const struct brw_tracked_state *atom = &atoms[i]; > > if (check_state(state, &atom->dirty)) { > atom->emit(brw); > @@ -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 > } > > +/*********************************************************************** > + * Emit all state: > + */ > +void brw_upload_render_state(struct brw_context *brw) > +{ > + brw_upload_pipeline_state(brw, BRW_RENDER_PIPELINE); > +} > > /** > * Clear dirty bits to account for the fact that the state emitted by > -- > 2.1.4 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev