[PATCH] gem_pin: Be explicit about GGTT needs

[PATCH] gem_pin: Be explicit about GGTT needs

[Ben Widawsky]

This test assumes that using gem_pin will pin in the same address space
used by execbuf. Since pin is for the global GTT only, use the existing
flag to notify i915.ko that the exec object is special.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 tests/gem_pin.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/gem_pin.c b/tests/gem_pin.c
index 273315f..ca48a04 100644
--- a/tests/gem_pin.c
+++ b/tests/gem_pin.c
@@ -45,6 +45,8 @@
 #define BLT_WRITE_ALPHA         (1<<21)
 #define BLT_WRITE_RGB           (1<<20)
 
+#define LOCAL__EXEC_OBJ_NEEDS_GTT (1<<1)
+
 static void exec(int fd, uint32_t handle, uint32_t offset)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
 	gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
 	gem_exec[0].alignment = 0;
 	gem_exec[0].offset = 0;
-	gem_exec[0].flags = 0;
+	gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;
 	gem_exec[0].rsvd1 = 0;
 	gem_exec[0].rsvd2 = 0;
 
-- 
1.8.4.2

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Chris Wilson]

On Wed, Nov 06, 2013 at 08:05:02PM -0800, Ben Widawsky wrote:
> This test assumes that using gem_pin will pin in the same address space
> used by execbuf. Since pin is for the global GTT only, use the existing
> flag to notify i915.ko that the exec object is special.

Interesting. Yes, I think you are right, the flag is required for the
test to be correct with full-ppgtt.
 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Daniel Vetter]

On Wed, Nov 06, 2013 at 08:05:02PM -0800, Ben Widawsky wrote:
> This test assumes that using gem_pin will pin in the same address space
> used by execbuf. Since pin is for the global GTT only, use the existing
> flag to notify i915.ko that the exec object is special.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  tests/gem_pin.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/gem_pin.c b/tests/gem_pin.c
> index 273315f..ca48a04 100644
> --- a/tests/gem_pin.c
> +++ b/tests/gem_pin.c
> @@ -45,6 +45,8 @@
>  #define BLT_WRITE_ALPHA         (1<<21)
>  #define BLT_WRITE_RGB           (1<<20)
>  
> +#define LOCAL__EXEC_OBJ_NEEDS_GTT (1<<1)
> +
>  static void exec(int fd, uint32_t handle, uint32_t offset)
>  {
>  	struct drm_i915_gem_execbuffer2 execbuf;
> @@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
>  	gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
>  	gem_exec[0].alignment = 0;
>  	gem_exec[0].offset = 0;
> -	gem_exec[0].flags = 0;
> +	gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;

This only really works with the aliasing ppgtt stuff on gen6, I'd just
skip the test -it's not really useful with real ppgtt.
-Daniel

>  	gem_exec[0].rsvd1 = 0;
>  	gem_exec[0].rsvd2 = 0;
>  
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Ben Widawsky]

On Thu, Nov 07, 2013 at 09:33:06AM +0100, Daniel Vetter wrote:
> On Wed, Nov 06, 2013 at 08:05:02PM -0800, Ben Widawsky wrote:
> > This test assumes that using gem_pin will pin in the same address space
> > used by execbuf. Since pin is for the global GTT only, use the existing
> > flag to notify i915.ko that the exec object is special.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  tests/gem_pin.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/gem_pin.c b/tests/gem_pin.c
> > index 273315f..ca48a04 100644
> > --- a/tests/gem_pin.c
> > +++ b/tests/gem_pin.c
> > @@ -45,6 +45,8 @@
> >  #define BLT_WRITE_ALPHA         (1<<21)
> >  #define BLT_WRITE_RGB           (1<<20)
> >  
> > +#define LOCAL__EXEC_OBJ_NEEDS_GTT (1<<1)
> > +
> >  static void exec(int fd, uint32_t handle, uint32_t offset)
> >  {
> >  	struct drm_i915_gem_execbuffer2 execbuf;
> > @@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
> >  	gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
> >  	gem_exec[0].alignment = 0;
> >  	gem_exec[0].offset = 0;
> > -	gem_exec[0].flags = 0;
> > +	gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;
> 
> This only really works with the aliasing ppgtt stuff on gen6, I'd just
> skip the test -it's not really useful with real ppgtt.
> -Daniel

It is really useful with real ppgtt. Please rethink your assertion.

> 
> >  	gem_exec[0].rsvd1 = 0;
> >  	gem_exec[0].rsvd2 = 0;
> >  
> > -- 
> > 1.8.4.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Daniel Vetter]

