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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC