From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 5/6] drm/i915: Reduce pointer indirection during cmd parser lookup
Date: Fri, 20 Nov 2015 17:27:43 +0200 [thread overview]
Message-ID: <20151120152743.GQ4437@intel.com> (raw)
In-Reply-To: <1448016961-25331-6-git-send-email-chris@chris-wilson.co.uk>
On Fri, Nov 20, 2015 at 10:56:00AM +0000, Chris Wilson wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 51 ++++++++--------------------------
> drivers/gpu/drm/i915/i915_drv.h | 4 ++-
> 2 files changed, 14 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index cfd07bfe6e75..ea9df2bb87de 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -113,7 +113,7 @@
>
> /* Command Mask Fixed Len Action
> ---------------------------------------------------------- */
> -static const struct drm_i915_cmd_descriptor common_cmds[] = {
> +static struct drm_i915_cmd_descriptor common_cmds[] = {
I'm a little sad to see the const gone. All this gets moved out of
rodata.
> CMD( MI_NOOP, SMI, F, 1, S ),
> CMD( MI_USER_INTERRUPT, SMI, F, 1, R ),
> CMD( MI_WAIT_FOR_EVENT, SMI, F, 1, M ),
> @@ -146,7 +146,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
> CMD( MI_BATCH_BUFFER_START, SMI, !F, 0xFF, S ),
> };
>
> -static const struct drm_i915_cmd_descriptor render_cmds[] = {
> +static struct drm_i915_cmd_descriptor render_cmds[] = {
> CMD( MI_FLUSH, SMI, F, 1, S ),
> CMD( MI_ARB_ON_OFF, SMI, F, 1, R ),
> CMD( MI_PREDICATE, SMI, F, 1, S ),
> @@ -207,7 +207,7 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
> }}, ),
> };
>
> -static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> +static struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> CMD( MI_SET_PREDICATE, SMI, F, 1, S ),
> CMD( MI_RS_CONTROL, SMI, F, 1, S ),
> CMD( MI_URB_ATOMIC_ALLOC, SMI, F, 1, S ),
> @@ -229,7 +229,7 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> CMD( GFX_OP_3DSTATE_BINDING_TABLE_EDIT_PS, S3D, !F, 0x1FF, S ),
> };
>
> -static const struct drm_i915_cmd_descriptor video_cmds[] = {
> +static struct drm_i915_cmd_descriptor video_cmds[] = {
> CMD( MI_ARB_ON_OFF, SMI, F, 1, R ),
> CMD( MI_SET_APPID, SMI, F, 1, S ),
> CMD( MI_STORE_DWORD_IMM, SMI, !F, 0xFF, B,
> @@ -273,7 +273,7 @@ static const struct drm_i915_cmd_descriptor video_cmds[] = {
> CMD( MFX_WAIT, SMFX, F, 1, S ),
> };
>
> -static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> +static struct drm_i915_cmd_descriptor vecs_cmds[] = {
> CMD( MI_ARB_ON_OFF, SMI, F, 1, R ),
> CMD( MI_SET_APPID, SMI, F, 1, S ),
> CMD( MI_STORE_DWORD_IMM, SMI, !F, 0xFF, B,
> @@ -311,7 +311,7 @@ static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> }}, ),
> };
>
> -static const struct drm_i915_cmd_descriptor blt_cmds[] = {
> +static struct drm_i915_cmd_descriptor blt_cmds[] = {
> CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ),
> CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3FF, B,
> .bits = {{
> @@ -344,7 +344,7 @@ static const struct drm_i915_cmd_descriptor blt_cmds[] = {
> CMD( SRC_COPY_BLT, S2D, !F, 0x3F, S ),
> };
>
> -static const struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
> +static struct drm_i915_cmd_descriptor hsw_blt_cmds[] = {
> CMD( MI_LOAD_SCAN_LINES_INCL, SMI, !F, 0x3F, M ),
> CMD( MI_LOAD_SCAN_LINES_EXCL, SMI, !F, 0x3F, R ),
> };
> @@ -618,11 +618,6 @@ static bool validate_regs_sorted(struct intel_engine_cs *ring)
> ring->master_reg_count);
> }
>
> -struct cmd_node {
> - const struct drm_i915_cmd_descriptor *desc;
> - struct hlist_node node;
> -};
> -
> /*
> * Different command ranges have different numbers of bits for the opcode. For
> * example, MI commands use bits 31:23 while 3D commands use bits 31:16. The
> @@ -651,16 +646,8 @@ static int init_hash_table(struct intel_engine_cs *ring,
> const struct drm_i915_cmd_table *table = &cmd_tables[i];
>
> for (j = 0; j < table->count; j++) {
> - const struct drm_i915_cmd_descriptor *desc =
> - &table->table[j];
> - struct cmd_node *desc_node =
> - kmalloc(sizeof(*desc_node), GFP_KERNEL);
> -
> - if (!desc_node)
> - return -ENOMEM;
init_hash_table() can no longer fail -> void?
> -
> - desc_node->desc = desc;
> - hash_add(ring->cmd_hash, &desc_node->node,
> + struct drm_i915_cmd_descriptor *desc = &table->table[j];
> + hash_add(ring->cmd_hash, &desc->node[ring->id],
> desc->cmd.value & CMD_HASH_MASK);
> }
> }
> @@ -668,18 +655,6 @@ static int init_hash_table(struct intel_engine_cs *ring,
> return 0;
> }
>
> -static void fini_hash_table(struct intel_engine_cs *ring)
> -{
> - struct hlist_node *tmp;
> - struct cmd_node *desc_node;
> - int i;
> -
> - hash_for_each_safe(ring->cmd_hash, i, tmp, desc_node, node) {
> - hash_del(&desc_node->node);
> - kfree(desc_node);
> - }
> -}
> -
> /**
> * i915_cmd_parser_init_ring() - set cmd parser related fields for a ringbuffer
> * @ring: the ringbuffer to initialize
> @@ -770,7 +745,6 @@ int i915_cmd_parser_init_ring(struct intel_engine_cs *ring)
> ret = init_hash_table(ring, cmd_tables, cmd_table_count);
> if (ret) {
> DRM_ERROR("CMD: cmd_parser_init failed!\n");
> - fini_hash_table(ring);
> return ret;
> }
>
> @@ -790,8 +764,6 @@ void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring)
> {
> if (!ring->needs_cmd_parser)
> return;
> -
> - fini_hash_table(ring);
> }
i915_cmd_parser_fini_ring() is a nop now. Kill it?
>
> /*
> @@ -801,11 +773,10 @@ static const struct drm_i915_cmd_descriptor*
> find_cmd_in_table(struct intel_engine_cs *ring,
> u32 cmd_header)
> {
> - struct cmd_node *desc_node;
> + const struct drm_i915_cmd_descriptor *desc;
>
> - hash_for_each_possible(ring->cmd_hash, desc_node, node,
> + hash_for_each_possible(ring->cmd_hash, desc, node[ring->id],
> cmd_header & CMD_HASH_MASK) {
> - const struct drm_i915_cmd_descriptor *desc = desc_node->desc;
> if (((cmd_header ^ desc->cmd.value) & desc->cmd.mask) == 0)
> return desc;
At least we still return this as const, so the caller can't accidentally
clobber it, even if we lose the rodata protection.
Apart from the int vs. void thing and the nop
i915_cmd_parser_fini_ring() the patch looks fine to me.
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 28d5bfceae3b..5960f76f1438 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2396,6 +2396,8 @@ struct drm_i915_cmd_descriptor {
> u32 condition_offset;
> u32 condition_mask;
> } bits[MAX_CMD_DESC_BITMASKS];
> +
> + struct hlist_node node[I915_NUM_RINGS];
> };
>
> /*
> @@ -2405,7 +2407,7 @@ struct drm_i915_cmd_descriptor {
> * descriptors, which must be sorted with command opcodes in ascending order.
> */
> struct drm_i915_cmd_table {
> - const struct drm_i915_cmd_descriptor *table;
> + struct drm_i915_cmd_descriptor *table;
> int count;
> };
>
> --
> 2.6.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2015-11-20 15:27 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-20 10:55 cmdparser overhead reduction Chris Wilson
2015-11-20 10:55 ` [PATCH v2 1/6] drm/i915: Eliminate vmap overhead for cmd parser Chris Wilson
2015-11-20 14:41 ` Ville Syrjälä
2015-11-20 14:52 ` Chris Wilson
2015-11-20 15:31 ` [PATCH v3] " Chris Wilson
2015-11-25 19:51 ` Ville Syrjälä
2015-11-25 20:13 ` Chris Wilson
2015-11-25 21:15 ` Ville Syrjälä
2015-11-20 10:55 ` [PATCH v2 2/6] drm/i915: Cache last cmd descriptor when parsing Chris Wilson
2015-11-20 15:08 ` Ville Syrjälä
2015-11-20 15:44 ` Chris Wilson
2015-12-01 17:30 ` Ville Syrjälä
2015-11-20 10:55 ` [PATCH v2 3/6] drm/i915: Use WC copies on !llc platforms for the command parser Chris Wilson
2015-11-20 15:05 ` Ville Syrjälä
2015-11-20 15:22 ` Chris Wilson
2015-12-01 17:32 ` Ville Syrjälä
2015-11-20 10:55 ` [PATCH v2 4/6] drm/i915: Reduce arithmetic operations during cmd parser lookup Chris Wilson
2015-11-20 15:02 ` Ville Syrjälä
2015-11-20 10:56 ` [PATCH v2 5/6] drm/i915: Reduce pointer indirection " Chris Wilson
2015-11-20 15:27 ` Ville Syrjälä [this message]
2015-11-20 15:34 ` Chris Wilson
2015-11-20 15:47 ` Ville Syrjälä
2015-11-23 8:09 ` Jani Nikula
2015-12-01 17:39 ` Ville Syrjälä
2015-11-20 10:56 ` [PATCH v2 6/6] drm/i915: Improve hash function for the command parser Chris Wilson
2015-11-20 15:13 ` Ville Syrjälä
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151120152743.GQ4437@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox