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

Daniel Vetter
2014-01-30T09:07:47Z

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.

But that should be done as a follow-up patch to avoid patch shuffling
hell.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch