[Mesa-dev] i965 CS state tracking

[Kenneth Graunke]

On Wednesday, September 03, 2014 09:22:12 AM Jordan Justen wrote:
> > 15:22:24 Kayden | jljusten: so how does the compute shader state upload work?
> > 15:22:35 Kayden | I know you have to issue PIPELINE_SELECT to pick the
> >                 | GPGPU pipe instead of 3D
> > 15:22:53 Kayden | but does the GPGPU pipe use a lot of the same commands?
> > 15:22:57 Kayden | do you have to reprogram them when switching?
> > 15:30:19 Kayden | I don't understand why we have to have separate
> >                 | dirty bit sets for compute vs. 3D
> > 15:30:52 Kayden | It looks like if you're called by glDispatchCompute,
> >                 | we should PIPELINE_SELECT to GPGPU, and do whatever
> >                 | state upload you need for compute
> > 15:31:13 Kayden | and if you're called by glDrawWhatever(), we should
> >                 | PIPELINE_SELECT to 3D and do the existing 3D state
> >                 | upload
> > 15:31:52 Kayden | unless things like 3DSTATE_BINDING_TABLE_POINTERS
> >                 | only takes effect on one pipe or the other, I
> >                 | guess...
> > 15:32:54 Kayden | but if most of the commands are different between
> >                 | the two pipes...
> > 15:34:20 Kayden | based on reading Paul's notes I don't see why he did
> >                 | that.
> > 15:55:08    krh | Kayden: there's about 3-4 commands you need to set up
> >                 | gpgpu
> > 15:55:18    krh | none of which are shared with the 3d pipeline
> > 15:55:36    krh | aside from the pipeline select, it's completely
> >                 | orthogonal
> > 15:56:29    krh | I guess I don't understand why we need to track all the
> >                 | state for all the things either
> > 15:57:05    krh | but we do need to make sure update_state doesn't emit
> >                 | dirty 3d state when we're trying to do cs and vice verse
> > 15:57:08    krh | versa
> > 15:57:41    krh | it seems like a pipeline mask should be enough
> > 15:58:11    krh | ie, mask the dirty flags against a
> >                 | pipeline_3d_flag_mask when we're emitting state for 3d
> 
> I'm not sure of Paul's thoughts.
> 
> One thought I have is, what if we decide to upload some state
> differently depending on the pipeline? Then when we switch, we'd need
> to make sure we re-emit it again for the other pipeline.

That's easy to handle.  In brw_upload_state, we can check brw->state.current_pipeline != pipeline to know when we've changed pipelines.  We can even detect if we're going from 3D -> GPGPU or vice versa by checking the existing value of brw->state.current_pipeline.  When the pipeline changes, we just need to flag the dirty bits for any state that gets implicitly clobbered.

> In fact, on the cs branch, we defined the brw_cs_abo_surfaces atom
> with an upload function of brw_upload_cs_abo_surfaces that differs
> from brw_upload_wm_abo_surfaces.

That's fine and doesn't pose a problem.  brw_upload_cs_abo_surfaces looks just like the existing brw_upload_vs_abo_surfaces, brw_upload_gs_abo_surfaces, and brw_upload_gs_abo_surfaces.  It's just an additional shader stage.  Plus, all these do is upload SURFACE_STATE entries in the state portion of the batch BO, so that later, 3DSTATE_BINDING_TABLE_POINTERS can point at that data.  It doesn't change state directly.

> Does this seem explanation seem plausible?
> 
> -Jordan

No, and I think we should revert the patch.  Having two state atom lists seems sensible, and passing an "enum brw_pipeline" parameter to brw_upload_state also seems sensible.  But having two sets of dirty bits doesn't yet make sense to me.

Now, if GPGPU and 3D had largely overlapping state packets, yet were on separate /rings/ that preserved their state independently of one another...then I think it would make sense.  But that doesn't seem to be the case.

I'm sorry I wasn't able to review this series within a week before it landed; I've been trying to go through a 23 patch series from Matt, a 13 patch series from Connor, and a 37 patch series from Iago and Sam, and still haven't gotten to your other 14 patch series, so I'm just a little behind these days...and with the holiday...

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140903/27fc6478/attachment.sig>