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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch