[igt-dev] [PATCH i-g-t v3 1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

[igt-dev] [PATCH i-g-t v3 1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

[Lukasz Kalamarz]

In lib code there were few functions using param number instead of
defines. This could lead to some issues (i.e. stolen support
pointing on different parameter) and we would like to avoid them.
v2: Rebased patch

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/ioctl_wrappers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 9f255508..f71f0e32 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -708,12 +708,12 @@ bool gem_mmap__has_wc(int fd)
 		has_wc = 0;
 
 		memset(&gp, 0, sizeof(gp));
-		gp.param = 40; /* MMAP_GTT_VERSION */
+		gp.param = I915_PARAM_MMAP_GTT_VERSION;
 		gp.value = &gtt_version;
 		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
 
 		memset(&gp, 0, sizeof(gp));
-		gp.param = 30; /* MMAP_VERSION */
+		gp.param = I915_PARAM_MMAP_VERSION;
 		gp.value = &mmap_version;
 		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
 
@@ -964,7 +964,7 @@ static int gem_gtt_type(int fd)
 	int val = 0;
 
 	memset(&gp, 0, sizeof(gp));
-	gp.param = 18; /* HAS_ALIASING_PPGTT */
+	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
 	gp.value = &val;
 
 	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
-- 
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v3 2/4] lib/igt_dummyload: use gem_mmap__cpu and gem_mmap__wc when applicable

[Lukasz Kalamarz]

We had some duplicates in code that are using direct call to
__gem_mmap__cpu or __gem_mmap__wc and then assert it result, which is what
gem_mmap__cpu and gem_mmap__wc is taking care for us.
v2: Rebased and reordered this patch in series

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/igt_dummyload.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
index 2027a4b7..982906f2 100644
--- a/lib/igt_dummyload.c
+++ b/lib/igt_dummyload.c
@@ -148,14 +148,13 @@ emit_recursive_batch(igt_spin_t *spin,
 
 		if (__gem_set_caching(fd, spin->poll_handle,
 				      I915_CACHING_CACHED) == 0)
-			spin->running = __gem_mmap__cpu(fd, spin->poll_handle,
-							0, 4096,
-							PROT_READ | PROT_WRITE);
+			spin->running = gem_mmap__cpu(fd, spin->poll_handle,
+						      0, 4096,
+						      PROT_READ | PROT_WRITE);
 		else
-			spin->running = __gem_mmap__wc(fd, spin->poll_handle,
-						       0, 4096,
-						       PROT_READ | PROT_WRITE);
-		igt_assert(spin->running);
+			spin->running = gem_mmap__wc(fd, spin->poll_handle,
+						     0, 4096,
+						     PROT_READ | PROT_WRITE);
 		igt_assert_eq(*spin->running, 0);
 
 		*batch++ = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
-- 
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 2/4] lib/igt_dummyload: use gem_mmap__cpu and gem_mmap__wc when applicable

[Daniele Ceraolo Spurio]

On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
> We had some duplicates in code that are using direct call to
> __gem_mmap__cpu or __gem_mmap__wc and then assert it result, which is what
> gem_mmap__cpu and gem_mmap__wc is taking care for us.
> v2: Rebased and reordered this patch in series
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

>   lib/igt_dummyload.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> index 2027a4b7..982906f2 100644
> --- a/lib/igt_dummyload.c
> +++ b/lib/igt_dummyload.c
> @@ -148,14 +148,13 @@ emit_recursive_batch(igt_spin_t *spin,
>   
>   		if (__gem_set_caching(fd, spin->poll_handle,
>   				      I915_CACHING_CACHED) == 0)
> -			spin->running = __gem_mmap__cpu(fd, spin->poll_handle,
> -							0, 4096,
> -							PROT_READ | PROT_WRITE);
> +			spin->running = gem_mmap__cpu(fd, spin->poll_handle,
> +						      0, 4096,
> +						      PROT_READ | PROT_WRITE);
>   		else
> -			spin->running = __gem_mmap__wc(fd, spin->poll_handle,
> -						       0, 4096,
> -						       PROT_READ | PROT_WRITE);
> -		igt_assert(spin->running);
> +			spin->running = gem_mmap__wc(fd, spin->poll_handle,
> +						     0, 4096,
> +						     PROT_READ | PROT_WRITE);
>   		igt_assert_eq(*spin->running, 0);
>   
>   		*batch++ = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v3 3/4] lib/ioctl_wrapper: Implement __gem_mmap

[Lukasz Kalamarz]

Previous implementation of __gem_mmap__cpu and __gem_mmap_wc only
differ with setting proper flag for caching. This patch implement
__gem_mmap, which merge those two functions into one.
v2: Reordered and splited this patch into two separete patches
v3: Dropped unnecessary check

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index f71f0e32..e9225da0 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -736,6 +736,42 @@ bool gem_mmap__has_wc(int fd)
 	return has_wc > 0;
 }
 
