[PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

and use it to initialize the align variable in drm_intel_bo.

In case of YF/YS tiled buffers libdrm need not know about the tiling
format because these buffers don't have hardware support to be tiled
or detiled through a fenced region. But, libdrm still need to know
about buffer alignment restrictions because kernel uses it when
resolving the relocation.

Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
So, use the passed alignment value in this function. Note that we continue
ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
the previous behavior.

Cc: Kristian Høgsberg <krh@bitplanet.net>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
---
 intel/intel_bufmgr_gem.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 5a67f53..51d87ae 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -655,7 +655,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
 				unsigned long size,
 				unsigned long flags,
 				uint32_t tiling_mode,
-				unsigned long stride)
+				unsigned long stride,
+				unsigned int alignment)
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
 	drm_intel_bo_gem *bo_gem;
@@ -754,6 +755,7 @@ retry:
 			return NULL;
 		}
 		bo_gem->bo.bufmgr = bufmgr;
+		bo_gem->bo.align = alignment;
 
 		bo_gem->tiling_mode = I915_TILING_NONE;
 		bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
@@ -797,7 +799,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
 					       BO_ALLOC_FOR_RENDER,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0,
+					       alignment);
 }
 
 static drm_intel_bo *
@@ -807,7 +810,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
 		       unsigned int alignment)
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0, 0);
 }
 
 static drm_intel_bo *
@@ -859,7 +862,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
 		stride = 0;
 
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
-					       tiling, stride);
+					       tiling, stride, 0);
 }
 
 static drm_intel_bo *
-- 
2.3.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Anuj Phogat]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Anuj Phogat]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Anuj Phogat]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Anuj Phogat]

+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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ben Widawsky]

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>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
-- 
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

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

[Anuj Phogat]

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

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
-- 
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

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

On Fri, Apr 10, 2015 at 5:20 PM, Anuj Phogat <anuj.phogat@gmail.com> wrote:
> and use it to initialize the align variable in drm_intel_bo.
>
> In case of YF/YS tiled buffers libdrm need not know about the tiling
> format because these buffers don't have hardware support to be tiled
> or detiled through a fenced region. But, libdrm still need to know
> about buffer alignment restrictions because kernel uses it when
> resolving the relocation.
>
> Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> So, use the passed alignment value in this function. Note that we continue
> ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> the previous behavior.
>
> Cc: Kristian Høgsberg <krh@bitplanet.net>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
> ---
>  intel/intel_bufmgr_gem.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 5a67f53..51d87ae 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -655,7 +655,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
>                                 unsigned long size,
>                                 unsigned long flags,
>                                 uint32_t tiling_mode,
> -                               unsigned long stride)
> +                               unsigned long stride,
> +                               unsigned int alignment)
>  {
>         drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
>         drm_intel_bo_gem *bo_gem;
> @@ -754,6 +755,7 @@ retry:
>                         return NULL;
>                 }
>                 bo_gem->bo.bufmgr = bufmgr;
> +               bo_gem->bo.align = alignment;
>
>                 bo_gem->tiling_mode = I915_TILING_NONE;
>                 bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
> @@ -797,7 +799,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
>  {
>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
>                                                BO_ALLOC_FOR_RENDER,
> -                                              I915_TILING_NONE, 0);
> +                                              I915_TILING_NONE, 0,
> +                                              alignment);
>  }
>
>  static drm_intel_bo *
> @@ -807,7 +810,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
>                        unsigned int alignment)
>  {
>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
> -                                              I915_TILING_NONE, 0);
> +                                              I915_TILING_NONE, 0, 0);
>  }
>
>  static drm_intel_bo *
> @@ -859,7 +862,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
>                 stride = 0;
>
>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
> -                                              tiling, stride);
> +                                              tiling, stride, 0);
>  }
>
>  static drm_intel_bo *
> --
> 2.3.4
>
Bump
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

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:
>> and use it to initialize the align variable in drm_intel_bo.
>>
>> In case of YF/YS tiled buffers libdrm need not know about the tiling
>> format because these buffers don't have hardware support to be tiled
>> or detiled through a fenced region. But, libdrm still need to know
>> about buffer alignment restrictions because kernel uses it when
>> resolving the relocation.
>>
>> Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
>> So, use the passed alignment value in this function. Note that we continue
>> ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
>> the previous behavior.
>>
>> Cc: Kristian Høgsberg <krh@bitplanet.net>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
>> ---
>>  intel/intel_bufmgr_gem.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
>> index 5a67f53..51d87ae 100644
>> --- a/intel/intel_bufmgr_gem.c
>> +++ b/intel/intel_bufmgr_gem.c
>> @@ -655,7 +655,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
>>                                 unsigned long size,
>>                                 unsigned long flags,
>>                                 uint32_t tiling_mode,
>> -                               unsigned long stride)
>> +                               unsigned long stride,
>> +                               unsigned int alignment)
>>  {
>>         drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
>>         drm_intel_bo_gem *bo_gem;
>> @@ -754,6 +755,7 @@ retry:
>>                         return NULL;
>>                 }
>>                 bo_gem->bo.bufmgr = bufmgr;
>> +               bo_gem->bo.align = alignment;
>>
>>                 bo_gem->tiling_mode = I915_TILING_NONE;
>>                 bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
>> @@ -797,7 +799,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
>>  {
>>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
>>                                                BO_ALLOC_FOR_RENDER,
>> -                                              I915_TILING_NONE, 0);
>> +                                              I915_TILING_NONE, 0,
>> +                                              alignment);
>>  }
>>
>>  static drm_intel_bo *
>> @@ -807,7 +810,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
>>                        unsigned int alignment)
>>  {
>>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
>> -                                              I915_TILING_NONE, 0);
>> +                                              I915_TILING_NONE, 0, 0);
>>  }
>>
>>  static drm_intel_bo *
>> @@ -859,7 +862,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
>>                 stride = 0;
>>
>>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
>> -                                              tiling, stride);
>> +                                              tiling, stride, 0);
>>  }
>>
>>  static drm_intel_bo *
>> --
>> 2.3.4
>>
> Bump

This patch is on the list for 8 weeks now. Please take a look so I can push
it upstream.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Damien Lespiau]

