[Mesa-dev] [PATCH 03/27] i965: Enable hardware-generated binding tables on render path.

[Abdiel Janulgue]

On 05/07/2015 05:46 PM, Pohjolainen, Topi wrote:
> On Thu, May 07, 2015 at 04:43:21PM +0300, Pohjolainen, Topi wrote:
>> On Tue, Apr 28, 2015 at 11:08:00PM +0300, Abdiel Janulgue wrote:
>>> This patch implements the binding table enable command which is also
>>> used to allocate a binding table pool where hardware-generated
>>> binding table entries are flushed into. Each binding table offset in
>>> the binding table pool is unique per each shader stage that are
>>> enabled within a batch.
>>>
>>> Also insert the required brw_tracked_state objects to enable
>>> hw-generated binding tables in normal render path.
>>>
>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_binding_tables.c | 70 ++++++++++++++++++++++++++
>>>  src/mesa/drivers/dri/i965/brw_context.c        |  4 ++
>>>  src/mesa/drivers/dri/i965/brw_context.h        |  5 ++
>>>  src/mesa/drivers/dri/i965/brw_state.h          |  7 +++
>>>  src/mesa/drivers/dri/i965/brw_state_upload.c   |  2 +
>>>  src/mesa/drivers/dri/i965/intel_batchbuffer.c  |  4 ++
>>>  6 files changed, 92 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_binding_tables.c b/src/mesa/drivers/dri/i965/brw_binding_tables.c
>>> index 459165a..a58e32e 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_binding_tables.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_binding_tables.c
>>> @@ -44,6 +44,11 @@
>>>  #include "brw_state.h"
>>>  #include "intel_batchbuffer.h"
>>>  
>>> +/* Somehow the hw-binding table pool offset must start here, otherwise
>>> + * the GPU will hang
>>> + */
>>> +#define HW_BT_START_OFFSET 256;
>>
>> I think we want to understand this a little better before enabling...

Actually, now that I remember I had my notes somewhere when I first
enabled this hw-binding table over a year ago. I dug it up and 256 is
the actually the size of a single stage's "hw-binding table state"
expressed in hw-binding table format. Details:

>From the Bspec 3DSTATE_BINDING_TABLE_POINTERS_x > Pointer to PS Binding
Table section lists the format as:

	"SurfaceStateOffset[16:6]BINDING_TABLE_STATE*256 When
	HW-generated binding table is enabled"

So this is 16-bits[1] x 256 = 512 bytes.

Now this offset must be expressed in "Pointer to PS Binding Table" using
the hw-generated binding table format which must be aligned to 16:6.
However the bit entry field in dw1 of 3DSTATE_BINDING_TABLE_POINTERS_x
must be set within 15:5 so this value should be >> 1. Hence, the 256
(similar case is evident on function gen7_update_binding_table() in
patch 4 of this series).

Seems the RS hardware is extremely intolerant of even slight variations
hence the hungs when this is not followed closely.

In the next version, I can make the magic numbers a bit more clearer.

[1] 3D-Media-GPGPU Engine > Shared Functions > 3D Sampler > State > HW
Generated BINDING_TABLE_STATE

>>
>>> +
>>>  /**
>>>   * Upload a shader stage's binding table as indirect state.
>>>   *
>>> @@ -163,6 +168,71 @@ const struct brw_tracked_state brw_gs_binding_table = {
>>>     .emit = brw_gs_upload_binding_table,
>>>  };
>>>  
>>> +/**
>>> + * Hardware-generated binding tables for the resource streamer
>>> + */
>>> +void
>>> +gen7_disable_hw_binding_tables(struct brw_context *brw)
>>> +{
>>> +   BEGIN_BATCH(3);
>>> +   OUT_BATCH(_3DSTATE_BINDING_TABLE_POOL_ALLOC << 16 | (3 - 2));
>>> +   OUT_BATCH(SET_FIELD(BRW_HW_BINDING_TABLE_OFF, BRW_HW_BINDING_TABLE_ENABLE) |
>>> +             brw->is_haswell ? HSW_HW_BINDING_TABLE_RESERVED : 0);
>>> +   OUT_BATCH(0);
>>> +   ADVANCE_BATCH();
>>> +
>>> +   /* Pipe control workaround */
>>> +   brw_emit_pipe_control_flush(brw, PIPE_CONTROL_STATE_CACHE_INVALIDATE);
>>> +}
>>> +
>>> +void
>>> +gen7_enable_hw_binding_tables(struct brw_context *brw)
>>> +{
>>> +   if (!brw->has_resource_streamer) {
>>> +      gen7_disable_hw_binding_tables(brw);
>>
>> I started wondering why we really need this - RS is disabled by default and
>> we haven't needed to do anything to disable it before.
> 
> Right, patch number eight gave me the answer, we want to disable it for blorp.
>