+/**
+ * __gem_mmap:
+ * @fd: open i915 drm file descriptor
+ * @handle: gem buffer object handle
+ * @offset: offset in the gem buffer of the mmap arena
+ * @size: size of the mmap arena
+ * @prot: memory protection bits as used by mmap()
+ * @flags: flags used to determine caching
+ *
+ * This functions wraps up procedure to establish a memory mapping through
+ * direct cpu access, bypassing the gpu (valid for wc == false). For wc == true
+ * it also bypass cpu caches completely and GTT system agent (i.e. there is no
+ * automatic tiling of the mmapping through the fence registers).
+ *
+ * Returns: A pointer to the created memory mapping, NULL on failure.
+ */
+static void
+*__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned int prot, uint64_t flags)
+{
+	struct drm_i915_gem_mmap arg;
+
+	memset(&arg, 0, sizeof(arg));
+	arg.handle = handle;
+	arg.offset = offset;
+	arg.size = size;
+	arg.flags = flags;
+
+	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg))
+		return NULL;
+
+	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(arg.addr_ptr), arg.size));
+
+	errno = 0;
+	return from_user_pointer(arg.addr_ptr);
+}
+
 /**
  * __gem_mmap__wc:
  * @fd: open i915 drm file descriptor
-- 
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 3/4] lib/ioctl_wrapper: Implement __gem_mmap

[Daniele Ceraolo Spurio]

On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
> Previous implementation of __gem_mmap__cpu and __gem_mmap_wc only
> differ with setting proper flag for caching. This patch implement
> __gem_mmap, which merge those two functions into one.
> v2: Reordered and splited this patch into two separete patches

I think it make more sense for this to be a single patch together with 
the next one. Easier to review as well.

> v3: Dropped unnecessary check
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index f71f0e32..e9225da0 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -736,6 +736,42 @@ bool gem_mmap__has_wc(int fd)
>   	return has_wc > 0;
>   }
>   
> +/**
> + * __gem_mmap:
> + * @fd: open i915 drm file descriptor
> + * @handle: gem buffer object handle
> + * @offset: offset in the gem buffer of the mmap arena
> + * @size: size of the mmap arena
> + * @prot: memory protection bits as used by mmap()
> + * @flags: flags used to determine caching
> + *
> + * This functions wraps up procedure to establish a memory mapping through
> + * direct cpu access, bypassing the gpu (valid for wc == false). For wc == true

This is a bit unclear IMO, as it sounds like the bypassing the GPU is 
only valid for wc == false, which isn't true since WC also bypasses the 
gpu caches.

Also, instead of using wc == true/false I would say "wc mmappings" and 
"!wc mmappings" or something like that.

Daniele

> + * it also bypass cpu caches completely and GTT system agent (i.e. there is no
> + * automatic tiling of the mmapping through the fence registers).
> + *
> + * Returns: A pointer to the created memory mapping, NULL on failure.
> + */
> +static void
> +*__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned int prot, uint64_t flags)
> +{
> +	struct drm_i915_gem_mmap arg;
> +
> +	memset(&arg, 0, sizeof(arg));
> +	arg.handle = handle;
> +	arg.offset = offset;
> +	arg.size = size;
> +	arg.flags = flags;
> +
> +	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg))
> +		return NULL;
> +
> +	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(arg.addr_ptr), arg.size));
> +
> +	errno = 0;
> +	return from_user_pointer(arg.addr_ptr);
> +}
> +
>   /**
>    * __gem_mmap__wc:
>    * @fd: open i915 drm file descriptor
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 3/4] lib/ioctl_wrapper: Implement __gem_mmap

[Kalamarz, Lukasz]

On Fri, 2019-01-11 at 09:39 -0800, Daniele Ceraolo Spurio wrote:
> 
> On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
> > Previous implementation of __gem_mmap__cpu and __gem_mmap_wc only
> > differ with setting proper flag for caching. This patch implement
> > __gem_mmap, which merge those two functions into one.
> > v2: Reordered and splited this patch into two separete patches
> 
> I think it make more sense for this to be a single patch together
> with 
> the next one. Easier to review as well.

Will merge those tomorrow then :)

> 
> > v3: Dropped unnecessary check
> > 
> > Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > ---
> >   lib/ioctl_wrappers.c | 36 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> > 
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index f71f0e32..e9225da0 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -736,6 +736,42 @@ bool gem_mmap__has_wc(int fd)
> >   	return has_wc > 0;
> >   }
> >   
> > +/**
> > + * __gem_mmap:
> > + * @fd: open i915 drm file descriptor
> > + * @handle: gem buffer object handle
> > + * @offset: offset in the gem buffer of the mmap arena
> > + * @size: size of the mmap arena
> > + * @prot: memory protection bits as used by mmap()
> > + * @flags: flags used to determine caching
> > + *
> > + * This functions wraps up procedure to establish a memory mapping
> > through
> > + * direct cpu access, bypassing the gpu (valid for wc == false).
> > For wc == true
> 
> This is a bit unclear IMO, as it sounds like the bypassing the GPU
> is 
> only valid for wc == false, which isn't true since WC also bypasses
> the 
> gpu caches.

Will change wording in description. My thought was that word 'also' in
second sentence will connect those two for a case, when wc == true.
----
Lukasz

> 
> Also, instead of using wc == true/false I would say "wc mmappings"
> and 
> "!wc mmappings" or something like that.
> 
> Daniele
> 
> > + * it also bypass cpu caches completely and GTT system agent (i.e.
> > there is no
> > + * automatic tiling of the mmapping through the fence registers).
> > + *
> > + * Returns: A pointer to the created memory mapping, NULL on
> > failure.
> > + */
> > +static void
> > +*__gem_mmap(int fd, uint32_t handle, uint64_t offset, uint64_t
> > size, unsigned int prot, uint64_t flags)
> > +{
> > +	struct drm_i915_gem_mmap arg;
> > +
> > +	memset(&arg, 0, sizeof(arg));
> > +	arg.handle = handle;
> > +	arg.offset = offset;
> > +	arg.size = size;
> > +	arg.flags = flags;
> > +
> > +	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg))
> > +		return NULL;
> > +
> > +	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(arg.addr_ptr),
> > arg.size));
> > +
> > +	errno = 0;
> > +	return from_user_pointer(arg.addr_ptr);
> > +}
> > +
> >   /**
> >    * __gem_mmap__wc:
> >    * @fd: open i915 drm file descriptor
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v3 4/4] lib/ioctl_wrapper: Use __gem_mmap within __gem_mmap__cpu/wc

[Lukasz Kalamarz]

This patch remove duplicated code from __gem_mmap__cpu/wc functions
and instead use call to __gem_mmap function, which is doing exactly
the same work.

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/ioctl_wrappers.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index e9225da0..19e59794 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -789,25 +789,7 @@ static void
  */
 void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
 {
-	struct drm_i915_gem_mmap arg;
-
-	if (!gem_mmap__has_wc(fd)) {
-		errno = ENOSYS;
-		return NULL;
-	}
-
-	memset(&arg, 0, sizeof(arg));
-	arg.handle = handle;
-	arg.offset = offset;
-	arg.size = size;
-	arg.flags = I915_MMAP_WC;
-	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &arg))
-		return NULL;
-
-	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(arg.addr_ptr), arg.size));
-
-	errno = 0;
-	return from_user_pointer(arg.addr_ptr);
+	return __gem_mmap(fd, handle, offset, size, prot, I915_MMAP_WC);
 }
 
 /**
@@ -844,19 +826,7 @@ void *gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsi
  */
 void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot)
 {
-	struct drm_i915_gem_mmap mmap_arg;
-
-	memset(&mmap_arg, 0, sizeof(mmap_arg));
-	mmap_arg.handle = handle;
-	mmap_arg.offset = offset;
-	mmap_arg.size = size;
-	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP, &mmap_arg))
-		return NULL;
-
-	VG(VALGRIND_MAKE_MEM_DEFINED(from_user_pointer(mmap_arg.addr_ptr), mmap_arg.size));
-
-	errno = 0;
-	return from_user_pointer(mmap_arg.addr_ptr);
+	return __gem_mmap(fd, handle, offset, size, prot, 0);
 }
 
 /**
-- 
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,v3,1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,v3,1/4] lib/ioctl_wrapper: use defines for get_param instead of param number
URL   : https://patchwork.freedesktop.org/series/55076/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5404 -> IGTPW_2225
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55076/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2225 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_contexts:
    - fi-icl-u2:          NOTRUN -> DMESG-FAIL [fdo#108569]

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        DMESG-FAIL [fdo#108735] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316


Participating hosts (45 -> 40)
------------------------------

  Additional (3): fi-byt-j1900 fi-icl-u2 fi-hsw-peppy 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-ivb-3770 fi-icl-y fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4763 -> IGTPW_2225

  CI_DRM_5404: c51dc608699b2dcfe6d2f6981773f98d1b9f0c86 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2225: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2225/
  IGT_4763: 805a99409542d7d72dda3b6dcd284a8869a3de16 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2225/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

[Daniele Ceraolo Spurio]

On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
> In lib code there were few functions using param number instead of
> defines. This could lead to some issues (i.e. stolen support
> pointing on different parameter) and we would like to avoid them.

The 3 defines you're touching are part of the published kernel uapi, so 
there is no way for them to change and cause issues. The change is still 
worth doing IMO since defines are better than hard-coded numbers.

> v2: Rebased patch
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

All 3 defs are in the i915_drm.h file in IGT, so with a cleaned commit 
message:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

> ---
>   lib/ioctl_wrappers.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..f71f0e32 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -708,12 +708,12 @@ bool gem_mmap__has_wc(int fd)
>   		has_wc = 0;
>   
>   		memset(&gp, 0, sizeof(gp));
> -		gp.param = 40; /* MMAP_GTT_VERSION */
> +		gp.param = I915_PARAM_MMAP_GTT_VERSION;
>   		gp.value = &gtt_version;
>   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>   
>   		memset(&gp, 0, sizeof(gp));
> -		gp.param = 30; /* MMAP_VERSION */
> +		gp.param = I915_PARAM_MMAP_VERSION;
>   		gp.value = &mmap_version;
>   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>   
> @@ -964,7 +964,7 @@ static int gem_gtt_type(int fd)
>   	int val = 0;
>   
>   	memset(&gp, 0, sizeof(gp));
> -	gp.param = 18; /* HAS_ALIASING_PPGTT */
> +	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
>   	gp.value = &val;
>   
>   	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