On Tue, Jun 09, 2015 at 02:59:33PM -0700, Anuj Phogat wrote:
> This patch is on the list for 8 weeks now. Please take a look so I can push
> it upstream.

Could I suggest you nominate a mesa team member working on SKL as well?
that would be the ideal match I believe.

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

On Wed, Jun 10, 2015 at 1:47 AM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Tue, Jun 09, 2015 at 02:59:33PM -0700, Anuj Phogat wrote:
>> This patch is on the list for 8 weeks now. Please take a look so I can push
>> it upstream.
>
> Could I suggest you nominate a mesa team member working on SKL as well?
> that would be the ideal match I believe.
I will request someone in Mesa time to take a look. But, I still expect
"looks fine/incorrect", "acked/nacked", "I don't know this code" comments by
people who reviewed the v1.

+Ben: Since he's reviewing my mesa patch:
"i965/gen9: Allocate YF/YS tiled buffer objects"
>
> --
> Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Daniel Vetter]

On Fri, Jun 19, 2015 at 03:10:49PM -0700, Anuj Phogat wrote:
> On Wed, Jun 10, 2015 at 1:47 AM, Damien Lespiau
> <damien.lespiau@intel.com> wrote:
> > On Tue, Jun 09, 2015 at 02:59:33PM -0700, Anuj Phogat wrote:
> >> This patch is on the list for 8 weeks now. Please take a look so I can push
> >> it upstream.
> >
> > Could I suggest you nominate a mesa team member working on SKL as well?
> > that would be the ideal match I believe.
> I will request someone in Mesa time to take a look. But, I still expect
> "looks fine/incorrect", "acked/nacked", "I don't know this code" comments by
> people who reviewed the v1.

Ack from kernel pov with the promise that if this passes with mesa folks
we'll sign up to maintain this kernel interface forever.
-Daniel

> 
> +Ben: Since he's reviewing my mesa patch:
> "i965/gen9: Allocate YF/YS tiled buffer objects"
> >
> > --
> > Damien

-- 
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

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

+Ben.

On Fri, Apr 10, 2015 at 5:20 PM, Anuj Phogat <anuj.phogat@gmail.com> wrote:
> and use it to initialize the align variable in drm_intel_bo.
>
> In case of YF/YS tiled buffers libdrm need not know about the tiling
> format because these buffers don't have hardware support to be tiled
> or detiled through a fenced region. But, libdrm still need to know
> about buffer alignment restrictions because kernel uses it when
> resolving the relocation.
>
> Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> So, use the passed alignment value in this function. Note that we continue
> ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> the previous behavior.
>
> Cc: Kristian Høgsberg <krh@bitplanet.net>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
> ---
>  intel/intel_bufmgr_gem.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 5a67f53..51d87ae 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -655,7 +655,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
>                                 unsigned long size,
>                                 unsigned long flags,
>                                 uint32_t tiling_mode,
> -                               unsigned long stride)
> +                               unsigned long stride,
> +                               unsigned int alignment)
>  {
>         drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
>         drm_intel_bo_gem *bo_gem;
> @@ -754,6 +755,7 @@ retry:
>                         return NULL;
>                 }
>                 bo_gem->bo.bufmgr = bufmgr;
> +               bo_gem->bo.align = alignment;
>
>                 bo_gem->tiling_mode = I915_TILING_NONE;
>                 bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
> @@ -797,7 +799,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
>  {
>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
>                                                BO_ALLOC_FOR_RENDER,
> -                                              I915_TILING_NONE, 0);
> +                                              I915_TILING_NONE, 0,
> +                                              alignment);
>  }
>
>  static drm_intel_bo *
> @@ -807,7 +810,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
>                        unsigned int alignment)
>  {
>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
> -                                              I915_TILING_NONE, 0);
> +                                              I915_TILING_NONE, 0, 0);
>  }
>
>  static drm_intel_bo *
> @@ -859,7 +862,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
>                 stride = 0;
>
>         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
> -                                              tiling, stride);
> +                                              tiling, stride, 0);
>  }
>
>  static drm_intel_bo *
> --
> 2.3.4
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Ben Widawsky]

On Fri, Jun 19, 2015 at 03:50:44PM -0700, Anuj Phogat wrote:
> +Ben.
> 
> On Fri, Apr 10, 2015 at 5:20 PM, Anuj Phogat <anuj.phogat@gmail.com> wrote:
> > and use it to initialize the align variable in drm_intel_bo.
> >
> > In case of YF/YS tiled buffers libdrm need not know about the tiling
> > format because these buffers don't have hardware support to be tiled
> > or detiled through a fenced region. But, libdrm still need to know
> > about buffer alignment restrictions because kernel uses it when
> > resolving the relocation.
> >
> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> > So, use the passed alignment value in this function. Note that we continue
> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> > the previous behavior.

I think there is a problem here if you're getting the BO from the cache. If you
allocate out of the cache, and the alignment is incorrect, I don't think
anything will fix it for you.

> >
> > Cc: Kristian Høgsberg <krh@bitplanet.net>
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
> > ---
> >  intel/intel_bufmgr_gem.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> > index 5a67f53..51d87ae 100644
> > --- a/intel/intel_bufmgr_gem.c
> > +++ b/intel/intel_bufmgr_gem.c
> > @@ -655,7 +655,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
> >                                 unsigned long size,
> >                                 unsigned long flags,
> >                                 uint32_t tiling_mode,
> > -                               unsigned long stride)
> > +                               unsigned long stride,
> > +                               unsigned int alignment)
> >  {
> >         drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
> >         drm_intel_bo_gem *bo_gem;

I think you need a check somewhere like:
if (alignment &&
    bo_gem->bo.align != alignment)
	alloc_from_cache = false;

> > @@ -754,6 +755,7 @@ retry:
> >                         return NULL;
> >                 }
> >                 bo_gem->bo.bufmgr = bufmgr;
> > +               bo_gem->bo.align = alignment;
> >
> >                 bo_gem->tiling_mode = I915_TILING_NONE;
> >                 bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
> > @@ -797,7 +799,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
> >  {
> >         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
> >                                                BO_ALLOC_FOR_RENDER,
> > -                                              I915_TILING_NONE, 0);
> > +                                              I915_TILING_NONE, 0,
> > +                                              alignment);
> >  }
> >
> >  static drm_intel_bo *
> > @@ -807,7 +810,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
> >                        unsigned int alignment)
> >  {
> >         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
> > -                                              I915_TILING_NONE, 0);
> > +                                              I915_TILING_NONE, 0, 0);
> >  }
> >
> >  static drm_intel_bo *
> > @@ -859,7 +862,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
> >                 stride = 0;
> >
> >         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
> > -                                              tiling, stride);
> > +                                              tiling, stride, 0);
> >  }
> >
> >  static drm_intel_bo *
> > --
> > 2.3.4
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

