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

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

On 05/08/2014 04:27 PM, Volkin, Bradley D wrote:
> On Thu, May 08, 2014 at 02:56:05AM -0700, 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?
>>
>> I know it won't happen almost ever so it's a minor point really. I just
>> dislike actively hosing the whole system if it is avoidable.
>
> Hi Tvrtko,
>
> I agree that a BUG_ON might be harsh here. I suppose we could log an
> error and disable the command parser. Most command buffers would
> still go through fine but HW parsing would reject some that the SW
> parser might otherwise allow. That could be a bit odd if we ever did
> get a failure - apps/functionality that worked the last time I booted
> suddenly don't this time. The issue would be in the log though.

That would indeed be weird.

So command parser is just a more permissive whitelist, a super set of 
hardware parser?

> I don't have a strong preference on this. Whatever people prefer.

Initially I was thinking of just rejecting all batch buffers if such 
fundamental initialisation fails. It beats BUG_ON in the sense that it 
leaves the system alive and it doesn't suffer from the random subtle 
behaviour like if it would just disable the command parser.

It is a bit theoretical because it is likely system won't be too healthy 
anyway if such a tiny allocation fails but you never know. It's better 
not to crash it if we don't have to.

Regards,

Tvrtko