[Kalamarz, Lukasz]

On Fri, 2019-01-11 at 09:54 -0800, Daniele Ceraolo Spurio wrote:
> 
> On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
> > In lib code there were few functions using param number instead of
> > defines. This could lead to some issues (i.e. stolen support
> > pointing on different parameter) and we would like to avoid them.
> 
> The 3 defines you're touching are part of the published kernel uapi,
> so 
> there is no way for them to change and cause issues. The change is 


In theory what You've said is true, but we had one example within IGT
that may be causing troubles. We had gem_stolen issue, where we are
pointing to param with direct number (38), but then uapi is pointing on
something different (changes in i915 for stole support were never
merged). When running those tests (only hibernate testcase is not run
in during fullrun executions) we may be touching something different
than we expect.

> still 
> worth doing IMO since defines are better than hard-coded numbers.
> 
> > v2: Rebased patch
> > 
> > Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> All 3 defs are in the i915_drm.h file in IGT, so with a cleaned
> commit 
> message:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> > ---
> >   lib/ioctl_wrappers.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 9f255508..f71f0e32 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -708,12 +708,12 @@ bool gem_mmap__has_wc(int fd)
> >   		has_wc = 0;
> >   
> >   		memset(&gp, 0, sizeof(gp));
> > -		gp.param = 40; /* MMAP_GTT_VERSION */
> > +		gp.param = I915_PARAM_MMAP_GTT_VERSION;
> >   		gp.value = &gtt_version;
> >   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> >   
> >   		memset(&gp, 0, sizeof(gp));
> > -		gp.param = 30; /* MMAP_VERSION */
> > +		gp.param = I915_PARAM_MMAP_VERSION;
> >   		gp.value = &mmap_version;
> >   		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> >   
> > @@ -964,7 +964,7 @@ static int gem_gtt_type(int fd)
> >   	int val = 0;
> >   
> >   	memset(&gp, 0, sizeof(gp));
> > -	gp.param = 18; /* HAS_ALIASING_PPGTT */
> > +	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
> >   	gp.value = &val;
> >   
> >   	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

