Re: [PATCH] drm/i915: Use hash tables for the command parser

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 05/08/2014 02:02 PM, Damien Lespiau wrote:
> On Thu, May 08, 2014 at 01:25:33PM +0100, Tvrtko Ursulin wrote:
>>
>> On 05/08/2014 12:44 PM, Damien Lespiau wrote:
>>> On Thu, May 08, 2014 at 10:56:05AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> Hi Brad,
>>>>
>>>> On 04/28/2014 04:22 PM, bradley.d.volkin@intel.com wrote:
>>>> [snip]
>>>>> -	BUG_ON(!validate_cmds_sorted(ring));
>>>>> +	BUG_ON(!validate_cmds_sorted(ring, cmd_tables, cmd_table_count));
>>>>>   	BUG_ON(!validate_regs_sorted(ring));
>>>>> +
>>>>> +	BUG_ON(init_hash_table(ring, cmd_tables, cmd_table_count));
>>>>
>>>> Is a BUG_ON a bit harsh since the above fails only on ENOMEM condition?
>>>>
>>>> If the concern is not allowing any command execution if parser setup
>>>> has failed, it would be nicer to the system as whole to just keep
>>>> rejecting everything, but let the rest of the kernel live to enable
>>>> debug or whatever?
>>>
>>> Those number_of_cmds allocations are a bit awkward though, couldn't we
>>> just embed the hlist_node into the desciptor struct?
>>
>> Until Brad comes online, I think that is because command descriptors
>> to hash table entries are not 1-to-1.
>
> Ah, I guess the common cmds are part of several hash tables. We could at
> least turn the multiple allocations into one big allocation though.

Probably a problem since commands can be added dynamically.

Also from the memory use point of view, single allocations are 12/24 
bytes so fit into 16/32 byte slabs. That's some wastage on the face of 
it, but bunching them up would make... say common plus render commands 
are ~30 in total, so 360/720 bytes. Which would waste 152/304 bytes (512 
and 1024 slabs) vs. 120/240 bytes for individual allocations. So for 30 
command array current approach is even better. :)

Maybe make a dedicated cache for struct cmd_node?

Tvrtko