Re: [PATCH 13/15] drm/i915: Allow execbuffer to use the first object as the batch

From: Kenneth Graunke <kenneth@whitecape.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 13/15] drm/i915: Allow execbuffer to use the first object as the batch
Date: Fri, 07 Jul 2017 20:54:41 -0700	[thread overview]
Message-ID: <78921289.IyuHic4ye4@kirito> (raw)
In-Reply-To: <CAKMK7uH4GhwOBCv7ChtrH5zrmSun-a6GKOVy4pn_yCgf=8vm-A@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 1930 bytes --]

On Friday, July 7, 2017 3:17:22 AM PDT Daniel Vetter wrote:
> On Fri, Mar 17, 2017 at 12:15 PM, Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com> wrote:
> > On to, 2017-03-16 at 13:20 +0000, Chris Wilson wrote:
> >> Currently, the last object in the execlist is the always the batch.
> >> However, when building the batch buffer we often know the batch object
> >> first and if we can use the first slot in the execlist we can emit
> >> relocation instructions relative to it immediately and avoid a separate
> >> pass to adjust the relocations to point to the last execlist slot.
> >>
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> This patch was reviewed/pushed full month before the mesa patch was
> fully reviewed and ready for merging. That's not how uapi is done.
> 
> I've fixed this up now by at least reviewing the mesa patch, but for
> next time around: If you review uapi, and you don't make sure the
> userspace side is in good shape too, then you've not reviewed the
> patch properly.
> 
> Same goes for reviewing and not making sure there's tests, but that's
> another rant.
> 
> Ken, pls make sure we don't end up with another case like resource
> streamer (or tell me I should revert this).
> -Daniel

Sorry, that's partly my bad - I had mentioned to Chris that I wanted this
feature, and planned to use it, but then got distracted with other work and
didn't get around to actually shipping a patch to do so.  Both Chris and I
wrote patches, and IIRC I was benchmarking things when I got distracted.

Basically, I915_EXEC_HANDLE_LUT appeared to be a performance loss in the
CPU bound benchmarks I looked at, because we had to walk over one of the
lists and patch up references to the batch (-1 => actual index).

BATCH_FIRST eliminates that overhead, making HANDLE_LUT actually useful
(although only a small amount).

--Ken

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx