[Mesa-dev] [PATCH] i965: Disable compaction for EOT send messages

[Matt Turner]

On Thu, May 28, 2015 at 3:31 AM, Neil Roberts <neil at linux.intel.com> wrote:
> Ben Widawsky <benjamin.widawsky at intel.com> writes:
>
>> AFAICT, there is no real way to make sure a send message with EOT is
>> properly ignored from compact, nor can I see a way to actually encode
>> EOT while compacting. Before the single send optimization we'd always
>> bail because we hit the is_immediate && !is_compactable_immediate
>> case. However, with single send, is_immediate is not true, and so we
>> end up trying to compact the un-compactible.
>>
>> Without this, any compacting single send instruction will hang because
>> the EOT isn't there. I am not sure how I didn't hit this when I
>> originally enabled the optimization. I didn't check if some
>> surrounding code changed.
>>
>> NOTE: This needs another piglit run or two before merge.
>>
>> I know Neil and Matt were both looking into this. I did a quick search
>> and didn't see any patches out there to handle this. Please ignore if
>> this has already been sent by someone. (Direct me to it and I will
>> review it).
>>
>> Cc: Matt Turner <mattst88 at gmail.com>
>> Cc: Neil Roberts <neil at linux.intel.com>
>> Cc: Mark Janes <mark.a.janes at intel.com>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_eu_compact.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_eu_compact.c b/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> index 69cb114..67f0b45 100644
>> --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> @@ -849,6 +849,12 @@ set_3src_source_index(const struct brw_device_info *devinfo,
>>  static bool
>>  has_unmapped_bits(const struct brw_device_info *devinfo, brw_inst *src)
>>  {
>> +   /* EOT can only be mapped on a send if the src1 is an immediate */
>
> Can we really map EOT if the src1 is immediate?

That's a good question. I think in practice we can but only under some
very rare circumstances.

I'd like to study the compaction code today a little and try to
understand how EOT is falling through the cracks.

>> +   if ((brw_inst_opcode(devinfo, src) == BRW_OPCODE_SENDC ||
>> +        brw_inst_opcode(devinfo, src) == BRW_OPCODE_SEND) &&
>
> Is there any reason to limit this to send and sendc? If there's no way
> to map EOT why not just to if (brw_inst_eot(...)) return true?
>
> For what it's worth, I ran my original patch¹ through shader-db and it
> didn't make any difference, which is good.
>
> Do we not also need to fix the problem with the destination register
> being used as a temporary? This was mentioned by Matt on IRC. Maybe he
> is looking into it?

Yes, we need to do that as well. I'm looking into it.