On Mon, Jun 22, 2015 at 10:01 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Jun 19, 2015 at 03:50:44PM -0700, Anuj Phogat wrote:
>> +Ben.
>>
>> On Fri, Apr 10, 2015 at 5:20 PM, Anuj Phogat <anuj.phogat@gmail.com> wrote:
>> > and use it to initialize the align variable in drm_intel_bo.
>> >
>> > In case of YF/YS tiled buffers libdrm need not know about the tiling
>> > format because these buffers don't have hardware support to be tiled
>> > or detiled through a fenced region. But, libdrm still need to know
>> > about buffer alignment restrictions because kernel uses it when
>> > resolving the relocation.
>> >
>> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
>> > So, use the passed alignment value in this function. Note that we continue
>> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
>> > the previous behavior.
>
> I think there is a problem here if you're getting the BO from the cache. If you
> allocate out of the cache, and the alignment is incorrect, I don't think
> anything will fix it for you.
>
Right. Thanks for catching this. I'll send out v2 with suggested changes.
>> >
>> > Cc: Kristian Høgsberg <krh@bitplanet.net>
>> > Cc: Damien Lespiau <damien.lespiau@intel.com>
>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>> > Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
>> > ---
>> >  intel/intel_bufmgr_gem.c | 11 +++++++----
>> >  1 file changed, 7 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
>> > index 5a67f53..51d87ae 100644
>> > --- a/intel/intel_bufmgr_gem.c
>> > +++ b/intel/intel_bufmgr_gem.c
>> > @@ -655,7 +655,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
>> >                                 unsigned long size,
>> >                                 unsigned long flags,
>> >                                 uint32_t tiling_mode,
>> > -                               unsigned long stride)
>> > +                               unsigned long stride,
>> > +                               unsigned int alignment)
>> >  {
>> >         drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
>> >         drm_intel_bo_gem *bo_gem;
>
> I think you need a check somewhere like:
> if (alignment &&
>     bo_gem->bo.align != alignment)
>         alloc_from_cache = false;
>
>> > @@ -754,6 +755,7 @@ retry:
>> >                         return NULL;
>> >                 }
>> >                 bo_gem->bo.bufmgr = bufmgr;
>> > +               bo_gem->bo.align = alignment;
>> >
>> >                 bo_gem->tiling_mode = I915_TILING_NONE;
>> >                 bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
>> > @@ -797,7 +799,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
>> >  {
>> >         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
>> >                                                BO_ALLOC_FOR_RENDER,
>> > -                                              I915_TILING_NONE, 0);
>> > +                                              I915_TILING_NONE, 0,
>> > +                                              alignment);
>> >  }
>> >
>> >  static drm_intel_bo *
>> > @@ -807,7 +810,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
>> >                        unsigned int alignment)
>> >  {
>> >         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
>> > -                                              I915_TILING_NONE, 0);
>> > +                                              I915_TILING_NONE, 0, 0);
>> >  }
>> >
>> >  static drm_intel_bo *
>> > @@ -859,7 +862,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
>> >                 stride = 0;
>> >
>> >         return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
>> > -                                              tiling, stride);
>> > +                                              tiling, stride, 0);
>> >  }
>> >
>> >  static drm_intel_bo *
>> > --
>> > 2.3.4
>> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

and use it to initialize the align variable in drm_intel_bo.

In case of YF/YS tiled buffers libdrm need not know about the tiling
format because these buffers don't have hardware support to be tiled
or detiled through a fenced region. But, libdrm still need to know
about buffer alignment restrictions because kernel uses it when
resolving the relocation.

Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
So, use the passed alignment value in this function. Note that we continue
ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
the previous behavior.

V2: Add a condition to avoid allocation from cache. (Ben)

Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 intel/intel_bufmgr_gem.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 60c06fc..60f494e 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -660,7 +660,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
 				unsigned long size,
 				unsigned long flags,
 				uint32_t tiling_mode,
-				unsigned long stride)
+				unsigned long stride,
+				unsigned int alignment)
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
 	drm_intel_bo_gem *bo_gem;
@@ -700,9 +701,14 @@ retry:
 			 */
 			bo_gem = DRMLISTENTRY(drm_intel_bo_gem,
 					      bucket->head.prev, head);
