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@xxxxxxxxx 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
