Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

[Eric Anholt]

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

On Sun,  4 Apr 2010 22:35:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If the buffer is currently bound and does not meet the requested
> alignment, then unbind it and repin.

Do we have any users legitimately requesting an alignment?  I thought
they never existed or only lied when they did.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

[Chris Wilson]

On Mon, 12 Apr 2010 10:10:22 -0700, Eric Anholt <eric@anholt.net> wrote:
> On Sun,  4 Apr 2010 22:35:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > If the buffer is currently bound and does not meet the requested
> > alignment, then unbind it and repin.
> 
> Do we have any users legitimately requesting an alignment?  I thought
> they never existed or only lied when they did.

Yes, I invented one. ;-) Reusing surfaces within a batch with different
per-surface tiling parameters on pre-i965, without informing the kernel
that the buffer is tiled [so we can get away with reusing the surface
multiple times in the batch with different parameters...], and so having 
to manually request the minimal legal alignment for the relocated bo.
Given the transient nature of clip masks, glyph masks and intermediate
back buffers, we can reuse a lot of buffers within a single batch and
avoid catastrophic aperture thrashing.
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

[Eric Anholt]

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

On Mon, 12 Apr 2010 18:23:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, 12 Apr 2010 10:10:22 -0700, Eric Anholt <eric@anholt.net> wrote:
> > On Sun,  4 Apr 2010 22:35:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > If the buffer is currently bound and does not meet the requested
> > > alignment, then unbind it and repin.
> > 
> > Do we have any users legitimately requesting an alignment?  I thought
> > they never existed or only lied when they did.
> 
> Yes, I invented one. ;-) Reusing surfaces within a batch with different
> per-surface tiling parameters on pre-i965, without informing the kernel
> that the buffer is tiled [so we can get away with reusing the surface
> multiple times in the batch with different parameters...], and so having 
> to manually request the minimal legal alignment for the relocated bo.
> Given the transient nature of clip masks, glyph masks and intermediate
> back buffers, we can reuse a lot of buffers within a single batch and
> avoid catastrophic aperture thrashing.
> -ickle

And you're guaranteeing that the transient tiled data doesn't need to
have coherent contents across batchbuffer boundaries? (since it could
get swapped, which means needing a17 swizzling, so the kernel would need
to know about it).  This sounds like madness.


[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

[Chris Wilson]

On Tue, 13 Apr 2010 16:49:46 -0700, Eric Anholt <eric@anholt.net> wrote:
> On Mon, 12 Apr 2010 18:23:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, 12 Apr 2010 10:10:22 -0700, Eric Anholt <eric@anholt.net> wrote:
> > > On Sun,  4 Apr 2010 22:35:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > If the buffer is currently bound and does not meet the requested
> > > > alignment, then unbind it and repin.
> > > 
> > > Do we have any users legitimately requesting an alignment?  I thought
> > > they never existed or only lied when they did.
> > 
> > Yes, I invented one. ;-) Reusing surfaces within a batch with different
> > per-surface tiling parameters on pre-i965, without informing the kernel
> > that the buffer is tiled [so we can get away with reusing the surface
> > multiple times in the batch with different parameters...], and so having 
> > to manually request the minimal legal alignment for the relocated bo.
> > Given the transient nature of clip masks, glyph masks and intermediate
> > back buffers, we can reuse a lot of buffers within a single batch and
> > avoid catastrophic aperture thrashing.
> > -ickle
> 
> And you're guaranteeing that the transient tiled data doesn't need to
> have coherent contents across batchbuffer boundaries? (since it could
> get swapped, which means needing a17 swizzling, so the kernel would need
> to know about it).  This sounds like madness.

Darn. The current incarnation was literally for glyph masks, clip masks
and all the other short-lived write-once, immediately use as source
buffers within the same batch, and then discard. But there is nothing to
ensure that they exist only within a batch, and so they could well fall
foul of a17. I wonder if we can simply lazily rebind the bo at the point
of fence allocation rather than at tiling change. I am not sure if that
even makes sense yet. The other approach would seem to be to SET_TILING
on the last use within a batch and hope that doesn't cause a stall. Maybe
also a mechanism to mark the buffer contents as uninitialized so that if
we need to unbind, we can simply discard and allocate fresh pages? (An
extension to madvise, MADV_UNINITIALIZED or MADV_CLEAN?)

For xf86-video-intel, being able to reuse in-batch surfaces wins around
10% performance across the board in cairo-perf-trace. The effect is
reduced by the need to match tiling and stride, which is proving hairy to
workaround. The bigger the batch with more intermediate buffers, the more
pronounced the effect.
-ickle
-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

[Daniel Vetter]