On Thu, Nov 7, 2013 at 7:11 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> > @@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
>> >     gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
>> >     gem_exec[0].alignment = 0;
>> >     gem_exec[0].offset = 0;
>> > -   gem_exec[0].flags = 0;
>> > +   gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;
>>
>> This only really works with the aliasing ppgtt stuff on gen6, I'd just
>> skip the test -it's not really useful with real ppgtt.
>> -Daniel
>
> It is really useful with real ppgtt. Please rethink your assertion.

The test links up the global gtt used by the current pin ioctl with
the ppgtt. That's not useful, except when they alias. If we want
soft-pinning, then we need a new ioctl mode to return the right
address from the right address space. Which means a new (sub)test.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Ben Widawsky]

On Thu, Nov 07, 2013 at 07:36:50PM +0100, Daniel Vetter wrote:
> On Thu, Nov 7, 2013 at 7:11 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> >> > @@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
> >> >     gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
> >> >     gem_exec[0].alignment = 0;
> >> >     gem_exec[0].offset = 0;
> >> > -   gem_exec[0].flags = 0;
> >> > +   gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;
> >>
> >> This only really works with the aliasing ppgtt stuff on gen6, I'd just
> >> skip the test -it's not really useful with real ppgtt.
> >> -Daniel
> >
> > It is really useful with real ppgtt. Please rethink your assertion.
> 
> The test links up the global gtt used by the current pin ioctl with
> the ppgtt. That's not useful, except when they alias. If we want
> soft-pinning, then we need a new ioctl mode to return the right
> address from the right address space. Which means a new (sub)test.
> -Daniel

This is only indirectly related to soft pinning. No context will ever have the
global GTT address space. If you want to support the pin IOCTL, (which
Chris has before said he requires - the original patch series disabled
it) you must do this. We already have a flag that does what we want,
and, demonstrably, a test which exercises it.

I do not think a new ioctl, nor a new subtest is the solution for this.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Daniel Vetter]

On Thu, Nov 7, 2013 at 7:46 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Thu, Nov 07, 2013 at 07:36:50PM +0100, Daniel Vetter wrote:
>> On Thu, Nov 7, 2013 at 7:11 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> >> > @@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
>> >> >     gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
>> >> >     gem_exec[0].alignment = 0;
>> >> >     gem_exec[0].offset = 0;
>> >> > -   gem_exec[0].flags = 0;
>> >> > +   gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;
>> >>
>> >> This only really works with the aliasing ppgtt stuff on gen6, I'd just
>> >> skip the test -it's not really useful with real ppgtt.
>> >> -Daniel
>> >
>> > It is really useful with real ppgtt. Please rethink your assertion.
>>
>> The test links up the global gtt used by the current pin ioctl with
>> the ppgtt. That's not useful, except when they alias. If we want
>> soft-pinning, then we need a new ioctl mode to return the right
>> address from the right address space. Which means a new (sub)test.
>> -Daniel
>
> This is only indirectly related to soft pinning. No context will ever have the
> global GTT address space. If you want to support the pin IOCTL, (which
> Chris has before said he requires - the original patch series disabled
> it) you must do this. We already have a flag that does what we want,
> and, demonstrably, a test which exercises it.
>
> I do not think a new ioctl, nor a new subtest is the solution for this.

Ok, I'll wait until I see the code that implements this on top of
ppgtt. But until we have that I don't think we need to change the test
at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Ben Widawsky]

On Thu, Nov 07, 2013 at 07:54:59PM +0100, Daniel Vetter wrote:
> On Thu, Nov 7, 2013 at 7:46 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Thu, Nov 07, 2013 at 07:36:50PM +0100, Daniel Vetter wrote:
> >> On Thu, Nov 7, 2013 at 7:11 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> >> >> > @@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
> >> >> >     gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
> >> >> >     gem_exec[0].alignment = 0;
> >> >> >     gem_exec[0].offset = 0;
> >> >> > -   gem_exec[0].flags = 0;
> >> >> > +   gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;
> >> >>
> >> >> This only really works with the aliasing ppgtt stuff on gen6, I'd just
> >> >> skip the test -it's not really useful with real ppgtt.
> >> >> -Daniel
> >> >
> >> > It is really useful with real ppgtt. Please rethink your assertion.
> >>
> >> The test links up the global gtt used by the current pin ioctl with
> >> the ppgtt. That's not useful, except when they alias. If we want
> >> soft-pinning, then we need a new ioctl mode to return the right
> >> address from the right address space. Which means a new (sub)test.
> >> -Daniel
> >
> > This is only indirectly related to soft pinning. No context will ever have the
> > global GTT address space. If you want to support the pin IOCTL, (which
> > Chris has before said he requires - the original patch series disabled
> > it) you must do this. We already have a flag that does what we want,
> > and, demonstrably, a test which exercises it.
> >
> > I do not think a new ioctl, nor a new subtest is the solution for this.
> 
> Ok, I'll wait until I see the code that implements this on top of
> ppgtt. But until we have that I don't think we need to change the test
> at all.
> -Daniel

This is a question for Chris. If DDX only depends on the behavior with
aliasing PPGTT, then I agree - no test change needed.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] gem_pin: Be explicit about GGTT needs

[Chris Wilson]

On Thu, Nov 07, 2013 at 10:58:24AM -0800, Ben Widawsky wrote:
> On Thu, Nov 07, 2013 at 07:54:59PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 7, 2013 at 7:46 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > On Thu, Nov 07, 2013 at 07:36:50PM +0100, Daniel Vetter wrote:
> > >> On Thu, Nov 7, 2013 at 7:11 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > >> >> > @@ -63,7 +65,7 @@ static void exec(int fd, uint32_t handle, uint32_t offset)
> > >> >> >     gem_exec[0].relocs_ptr = (uintptr_t) gem_reloc;
> > >> >> >     gem_exec[0].alignment = 0;
> > >> >> >     gem_exec[0].offset = 0;
> > >> >> > -   gem_exec[0].flags = 0;
> > >> >> > +   gem_exec[0].flags = LOCAL__EXEC_OBJ_NEEDS_GTT;
> > >> >>
> > >> >> This only really works with the aliasing ppgtt stuff on gen6, I'd just
> > >> >> skip the test -it's not really useful with real ppgtt.
> > >> >> -Daniel
> > >> >
> > >> > It is really useful with real ppgtt. Please rethink your assertion.
> > >>
> > >> The test links up the global gtt used by the current pin ioctl with
> > >> the ppgtt. That's not useful, except when they alias. If we want
> > >> soft-pinning, then we need a new ioctl mode to return the right
> > >> address from the right address space. Which means a new (sub)test.
> > >> -Daniel
> > >
> > > This is only indirectly related to soft pinning. No context will ever have the
> > > global GTT address space. If you want to support the pin IOCTL, (which
> > > Chris has before said he requires - the original patch series disabled
> > > it) you must do this. We already have a flag that does what we want,
> > > and, demonstrably, a test which exercises it.
> > >
> > > I do not think a new ioctl, nor a new subtest is the solution for this.
> > 
> > Ok, I'll wait until I see the code that implements this on top of
> > ppgtt. But until we have that I don't think we need to change the test
> > at all.
> > -Daniel
> 
> This is a question for Chris. If DDX only depends on the behavior with
> aliasing PPGTT, then I agree - no test change needed.

Where pin has come in handy has been to w/a hw bugs, so predicting when
and why we might need it again is difficult. As such I'd rather we
maintained its existing semantics of pinning an object in the ggtt into
the full-ppgtt future. As such it is should be trivial to implement the
ioctl with very little impact.
-chris

-- 
Chris Wilson, Intel Open Source Technology Centre