[Mesa-dev] [PATCH] intel/decoder: Use 'DWord Length' and 'bias' fields for packet length.

[Toni Lönnberg]

----- Forwarded message from Toni Lönnberg <toni.lonnberg at intel.com> -----

Date: Fri, 26 Oct 2018 17:03:54 +0300
From: Toni Lönnberg <toni.lonnberg at intel.com>
To: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
Message-ID: <20181026140354.GD3100 at intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Subject: Re: [Mesa-dev] [PATCH] intel/decoder: Use 'DWord Length' and 'bias' fields for packet length.

On Fri, Oct 26, 2018 at 02:24:19PM +0100, Lionel Landwerlin wrote:
> Overall I agree this probably what we should do rather than this long
> special case switch in gen_group_get_length().

There are some other changes that I'm trying to do because some of the opcodes are being shared between engines but have completely different formats depending on the engine they're sent to, MEDIA_CURBE_LOAD vs. MFX_SURFACE_STATE for example, making it impossible to distinguish between the two of them at the moment AFAIK.

> I have one suggestion below.
> 
> On 26/10/2018 14:07, Toni Lönnberg wrote:
> > Use the 'DWord Length' and 'bias' fields from the instruction definition to
> > parse the packet length from the command stream when possible. The hardcoded
> > mechanism is used whenever an instruction doesn't have this field.
> > ---
> >   src/intel/common/gen_decoder.c | 16 ++++++++++++++--
> >   src/intel/common/gen_decoder.h |  1 +
> >   2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder.c
> > index 5f6e7a0b808..2d58708f5dd 100644
> > --- a/src/intel/common/gen_decoder.c
> > +++ b/src/intel/common/gen_decoder.c
> > @@ -163,11 +163,15 @@ create_group(struct parser_context *ctx,
> >      group->spec = ctx->spec;
> >      group->variable = false;
> >      group->fixed_length = fixed_length;
> > +   group->dw_length = 0;
> > +   group->bias = 1;
> >      for (int i = 0; atts[i]; i += 2) {
> >         char *p;
> >         if (strcmp(atts[i], "length") == 0) {
> >            group->dw_length = strtoul(atts[i + 1], &p, 0);
> > +      } else if (strcmp(atts[i], "bias") == 0) {
> > +         group->bias = strtoul(atts[i + 1], &p, 0);
> >         }
> >      }
> > @@ -743,8 +747,16 @@ gen_group_find_field(struct gen_group *group, const char *name)
> >   int
> >   gen_group_get_length(struct gen_group *group, const uint32_t *p)
> >   {
> > -   if (group && group->fixed_length)
> > -      return group->dw_length;
> > +   if (group) {
> > +      if (group->fixed_length)
> > +         return group->dw_length;
> > +      else {
> > +         struct gen_field *field = gen_group_find_field(group, "DWord Length");
> 
> 
> Since "DWord Length" is a field we're going to use quite often, I would put
> a pointer to the field in gen_group.
> 
> You can do that in the create_field() function and save your self the
> find_field() for every instruction.

Good point.

> 
> 
> > +         if (field) {
> > +            return field_value(p[0], field->start, field->end) + group->bias;
> >           }
> > +      }
> > +   }
> 
> 
> If we had tests (I'm meant to write some), I would go as far as removing the
> whole thing below and just return 1.
> 
> It means we don't have a group and in that case we're likely going to just
> iterate dword by dword.

Since we don't have tests for this, IMHO it's better to leave it there for the time being. I don't have any information whether there are any non-single dword packets without the 'DWord Length' field, but I'm doubtful.

> 
> 
> >      uint32_t h = p[0];
> >      uint32_t type = field_value(h, 29, 31);
> > diff --git a/src/intel/common/gen_decoder.h b/src/intel/common/gen_decoder.h
> > index a80c50b6647..0b33eb32cfc 100644
> > --- a/src/intel/common/gen_decoder.h
> > +++ b/src/intel/common/gen_decoder.h
> > @@ -101,6 +101,7 @@ struct gen_group {
> >      struct gen_field *fields; /* linked list of fields */
> >      uint32_t dw_length;
> > +   uint32_t bias; /* <instruction> specific */
> >      uint32_t group_offset, group_count;
> >      uint32_t group_size;
> >      bool variable; /* <group> specific */
> 
> 

----- End forwarded message -----