Re: [PATCH 02/13] drm/i915: Implement command buffer parsing logic

Chris Wilson
2014-01-30T10:57:26Z

On Thu, Jan 30, 2014 at 10:07:42AM +0100, Daniel Vetter wrote:
> On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.volkin@intel.com wrote:
> > From: Brad Volkin <bradley.d.volkin@intel.com>
> >
> > The command parser scans batch buffers submitted via execbuffer ioctls before
> > the driver submits them to hardware. At a high level, it looks for several
> > things:
> >
> > 1) Commands which are explicitly defined as privileged or which should only be
> > used by the kernel driver. The parser generally rejects such commands, with
> > the provision that it may allow some from the drm master process.
> > 2) Commands which access registers. To support correct/enhanced userspace
> > functionality, particularly certain OpenGL extensions, the parser provides a
> > whitelist of registers which userspace may safely access (for both normal and
> > drm master processes).
> > 3) Commands which access privileged memory (i.e. GGTT, HWS page, etc). The
> > parser always rejects such commands.
> >
> > Each ring maintains tables of commands and registers which the parser uses in
> > scanning batch buffers submitted to that ring.
> >
> > The set of commands that the parser must check for is significantly smaller
> > than the number of commands supported, especially on the render ring. As such,
> > the parser tables (built up in subsequent patches) contain only those commands
> > required by the parser. This generally works because command opcode ranges have
> > standard command length encodings. So for commands that the parser does not need
> > to check, it can easily skip them. This is implementated via a per-ring length
> > decoding vfunc.
> >
> > Unfortunately, there are a number of commands that do not follow the standard
> > length encoding for their opcode range, primarily amongst the MI_* commands. To
> > handle this, the parser provides a way to define explicit "skip" entries in the
> > per-ring command tables.
> >
> > Other command table entries will map fairly directly to high level categories
> > mentioned above: rejected, master-only, register whitelist. A number of checks,
> > including the privileged memory checks, are implemented via a general bitmasking
> > mechanism.
> >
> > OTC-Tracker: AXIA-4631
> > Change-Id: I50b98c71c6655893291c78a2d1b8954577b37a30
> > Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
>
>
> > +#include "i915_drv.h"
> > +
> > +#define CLIENT_MASK 0xE0000000
> > +#define SUBCLIENT_MASK 0x18000000
> > +#define MI_CLIENT 0x00000000
> > +#define RC_CLIENT 0x60000000
> > +#define BC_CLIENT 0x40000000
> > +#define MEDIA_SUBCLIENT 0x10000000
>
> I think these would fit neatly right next to all the other MI_* #defines
> in i915_reg.h. The other idea that just crossed my mind is to extract all
> the command #defines into a new i915_cmd.h (included by i915_drv.h by
> default) since i915_reg.h is already giant.

i915_cmd.h please. (i915_cs.h? i915_command_stream.h?)
-Chris

--
Chris Wilson, Intel Open Source Technology Centre