-			DRMLISTDEL(&bo_gem->head);
-			alloc_from_cache = true;
+			if (alignment > 0 && bo_gem->bo.align != alignment) {
+				alloc_from_cache = false;
+			} else {
+				alloc_from_cache = true;
+				DRMLISTDEL(&bo_gem->head);
+			}
 		} else {
+			assert(alignment == 0);
 			/* For non-render-target BOs (where we're probably
 			 * going to map it first thing in order to fill it
 			 * with data), check if the last BO in the cache is
@@ -759,6 +765,7 @@ retry:
 			return NULL;
 		}
 		bo_gem->bo.bufmgr = bufmgr;
+		bo_gem->bo.align = alignment;
 
 		bo_gem->tiling_mode = I915_TILING_NONE;
 		bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
@@ -802,7 +809,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
 					       BO_ALLOC_FOR_RENDER,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0,
+					       alignment);
 }
 
 static drm_intel_bo *
@@ -812,7 +820,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
 		       unsigned int alignment)
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0, 0);
 }
 
 static drm_intel_bo *
@@ -864,7 +872,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
 		stride = 0;
 
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
-					       tiling, stride);
+					       tiling, stride, 0);
 }
 
 static drm_intel_bo *
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Daniel Vetter]

On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
> and use it to initialize the align variable in drm_intel_bo.
> 
> In case of YF/YS tiled buffers libdrm need not know about the tiling
> format because these buffers don't have hardware support to be tiled
> or detiled through a fenced region. But, libdrm still need to know
> about buffer alignment restrictions because kernel uses it when
> resolving the relocation.
> 
> Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> So, use the passed alignment value in this function. Note that we continue
> ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> the previous behavior.
> 
> V2: Add a condition to avoid allocation from cache. (Ben)

This will hurt badly since some programs love to cycle through textures.
You can still allocate from the cache, you only need to update the
alignement constraint. The kernel will move the buffer on the next execbuf
if it's misplaced.
-Daniel

> 
> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  intel/intel_bufmgr_gem.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 60c06fc..60f494e 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -660,7 +660,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
>  				unsigned long size,
>  				unsigned long flags,
>  				uint32_t tiling_mode,
> -				unsigned long stride)
> +				unsigned long stride,
> +				unsigned int alignment)
>  {
>  	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
>  	drm_intel_bo_gem *bo_gem;
> @@ -700,9 +701,14 @@ retry:
>  			 */
>  			bo_gem = DRMLISTENTRY(drm_intel_bo_gem,
>  					      bucket->head.prev, head);
> -			DRMLISTDEL(&bo_gem->head);
> -			alloc_from_cache = true;
> +			if (alignment > 0 && bo_gem->bo.align != alignment) {
> +				alloc_from_cache = false;
> +			} else {
> +				alloc_from_cache = true;
> +				DRMLISTDEL(&bo_gem->head);
> +			}
>  		} else {
> +			assert(alignment == 0);
>  			/* For non-render-target BOs (where we're probably
>  			 * going to map it first thing in order to fill it
>  			 * with data), check if the last BO in the cache is
> @@ -759,6 +765,7 @@ retry:
>  			return NULL;
>  		}
>  		bo_gem->bo.bufmgr = bufmgr;
> +		bo_gem->bo.align = alignment;
>  
>  		bo_gem->tiling_mode = I915_TILING_NONE;
>  		bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
> @@ -802,7 +809,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
>  {
>  	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
>  					       BO_ALLOC_FOR_RENDER,
> -					       I915_TILING_NONE, 0);
> +					       I915_TILING_NONE, 0,
> +					       alignment);
>  }
>  
>  static drm_intel_bo *
> @@ -812,7 +820,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
>  		       unsigned int alignment)
>  {
>  	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
> -					       I915_TILING_NONE, 0);
> +					       I915_TILING_NONE, 0, 0);
>  }
>  
>  static drm_intel_bo *
> @@ -864,7 +872,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
>  		stride = 0;
>  
>  	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
> -					       tiling, stride);
> +					       tiling, stride, 0);
>  }
>  
>  static drm_intel_bo *
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Chris Wilson]

On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
> > and use it to initialize the align variable in drm_intel_bo.
> > 
> > In case of YF/YS tiled buffers libdrm need not know about the tiling
> > format because these buffers don't have hardware support to be tiled
> > or detiled through a fenced region. But, libdrm still need to know
> > about buffer alignment restrictions because kernel uses it when
> > resolving the relocation.
> > 
> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> > So, use the passed alignment value in this function. Note that we continue
> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> > the previous behavior.
> > 
> > V2: Add a condition to avoid allocation from cache. (Ben)
> 
> This will hurt badly since some programs love to cycle through textures.
> You can still allocate from the cache, you only need to update the
> alignement constraint. The kernel will move the buffer on the next execbuf
> if it's misplaced.

For llc, using fresh pages just puts memory and aperture pressure (plus
a small amount of interface pressure) on the system by allocating more bo.

For !llc, it is far better to move an object in the GTT to match a
change in alignment than it is to allocate fresh pages (and deallocate
stale pages).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

On Mon, Jun 22, 2015 at 1:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
>> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
>> > and use it to initialize the align variable in drm_intel_bo.
>> >
>> > In case of YF/YS tiled buffers libdrm need not know about the tiling
>> > format because these buffers don't have hardware support to be tiled
>> > or detiled through a fenced region. But, libdrm still need to know
>> > about buffer alignment restrictions because kernel uses it when
>> > resolving the relocation.
>> >
>> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
>> > So, use the passed alignment value in this function. Note that we continue
>> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
>> > the previous behavior.
>> >
>> > V2: Add a condition to avoid allocation from cache. (Ben)
>>
>> This will hurt badly since some programs love to cycle through textures.
>> You can still allocate from the cache, you only need to update the
>> alignement constraint. The kernel will move the buffer on the next execbuf
>> if it's misplaced.
>
> For llc, using fresh pages just puts memory and aperture pressure (plus
> a small amount of interface pressure) on the system by allocating more bo.
>
> For !llc, it is far better to move an object in the GTT to match a
> change in alignment than it is to allocate fresh pages (and deallocate
> stale pages).
Could you please explain this and point me to what you want to be
changed in this patch?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Chris Wilson]

On Tue, Jun 23, 2015 at 04:44:52PM -0700, Anuj Phogat wrote:
> On Mon, Jun 22, 2015 at 1:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
> >> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
> >> > and use it to initialize the align variable in drm_intel_bo.
> >> >
> >> > In case of YF/YS tiled buffers libdrm need not know about the tiling
> >> > format because these buffers don't have hardware support to be tiled
> >> > or detiled through a fenced region. But, libdrm still need to know
> >> > about buffer alignment restrictions because kernel uses it when
> >> > resolving the relocation.
> >> >
> >> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> >> > So, use the passed alignment value in this function. Note that we continue
> >> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> >> > the previous behavior.
> >> >
> >> > V2: Add a condition to avoid allocation from cache. (Ben)
> >>
> >> This will hurt badly since some programs love to cycle through textures.
> >> You can still allocate from the cache, you only need to update the
> >> alignement constraint. The kernel will move the buffer on the next execbuf
> >> if it's misplaced.
> >
> > For llc, using fresh pages just puts memory and aperture pressure (plus
> > a small amount of interface pressure) on the system by allocating more bo.
> >
> > For !llc, it is far better to move an object in the GTT to match a
> > change in alignment than it is to allocate fresh pages (and deallocate
> > stale pages).
> Could you please explain this and point me to what you want to be
> changed in this patch?

You can reuse the cached bo if the alignment mismatches. You can
experiment with searching for a better match, but it's unlikely to be
useful (numbers talk here).

Reusing the cache and keeping the working set small (with zero turnover)
is vital on !llc. Moving a GTT binding is far cheaper than preparing the
backing store for access with the GPU.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Daniel Vetter]

On Wed, Jun 24, 2015 at 08:28:13AM +0100, Chris Wilson wrote:
> On Tue, Jun 23, 2015 at 04:44:52PM -0700, Anuj Phogat wrote:
> > On Mon, Jun 22, 2015 at 1:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
> > >> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
> > >> > and use it to initialize the align variable in drm_intel_bo.
> > >> >
> > >> > In case of YF/YS tiled buffers libdrm need not know about the tiling
> > >> > format because these buffers don't have hardware support to be tiled
> > >> > or detiled through a fenced region. But, libdrm still need to know
> > >> > about buffer alignment restrictions because kernel uses it when
> > >> > resolving the relocation.
> > >> >
> > >> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> > >> > So, use the passed alignment value in this function. Note that we continue
> > >> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> > >> > the previous behavior.
> > >> >
> > >> > V2: Add a condition to avoid allocation from cache. (Ben)
> > >>
> > >> This will hurt badly since some programs love to cycle through textures.
> > >> You can still allocate from the cache, you only need to update the
> > >> alignement constraint. The kernel will move the buffer on the next execbuf
> > >> if it's misplaced.
> > >
> > > For llc, using fresh pages just puts memory and aperture pressure (plus
> > > a small amount of interface pressure) on the system by allocating more bo.
> > >
> > > For !llc, it is far better to move an object in the GTT to match a
> > > change in alignment than it is to allocate fresh pages (and deallocate
> > > stale pages).
> > Could you please explain this and point me to what you want to be
> > changed in this patch?
> 
> You can reuse the cached bo if the alignment mismatches. You can
> experiment with searching for a better match, but it's unlikely to be
> useful (numbers talk here).
> 
> Reusing the cache and keeping the working set small (with zero turnover)
> is vital on !llc. Moving a GTT binding is far cheaper than preparing the
> backing store for access with the GPU.

In other words Chris just supported my suggestion to reuse from the bo
cache and just update the alignment with his own experience: It's a lot
faster than allocating new bo, especially on machines lacking llc (like
vlv/chv).
-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

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Ben Widawsky]

On Wed, Jun 24, 2015 at 08:28:13AM +0100, Chris Wilson wrote:
> On Tue, Jun 23, 2015 at 04:44:52PM -0700, Anuj Phogat wrote:
> > On Mon, Jun 22, 2015 at 1:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
> > >> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
> > >> > and use it to initialize the align variable in drm_intel_bo.
> > >> >
> > >> > In case of YF/YS tiled buffers libdrm need not know about the tiling
> > >> > format because these buffers don't have hardware support to be tiled
> > >> > or detiled through a fenced region. But, libdrm still need to know
> > >> > about buffer alignment restrictions because kernel uses it when
> > >> > resolving the relocation.
> > >> >
> > >> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> > >> > So, use the passed alignment value in this function. Note that we continue
> > >> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> > >> > the previous behavior.
> > >> >
> > >> > V2: Add a condition to avoid allocation from cache. (Ben)
> > >>
> > >> This will hurt badly since some programs love to cycle through textures.
> > >> You can still allocate from the cache, you only need to update the
> > >> alignement constraint. The kernel will move the buffer on the next execbuf
> > >> if it's misplaced.
> > >
> > > For llc, using fresh pages just puts memory and aperture pressure (plus
> > > a small amount of interface pressure) on the system by allocating more bo.
> > >
> > > For !llc, it is far better to move an object in the GTT to match a
> > > change in alignment than it is to allocate fresh pages (and deallocate
> > > stale pages).
> > Could you please explain this and point me to what you want to be
> > changed in this patch?
> 
> You can reuse the cached bo if the alignment mismatches. You can
> experiment with searching for a better match, but it's unlikely to be
> useful (numbers talk here).

There's no "better" match, there's only match or no match. It seems pretty
intuitive to me that trying to find a match would be better though, I'm
curious why you think it's unlikely. Even though we don't specifically get the
alignment back from the kernel after the relocation, we can certainly use the
presumed offset as a guess when trying to find a buffer in the cache.

> 
> Reusing the cache and keeping the working set small (with zero turnover)
> is vital on !llc. Moving a GTT binding is far cheaper than preparing the
> backing store for access with the GPU.
> -Chris
> 

FWIW, none of the buffers we're talking about here should ever have a GTT
binding as (according to Anuj, I didn't check) there is no way to get fenced
detiling.

Anuj, I think what Chris is saying is, move the bo->align = XXX outside of the
alloc_from_cache block, and do it unconditionally. Leave the cache searching
part alone. If the kernel has to move it, it will. As I questioned above, I
think it does make sense to try to find a buffer with the right alignment in the
cache.

> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Chris Wilson]

On Thu, Jun 25, 2015 at 11:01:44AM -0700, Ben Widawsky wrote:
> On Wed, Jun 24, 2015 at 08:28:13AM +0100, Chris Wilson wrote:
> > On Tue, Jun 23, 2015 at 04:44:52PM -0700, Anuj Phogat wrote:
> > > On Mon, Jun 22, 2015 at 1:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
> > > >> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
> > > >> > and use it to initialize the align variable in drm_intel_bo.
> > > >> >
> > > >> > In case of YF/YS tiled buffers libdrm need not know about the tiling
> > > >> > format because these buffers don't have hardware support to be tiled
> > > >> > or detiled through a fenced region. But, libdrm still need to know
> > > >> > about buffer alignment restrictions because kernel uses it when
> > > >> > resolving the relocation.
> > > >> >
> > > >> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> > > >> > So, use the passed alignment value in this function. Note that we continue
> > > >> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> > > >> > the previous behavior.
> > > >> >
> > > >> > V2: Add a condition to avoid allocation from cache. (Ben)
> > > >>
> > > >> This will hurt badly since some programs love to cycle through textures.
> > > >> You can still allocate from the cache, you only need to update the
> > > >> alignement constraint. The kernel will move the buffer on the next execbuf
> > > >> if it's misplaced.
> > > >
> > > > For llc, using fresh pages just puts memory and aperture pressure (plus
> > > > a small amount of interface pressure) on the system by allocating more bo.
> > > >
> > > > For !llc, it is far better to move an object in the GTT to match a
> > > > change in alignment than it is to allocate fresh pages (and deallocate
> > > > stale pages).
> > > Could you please explain this and point me to what you want to be
> > > changed in this patch?
> > 
> > You can reuse the cached bo if the alignment mismatches. You can
> > experiment with searching for a better match, but it's unlikely to be
> > useful (numbers talk here).
> 
> There's no "better" match, there's only match or no match. It seems pretty
> intuitive to me that trying to find a match would be better though, I'm
> curious why you think it's unlikely. Even though we don't specifically get the
> alignment back from the kernel after the relocation, we can certainly use the
> presumed offset as a guess when trying to find a buffer in the cache.

I was thinking in terms of cycles saved, the cost of walking the cache
for an exact match vs returning the first available buffer + the cost of
any relocation, and whether there would be a measurable difference. I
would have thought two patches, do the naive search that doesn't require
any changes to the cached allocation strategy then a second
demonstrating the perf improvement from being a bit smarter in buffer
reuse would have been your ideal anyway.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Ben Widawsky]

On Thu, Jun 25, 2015 at 07:11:21PM +0100, Chris Wilson wrote:
> On Thu, Jun 25, 2015 at 11:01:44AM -0700, Ben Widawsky wrote:
> > On Wed, Jun 24, 2015 at 08:28:13AM +0100, Chris Wilson wrote:
> > > On Tue, Jun 23, 2015 at 04:44:52PM -0700, Anuj Phogat wrote:
> > > > On Mon, Jun 22, 2015 at 1:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > > On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
> > > > >> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
> > > > >> > and use it to initialize the align variable in drm_intel_bo.
> > > > >> >
> > > > >> > In case of YF/YS tiled buffers libdrm need not know about the tiling
> > > > >> > format because these buffers don't have hardware support to be tiled
> > > > >> > or detiled through a fenced region. But, libdrm still need to know
> > > > >> > about buffer alignment restrictions because kernel uses it when
> > > > >> > resolving the relocation.
> > > > >> >
> > > > >> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> > > > >> > So, use the passed alignment value in this function. Note that we continue
> > > > >> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
> > > > >> > the previous behavior.
> > > > >> >
> > > > >> > V2: Add a condition to avoid allocation from cache. (Ben)
> > > > >>
> > > > >> This will hurt badly since some programs love to cycle through textures.
> > > > >> You can still allocate from the cache, you only need to update the
> > > > >> alignement constraint. The kernel will move the buffer on the next execbuf
> > > > >> if it's misplaced.
> > > > >
> > > > > For llc, using fresh pages just puts memory and aperture pressure (plus
> > > > > a small amount of interface pressure) on the system by allocating more bo.
> > > > >
> > > > > For !llc, it is far better to move an object in the GTT to match a
> > > > > change in alignment than it is to allocate fresh pages (and deallocate
> > > > > stale pages).
> > > > Could you please explain this and point me to what you want to be
> > > > changed in this patch?
> > > 
> > > You can reuse the cached bo if the alignment mismatches. You can
> > > experiment with searching for a better match, but it's unlikely to be
> > > useful (numbers talk here).
> > 
> > There's no "better" match, there's only match or no match. It seems pretty
> > intuitive to me that trying to find a match would be better though, I'm
> > curious why you think it's unlikely. Even though we don't specifically get the
> > alignment back from the kernel after the relocation, we can certainly use the
> > presumed offset as a guess when trying to find a buffer in the cache.
> 
> I was thinking in terms of cycles saved, the cost of walking the cache
> for an exact match vs returning the first available buffer + the cost of
> any relocation, and whether there would be a measurable difference. I
> would have thought two patches, do the naive search that doesn't require
> any changes to the cached allocation strategy then a second
> demonstrating the perf improvement from being a bit smarter in buffer
> reuse would have been your ideal anyway.
> -Chris

That's exactly what I suggested Anuj do :-)
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

On Thu, Jun 25, 2015 at 12:17 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Thu, Jun 25, 2015 at 07:11:21PM +0100, Chris Wilson wrote:
>> On Thu, Jun 25, 2015 at 11:01:44AM -0700, Ben Widawsky wrote:
>> > On Wed, Jun 24, 2015 at 08:28:13AM +0100, Chris Wilson wrote:
>> > > On Tue, Jun 23, 2015 at 04:44:52PM -0700, Anuj Phogat wrote:
>> > > > On Mon, Jun 22, 2015 at 1:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > > > > On Mon, Jun 22, 2015 at 09:51:08PM +0200, Daniel Vetter wrote:
>> > > > >> On Mon, Jun 22, 2015 at 11:47:02AM -0700, Anuj Phogat wrote:
>> > > > >> > and use it to initialize the align variable in drm_intel_bo.
>> > > > >> >
>> > > > >> > In case of YF/YS tiled buffers libdrm need not know about the tiling
>> > > > >> > format because these buffers don't have hardware support to be tiled
>> > > > >> > or detiled through a fenced region. But, libdrm still need to know
>> > > > >> > about buffer alignment restrictions because kernel uses it when
>> > > > >> > resolving the relocation.
>> > > > >> >
>> > > > >> > Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
>> > > > >> > So, use the passed alignment value in this function. Note that we continue
>> > > > >> > ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
>> > > > >> > the previous behavior.
>> > > > >> >
>> > > > >> > V2: Add a condition to avoid allocation from cache. (Ben)
>> > > > >>
>> > > > >> This will hurt badly since some programs love to cycle through textures.
>> > > > >> You can still allocate from the cache, you only need to update the
>> > > > >> alignement constraint. The kernel will move the buffer on the next execbuf
>> > > > >> if it's misplaced.
>> > > > >
>> > > > > For llc, using fresh pages just puts memory and aperture pressure (plus
>> > > > > a small amount of interface pressure) on the system by allocating more bo.
>> > > > >
>> > > > > For !llc, it is far better to move an object in the GTT to match a
>> > > > > change in alignment than it is to allocate fresh pages (and deallocate
>> > > > > stale pages).
>> > > > Could you please explain this and point me to what you want to be
>> > > > changed in this patch?
>> > >
>> > > You can reuse the cached bo if the alignment mismatches. You can
>> > > experiment with searching for a better match, but it's unlikely to be
>> > > useful (numbers talk here).
>> >
>> > There's no "better" match, there's only match or no match. It seems pretty
>> > intuitive to me that trying to find a match would be better though, I'm
>> > curious why you think it's unlikely. Even though we don't specifically get the
>> > alignment back from the kernel after the relocation, we can certainly use the
>> > presumed offset as a guess when trying to find a buffer in the cache.
>>
>> I was thinking in terms of cycles saved, the cost of walking the cache
>> for an exact match vs returning the first available buffer + the cost of
>> any relocation, and whether there would be a measurable difference. I
>> would have thought two patches, do the naive search that doesn't require
>> any changes to the cached allocation strategy then a second
>> demonstrating the perf improvement from being a bit smarter in buffer
>> reuse would have been your ideal anyway.
>> -Chris
>
> That's exactly what I suggested Anuj do :-)
Thanks for helping me to find the right fix. I'll send out an updated patch
doing the naive search with passed alignment and a new patch adding
up the alignment in aperture size.
>>
>> --
>> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

and use it to initialize the align variable in drm_intel_bo.

In case of YF/YS tiled buffers libdrm need not know about the tiling
format because these buffers don't have hardware support to be tiled
or detiled through a fenced region. But, libdrm still need to know
about buffer alignment restrictions because kernel uses it when
resolving the relocation.

Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
So, use the passed alignment value in this function. Note that we continue
ignoring the alignment value passed to drm_intel_gem_bo_alloc() to follow
the previous behavior.

V2: Add a condition to avoid allocation from cache. (Ben)
V3: Make no changes in cache allocation strategy. Just update the alignment.
    Update the aperture size estimate including the alignment.

Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 intel/intel_bufmgr_gem.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 60c06fc..6e711a5 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -660,7 +660,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
 				unsigned long size,
 				unsigned long flags,
 				uint32_t tiling_mode,
-				unsigned long stride)
+				unsigned long stride,
+				unsigned int alignment)
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
 	drm_intel_bo_gem *bo_gem;
@@ -702,7 +703,9 @@ retry:
 					      bucket->head.prev, head);
 			DRMLISTDEL(&bo_gem->head);
 			alloc_from_cache = true;
+			bo_gem->bo.align = alignment;
 		} else {
+			assert(alignment == 0);
 			/* For non-render-target BOs (where we're probably
 			 * going to map it first thing in order to fill it
 			 * with data), check if the last BO in the cache is
@@ -759,6 +762,7 @@ retry:
 			return NULL;
 		}
 		bo_gem->bo.bufmgr = bufmgr;
+		bo_gem->bo.align = alignment;
 
 		bo_gem->tiling_mode = I915_TILING_NONE;
 		bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
@@ -787,6 +791,8 @@ retry:
 	bo_gem->aub_annotation_count = 0;
 
 	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem);
+	/* Update the aperture size estimate assuming worst case */
+	bo_gem->reloc_tree_size += alignment;
 
 	DBG("bo_create: buf %d (%s) %ldb\n",
 	    bo_gem->gem_handle, bo_gem->name, size);
@@ -802,7 +808,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
 					       BO_ALLOC_FOR_RENDER,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0,
+					       alignment);
 }
 
 static drm_intel_bo *
