On Thu, May 28, 2015 at 07:00:38AM -0700, Matt Turner wrote:
> On Thu, May 28, 2015 at 3:31 AM, Neil Roberts <n...@linux.intel.com> wrote:
> > Ben Widawsky <benjamin.widaw...@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 <matts...@gmail.com>
> >> Cc: Neil Roberts <n...@linux.intel.com>
> >> Cc: Mark Janes <mark.a.ja...@intel.com>
> >> Signed-off-by: Ben Widawsky <b...@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.
>
That's right, I meant to remove this before submitting the patch. For all
intents, we can't do it.
> I'd like to study the compaction code today a little and try to
> understand how EOT is falling through the cracks.
>
Sure, see my first paragraph above, I explain why we don't hit it today, but I'd
very much appreciate you double checking.
> >> + 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.
Yep, this solves a different problem I believe.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
