On Fri, Nov 20, 2015 at 05:27:43PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 20, 2015 at 10:56:00AM +0000, Chris Wilson wrote:
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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.
This too can get
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
since the now unnecessary error handling doesn't actually do anything.
Feel free to keep the r-b even if you decide to throw out the error
handling parts.
>
> > }
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