@@ -812,7 +819,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
 		       unsigned int alignment)
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0, 0);
 }
 
 static drm_intel_bo *
@@ -864,7 +871,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
 		stride = 0;
 
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
-					       tiling, stride);
+					       tiling, stride, 0);
 }
 
 static drm_intel_bo *
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Chris Wilson]

On Thu, Jul 02, 2015 at 12:00:43PM -0700, Anuj Phogat wrote:
> and use it to initialize the align variable in drm_intel_bo.

Please don't split sentences across the one-line header and the
changelog.
 
> @@ -787,6 +791,8 @@ retry:
>  	bo_gem->aub_annotation_count = 0;
>  
>  	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem);
> +	/* Update the aperture size estimate assuming worst case */
> +	bo_gem->reloc_tree_size += alignment;

This should be inside drm_intel_bo_gem_set_in_aperture_size() as that
is its raison-d'etre. Something like

 static void
 drm_intel_bo_gem_set_in_aperture_size(drm_intel_bufmgr_gem *bufmgr_gem,
-                                     drm_intel_bo_gem *bo_gem)
+                                     drm_intel_bo_gem *bo_gem,
+                                     uint64_t alignment)
 {
        int size;
 
@@ -522,10 +523,10 @@ drm_intel_bo_gem_set_in_aperture_size(drm_intel_bufmgr_gem *bufmgr_gem,
                        min_size = size;
 
                /* Account for worst-case alignment. */
-               size = 2 * min_size;
+               alignment = max(alignment, min_size);
        }
 
-       bo_gem->reloc_tree_size = size;
+       bo_gem->reloc_tree_size = size + alignment;
 }
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Anuj Phogat]