[Daniele Ceraolo Spurio]

On 01/14/2019 07:55 AM, Kalamarz, Lukasz wrote:
> On Fri, 2019-01-11 at 09:54 -0800, Daniele Ceraolo Spurio wrote:
>>
>> On 01/11/2019 08:15 AM, Lukasz Kalamarz wrote:
>>> In lib code there were few functions using param number instead of
>>> defines. This could lead to some issues (i.e. stolen support
>>> pointing on different parameter) and we would like to avoid them.
>>
>> The 3 defines you're touching are part of the published kernel uapi,
>> so
>> there is no way for them to change and cause issues. The change is
> 
> 
> In theory what You've said is true, but we had one example within IGT
> that may be causing troubles. We had gem_stolen issue, where we are
> pointing to param with direct number (38), but then uapi is pointing on
> something different (changes in i915 for stole support were never
> merged). When running those tests (only hibernate testcase is not run
> in during fullrun executions) we may be touching something different
> than we expect.
> 

My comment was regarding exclusively the flags touched by this patch. 
The test in your example was merged to IGT before the flag was merged in 
the kernel, which meant the latter was still not part of the uapi and 
thus could still change.

Daniele

>> still
>> worth doing IMO since defines are better than hard-coded numbers.
>>
>>> v2: Rebased patch
>>>
>>> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
>>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>>> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> All 3 defs are in the i915_drm.h file in IGT, so with a cleaned
>> commit
>> message:
>>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>>> ---
>>>    lib/ioctl_wrappers.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>>> index 9f255508..f71f0e32 100644
>>> --- a/lib/ioctl_wrappers.c
>>> +++ b/lib/ioctl_wrappers.c
>>> @@ -708,12 +708,12 @@ bool gem_mmap__has_wc(int fd)
>>>    		has_wc = 0;
>>>    
>>>    		memset(&gp, 0, sizeof(gp));
>>> -		gp.param = 40; /* MMAP_GTT_VERSION */
>>> +		gp.param = I915_PARAM_MMAP_GTT_VERSION;
>>>    		gp.value = &gtt_version;
>>>    		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>>    
>>>    		memset(&gp, 0, sizeof(gp));
>>> -		gp.param = 30; /* MMAP_VERSION */
>>> +		gp.param = I915_PARAM_MMAP_VERSION;
>>>    		gp.value = &mmap_version;
>>>    		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>>    
>>> @@ -964,7 +964,7 @@ static int gem_gtt_type(int fd)
>>>    	int val = 0;
>>>    
>>>    	memset(&gp, 0, sizeof(gp));
>>> -	gp.param = 18; /* HAS_ALIASING_PPGTT */
>>> +	gp.param = I915_PARAM_HAS_ALIASING_PPGTT;
>>>    	gp.value = &val;
>>>    
>>>    	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
>>>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,v3,1/4] lib/ioctl_wrapper: use defines for get_param instead of param number

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,v3,1/4] lib/ioctl_wrapper: use defines for get_param instead of param number
URL   : https://patchwork.freedesktop.org/series/55076/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5404_full -> IGTPW_2225_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55076/revisions/1/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2225_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-hsw:          PASS -> DMESG-WARN [fdo#107956]

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - shard-glk:          PASS -> FAIL [fdo#108145]

  * igt@kms_color@pipe-c-degamma:
    - shard-apl:          PASS -> FAIL [fdo#104782]

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +5

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x256-random:
    - shard-glk:          PASS -> FAIL [fdo#103232] +8

  * igt@kms_cursor_crc@cursor-64x64-dpms:
    - shard-apl:          NOTRUN -> FAIL [fdo#103232]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
    - shard-apl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move:
    - shard-glk:          PASS -> FAIL [fdo#103167] +13

  * igt@kms_frontbuffer_tracking@fbc-2p-rte:
    - shard-glk:          PASS -> FAIL [fdo#103167] / [fdo#105682]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166] +7
    - shard-apl:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-glk:          PASS -> DMESG-FAIL [fdo#105763] / [fdo#106538]

  * igt@kms_vblank@pipe-b-query-idle-hang:
    - shard-hsw:          PASS -> DMESG-WARN [fdo#102614]

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vecs0-dirty-switch:
    - shard-apl:          INCOMPLETE [fdo#103927] -> PASS

  * igt@kms_color@pipe-b-degamma:
    - shard-apl:          FAIL [fdo#104782] -> PASS

  * igt@kms_cursor_crc@cursor-128x128-onscreen:
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +5

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-xtiled:
    - shard-glk:          FAIL [fdo#107791] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
    - shard-apl:          FAIL [fdo#103167] -> PASS +5

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-glk:          FAIL [fdo#103167] / [fdo#105682] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-cpu:
    - shard-glk:          FAIL [fdo#103167] -> PASS +3

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
    - shard-glk:          FAIL [fdo#108145] -> PASS +3

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
    - shard-glk:          FAIL [fdo#103166] -> PASS
    - shard-apl:          FAIL [fdo#103166] -> PASS +1

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-snb:          {SKIP} [fdo#109271] -> PASS

  
#### Warnings ####

  * igt@kms_draw_crc@draw-method-xrgb2101010-render-ytiled:
    - shard-snb:          {SKIP} [fdo#109271] -> INCOMPLETE [fdo#105411] / [fdo#107469]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469
  [fdo#107791]: https://bugs.freedesktop.org/show_bug.cgi?id=107791
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 4)
------------------------------

  Missing    (3): shard-skl shard-kbl shard-iclb 


Build changes
-------------

    * IGT: IGT_4763 -> IGTPW_2225
    * Piglit: piglit_4509 -> None

  CI_DRM_5404: c51dc608699b2dcfe6d2f6981773f98d1b9f0c86 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2225: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2225/
  IGT_4763: 805a99409542d7d72dda3b6dcd284a8869a3de16 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2225/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev