On Wed, Feb 05, 2014 at 07:15:35AM -0800, Jani Nikula wrote:
> On Wed, 29 Jan 2014, bradley.d.volkin@xxxxxxxxx wrote:
> > From: Brad Volkin <bradley.d.volkin@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/Makefile | 3 +-
> > drivers/gpu/drm/i915/i915_cmd_parser.c | 404 +++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/i915_drv.h | 94 +++++++
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 17 ++
> > drivers/gpu/drm/i915/i915_params.c | 5 +
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 32 +++
> > 7 files changed, 556 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/i915/i915_cmd_parser.c
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 4850494..2da81bf 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -47,7 +47,8 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
> > dvo_tfp410.o \
> > dvo_sil164.o \
> > dvo_ns2501.o \
> > - i915_gem_dmabuf.o
> > + i915_gem_dmabuf.o \
> > + i915_cmd_parser.o
>
> If you add this anywhere but last, you only need to touch one line
> instead of two. It's nitpicky, but helps with things like git blame
> (which would now point at you for i915_gem_dmabuf.o too ;).
Sounds good
>
> >
> > i915-$(CONFIG_COMPAT) += i915_ioc32.o
> >
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > new file mode 100644
> > index 0000000..7639dbc
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -0,0 +1,404 @@
> > +/*
> > + * Copyright © 2013 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + * Brad Volkin <bradley.d.volkin@xxxxxxxxx>
> > + *
> > + */
> > +
> > +#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
> > +
> > +static u32 gen7_render_get_cmd_length_mask(u32 cmd_header)
> > +{
> > + u32 client = cmd_header & CLIENT_MASK;
> > + u32 subclient = cmd_header & SUBCLIENT_MASK;
> > +
> > + if (client == MI_CLIENT)
> > + return 0x3F;
> > + else if (client == RC_CLIENT) {
> > + if (subclient == MEDIA_SUBCLIENT)
> > + return 0xFFFF;
> > + else
> > + return 0xFF;
> > + }
> > +
> > + DRM_DEBUG_DRIVER("CMD: Abnormal rcs cmd length! 0x%08X\n", cmd_header);
> > + return 0;
> > +}
> > +
> > +static u32 gen7_bsd_get_cmd_length_mask(u32 cmd_header)
> > +{
> > + u32 client = cmd_header & CLIENT_MASK;
> > + u32 subclient = cmd_header & SUBCLIENT_MASK;
> > +
> > + if (client == MI_CLIENT)
> > + return 0x3F;
> > + else if (client == RC_CLIENT) {
> > + if (subclient == MEDIA_SUBCLIENT)
> > + return 0xFFF;
> > + else
> > + return 0xFF;
> > + }
> > +
> > + DRM_DEBUG_DRIVER("CMD: Abnormal bsd cmd length! 0x%08X\n", cmd_header);
> > + return 0;
> > +}
> > +
> > +static u32 gen7_blt_get_cmd_length_mask(u32 cmd_header)
> > +{
> > + u32 client = cmd_header & CLIENT_MASK;
> > +
> > + if (client == MI_CLIENT)
> > + return 0x3F;
> > + else if (client == BC_CLIENT)
> > + return 0xFF;
> > +
> > + DRM_DEBUG_DRIVER("CMD: Abnormal blt cmd length! 0x%08X\n", cmd_header);
> > + return 0;
> > +}
> > +
> > +static void validate_cmds_sorted(struct intel_ring_buffer *ring)
> > +{
> > + int i;
> > +
> > + if (!ring->cmd_tables || ring->cmd_table_count == 0)
> > + return;
> > +
> > + for (i = 0; i < ring->cmd_table_count; i++) {
> > + const struct drm_i915_cmd_table *table = &ring->cmd_tables[i];
> > + u32 previous = 0;
> > + int j;
> > +
> > + for (j = 0; j < table->count; j++) {
> > + const struct drm_i915_cmd_descriptor *desc =
> > + &table->table[i];
> > + u32 curr = desc->cmd.value & desc->cmd.mask;
> > +
> > + if (curr < previous) {
> > + DRM_ERROR("CMD: table not sorted ring=%d table=%d entry=%d cmd=0x%08X\n",
> > + ring->id, i, j, curr);
> > + return;
>
> So this checks the hand-filled tables, right?
>
> I think this should not stop at the first error, but rather scan the
> whole table and DRM_ERROR all cases where curr < previous, and after the
> full scan BUG_ON() if there were any errors.
Will change, and below
>
> > + }
> > +
> > + previous = curr;
> > + }
> > + }
> > +}
> > +
> > +static void check_sorted(int ring_id, const u32 *reg_table, int reg_count)
> > +{
> > + int i;
> > + u32 previous = 0;
> > +
> > + for (i = 0; i < reg_count; i++) {
> > + u32 curr = reg_table[i];
> > +
> > + if (curr < previous) {
> > + DRM_ERROR("CMD: table not sorted ring=%d entry=%d reg=0x%08X\n",
> > + ring_id, i, curr);
> > + return;
>
> Same here.
>
> > + }
> > +
> > + previous = curr;
> > + }
> > +}
> > +
> > +static void validate_regs_sorted(struct intel_ring_buffer *ring)
> > +{
> > + if (ring->reg_table && ring->reg_count > 0)
> > + check_sorted(ring->id, ring->reg_table, ring->reg_count);
> > +
> > + if (ring->master_reg_table && ring->master_reg_count > 0)
> > + check_sorted(ring->id, ring->master_reg_table,
> > + ring->master_reg_count);
>
> Somehow I think the ifs here are redundant. check_sorted() is a no-op if
> reg_count == 0, and if reg_count > 0 while reg_table == NULL, it
> deserves to oops!
Agreed
>
> > +}
> > +
> > +void i915_cmd_parser_init_ring(struct intel_ring_buffer *ring)
> > +{
> > + if (!IS_GEN7(ring->dev))
> > + return;
> > +
> > + switch (ring->id) {
> > + case RCS:
> > + ring->get_cmd_length_mask = gen7_render_get_cmd_length_mask;
> > + break;
> > + case VCS:
> > + ring->get_cmd_length_mask = gen7_bsd_get_cmd_length_mask;
> > + break;
> > + case BCS:
> > + ring->get_cmd_length_mask = gen7_blt_get_cmd_length_mask;
> > + break;
> > + case VECS:
> > + /* VECS can use the same length_mask function as VCS */
> > + ring->get_cmd_length_mask = gen7_bsd_get_cmd_length_mask;
> > + break;
> > + default:
> > + DRM_ERROR("CMD: cmd_parser_init with unknown ring: %d\n",
> > + ring->id);
>
> You'll oops later for calling NULL ring->get_cmd_length_mask(), so might
> as well BUG() here.
Agreed
>
> > + }
> > +
> > + validate_cmds_sorted(ring);
> > + validate_regs_sorted(ring);
> > +}
> > +
> > +static const struct drm_i915_cmd_descriptor*
> > +find_cmd_in_table(const struct drm_i915_cmd_table *table,
> > + u32 cmd_header)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < table->count; i++) {
> > + const struct drm_i915_cmd_descriptor *desc = &table->table[i];
> > + u32 masked_cmd = desc->cmd.mask & cmd_header;
> > + u32 masked_value = desc->cmd.value & desc->cmd.mask;
> > +
> > + if (masked_cmd == masked_value)
> > + return desc;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * Returns a pointer to a descriptor for the command specified by cmd_header.
> > + *
> > + * The caller must supply space for a default descriptor via the default_desc
> > + * parameter. If no descriptor for the specified command exists in the ring's
> > + * command parser tables, this function fills in default_desc based on the
> > + * ring's default length encoding and returns default_desc.
> > + */
> > +static const struct drm_i915_cmd_descriptor*
> > +find_cmd(struct intel_ring_buffer *ring,
> > + u32 cmd_header,
> > + struct drm_i915_cmd_descriptor *default_desc)
> > +{
> > + u32 mask;
> > + int i;
> > +
> > + for (i = 0; i < ring->cmd_table_count; i++) {
> > + const struct drm_i915_cmd_descriptor *desc;
> > +
> > + desc = find_cmd_in_table(&ring->cmd_tables[i], cmd_header);
> > + if (desc)
> > + return desc;
> > + }
> > +
> > + mask = ring->get_cmd_length_mask(cmd_header);
> > + if (!mask)
> > + return NULL;
> > +
> > + BUG_ON(!default_desc);
> > + default_desc->flags = CMD_DESC_SKIP;
> > + default_desc->length.mask = mask;
> > +
> > + return default_desc;
> > +}
> > +
> > +static int valid_reg(const u32 *table, int count, u32 addr)
>
> I like bools for boolean stuff.
I'll reevaluate int vs bool throughout. I think the use is a bit inconsistent
throughout the driver at the moment, but I don't mind improving it.
>
> > +{
> > + if (table && count != 0) {
> > + int i;
> > +
> > + for (i = 0; i < count; i++) {
> > + if (table[i] == addr)
> > + return 1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static u32 *vmap_batch(struct drm_i915_gem_object *obj)
> > +{
> > + int i;
> > + void *addr = NULL;
> > + struct sg_page_iter sg_iter;
> > + struct page **pages;
> > +
> > + pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages));
> > + if (pages == NULL) {
> > + DRM_DEBUG_DRIVER("Failed to get space for pages\n");
> > + goto finish;
> > + }
> > +
> > + i = 0;
> > + for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
> > + pages[i] = sg_page_iter_page(&sg_iter);
> > + i++;
> > + }
> > +
> > + addr = vmap(pages, i, 0, PAGE_KERNEL);
> > + if (addr == NULL) {
> > + DRM_DEBUG_DRIVER("Failed to vmap pages\n");
> > + goto finish;
> > + }
> > +
> > +finish:
> > + if (pages)
> > + drm_free_large(pages);
> > + return (u32*)addr;
> > +}
> > +
> > +int i915_needs_cmd_parser(struct intel_ring_buffer *ring)
>
> bool
>
> > +{
> > + /* No command tables indicates a platform without parsing */
> > + if (!ring->cmd_tables)
> > + return 0;
> > +
> > + return i915.enable_cmd_parser;
> > +}
> > +
> > +#define LENGTH_BIAS 2
> > +
> > +int i915_parse_cmds(struct intel_ring_buffer *ring,
> > + struct drm_i915_gem_object *batch_obj,
> > + u32 batch_start_offset,
> > + bool is_master)
> > +{
> > + int ret = 0;
> > + u32 *cmd, *batch_base, *batch_end;
> > + struct drm_i915_cmd_descriptor default_desc = { 0 };
> > + int needs_clflush = 0;
> > +
> > + ret = i915_gem_obj_prepare_shmem_read(batch_obj, &needs_clflush);
> > + if (ret) {
> > + DRM_DEBUG_DRIVER("CMD: failed to prep read\n");
> > + return ret;
> > + }
> > +
> > + batch_base = vmap_batch(batch_obj);
> > + if (!batch_base) {
> > + DRM_DEBUG_DRIVER("CMD: Failed to vmap batch\n");
> > + i915_gem_object_unpin_pages(batch_obj);
> > + return -ENOMEM;
> > + }
> > +
> > + if (needs_clflush)
> > + drm_clflush_virt_range((char *)batch_base, batch_obj->base.size);
> > +
> > + cmd = batch_base + (batch_start_offset / sizeof(*cmd));
> > + batch_end = cmd + (batch_obj->base.size / sizeof(*batch_end));
> > +
> > + while (cmd < batch_end) {
> > + const struct drm_i915_cmd_descriptor *desc;
> > + u32 length;
> > +
> > + if (*cmd == MI_BATCH_BUFFER_END)
> > + break;
> > +
> > + desc = find_cmd(ring, *cmd, &default_desc);
> > + if (!desc) {
> > + DRM_DEBUG_DRIVER("CMD: Unrecognized command: 0x%08X\n",
> > + *cmd);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + if (desc->flags & CMD_DESC_FIXED)
> > + length = desc->length.fixed;
> > + else
> > + length = ((*cmd & desc->length.mask) + LENGTH_BIAS);
> > +
> > + if ((batch_end - cmd) < length) {
> > + DRM_DEBUG_DRIVER("CMD: Command length exceeds batch length: 0x%08X length=%d batchlen=%ld\n",
> > + *cmd,
> > + length,
> > + batch_end - cmd);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + if (desc->flags & CMD_DESC_REJECT) {
> > + DRM_DEBUG_DRIVER("CMD: Rejected command: 0x%08X\n", *cmd);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + if ((desc->flags & CMD_DESC_MASTER) && !is_master) {
> > + DRM_DEBUG_DRIVER("CMD: Rejected master-only command: 0x%08X\n",
> > + *cmd);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + if (desc->flags & CMD_DESC_REGISTER) {
> > + u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
> > +
> > + if (!valid_reg(ring->reg_table,
> > + ring->reg_count, reg_addr)) {
> > + if (!is_master ||
> > + !valid_reg(ring->master_reg_table,
> > + ring->master_reg_count,
> > + reg_addr)) {
> > + DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n",
> > + reg_addr,
> > + *cmd,
> > + ring->id);
> > + ret = -EINVAL;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (desc->flags & CMD_DESC_BITMASK) {
> > + int i;
> > +
> > + for (i = 0; i < desc->bits_count; i++) {
> > + u32 dword = cmd[desc->bits[i].offset] &
> > + desc->bits[i].mask;
> > +
> > + if (dword != desc->bits[i].expected) {
> > + DRM_DEBUG_DRIVER("CMD: Rejected command 0x%08X for bitmask 0x%08X (exp=0x%08X act=0x%08X) (ring=%d)\n",
> > + *cmd,
> > + desc->bits[i].mask,
> > + desc->bits[i].expected,
> > + dword, ring->id);
> > + ret = -EINVAL;
> > + break;
> > + }
> > + }
> > +
> > + if (ret)
> > + break;
> > + }
> > +
> > + cmd += length;
> > + }
> > +
> > + if (cmd >= batch_end) {
> > + DRM_DEBUG_DRIVER("CMD: Got to the end of the buffer w/o a BBE cmd!\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + vunmap(batch_base);
> > +
> > + i915_gem_object_unpin_pages(batch_obj);
> > +
> > + return ret;
> > +}
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bfb30df..8aed80f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1765,6 +1765,91 @@ struct drm_i915_file_private {
> > atomic_t rps_wait_boost;
> > };
> >
> > +/**
> > + * A command that requires special handling by the command parser.
> > + */
>
> You have plenty of kernel-doc comments here and in other patches. They
> do expect a certain format, however. Please either make them regular
> comments (the easy way) or adhere to proper kernel-doc format.
Again, current use is inconsistent in the driver, but I'll reevaluate throughout.
>
> > +struct drm_i915_cmd_descriptor {
> > + /**
> > + * Flags describing how the command parser processes the command.
> > + *
> > + * CMD_DESC_FIXED: The command has a fixed length if this is set,
> > + * a length mask if not set
> > + * CMD_DESC_SKIP: The command is allowed but does not follow the
> > + * standard length encoding for the opcode range in
> > + * which it falls
> > + * CMD_DESC_REJECT: The command is never allowed
> > + * CMD_DESC_REGISTER: The command should be checked against the
> > + * register whitelist for the appropriate ring
> > + * CMD_DESC_MASTER: The command is allowed if the submitting process
> > + * is the DRM master
> > + */
> > + u32 flags;
> > +#define CMD_DESC_FIXED (1<<0)
> > +#define CMD_DESC_SKIP (1<<1)
> > +#define CMD_DESC_REJECT (1<<2)
> > +#define CMD_DESC_REGISTER (1<<3)
> > +#define CMD_DESC_BITMASK (1<<4)
> > +#define CMD_DESC_MASTER (1<<5)
>
> Feels like flags should be named FLAG, not DESC. *shrug*.
>
> > +
> > + /**
> > + * The command's unique identification bits and the bitmask to get them.
> > + * This isn't strictly the opcode field as defined in the spec and may
> > + * also include type, subtype, and/or subop fields.
> > + */
> > + struct {
> > + u32 value;
> > + u32 mask;
> > + } cmd;
> > +
> > + /**
> > + * The command's length. The command is either fixed length (i.e. does
> > + * not include a length field) or has a length field mask. The flag
> > + * CMD_DESC_FIXED indicates a fixed length. Otherwise, the command has
> > + * a length mask. All command entries in a command table must include
> > + * length information.
> > + */
> > + union {
> > + u32 fixed;
> > + u32 mask;
> > + } length;
> > +
> > + /**
> > + * Describes where to find a register address in the command to check
> > + * against the ring's register whitelist. Only valid if flags has the
> > + * CMD_DESC_REGISTER bit set.
> > + */
> > + struct {
> > + u32 offset;
> > + u32 mask;
> > + } reg;
> > +
> > +#define MAX_CMD_DESC_BITMASKS 3
> > + /**
> > + * Describes command checks where a particular dword is masked and
> > + * compared against an expected value. If the command does not match
> > + * the expected value, the parser rejects it. Only valid if flags has
> > + * the CMD_DESC_BITMASK bit set.
> > + */
> > + struct {
> > + u32 offset;
> > + u32 mask;
> > + u32 expected;
> > + } bits[MAX_CMD_DESC_BITMASKS];
> > + /** Number of valid entries in the bits array */
> > + int bits_count;
> > +};
> > +
> > +/**
> > + * A table of commands requiring special handling by the command parser.
> > + *
> > + * Each ring has an array of tables. Each table consists of an array of command
> > + * descriptors, which must be sorted with command opcodes in ascending order.
> > + */
> > +struct drm_i915_cmd_table {
> > + const struct drm_i915_cmd_descriptor *table;
> > + int count;
> > +};
> > +
> > #define INTEL_INFO(dev) (to_i915(dev)->info)
> >
> > #define IS_I830(dev) ((dev)->pdev->device == 0x3577)
> > @@ -1923,6 +2008,7 @@ struct i915_params {
> > bool prefault_disable;
> > bool reset;
> > int invert_brightness;
> > + int enable_cmd_parser;
> > };
> > extern struct i915_params i915 __read_mostly;
> >
> > @@ -2428,6 +2514,14 @@ void i915_destroy_error_state(struct drm_device *dev);
> > void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone);
> > const char *i915_cache_level_str(int type);
> >
> > +/* i915_cmd_parser.c */
> > +void i915_cmd_parser_init_ring(struct intel_ring_buffer *ring);
> > +int i915_needs_cmd_parser(struct intel_ring_buffer *ring);
> > +int i915_parse_cmds(struct intel_ring_buffer *ring,
> > + struct drm_i915_gem_object *batch_obj,
> > + u32 batch_start_offset,
> > + bool is_master);
> > +
> > /* i915_suspend.c */
> > extern int i915_save_state(struct drm_device *dev);
> > extern int i915_restore_state(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 032def9..c953445 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1180,6 +1180,23 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > }
> > batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;
> >
> > + if (i915_needs_cmd_parser(ring)) {
> > + ret = i915_parse_cmds(ring,
> > + batch_obj,
> > + args->batch_start_offset,
> > + file->is_master);
> > + if (ret)
> > + goto err;
> > +
> > + /*
> > + * Set the DISPATCH_SECURE bit to remove the NON_SECURE bit
> > + * from MI_BATCH_BUFFER_START commands issued in the
> > + * dispatch_execbuffer implementations. We specifically don't
> > + * want that set when the command parser is enabled.
> > + */
> > + flags |= I915_DISPATCH_SECURE;
> > + }
> > +
> > /* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
> > * batch" bit. Hence we need to pin secure batches into the global gtt.
> > * hsw should have this fixed, but bdw mucks it up again. */
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index c743057..6d3d906 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -47,6 +47,7 @@ struct i915_params i915 __read_mostly = {
> > .prefault_disable = 0,
> > .reset = true,
> > .invert_brightness = 0,
> > + .enable_cmd_parser = 0
>
> Please add a comma in the end so the next addition won't have to, just
> like this doesn't have to touch the previous line.
Will do throughout
>
> > };
> >
> > module_param_named(modeset, i915.modeset, int, 0400);
> > @@ -153,3 +154,7 @@ MODULE_PARM_DESC(invert_brightness,
> > "report PCI device ID, subsystem vendor and subsystem device ID "
> > "to dri-devel@xxxxxxxxxxxxxxxxxxxxx, if your machine needs it. "
> > "It will then be included in an upcoming module version.");
> > +
> > +module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int, 0600);
> > +MODULE_PARM_DESC(enable_cmd_parser,
> > + "Enable command parsing (default: false)");
>
> If it's a bool, make it a bool, or change the default text to 0.
I'll change it to 0 for now. We might want to do the -1/0/1 style thing at some
point, though I don't necessarily have a good case for it right now.
- Brad
>
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index a0d61f8..77fc61d 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1388,6 +1388,8 @@ static int intel_init_ring_buffer(struct drm_device *dev,
> > if (IS_I830(ring->dev) || IS_845G(ring->dev))
> > ring->effective_size -= 128;
> >
> > + i915_cmd_parser_init_ring(ring);
> > +
> > return 0;
> >
> > err_unmap:
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index 71a73f4..cff2b35 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -162,6 +162,38 @@ struct intel_ring_buffer {
> > u32 gtt_offset;
> > volatile u32 *cpu_page;
> > } scratch;
> > +
> > + /**
> > + * Tables of commands the command parser needs to know about
> > + * for this ring.
> > + */
> > + const struct drm_i915_cmd_table *cmd_tables;
> > + int cmd_table_count;
> > +
> > + /**
> > + * Table of registers allowed in commands that read/write registers.
> > + */
> > + const u32 *reg_table;
> > + int reg_count;
> > +
> > + /**
> > + * Table of registers allowed in commands that read/write registers, but
> > + * only from the DRM master.
> > + */
> > + const u32 *master_reg_table;
> > + int master_reg_count;
> > +
> > + /**
> > + * Returns the bitmask for the length field of the specified command.
> > + * Return 0 for an unrecognized/invalid command.
> > + *
> > + * If the command parser finds an entry for a command in the ring's
> > + * cmd_tables, it gets the command's length based on the table entry.
> > + * If not, it calls this function to determine the per-ring length field
> > + * encoding for the command (i.e. certain opcode ranges use certain bits
> > + * to encode the command length in the header).
> > + */
> > + u32 (*get_cmd_length_mask)(u32 cmd_header);
> > };
>
> Plenty of non-conforming kernel-doc comments here too.
>
> >
> > static inline bool
> > --
> > 1.8.5.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
