<div dir="ltr">On 8 November 2013 04:49, Rogovin, Kevin <span dir="ltr"><<a href="mailto:kevin.rogovin@intel.com" target="_blank">kevin.rogovin@intel.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi all,<br>
<br>
As I was poking into the magicks for the batchbuffer, I saw the following logical bits of code, that make sense by themselves but get me paranoid together. Firstly in intel_batchbuffer_begin() [ intel_batchbuffer.h, and this is what BEGIN_BATCH maps to] there is a intel_batchbuffer_require_space() call that if too much room is needed then calls intel_batchbuffer_begin():<br>
<br>
from intel_batchbuffer_require_space():<br>
<br>
115 if (intel_batchbuffer_space(brw) < sz)<br>
116 intel_batchbuffer_flush(brw);<br>
<br>
and from intel_batchbuffer_space():<br>
<br>
80 intel_batchbuffer_space(struct brw_context *brw)<br>
81 {<br>
82 return (brw->batch.state_batch_offset - brw->batch.reserved_space)<br>
83 - brw->batch.used*4;<br>
84 }<br>
<br>
<br>
Now, for allocating space for state, there is brw_state_batch():<br>
<br>
<br>
128 offset = ROUND_DOWN_TO(batch->state_batch_offset - size, alignment);<br>
129<br>
130 /* If allocating from the top would wrap below the batchbuffer, or<br>
131 * if the batch's used space (plus the reserved pad) collides with our<br>
132 * space, then flush and try again.<br>
133 */<br>
134 if (batch->state_batch_offset < size ||<br>
135 offset < 4*batch->used + batch->reserved_space) {<br>
136 intel_batchbuffer_flush(brw);<br>
137 offset = ROUND_DOWN_TO(batch->state_batch_offset - size, alignment);<br>
138 }<br>
<br>
<br>
These taken together, I interpret as meaning that state and commands try to be separated by atleast batch->reserved_space bytes. I guess state could take up more than (batch->bo->size - batch->reserved_space) from that second ROUND_DOWN_TO, but that would only happen right after a flush and any state or command afterwards would flush too.<br>
<br>
Now my questions:<br>
1) it looks like the reserved space is not to be used for either state or commands. Is that correct? What is it used for?<br></blockquote><div><br></div><div>It is used by commands that have to go at the end of every batch. When we're deciding whether or not to go to a new batch buffer, those commands haven't been emitted yet, so we have to account for the amount of space they take up when we determine whether the next command is going to fill up the batch buffer. See the documentation for BATCH_RESERVED in intel_batchbuffer.h.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
2) if a function first calls brw_state_batch() to place state and it barely fits and then calls BEGIN_BATCH that does not fit, then the command will refer to an offset on the previous batch buffer, that cannot be good. Or does this never happen for other reasons? If so what are those reasons?<br>
</blockquote><div><br></div><div>You are correct that this cannot be good. It probably would lead to a GPU hang. The reason it doesn't happen is that we have an additional mechanism for flushing the batch at a safe time, before we have started emitting the state for a draw call. Near the top of brw_try_draw_prims(), we estimate how many bytes of batch buffer space will be needed to complete the draw call (estimated_max_prim_size). Then we pad it by 512 bytes to be safe, and then we call intel_batchbuffer_require_space(), which flushes the batchbuffer if there isn't enough space. This is safe because we haven't started emitting state yet. And it ensures that once we do emit state, there will be enough batch buffer space available to contain it without needing to flush at an inopportune time.<br>
<br></div><div>There's only one problem. As we continue to add features to the driver, the maximum possible amount of batch buffer space needed to hold draw state increases, and we rarely remember to update estimated_max_prim_size to reflect that. So there's a danger of not passing a large enough number to intel_batchbuffer_require_space(), either now or in the future.<br>
<br>To reduce the risk of that, we have a safety mechanism in debug builds: if we run out of batch buffer space while emitting draw state, we will assert rather than flush. Grep for "no_batch_wrap" to see the code that does this.<br>
<br></div><div>IMHO, we could do even better than this. What we really should do is keep track of the amount of batch buffer space used by every draw call and compare it to estimated_max_prim_size. If the difference between the actual space used and the estimate is ever less than the 512 pad bytes in a debug build, we should assertion fail. That way the assertion failure would happen if we're *ever* wrong in our estimate of how much batch space is needed (rather than only happening if being wrong causes an unsafe flush). Kevin, would you be interested in submitting a patch to improve this? It might be a nice way for you to continue building up your familiarity with the codebase. If not I can add it to my to-do list.<br>
</div></div></div></div>