[Intel-gfx] [PATCH 2/2] Set alignment value in drm_intel_add_validate_buffer()

[Daniel Vetter]

On Wed, Jun 24, 2015 at 08:33:50AM +0100, Chris Wilson wrote:
> On Tue, Jun 23, 2015 at 04:37:19PM -0700, Anuj Phogat wrote:
> > On Mon, Jun 22, 2015 at 12:49 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > > On Mon, Jun 22, 2015 at 10:21:46AM -0700, Ben Widawsky wrote:
> > >> On Fri, Jun 19, 2015 at 03:52:01PM -0700, Anuj Phogat wrote:
> > >> > +Ben
> > >> >
> > >> > On Fri, Apr 10, 2015 at 5:20 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> > >> > > Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> > >> > > ---
> > >> > >  intel/intel_bufmgr_gem.c | 4 ++--
> > >> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> > >
> > >> > > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> > >> > > index 51d87ae..92701a5 100644
> > >> > > --- a/intel/intel_bufmgr_gem.c
> > >> > > +++ b/intel/intel_bufmgr_gem.c
> > >> > > @@ -459,7 +459,7 @@ drm_intel_add_validate_buffer(drm_intel_bo *bo)
> > >> > >         bufmgr_gem->exec_objects[index].handle = bo_gem->gem_handle;
> > >> > >         bufmgr_gem->exec_objects[index].relocation_count = bo_gem->reloc_count;
> > >> > >         bufmgr_gem->exec_objects[index].relocs_ptr = (uintptr_t) bo_gem->relocs;
> > >> > > -       bufmgr_gem->exec_objects[index].alignment = 0;
> > >> > > +       bufmgr_gem->exec_objects[index].alignment = bo->align;
> > >> > >         bufmgr_gem->exec_objects[index].offset = 0;
> > >> > >         bufmgr_gem->exec_bos[index] = bo;
> > >> > >         bufmgr_gem->exec_count++;
> > >>
> > >> I'm a bit hesitant about this hunk. We're never going to use this from a mesa
> > >> that supports Yf/Ys - and going this path wouldn't be expected. Maybe add a
> > >> warning if bo->align? (From your other patch I don't think it can ever happen,
> > >> but just to future proof it.
> > >>
> > >> > > @@ -501,7 +501,7 @@ drm_intel_add_validate_buffer2(drm_intel_bo *bo, int need_fence)
> > >> > >         bufmgr_gem->exec2_objects[index].handle = bo_gem->gem_handle;
> > >> > >         bufmgr_gem->exec2_objects[index].relocation_count = bo_gem->reloc_count;
> > >> > >         bufmgr_gem->exec2_objects[index].relocs_ptr = (uintptr_t)bo_gem->relocs;
> > >> > > -       bufmgr_gem->exec2_objects[index].alignment = 0;
> > >> > > +       bufmgr_gem->exec2_objects[index].alignment = bo->align;
> > >> > >         bufmgr_gem->exec2_objects[index].offset = 0;
> > >> > >         bufmgr_gem->exec_bos[index] = bo;
> > >> > >         bufmgr_gem->exec2_objects[index].flags = 0;
> > >>
> > >> I was about to argue this should be part of patch 1, but nope, it should be a
> > >> separate patch :-)
> > >>
> > >> I started digging into whether we have a reasonable way to determine if a bo
> > >> alignment failed, and fall back to a softer restriction. It didn't seem doable
> > >> with the current interfaces, but it's something to think about.
> > >
> > > On gen2/3 we have a lot more severe alignment problems and there the only
> > > thing we've done is to take worst-case space loss for alignment into
> > > account for the execbuf space estimation. But if that failed (and it did
> > > every now and then) userspace just died. I don't think we need any of this
> > > for new tiling layouts since they're all uniformly using a 64kb alignment.
> > > The kernel should be able to pack this very well.
> > Daniel, I don't know much about how kernel is handling the alignment
> > constraints for new tiling layouts during relocation. I'm here mostly
> > relying on the advice from kernel guys. These two patches are based on
> > your comment on an earlier patch on intel-gfx:
> > "[PATCH 4/5] Align YS tile base address to 64KB"
> > 
> > Even if the kernel is already packing the new tiling layouts correctly, these
> > patches are passing some valid alignment value in place of always zero.
> > Isn't it a harmless change?
> 
> I think Daniel is talking here about the estimated GTT usage for a
> batch. mesa/libdrm tracks the estimated usage so that it can flush
> before the batch can no longer fit into the GTT. There is a one
> primitive back-off in mesa, but if the estimate is far, that may not be
> enough to trim the batch sufficiently for us to be able to render it. To
> accommodate that worry, you just want to tweak
> drm_intel_bo_gem_set_in_aperture_size() to include the alignment as
> overhead (and not assume anything about the ability of the kernel to
> pack buffers together).

Well actually I just replied to Ben's concern that alignment might fail
stating that we have some tricks already to handle the much worse
situation on gen2/3, like Chris explained. But also that I don't think we
actually need to update them. But ofc you can update the aperture size
estimate like Chris suggested: size + alignment is a good worst-case
estimate of what we'll need in gtt space. With that change we _really_
should be covered.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch