Re: [PATCH 1/3] tools/null_state_gen: add macro to emit commands with null state

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

"Volkin, Bradley D" <bradley.d.volkin@intel.com> writes:

> On Wed, Sep 24, 2014 at 05:50:30AM -0700, Mika Kuoppala wrote:
>> In null/golden context there are multiple state commands where
>> the actual state is always zero. For more compact batch representation
>> add a macro which just emits command and the rest of the state as zero.
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  tools/null_state_gen/intel_batchbuffer.c    | 12 ++++++++++++
>>  tools/null_state_gen/intel_batchbuffer.h    |  6 +++++-
>>  tools/null_state_gen/intel_null_state_gen.c | 12 +++++++++++-
>>  3 files changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/null_state_gen/intel_batchbuffer.c b/tools/null_state_gen/intel_batchbuffer.c
>> index 2a0b340..6e86aef 100644
>> --- a/tools/null_state_gen/intel_batchbuffer.c
>> +++ b/tools/null_state_gen/intel_batchbuffer.c
>> @@ -274,3 +274,15 @@ const char *intel_batch_type_as_str(const struct bb_item *item)
>>  
>>  	return "UNKNOWN";
>>  }
>> +
>> +void intel_batch_cmd_emit_null(struct intel_batchbuffer *batch, const int cmd, const int len, const char *str)
>> +{
>> +	int i;
>> +
>> +	assert(len > 1);
>> +
>> +	bb_area_emit(batch->cmds, (cmd | (len - 2)), CMD, str);
>
> I'm a little hesitant about the (len - 2) here just because there are
> a number of commands for which those bits are not the length field, and
> one or two where the length field is (len - 1). I think we're unlikely
> to use those commands in a null batch, so maybe it's not something to
> worry about.
>

Noted. Perhaps this could be encoded into the macro name so that it
would clear that it is for a len 2 bias null setup.

>> +
>> +	for (i = 1; i < len; i++)
>> +		OUT_BATCH(0);
>> +}
>> diff --git a/tools/null_state_gen/intel_batchbuffer.h b/tools/null_state_gen/intel_batchbuffer.h
>> index e44c5c9..b4eed25 100644
>> --- a/tools/null_state_gen/intel_batchbuffer.h
>> +++ b/tools/null_state_gen/intel_batchbuffer.h
>> @@ -34,7 +34,7 @@
>>  #include <stdint.h>
>>  
>>  #define MAX_RELOCS 64
>> -#define MAX_ITEMS 4096
>> +#define MAX_ITEMS 1024
>>  #define MAX_STRLEN 256
>>  
>>  #define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1))
>> @@ -69,6 +69,7 @@ struct intel_batchbuffer {
>>  
>>  struct intel_batchbuffer *intel_batchbuffer_create(void);
>>  
>> +#define OUT_CMD(cmd, len) intel_batch_cmd_emit_null(batch, cmd, len, #cmd " " #len)
>>  #define OUT_BATCH(d) bb_area_emit(batch->cmds, d, CMD, #d)
>>  #define OUT_BATCH_STATE_OFFSET(d) bb_area_emit(batch->cmds, d, STATE_OFFSET, #d)
>>  #define OUT_RELOC(batch, read_domain, write_domain, d) bb_area_emit(batch->cmds, d, RELOC, #d)
>> @@ -81,6 +82,7 @@ uint32_t intel_batch_state_copy(struct intel_batchbuffer *batch, void *d, unsign
>>  				const char *name);
>>  uint32_t intel_batch_state_alloc(struct intel_batchbuffer *batch, unsigned bytes, unsigned align,
>>  				 const char *name);
>> +uint32_t intel_batch_state_offset(struct intel_batchbuffer *batch, unsigned align);
>
> I see that at least patch 2 uses this function outside of intel_batchbuffer.c
> but I wasn't expecting this change based on the commit title or message.

This should have been in included in the second patch, jumped here by
mistake. The second patch is missing s-o-b also.

Thanks,
-Mika

> Thanks,
> Brad
>
>>  
>>  unsigned intel_batch_num_cmds(struct intel_batchbuffer *batch);
>>  
>> @@ -94,4 +96,6 @@ const char *intel_batch_type_as_str(const struct bb_item *item);
>>  void bb_area_emit(struct bb_area *a, uint32_t dword, item_type type, const char *str);
>>  void bb_area_emit_offset(struct bb_area *a, unsigned i, uint32_t dword, item_type type, const char *str);
>>  
>> +void intel_batch_cmd_emit_null(struct intel_batchbuffer *batch, const int cmd, const int len, const char *str);
>> +
>>  #endif
>> diff --git a/tools/null_state_gen/intel_null_state_gen.c b/tools/null_state_gen/intel_null_state_gen.c
>> index b337706..a7eb22b 100644
>> --- a/tools/null_state_gen/intel_null_state_gen.c
>> +++ b/tools/null_state_gen/intel_null_state_gen.c
>> @@ -23,6 +23,9 @@ static void print_usage(char *s)
>>  static int print_state(int gen, struct intel_batchbuffer *batch)
>>  {
>>  	int i;
>> +	unsigned long cmds;
>> +
>> +	fprintf(stderr, "Generating for gen%d\n", gen);
>>  
>>  	printf("#include \"intel_renderstate.h\"\n\n");
>>  
>> @@ -43,8 +46,10 @@ static int print_state(int gen, struct intel_batchbuffer *batch)
>>  			printf("\t /* 0x%08x %s '%s' */", i * 4,
>>  			       intel_batch_type_as_str(cmd), cmd->str);
>>  
>> -		if (i * 4 == batch->cmds_end_offset)
>> +		if (i * 4 == batch->cmds_end_offset) {
>> +			cmds = i + 1;
>>  			printf("\t /* cmds end */");
>> +		}
>>  
>>  		if (intel_batch_is_reloc(batch, i))
>>  			printf("\t /* reloc */");
>> @@ -60,6 +65,11 @@ static int print_state(int gen, struct intel_batchbuffer *batch)
>>  
>>  	printf("};\n\nRO_RENDERSTATE(%d);\n", gen);
>>  
>> +	fprintf(stderr, "Commands %lu (%lu bytes)\n", cmds, cmds * 4);
>> +	fprintf(stderr, "State    %lu (%lu bytes)\n", batch->state->num_items, batch->state->num_items * 4);
>> +	fprintf(stderr, "Total    %lu (%lu bytes)\n", batch->cmds->num_items, batch->cmds->num_items * 4);
>> +	fprintf(stderr, "\n");
>> +
>>  	return 0;
>>  }
>>  
>> -- 
>> 1.9.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx