[Mesa-dev] [PATCH] intel/decoder: Use 'DWord Length' and 'bias' fields for packet length.
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Fri Oct 26 15:32:31 UTC 2018
On 26/10/2018 15:34, Toni Lönnberg wrote:
> On Fri, Oct 26, 2018 at 03:15:50PM +0100, Lionel Landwerlin wrote:
>> On 26/10/2018 15:06, Toni Lönnberg wrote:
>>> ----- 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 guess the way to do this would be to add a field engine="(3d|media|all)"
>> for each instruction in the genxml file.
>>
>> Then change gen_spec_find_instruction() to add an engine enum and do the
>> matching there.
> I'm trying to figure out a way to do this with as little modifications to the
> current genxml files as possible but so far have not come up with a solution
> that I'm perfectly comfortable with.
I think you could almost do it with a regexp, if it's "3DSTATE.*" -> 3d
command, "MI_*" -> all, and the few remaining cases are :
src/intel/genxml/gen9.xml: <instruction name="GPGPU_CSR_BASE_ADDRESS"
bias="2" length="3">
src/intel/genxml/gen9.xml: <instruction name="GPGPU_WALKER" bias="2"
length="15">
src/intel/genxml/gen9.xml: <instruction name="MEDIA_CURBE_LOAD"
bias="2" length="4">
src/intel/genxml/gen9.xml: <instruction
name="MEDIA_INTERFACE_DESCRIPTOR_LOAD" bias="2" length="4">
src/intel/genxml/gen9.xml: <instruction name="MEDIA_OBJECT" bias="2">
src/intel/genxml/gen9.xml: <instruction name="MEDIA_OBJECT_GRPID" bias="2">
src/intel/genxml/gen9.xml: <instruction name="MEDIA_OBJECT_PRT"
bias="2" length="16">
src/intel/genxml/gen9.xml: <instruction name="MEDIA_OBJECT_WALKER"
bias="2">
src/intel/genxml/gen9.xml: <instruction name="MEDIA_STATE_FLUSH"
bias="2" length="2">
src/intel/genxml/gen9.xml: <instruction name="MEDIA_VFE_STATE" bias="2"
length="9">
src/intel/genxml/gen9.xml: <instruction name="PIPELINE_SELECT" bias="1"
length="1">
src/intel/genxml/gen9.xml: <instruction name="PIPE_CONTROL" bias="2"
length="6">
src/intel/genxml/gen9.xml: <instruction name="STATE_BASE_ADDRESS"
bias="2" length="19">
src/intel/genxml/gen9.xml: <instruction name="STATE_PREFETCH" bias="2"
length="2">
src/intel/genxml/gen9.xml: <instruction name="STATE_SIP" bias="2"
length="3">
>
>> We could set the engine on the gen_batch_decoder
>> (src/intel/common/gen_batch_decoder.c) when we initialize it.
>>
>>
>> I think some discussion happened at some point to try to split things a bit,
>> so we can reduce the xml file and maybe not embed everything in the 3d
>> driver.
> I agree completely, it would be better not to pollute the 3D driver with the
> media stuff. So I'm in all favor of splitting them into different files
> altogether as long as building the debug tools will not require manual work
> when one wants all the definitions.
>
>> For instance we won't care much about media instructions. Similarly we've
>> pretty much stopped using the blitter (I have patches for those
>> instructions), if we want them for debug tools, better have that separate...
>>
>>
>> -
>>
>> Lionel
>>
>>
>>>> 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.
>>
>> If you ever have a pressing need to write tests, I could thing a few things
>> that could be automated so we have decent coverage across gens.
>>
>>
>>>>> 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 -----
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
More information about the mesa-dev
mailing list