[i-g-t,v4,1/3] lib/ioctl_wrapper: Add i915_get_param helper

Submitted by Lukasz Kalamarz on Dec. 4, 2018, 4:25 p.m.

Details

Message ID 20181204162526.26687-1-lukasz.kalamarz@intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Lukasz Kalamarz Dec. 4, 2018, 4:25 p.m.
getparam is used in few places across IGT, but no helper function
is used to reduce code duplication.

v2: Added doc part and changed return value in case of error
v3: Renamed function to i915_get_param, created internal error
checking function and helper function to check if given feature
is enabled.

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: Petri Latvala <Petri.latvala@intel.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Antonio Argenziano <antonio.argenziano@intel.com>
Cc: Ankit K Nautiyal <ankit.k.nautiyal@intel.com>
---
 lib/ioctl_wrappers.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h |  2 ++
 2 files changed, 52 insertions(+)

Patch hide | download patch | download mbox

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 9f255508..b48dad5b 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -468,6 +468,56 @@  void gem_sync(int fd, uint32_t handle)
 	errno = 0;
 }
 
+static int
+__i915_get_param(int fd, struct drm_i915_getparam *gp)
+{
+	int err;
+
+	err = 0;
+	if (igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, gp))
+		err = -errno;
+
+	errno = 0;
+	return err;
+}
+
+/**
+ * i915_get_param:
+ * @fd: open i915 drm file descriptor
+ * @param: drm parameter we want to read
+ *
+ * Helper function that execute GETPARAM ioctl for a given parameter.
+ *
+ * Return: Read value from GETPARAM
+ */
+int i915_get_param(int fd, uint32_t param)
+{
+	int value;
+	drm_i915_getparam_t gp = {
+		.param = param,
+		.value = &value
+	};
+
+	igt_assert_eq(__i915_get_param(fd, &gp), 0);
+
+	return value;
+}
+
+/**
+ * i915_has_feature:
+ * @fd: open i915 drm file descriptor
+ * @param: drm parameter we want to read
+ *
+ * Helper function that check if given functionality is enabled.
+ * Check is done via getparam IOCTL.
+ *
+ * Return: Value whether feature is enabled or not
+ */
+
+bool i915_has_feature(int fd, uint32_t param)
+{
+	return i915_get_param(fd, param) > 0;
+}
 
 bool gem_create__has_stolen_support(int fd)
 {
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index b22b36b0..c314a070 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -74,6 +74,8 @@  int __gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
 void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
 int gem_wait(int fd, uint32_t handle, int64_t *timeout_ns);
 void gem_sync(int fd, uint32_t handle);
+int i915_get_param(int fd, uint32_t param);
+bool i915_has_feature(int fd, uint32_t param);
 bool gem_create__has_stolen_support(int fd);
 uint32_t __gem_create_stolen(int fd, uint64_t size);
 uint32_t gem_create_stolen(int fd, uint64_t size);

Comments

Chris Wilson Dec. 4, 2018, 4:39 p.m.
Quoting Lukasz Kalamarz (2018-12-04 16:25:24)
> getparam is used in few places across IGT, but no helper function
> is used to reduce code duplication.
> 
> v2: Added doc part and changed return value in case of error
> v3: Renamed function to i915_get_param, created internal error
> checking function and helper function to check if given feature
> is enabled.
> 
> 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: Petri Latvala <Petri.latvala@intel.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Ankit K Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  lib/ioctl_wrappers.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  2 ++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..b48dad5b 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c

Out of the freedom of lib/i915/*, you chose lib/ioctl_wrappers.c for this
i915 specific interface?
-Chris
Chris Wilson Dec. 4, 2018, 4:40 p.m.
Quoting Lukasz Kalamarz (2018-12-04 16:25:24)
> getparam is used in few places across IGT, but no helper function
> is used to reduce code duplication.
> 
> v2: Added doc part and changed return value in case of error
> v3: Renamed function to i915_get_param, created internal error
> checking function and helper function to check if given feature
> is enabled.
> 
> 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: Petri Latvala <Petri.latvala@intel.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Ankit K Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  lib/ioctl_wrappers.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  2 ++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..b48dad5b 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -468,6 +468,56 @@ void gem_sync(int fd, uint32_t handle)
>         errno = 0;
>  }
>  
> +static int
> +__i915_get_param(int fd, struct drm_i915_getparam *gp)
> +{
> +       int err;
> +
> +       err = 0;
> +       if (igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, gp))
> +               err = -errno;
> +
> +       errno = 0;
> +       return err;
> +}
> +
> +/**
> + * i915_get_param:
> + * @fd: open i915 drm file descriptor
> + * @param: drm parameter we want to read
> + *
> + * Helper function that execute GETPARAM ioctl for a given parameter.
> + *
> + * Return: Read value from GETPARAM
> + */
> +int i915_get_param(int fd, uint32_t param)
> +{
> +       int value;
> +       drm_i915_getparam_t gp = {
> +               .param = param,
> +               .value = &value
> +       };
> +
> +       igt_assert_eq(__i915_get_param(fd, &gp), 0);
> +
> +       return value;
> +}
> +
> +/**
> + * i915_has_feature:
> + * @fd: open i915 drm file descriptor
> + * @param: drm parameter we want to read
> + *
> + * Helper function that check if given functionality is enabled.
> + * Check is done via getparam IOCTL.
> + *
> + * Return: Value whether feature is enabled or not
> + */
> +
> +bool i915_has_feature(int fd, uint32_t param)
> +{
> +       return i915_get_param(fd, param) > 0;

Nope, this should return false not assert if the kernel doesn't
recognise said feature.
-Chris
Daniele Ceraolo Spurio Dec. 5, 2018, 12:39 a.m.
On 04/12/2018 08:25, Lukasz Kalamarz wrote:
> getparam is used in few places across IGT, but no helper function
> is used to reduce code duplication.
> 
> v2: Added doc part and changed return value in case of error
> v3: Renamed function to i915_get_param, created internal error
> checking function and helper function to check if given feature
> is enabled.
> 
> 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: Petri Latvala <Petri.latvala@intel.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Ankit K Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>   lib/ioctl_wrappers.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
>   lib/ioctl_wrappers.h |  2 ++
>   2 files changed, 52 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..b48dad5b 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -468,6 +468,56 @@ void gem_sync(int fd, uint32_t handle)
>   	errno = 0;
>   }
>   
> +static int
> +__i915_get_param(int fd, struct drm_i915_getparam *gp)

This is only called in 1 place. Is it really worth having a separate 
fuction?

> +{
> +	int err;
> +
> +	err = 0;
> +	if (igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, gp))
> +		err = -errno;
> +
> +	errno = 0;
> +	return err;
> +}
> +
> +/**
> + * i915_get_param:
> + * @fd: open i915 drm file descriptor
> + * @param: drm parameter we want to read
> + *
> + * Helper function that execute GETPARAM ioctl for a given parameter.
> + *
> + * Return: Read value from GETPARAM
> + */
> +int i915_get_param(int fd, uint32_t param)
> +{
> +	int value;
> +	drm_i915_getparam_t gp = {
> +		.param = param,
> +		.value = &value
> +	};
> +
> +	igt_assert_eq(__i915_get_param(fd, &gp), 0);

This assert breaks support for running on older kernels that don't have 
the param flag defined. Most of the current code assumes a default value 
if the flag is not defined (usually feature missing or version 0), but 
with this assert here we'd fail the test instead.
A good example is I915_PARAM_MMAP_GTT_COHERENT, which was only added 
recently and for which the test assumes a default of "true" if the flag 
is unsupported by the kernel.

> +
> +	return value;
> +}
> +
> +/**
> + * i915_has_feature:
> + * @fd: open i915 drm file descriptor
> + * @param: drm parameter we want to read
> + *
> + * Helper function that check if given functionality is enabled.
> + * Check is done via getparam IOCTL.
> + *
> + * Return: Value whether feature is enabled or not
> + */
> +
> +bool i915_has_feature(int fd, uint32_t param)

Just realized that there is already an has_param() wrapper that does 
more or less the same thing, so we can reuse that and/or adapt it to use 
i915_get_param.

Daniele

> +{
> +	return i915_get_param(fd, param) > 0;
> +}
>   
>   bool gem_create__has_stolen_support(int fd)
>   {
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b22b36b0..c314a070 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -74,6 +74,8 @@ int __gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
>   void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
>   int gem_wait(int fd, uint32_t handle, int64_t *timeout_ns);
>   void gem_sync(int fd, uint32_t handle);
> +int i915_get_param(int fd, uint32_t param);
> +bool i915_has_feature(int fd, uint32_t param);
>   bool gem_create__has_stolen_support(int fd);
>   uint32_t __gem_create_stolen(int fd, uint64_t size);
>   uint32_t gem_create_stolen(int fd, uint64_t size);
>