diff mbox

[2/2] Set alignment value in drm_intel_add_validate_buffer()

Message ID 1428711656-5878-2-git-send-email-anuj.phogat@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Anuj Phogat April 11, 2015, 12:20 a.m. UTC
Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
---
 intel/intel_bufmgr_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Anuj Phogat May 20, 2015, 9:01 p.m. UTC | #1
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++;
> @@ -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;
> --
> 2.3.4
>
Bump
Anuj Phogat June 9, 2015, 10 p.m. UTC | #2
On Wed, May 20, 2015 at 2:01 PM, Anuj Phogat <anuj.phogat@gmail.com> wrote:
> 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++;
>> @@ -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;
>> --
>> 2.3.4
>>
> Bump
This patch is on the list for 8 weeks now. Please take a look so I can push
it upstream.
Anuj Phogat June 19, 2015, 10:52 p.m. UTC | #3
+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++;
> @@ -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;
> --
> 2.3.4
>
Ben Widawsky June 22, 2015, 5:21 p.m. UTC | #4
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.

With or without the first recommendation:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Daniel Vetter June 22, 2015, 7:49 p.m. UTC | #5
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
Anuj Phogat June 23, 2015, 11:37 p.m. UTC | #6
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
Chris Wilson June 24, 2015, 7:33 a.m. UTC | #7
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@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?

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
Daniel Vetter June 24, 2015, 8:22 a.m. UTC | #8
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@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?
> 
> 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
diff mbox

Patch

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++;
@@ -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;