[Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
Jani Nikula
jani.nikula at linux.intel.com
Wed Feb 5 16:15:35 CET 2014
On Wed, 29 Jan 2014, bradley.d.volkin at intel.com wrote:
> From: Brad Volkin <bradley.d.volkin at 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 at intel.com>
> ---
> 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 ;).
>
> 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 at 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
> +
> +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.
> + }
> +
> + 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!
> +}
> +
> +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.
> + }
> +
> + 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.
> +{
> + 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.
> +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.
> };
>
> 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 at lists.freedesktop.org, 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.
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list