Re: [PATCH 2/2] Set alignment value in drm_intel_add_validate_buffer()

[Anuj Phogat <anuj.phogat@gmail.com>]

On Mon, Jun 22, 2015 at 12:49 PM, Daniel Vetter <daniel@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@gmail.com> wrote:
>> > > Signed-off-by: Anuj Phogat <anuj.phogat@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?

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx