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

[Volkin, Bradley D]

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