In case of YF/YS tiled buffers libdrm need not know about the tiling
format because these buffers don't have hardware support to be tiled
or detiled through a fenced region. But, libdrm still need to know
about buffer alignment restrictions because kernel uses it when
resolving the relocation.

Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
So, use the passed alignment value in this function to initialize the
align variable in drm_intel_bo. Note that we continue ignoring the
alignment value passed to drm_intel_gem_bo_alloc() to follow the
previous behavior.

V2: Add a condition to avoid allocation from cache. (Ben)
V3: Make no changes in cache allocation strategy. Just update the alignment.
    Update the aperture size estimate including the alignment. (Ben, Chris)
V4: Move aperture size adjustments inside drm_intel_bo_gem_set_in_aperture_size()
    Don't split sentences across the one-line header and the changelog. (Chris)

Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 intel/intel_bufmgr_gem.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 60c06fc..3018081 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -82,6 +82,7 @@
 } while (0)
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#define MAX2(A, B) ((A) > (B) ? (A) : (B))
 
 typedef struct _drm_intel_bo_gem drm_intel_bo_gem;
 
@@ -524,9 +525,10 @@ drm_intel_add_validate_buffer2(drm_intel_bo *bo, int need_fence)
 
 static void
 drm_intel_bo_gem_set_in_aperture_size(drm_intel_bufmgr_gem *bufmgr_gem,
-				      drm_intel_bo_gem *bo_gem)
+				      drm_intel_bo_gem *bo_gem,
+				      unsigned int alignment)
 {
-	int size;
+	unsigned int size;
 
 	assert(!bo_gem->used_as_reloc_target);
 
@@ -538,7 +540,7 @@ drm_intel_bo_gem_set_in_aperture_size(drm_intel_bufmgr_gem *bufmgr_gem,
 	 */
 	size = bo_gem->bo.size;
 	if (bufmgr_gem->gen < 4 && bo_gem->tiling_mode != I915_TILING_NONE) {
-		int min_size;
+		unsigned int min_size;
 
 		if (bufmgr_gem->has_relaxed_fencing) {
 			if (bufmgr_gem->gen == 3)
@@ -552,10 +554,10 @@ drm_intel_bo_gem_set_in_aperture_size(drm_intel_bufmgr_gem *bufmgr_gem,
 			min_size = size;
 
 		/* Account for worst-case alignment. */
-		size = 2 * min_size;
+		alignment = MAX2(alignment, min_size);
 	}
 
-	bo_gem->reloc_tree_size = size;
+	bo_gem->reloc_tree_size = size + alignment;
 }
 
 static int
@@ -660,7 +662,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
 				unsigned long size,
 				unsigned long flags,
 				uint32_t tiling_mode,
-				unsigned long stride)
+				unsigned long stride,
+				unsigned int alignment)
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
 	drm_intel_bo_gem *bo_gem;
@@ -702,7 +705,9 @@ retry:
 					      bucket->head.prev, head);
 			DRMLISTDEL(&bo_gem->head);
 			alloc_from_cache = true;