On Wed, Apr 14, 2010 at 10:29:24AM +0100, Chris Wilson wrote:
> Darn. The current incarnation was literally for glyph masks, clip masks
> and all the other short-lived write-once, immediately use as source
> buffers within the same batch, and then discard. But there is nothing to
> ensure that they exist only within a batch, and so they could well fall
> foul of a17. I wonder if we can simply lazily rebind the bo at the point
> of fence allocation rather than at tiling change. I am not sure if that
> even makes sense yet. The other approach would seem to be to SET_TILING
> on the last use within a batch and hope that doesn't cause a stall. Maybe
> also a mechanism to mark the buffer contents as uninitialized so that if
> we need to unbind, we can simply discard and allocate fresh pages? (An
> extension to madvise, MADV_UNINITIALIZED or MADV_CLEAN?)

At least currently, set_tiling always stalls when actually changing
something (if the alignment doesn't match or if there's a wrong fence reg
attached to the buffer). btw I think the libdrm bo caching has a bug wrt
to this: It resets tiling parameters before inserting a bo into the cache,
while the bo might still be busy. My first try at a patch a few weeks ago
blew up tough, so I left it as-is. Comments?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

[Eric Anholt]

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

On Wed, 14 Apr 2010 12:23:37 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Apr 14, 2010 at 10:29:24AM +0100, Chris Wilson wrote:
> > Darn. The current incarnation was literally for glyph masks, clip masks
> > and all the other short-lived write-once, immediately use as source
> > buffers within the same batch, and then discard. But there is nothing to
> > ensure that they exist only within a batch, and so they could well fall
> > foul of a17. I wonder if we can simply lazily rebind the bo at the point
> > of fence allocation rather than at tiling change. I am not sure if that
> > even makes sense yet. The other approach would seem to be to SET_TILING
> > on the last use within a batch and hope that doesn't cause a stall. Maybe
> > also a mechanism to mark the buffer contents as uninitialized so that if
> > we need to unbind, we can simply discard and allocate fresh pages? (An
> > extension to madvise, MADV_UNINITIALIZED or MADV_CLEAN?)
> 
> At least currently, set_tiling always stalls when actually changing
> something (if the alignment doesn't match or if there's a wrong fence reg
> attached to the buffer). btw I think the libdrm bo caching has a bug wrt
> to this: It resets tiling parameters before inserting a bo into the cache,
> while the bo might still be busy. My first try at a patch a few weeks ago
> blew up tough, so I left it as-is. Comments?

I'd also tried to avoid that stall by just freeing tiled buffers, with
no performance success.  Your resetting tiling at realloc time instead
might be a better plan than I had.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 1/3] drm/i915: Unbind the bo if the user requests a different alignment

[Owain Ainsworth]

On Wed, Apr 14, 2010 at 12:23:37PM +0200, Daniel Vetter wrote:
> On Wed, Apr 14, 2010 at 10:29:24AM +0100, Chris Wilson wrote:
> > Darn. The current incarnation was literally for glyph masks, clip masks
> > and all the other short-lived write-once, immediately use as source
> > buffers within the same batch, and then discard. But there is nothing to
> > ensure that they exist only within a batch, and so they could well fall
> > foul of a17. I wonder if we can simply lazily rebind the bo at the point
> > of fence allocation rather than at tiling change. I am not sure if that
> > even makes sense yet. The other approach would seem to be to SET_TILING
> > on the last use within a batch and hope that doesn't cause a stall. Maybe
> > also a mechanism to mark the buffer contents as uninitialized so that if
> > we need to unbind, we can simply discard and allocate fresh pages? (An
> > extension to madvise, MADV_UNINITIALIZED or MADV_CLEAN?)
> 
> At least currently, set_tiling always stalls when actually changing
> something (if the alignment doesn't match or if there's a wrong fence reg
> attached to the buffer). btw I think the libdrm bo caching has a bug wrt
> to this: It resets tiling parameters before inserting a bo into the cache,
> while the bo might still be busy. My first try at a patch a few weeks ago
> blew up tough, so I left it as-is. Comments?

Another alternative would be to make it completely lazy. kill gtt
mappings in when we switch tiling, then at next pin for execbuf or for
next gtt map, as well as checking if we need a fence reg, check if we
*don't* then since in the latter case we're stalling anyway, and the
former we may be done or at least we have saved some time.

In OpenBSD we already check fence alignment, etc on pin, so adding this
check would be almost trivial.

-0-
-- 
Laetrile is the pits.

Re: [PATCH] drm/i915: Remove spurious warning "Failure to install fence"

[Eric Anholt]

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

On Sun,  4 Apr 2010 22:52:53 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This particular warning is harmless as we emit during the normal
> pinning process where the batch buffer requires more fences than is
> available without eviction. Only if we fail to evict enough fences does
> this become a problem, so include the requested number of fences in the
> ultimate *error* message.
> 
> v2: Remember to compile test even trial patches to remove warnings.

Care to rebase (and test) this one against -next?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

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

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