[Mesa-dev] [PATCH resend 4/7] i965: Implement interface to edit binding table entries
Kenneth Graunke
kenneth at whitecape.org
Mon Jun 1 23:01:41 PDT 2015
On Monday, June 01, 2015 03:14:27 PM Abdiel Janulgue wrote:
> Unlike normal software binding tables where the driver has to manually
> generate and fill a binding table array which are then uploaded to the
> hardware, the resource streamer instead presents the driver with an option
> to fill out slots for individual binding table indices. The hardware
> accumulates the state for these combined edits which it then automatically
> flushes to a binding table pool when the binding table pointer state
> command is invoked.
>
> v2: Clarify binding table edit bit aligment. (Topi)
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> ---
> src/mesa/drivers/dri/i965/brw_binding_tables.c | 49 ++++++++++++++++++++++++++
> src/mesa/drivers/dri/i965/brw_state.h | 9 +++++
> 2 files changed, 58 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_binding_tables.c b/src/mesa/drivers/dri/i965/brw_binding_tables.c
> index 7554818..ddf68fe 100644
> --- a/src/mesa/drivers/dri/i965/brw_binding_tables.c
> +++ b/src/mesa/drivers/dri/i965/brw_binding_tables.c
> @@ -61,6 +61,12 @@
> */
> static const uint32_t hw_bt_start_offset = 256 * sizeof(uint16_t);
>
> +static const GLuint stage_to_bt_edit[MESA_SHADER_FRAGMENT + 1] = {
> + _3DSTATE_BINDING_TABLE_EDIT_VS,
> + _3DSTATE_BINDING_TABLE_EDIT_GS,
> + _3DSTATE_BINDING_TABLE_EDIT_PS,
> +};
> +
> /**
> * Upload a shader stage's binding table as indirect state.
> *
> @@ -191,6 +197,49 @@ const struct brw_tracked_state brw_gs_binding_table = {
> * Hardware-generated binding tables for the resource streamer
> */
This comment doesn't really describe what the function does. Comments
above functions should describe the action the function performs, so
developers can tell when and how to use it.
Something like
/**
* Edit a single entry in a hardware binding table.
*/
(feel free to write something else, you don't have to use my wording)
> void
> +gen7_update_binding_table(struct brw_context *brw,
> + gl_shader_stage stage,
> + uint32_t index,
> + uint32_t surf_offset)
> +{
> + assert(stage <= MESA_SHADER_FRAGMENT);
> +
> + uint32_t dw2 = SET_FIELD(index, BRW_BINDING_TABLE_INDEX) |
> + (brw->gen >= 8 ? BDW_SURFACE_STATE_EDIT(surf_offset) :
> + BDW_SURFACE_STATE_EDIT(surf_offset));
> +
> + BEGIN_BATCH(3);
> + OUT_BATCH(stage_to_bt_edit[stage] << 16 | (3 - 2));
> + OUT_BATCH(BRW_BINDING_TABLE_EDIT_TARGET_ALL);
> + OUT_BATCH(dw2);
> + ADVANCE_BATCH();
> +}
You don't appear to use this function in your hardware binding table
series - it looks like you only use it in the gather constants series?
Still, this seems like a reasonable thing to have.
gen7_edit_hw_binding_table_entry() might be a clearer function name.
> +
> +/**
> + * Hardware-generated binding tables for the resource streamer
> + */
Here's that same comment again. This one does something else, let's
give it a comment that reflects that. Perhaps something like:
/**
* Upload a whole hardware binding table for the given stage.
*
* Takes an array of surface offsets and the number of binding table
* entries.
*/
Since we've basically concluded that the primary benefit of doing this
is the ability to use gather constants, I like how you've added an
interface which lets us easily use hardware binding tables like our old
software binding tables (i.e. upload the whole thing at once). Thanks!
> +void
> +gen7_update_binding_table_from_array(struct brw_context *brw,
> + gl_shader_stage stage,
> + const uint32_t* binding_table,
> + int size)
Could you rename "size" to "num_entries", "num_surfaces", or
something along those lines?
We have a real problem in the driver where people call things "size" but
don't specify units (i.e. is this the size of the table in bytes, or...?)
With those changes, this would get:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> +{
> + uint32_t dw2 = 0;
> + assert(stage <= MESA_SHADER_FRAGMENT);
> +
> + BEGIN_BATCH(size + 2);
> + OUT_BATCH(stage_to_bt_edit[stage] << 16 | size);
> + OUT_BATCH(BRW_BINDING_TABLE_EDIT_TARGET_ALL);
> + for (int i = 0; i < size; i++) {
> + dw2 = SET_FIELD(i, BRW_BINDING_TABLE_INDEX) |
> + (brw->gen >= 8 ? BDW_SURFACE_STATE_EDIT(binding_table[i]) :
> + HSW_SURFACE_STATE_EDIT(binding_table[i]));
> + OUT_BATCH(dw2);
> + }
> + ADVANCE_BATCH();
> +}
I wonder if there is any value in zeroing out the rest of the table.
If so, we might want to set the "Binding Table Block Clear" bits in DW1,
and issue surfaces for each block of 16.
I suppose we don't bother doing that in the software binding table case,
so it's probably fine as is.
> +
> +void
> gen7_disable_hw_binding_tables(struct brw_context *brw)
> {
> int pkt_len = brw->gen >= 8 ? 4 : 3;
> diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h
> index 622ce3f..51983f2 100644
> --- a/src/mesa/drivers/dri/i965/brw_state.h
> +++ b/src/mesa/drivers/dri/i965/brw_state.h
> @@ -374,6 +374,15 @@ gen7_upload_constant_state(struct brw_context *brw,
> bool active, unsigned opcode);
>
> void gen7_rs_control(struct brw_context *brw, int enable);
> +
> +void gen7_update_binding_table(struct brw_context *brw,
> + gl_shader_stage stage,
> + uint32_t index,
> + uint32_t surf_offset);
> +void gen7_update_binding_table_from_array(struct brw_context *brw,
> + gl_shader_stage stage,
> + const uint32_t* binding_table,
> + int size);
> void gen7_enable_hw_binding_tables(struct brw_context *brw);
> void gen7_disable_hw_binding_tables(struct brw_context *brw);
> void gen7_reset_rs_pool_offsets(struct brw_context *brw);
>
-------------- 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/20150601/c8c0d12d/attachment.sig>
More information about the mesa-dev
mailing list