+			bo_gem->bo.align = alignment;
 		} else {
+			assert(alignment == 0);
 			/* For non-render-target BOs (where we're probably
 			 * going to map it first thing in order to fill it
 			 * with data), check if the last BO in the cache is
@@ -759,6 +764,7 @@ retry:
 			return NULL;
 		}
 		bo_gem->bo.bufmgr = bufmgr;
+		bo_gem->bo.align = alignment;
 
 		bo_gem->tiling_mode = I915_TILING_NONE;
 		bo_gem->swizzle_mode = I915_BIT_6_SWIZZLE_NONE;
@@ -786,7 +792,7 @@ retry:
 	bo_gem->aub_annotations = NULL;
 	bo_gem->aub_annotation_count = 0;
 
-	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem);
+	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, alignment);
 
 	DBG("bo_create: buf %d (%s) %ldb\n",
 	    bo_gem->gem_handle, bo_gem->name, size);
@@ -802,7 +808,8 @@ drm_intel_gem_bo_alloc_for_render(drm_intel_bufmgr *bufmgr,
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size,
 					       BO_ALLOC_FOR_RENDER,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0,
+					       alignment);
 }
 
 static drm_intel_bo *
@@ -812,7 +819,7 @@ drm_intel_gem_bo_alloc(drm_intel_bufmgr *bufmgr,
 		       unsigned int alignment)
 {
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, 0,
-					       I915_TILING_NONE, 0);
+					       I915_TILING_NONE, 0, 0);
 }
 
 static drm_intel_bo *
@@ -864,7 +871,7 @@ drm_intel_gem_bo_alloc_tiled(drm_intel_bufmgr *bufmgr, const char *name,
 		stride = 0;
 
 	return drm_intel_gem_bo_alloc_internal(bufmgr, name, size, flags,
-					       tiling, stride);
+					       tiling, stride, 0);
 }
 
 static drm_intel_bo *
@@ -931,7 +938,7 @@ drm_intel_gem_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
 	bo_gem->has_error = false;
 	bo_gem->reusable = false;
 
-	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem);
+	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0);
 
 	DBG("bo_create_userptr: "
 	    "ptr %p buf %d (%s) size %ldb, stride 0x%x, tile mode %d\n",
@@ -1099,7 +1106,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
 	bo_gem->tiling_mode = get_tiling.tiling_mode;
 	bo_gem->swizzle_mode = get_tiling.swizzle_mode;
 	/* XXX stride is unknown */
-	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem);
+	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0);
 
 	DRMINITLISTHEAD(&bo_gem->vma_list);
 	DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
