On Thu, May 08, 2014 at 08:45:07AM -0700, Ville Syrjälä wrote: > On Thu, May 08, 2014 at 08:27:16AM -0700, 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. > > > > I don't have a strong preference on this. Whatever people prefer. > > If the memory allocation fails there's probably not much point in > trying to limp along and continue the driver init. So just pass error > up and let the caller deal with it. > > Looking at the error paths up from ring init, we probably leak a ton > of junk but at least the kernel should remain otherwise operational. Passing the error up is probably a good option. There was a recent change to mark the GPU as wedged if ring init fails, which seems like a reasonable enough response to parser init failure. I'll have to look at the cleanup paths. The per-ring parser init can probably move around to avoid excessive additional cleanup code on failure paths. Brad > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
