[Intel-gfx] [PATCH] drm/i915: Use hash tables for the command parser

[Volkin, Bradley D]

On Thu, May 08, 2014 at 08:50:40AM -0700, Tvrtko Ursulin wrote:
> 
> 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 at 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?

Yes. The thing we're trying to accomplish is that HW rejects all
MI_LOAD/STORE_REGISTER_* commands, but it's safe and required to access
some registers from a batch in order to implement GL functionality.

> 
> > 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.

Yeah, see my response to Ville. I think there's a good solution there.

Brad

> 
> Regards,
> 
> Tvrtko