Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-03-13 Thread Andi Shyti 
Hi Chris,

> We [will] expose various per-engine scheduling controls. One of which,
> 'preempt_timeout_ms', defines how we wait for a preemption request to be
> honoured by the currently executing context. If it fails to relieve the
> GPU within the required timeout, the engine is reset and the miscreant
> forcibly evicted.
> 
> Signed-off-by: Chris Wilson 

for the next three patches (3, 4 and 5) I'm not going to repeat
myself :P

Reviewed-by: Andi Shyti 

Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-02-29 Thread Chris Wilson 
Quoting Chris Wilson (2020-02-29 18:34:49)
> Quoting Andi Shyti (2020-02-29 12:45:27)
> > > > > > > + char buf[512];
> > > > > > > + int len;
> > > > > > > +
> > > > > > > + lseek(engines, 0, SEEK_SET);
> > > > > > > + while ((len = syscall(SYS_getdents64, engines, buf, 
> > > > > > > sizeof(buf))) > 0) {
> > > > > > > + void *ptr = buf;
> > > > > > > +
> > > > > > > + while (len) {
> > > > > > > + struct linux_dirent64 {
> > > > > > > + ino64_td_ino;
> > > > > > > + off64_td_off;
> > > > > > > + unsigned short d_reclen;
> > > > > > > + unsigned char  d_type;
> > > > > > > + char   d_name[];
> > > > > > > + } *de = ptr;
> > > > > > 
> > > > > > what is the need for having your own linux_dirent64?
> > > > > 
> > > > > fdopendir() takes ownership of the fd, preventing reuse. And
> > > > > fdopendir(dup()) is getting ridiculous.
> > > > 
> > > > why not using dirent64?
> > > 
> > > It's still the same problem that it takes a DIR, assuming ownership of
> > > the fd. Why using linux_dirent64 because the manpage says so -- if you
> > > are going to use the syscall, you need to match it's calling
> > > conventions, not a middleman's.
> > 
> > I understand, but in bits/dirent.h there is, with some
> > assumptions, this part:
> > 
> > #ifdef __USE_LARGEFILE64
> > struct dirent64
> >   {
> > __ino64_t d_ino;
> > __off64_t d_off;
> > unsigned short int d_reclen;
> > unsigned char d_type;
> > char d_name[256];   /* We must not include limits.h! */
> >   };
> > #endif
> > 
> > why redefine a struct linux_dirent64?
> 
> Because the manpage didn't mention that!

Though it is still a libc structure rather than the syscall definition
:(
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-02-29 Thread Chris Wilson 
Quoting Andi Shyti (2020-02-29 12:45:27)
> > > > > > + char buf[512];
> > > > > > + int len;
> > > > > > +
> > > > > > + lseek(engines, 0, SEEK_SET);
> > > > > > + while ((len = syscall(SYS_getdents64, engines, buf, 
> > > > > > sizeof(buf))) > 0) {
> > > > > > + void *ptr = buf;
> > > > > > +
> > > > > > + while (len) {
> > > > > > + struct linux_dirent64 {
> > > > > > + ino64_td_ino;
> > > > > > + off64_td_off;
> > > > > > + unsigned short d_reclen;
> > > > > > + unsigned char  d_type;
> > > > > > + char   d_name[];
> > > > > > + } *de = ptr;
> > > > > 
> > > > > what is the need for having your own linux_dirent64?
> > > > 
> > > > fdopendir() takes ownership of the fd, preventing reuse. And
> > > > fdopendir(dup()) is getting ridiculous.
> > > 
> > > why not using dirent64?
> > 
> > It's still the same problem that it takes a DIR, assuming ownership of
> > the fd. Why using linux_dirent64 because the manpage says so -- if you
> > are going to use the syscall, you need to match it's calling
> > conventions, not a middleman's.
> 
> I understand, but in bits/dirent.h there is, with some
> assumptions, this part:
> 
> #ifdef __USE_LARGEFILE64
> struct dirent64
>   {
> __ino64_t d_ino;
> __off64_t d_off;
> unsigned short int d_reclen;
> unsigned char d_type;
> char d_name[256];   /* We must not include limits.h! */
>   };
> #endif
> 
> why redefine a struct linux_dirent64?

Because the manpage didn't mention that!
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-02-29 Thread Andi Shyti 
> > > > > + char buf[512];
> > > > > + int len;
> > > > > +
> > > > > + lseek(engines, 0, SEEK_SET);
> > > > > + while ((len = syscall(SYS_getdents64, engines, buf, 
> > > > > sizeof(buf))) > 0) {
> > > > > + void *ptr = buf;
> > > > > +
> > > > > + while (len) {
> > > > > + struct linux_dirent64 {
> > > > > + ino64_td_ino;
> > > > > + off64_td_off;
> > > > > + unsigned short d_reclen;
> > > > > + unsigned char  d_type;
> > > > > + char   d_name[];
> > > > > + } *de = ptr;
> > > > 
> > > > what is the need for having your own linux_dirent64?
> > > 
> > > fdopendir() takes ownership of the fd, preventing reuse. And
> > > fdopendir(dup()) is getting ridiculous.
> > 
> > why not using dirent64?
> 
> It's still the same problem that it takes a DIR, assuming ownership of
> the fd. Why using linux_dirent64 because the manpage says so -- if you
> are going to use the syscall, you need to match it's calling
> conventions, not a middleman's.

I understand, but in bits/dirent.h there is, with some
assumptions, this part:

#ifdef __USE_LARGEFILE64
struct dirent64
  {
__ino64_t d_ino;
__off64_t d_off;
unsigned short int d_reclen;
unsigned char d_type;
char d_name[256];   /* We must not include limits.h! */
  };
#endif

why redefine a struct linux_dirent64?

Andi

PS We have time until Monday morning to discuss this, right? :)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-02-29 Thread Chris Wilson 
Quoting Andi Shyti (2020-02-28 23:51:24)
> > > > +void dyn_sysfs_engines(int i915, int engines, const char *file,
> > > > +void (*test)(int, int))
> > > > +{
> > > > + char buf[512];
> > > > + int len;
> > > > +
> > > > + lseek(engines, 0, SEEK_SET);
> > > > + while ((len = syscall(SYS_getdents64, engines, buf, sizeof(buf))) 
> > > > > 0) {
> > > > + void *ptr = buf;
> > > > +
> > > > + while (len) {
> > > > + struct linux_dirent64 {
> > > > + ino64_td_ino;
> > > > + off64_td_off;
> > > > + unsigned short d_reclen;
> > > > + unsigned char  d_type;
> > > > + char   d_name[];
> > > > + } *de = ptr;
> > > 
> > > what is the need for having your own linux_dirent64?
> > 
> > fdopendir() takes ownership of the fd, preventing reuse. And
> > fdopendir(dup()) is getting ridiculous.
> 
> why not using dirent64?

It's still the same problem that it takes a DIR, assuming ownership of
the fd. Why using linux_dirent64 because the manpage says so -- if you
are going to use the syscall, you need to match it's calling
conventions, not a middleman's.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-02-28 Thread Andi Shyti 
> > > +void dyn_sysfs_engines(int i915, int engines, const char *file,
> > > +void (*test)(int, int))
> > > +{
> > > + char buf[512];
> > > + int len;
> > > +
> > > + lseek(engines, 0, SEEK_SET);
> > > + while ((len = syscall(SYS_getdents64, engines, buf, sizeof(buf))) > 
> > > 0) {
> > > + void *ptr = buf;
> > > +
> > > + while (len) {
> > > + struct linux_dirent64 {
> > > + ino64_td_ino;
> > > + off64_td_off;
> > > + unsigned short d_reclen;
> > > + unsigned char  d_type;
> > > + char   d_name[];
> > > + } *de = ptr;
> > 
> > what is the need for having your own linux_dirent64?
> 
> fdopendir() takes ownership of the fd, preventing reuse. And
> fdopendir(dup()) is getting ridiculous.

why not using dirent64?

Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-02-28 Thread Andi Shyti 
Hi Chris,

> +static int create_ext_ioctl(int i915,
> + struct drm_i915_gem_context_create_ext *arg)
> +{
> + int err;
> +
> + err = 0;
> + if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, arg)) {
> + err = -errno;
> + igt_assume(err);
> + }
> +
> + errno = 0;
> + return err;
> +}
> +
>  /**
>   * gem_has_contexts:
>   * @fd: open i915 drm file descriptor
> @@ -324,17 +339,14 @@ __gem_context_clone(int i915,
>   .flags = flags | I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
>   .extensions = to_user_pointer(&clone),
>   };
> - int err = 0;
> + int err;
>  
> - if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, &arg)) {
> - err = -errno;
> - igt_assume(err);
> - }
> + err = create_ext_ioctl(i915, &arg);
> + if (err)
> + return err;
>  
>   *out = arg.ctx_id;
> -
> - errno = 0;
> - return err;
> + return 0;
>  }
>  
>  static bool __gem_context_has(int i915, uint32_t share, unsigned int flags)
> @@ -382,16 +394,8 @@ bool gem_has_context_clone(int i915)
>   .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
>   .extensions = to_user_pointer(&ext),
>   };
> - int err;
> -
> - err = 0;
> - if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, &create)) {
> - err = -errno;
> - igt_assume(err);
> - }
> - errno = 0;
>  
> - return err == -ENOENT;
> + return create_ext_ioctl(i915, &create) == -ENOENT;
>  }

I'd like to see the above in a separate patch.

> +void dyn_sysfs_engines(int i915, int engines, const char *file,
> +void (*test)(int, int))
> +{
> + char buf[512];
> + int len;
> +
> + lseek(engines, 0, SEEK_SET);
> + while ((len = syscall(SYS_getdents64, engines, buf, sizeof(buf))) > 0) {
> + void *ptr = buf;
> +
> + while (len) {
> + struct linux_dirent64 {
> + ino64_td_ino;
> + off64_td_off;
> + unsigned short d_reclen;
> + unsigned char  d_type;
> + char   d_name[];
> + } *de = ptr;

what is the need for having your own linux_dirent64?

All the rest look good.

Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 3/5] i915: Exercise preemption timeout controls in sysfs

2020-02-28 Thread Chris Wilson 
Quoting Andi Shyti (2020-02-28 23:27:04)
> Hi Chris,
> 
> > +static int create_ext_ioctl(int i915,
> > + struct drm_i915_gem_context_create_ext *arg)
> > +{
> > + int err;
> > +
> > + err = 0;
> > + if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, arg)) {
> > + err = -errno;
> > + igt_assume(err);
> > + }
> > +
> > + errno = 0;
> > + return err;
> > +}
> > +
> >  /**
> >   * gem_has_contexts:
> >   * @fd: open i915 drm file descriptor
> > @@ -324,17 +339,14 @@ __gem_context_clone(int i915,
> >   .flags = flags | I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
> >   .extensions = to_user_pointer(&clone),
> >   };
> > - int err = 0;
> > + int err;
> >  
> > - if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, &arg)) {
> > - err = -errno;
> > - igt_assume(err);
> > - }
> > + err = create_ext_ioctl(i915, &arg);
> > + if (err)
> > + return err;
> >  
> >   *out = arg.ctx_id;
> > -
> > - errno = 0;
> > - return err;
> > + return 0;
> >  }
> >  
> >  static bool __gem_context_has(int i915, uint32_t share, unsigned int flags)
> > @@ -382,16 +394,8 @@ bool gem_has_context_clone(int i915)
> >   .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
> >   .extensions = to_user_pointer(&ext),
> >   };
> > - int err;
> > -
> > - err = 0;
> > - if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, &create)) {
> > - err = -errno;
> > - igt_assume(err);
> > - }
> > - errno = 0;
> >  
> > - return err == -ENOENT;
> > + return create_ext_ioctl(i915, &create) == -ENOENT;
> >  }
> 
> I'd like to see the above in a separate patch.

It's part of the test, I can put it back inside each .c if you prefer.

> > +void dyn_sysfs_engines(int i915, int engines, const char *file,
> > +void (*test)(int, int))
> > +{
> > + char buf[512];
> > + int len;
> > +
> > + lseek(engines, 0, SEEK_SET);
> > + while ((len = syscall(SYS_getdents64, engines, buf, sizeof(buf))) > 
> > 0) {
> > + void *ptr = buf;
> > +
> > + while (len) {
> > + struct linux_dirent64 {
> > + ino64_td_ino;
> > + off64_td_off;
> > + unsigned short d_reclen;
> > + unsigned char  d_type;
> > + char   d_name[];
> > + } *de = ptr;
> 
> what is the need for having your own linux_dirent64?

fdopendir() takes ownership of the fd, preventing reuse. And
fdopendir(dup()) is getting ridiculous.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx