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

Chris Wilson chris at chris-wilson.co.uk
Wed Jun 24 00:33:50 PDT 2015


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).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre


More information about the Intel-gfx mailing list