@@ -2694,7 +2701,7 @@ drm_intel_gem_bo_set_tiling(drm_intel_bo *bo, uint32_t * tiling_mode,
 
 	ret = drm_intel_gem_bo_set_tiling_internal(bo, *tiling_mode, stride);
 	if (ret == 0)
-		drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem);
+		drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0);
 
 	*tiling_mode = bo_gem->tiling_mode;
 	return ret;
@@ -2792,7 +2799,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
 	bo_gem->tiling_mode = get_tiling.tiling_mode;
 	bo_gem->swizzle_mode = get_tiling.swizzle_mode;
 	/* XXX stride is unknown */
-	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem);
+	drm_intel_bo_gem_set_in_aperture_size(bufmgr_gem, bo_gem, 0);
 
 	return &bo_gem->bo;
 }
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/2] i965/gen9: Pass alignment as function parameter in drm_intel_gem_bo_alloc_internal()

[Chris Wilson]

On Thu, Jul 02, 2015 at 02:44:10PM -0700, Anuj Phogat wrote:
> In case of YF/YS tiled buffers libdrm need not know about the tiling
> format because these buffers don't have hardware support to be tiled
> or detiled through a fenced region. But, libdrm still need to know
> about buffer alignment restrictions because kernel uses it when
> resolving the relocation.
> 
> Mesa uses drm_intel_gem_bo_alloc_for_render() to allocate Yf/Ys buffers.
> So, use the passed alignment value in this function to initialize the
> align variable in drm_intel_bo. Note that we continue ignoring the
> alignment value passed to drm_intel_gem_bo_alloc() to follow the
> previous behavior.
> 
> V2: Add a condition to avoid allocation from cache. (Ben)
> V3: Make no changes in cache allocation strategy. Just update the alignment.
>     Update the aperture size estimate including the alignment. (Ben, Chris)
> V4: Move aperture size adjustments inside drm_intel_bo_gem_set_in_aperture_size()
>     Don't split sentences across the one-line header and the changelog. (Chris)
> 
> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

We have pending issues with not being 48bit safe, but I don't think this
patch makes matters worse.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx