[Mesa-dev] [PATCH 2/2] i965/draw state: Allow state upload to proceed when inputs are not set

[Jordan Justen]

On 2014-11-22 23:50:29, Kenneth Graunke wrote:
> On Saturday, November 22, 2014 11:26:39 PM Jordan Justen wrote:
> > On 2014-11-22 17:33:54, Kenneth Graunke wrote:
> > > On Saturday, November 22, 2014 12:02:31 PM Jordan Justen wrote:
> > > > For drawing the OpenGL API should validate this before we try to upload the
> > > > state.
> > > > 
> > > > For compute, these might not be set.
> > > > 
> > > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > > > ---
> > > >  src/mesa/drivers/dri/i965/brw_draw_upload.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c b/src/mesa/drivers/dri/i965/brw_draw_upload.c
> > > > index 5a12439..28136e2 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
> > > > @@ -433,6 +433,10 @@ brw_prepare_vertices(struct brw_context *brw)
> > > >        struct brw_vertex_element *input = &brw->vb.inputs[i];
> > > >  
> > > >        vs_inputs &= ~BITFIELD64_BIT(i);
> > > > +
> > > > +      if (!input->glarray)
> > > > +         continue;
> > > > +
> > > >        brw->vb.enabled[brw->vb.nr_enabled++] = input;
> > > >     }
> > > 
> > > Seems like you don't want to be calling this code at all when doing compute.
> > > 
> > > The draw upload code is already complicated and difficult to understand;
> > > although this is a small addition, the "Inputs can be NULL?!?!?" question
> > > readers might have could add to the confusion.  Which is fine, if compute
> > > needs to hit this code...but if it doesn't need it, skipping it altogether
> > > would be clearer and simpler.
> > > 
> > > Does compute actually want 3DSTATE_VERTEX_BUFFERS/ELEMENTS?  Does it use
> > > 3DPRIMITIVE?
> > 
> > No. But, as I understanded your previous feedback, you didn't want to
> > have separate state tracking for compute.
> 
> Not necessarily.
> 
> Jordan, could you send out a list of the command packets and state needed
> to do meaningful compute work?  I think that would help me understand
> what's required.  INTEL_DEBUG=bat would show what you're currently emitting.

Hmm. Maybe I overreacted to your previous feedback.

I basically assumed that the separate pipeline state tracking was a
no-go and moved forward assuming I'd just merge the compute state into
the current set of state upload.

In order to know how much overlaps, I'd need to separate it back out.

> My assumption so far (which I'd like to confirm with the above list) is that
> there's actually very little overlap between normal 3D drawing and general
> compute work.  I also know that compute shaders are kicked off via separate
> glDispatchCompute and glDispatchComputeIndirect functions.  I don't really
> understand why we're even hitting the draw_prims driver hook.  If there's
> really little overlap, then creating a totally separate driver hook might
> make more sense.

These are state upload based code paths. We're not going through
draw_prims.

These paths were not hit previously when we were using Paul's separate
state pipelines.

> Another idea I've had is that we could use a separate hardware context
> (brw->hw_ctx_3d and brw->hw_ctx_compute), so both the 3D and compute pipes
> could operate without interfering with one another - we wouldn't need to
> consider state dirtied by one or the other.

In order to do something like this, I think we'd need to go back to
tracking the state independently for a draw vs. compute. We could then
either re-emit state to a single context or separate contexts if we
wanted.

But, I'm not sure this is 100% achievable. ("we wouldn't need to
consider state dirtied by one or the other") It is not like we set the
GL state to either draw or compute. So, if state changes, we need to
mark it as dirty for both, and be sure to update based on that before
either a draw or compute.

I guess I need to better understand what needed to change from Paul's
separate pipelines series, because I (too quickly) assumed it had been
NAKed in its entirety.

One idea that is a bit different from Paul's implementation might be
to add a new bitmap to brw_tracked_state which allows a state object
to indicate draw and/or compute applicability. We could then keep a
single array of state objects.

Another idea is that we could retain a single set of dirty bits when
marking state as dirty, but duplicate it for the separate pipelines at
state upload time. This might be able to avoid the somewhat ugly state
macros. (These macros were a common source of conflicts during
rebases.)

-Jordan

> Again, I think seeing a list of commands would help us figure out the
> best solution (hopefully both efficient and simple).
> 
> > Therefore, when we we upload dirty state before running compute
> > operations, it is possible (and apparently likely) that the
> > brw_vertices state atom will call this code.
> > 
> > When I removed the separate compute state tracking pipeline, I found I
> > only needed these two changes to be able to still use the current
> > state upload with compute.
> > 
> > -Jordan
> 
> That's surprisingly small!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141123/7dc5a8ff/attachment-0001.sig>