[i-g-t,v3,3/3] lib: Use drm_get_param where it is applicable

Submitted by Lukasz Kalamarz on Nov. 26, 2018, 3:36 p.m.

Details

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

Commit Message

Lukasz Kalamarz Nov. 26, 2018, 3:36 p.m.
With usage of implemented drm_get_param helper function, we can
remove some duplicated code lines across few libs.

v2: Fix typos and missing include
v3: Drop unnecessary getparam structure in one function

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>
---
 lib/drmtest.c             |  7 ++---
 lib/i915/gem_scheduler.c  |  8 +----
 lib/i915/gem_submission.c | 10 +++---
 lib/igt_gt.c              |  7 ++---
 lib/intel_chipset.c       |  7 ++---
 lib/ioctl_wrappers.c      | 66 ++++++---------------------------------
 6 files changed, 22 insertions(+), 83 deletions(-)

Patch hide | download patch | download mbox

diff --git a/lib/drmtest.c b/lib/drmtest.c
index fee9d33a..81383fd4 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -107,14 +107,11 @@  bool is_i915_device(int fd)
 
 static bool has_known_intel_chipset(int fd)
 {
-	struct drm_i915_getparam gp;
 	int devid = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_CHIPSET_ID;
-	gp.value = &devid;
+	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
 
-	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
+	if (devid <= 0)
 		return false;
 
 	if (!intel_gen(devid))
diff --git a/lib/i915/gem_scheduler.c b/lib/i915/gem_scheduler.c
index ad156306..079559a2 100644
--- a/lib/i915/gem_scheduler.c
+++ b/lib/i915/gem_scheduler.c
@@ -52,14 +52,8 @@  unsigned gem_scheduler_capability(int fd)
 	static int caps = -1;
 
 	if (caps < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = LOCAL_I915_PARAM_HAS_SCHEDULER;
-		gp.value = &caps;
-
 		caps = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		caps = drm_get_param(fd, LOCAL_I915_PARAM_HAS_SCHEDULER);
 		errno = 0;
 	}
 
diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
index 2fd460d5..583bb133 100644
--- a/lib/i915/gem_submission.c
+++ b/lib/i915/gem_submission.c
@@ -53,12 +53,12 @@ 
 static bool has_semaphores(int fd, int dir)
 {
 	int val = 0;
-	struct drm_i915_getparam gp = {
-		gp.param = I915_PARAM_HAS_SEMAPHORES,
-		gp.value = &val,
-	};
-	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) < 0)
+
+	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
+
+	if (val < 0)
 		val = igt_sysfs_get_boolean(dir, "semaphores");
+
 	return val;
 }
 
diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a2061924..18cd922b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -57,14 +57,11 @@  static bool has_gpu_reset(int fd)
 {
 	static int once = -1;
 	if (once < 0) {
-		struct drm_i915_getparam gp;
 		int val = 0;
 
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 35; /* HAS_GPU_RESET */
-		gp.value = &val;
+		val = drm_get_param(fd, 35); /* HAS_GPU_RESET */
 
-		if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
+		if (val > 0)
 			once = intel_gen(intel_get_drm_devid(fd)) >= 5;
 		else
 			once = val > 0;
diff --git a/lib/intel_chipset.c b/lib/intel_chipset.c
index 4748a3fb..da354fb6 100644
--- a/lib/intel_chipset.c
+++ b/lib/intel_chipset.c
@@ -41,6 +41,7 @@ 
 #include "drmtest.h"
 #include "intel_chipset.h"
 #include "igt_core.h"
+#include "ioctl_wrappers.h"
 
 /**
  * SECTION:intel_chipset
@@ -125,7 +126,6 @@  intel_get_pci_device(void)
 uint32_t
 intel_get_drm_devid(int fd)
 {
-	struct drm_i915_getparam gp;
 	const char *override;
 	int devid = 0;
 
@@ -135,10 +135,7 @@  intel_get_drm_devid(int fd)
 	if (override)
 		return strtol(override, NULL, 0);
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_CHIPSET_ID;
-	gp.value = &devid;
-	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
 
 	return devid;
 }
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index aec4f15e..d156103b 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -494,16 +494,12 @@  int drm_get_param(int fd, uint32_t param)
 bool gem_create__has_stolen_support(int fd)
 {
 	static int has_stolen_support = -1;
-	struct drm_i915_getparam gp;
 	int val = -1;
 
 	if (has_stolen_support < 0) {
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 38; /* CREATE_VERSION */
-		gp.value = &val;
+		val = drm_get_param(fd, 38); /* CREATE_VERSION */
 
 		/* Do we have the extended gem_create_ioctl? */
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
 		has_stolen_support = val >= 2;
 	}
 
@@ -723,21 +719,13 @@  bool gem_mmap__has_wc(int fd)
 	static int has_wc = -1;
 
 	if (has_wc == -1) {
-		struct drm_i915_getparam gp;
 		int mmap_version = -1;
 		int gtt_version = -1;
 
 		has_wc = 0;
 
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 40; /* 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.value = &mmap_version;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+		gtt_version = drm_get_param(fd, 40); /* MMAP_GTT_VERSION */
+		mmap_version = drm_get_param(fd, 30); /* MMAP_VERSION */
 
 		/* Do we have the new mmap_ioctl with DOMAIN_WC? */
 		if (mmap_version >= 1 && gtt_version >= 2) {
@@ -982,17 +970,11 @@  bool gem_bo_busy(int fd, uint32_t handle)
  */
 static int gem_gtt_type(int fd)
 {
-	struct drm_i915_getparam gp;
 	int val = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = 18; /* HAS_ALIASING_PPGTT */
-	gp.value = &val;
-
-	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
-		return 0;
-
+	val = drm_get_param(fd, 18); /* HAS_ALIASING_PPGTT */
 	errno = 0;
+
 	return val;
 }
 
@@ -1036,13 +1018,9 @@  bool gem_uses_full_ppgtt(int fd)
  */
 int gem_gpu_reset_type(int fd)
 {
-	struct drm_i915_getparam gp;
 	int gpu_reset_type = -1;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_HAS_GPU_RESET;
-	gp.value = &gpu_reset_type;
-	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	gpu_reset_type = drm_get_param(fd, I915_PARAM_HAS_GPU_RESET);
 
 	return gpu_reset_type;
 }
@@ -1090,14 +1068,8 @@  int gem_available_fences(int fd)
 	static int num_fences = -1;
 
 	if (num_fences < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_NUM_FENCES_AVAIL;
-		gp.value = &num_fences;
-
 		num_fences = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		num_fences = drm_get_param(fd, I915_PARAM_NUM_FENCES_AVAIL);
 		errno = 0;
 	}
 
@@ -1109,14 +1081,8 @@  bool gem_has_llc(int fd)
 	static int has_llc = -1;
 
 	if (has_llc < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_HAS_LLC;
-		gp.value = &has_llc;
-
 		has_llc = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		has_llc = drm_get_param(fd, I915_PARAM_HAS_LLC);
 		errno = 0;
 	}
 
@@ -1366,14 +1332,8 @@  bool gem_has_softpin(int fd)
 	static int has_softpin = -1;
 
 	if (has_softpin < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_HAS_EXEC_SOFTPIN;
-		gp.value = &has_softpin;
-
 		has_softpin = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		has_softpin = drm_get_param(fd, I915_PARAM_HAS_EXEC_SOFTPIN);
 		errno = 0;
 	}
 
@@ -1394,14 +1354,8 @@  bool gem_has_exec_fence(int fd)
 	static int has_exec_fence = -1;
 
 	if (has_exec_fence < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_HAS_EXEC_FENCE;
-		gp.value = &has_exec_fence;
-
 		has_exec_fence = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		has_exec_fence = drm_get_param(fd, I915_PARAM_HAS_EXEC_FENCE);
 		errno = 0;
 	}
 

Comments

Daniele Ceraolo Spurio Nov. 28, 2018, 11:01 p.m.
On 26/11/2018 07:36, Lukasz Kalamarz wrote:
> With usage of implemented drm_get_param helper function, we can
> remove some duplicated code lines across few libs.
> 
> v2: Fix typos and missing include
> v3: Drop unnecessary getparam structure in one function
> 
> 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>
> ---
>   lib/drmtest.c             |  7 ++---
>   lib/i915/gem_scheduler.c  |  8 +----
>   lib/i915/gem_submission.c | 10 +++---
>   lib/igt_gt.c              |  7 ++---
>   lib/intel_chipset.c       |  7 ++---
>   lib/ioctl_wrappers.c      | 66 ++++++---------------------------------
>   6 files changed, 22 insertions(+), 83 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index fee9d33a..81383fd4 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -107,14 +107,11 @@ bool is_i915_device(int fd)
>   
>   static bool has_known_intel_chipset(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int devid = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_CHIPSET_ID;
> -	gp.value = &devid;
> +	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
>   
> -	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> +	if (devid <= 0)
>   		return false;
>   
>   	if (!intel_gen(devid))

You can now potentially simplify this function with something like:

static bool has_known_intel_chipset(int fd)
{
	int devid = 0;
	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
	return devid > 0 && intel_gen(devid);
}

> diff --git a/lib/i915/gem_scheduler.c b/lib/i915/gem_scheduler.c
> index ad156306..079559a2 100644
> --- a/lib/i915/gem_scheduler.c
> +++ b/lib/i915/gem_scheduler.c
> @@ -52,14 +52,8 @@ unsigned gem_scheduler_capability(int fd)
>   	static int caps = -1;
>   
>   	if (caps < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = LOCAL_I915_PARAM_HAS_SCHEDULER;
> -		gp.value = &caps;
> -
>   		caps = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		caps = drm_get_param(fd, LOCAL_I915_PARAM_HAS_SCHEDULER);

Now caps can be < 0 on error, which will break the logic. I guess you want:

	caps = drm_get_param(fd, LOCAL_I915_PARAM_HAS_SCHEDULER);
	if (caps < 0)
		caps = 0;

>   		errno = 0;
>   	}
>   
> diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
> index 2fd460d5..583bb133 100644
> --- a/lib/i915/gem_submission.c
> +++ b/lib/i915/gem_submission.c
> @@ -53,12 +53,12 @@
>   static bool has_semaphores(int fd, int dir)
>   {
>   	int val = 0;
> -	struct drm_i915_getparam gp = {
> -		gp.param = I915_PARAM_HAS_SEMAPHORES,
> -		gp.value = &val,
> -	};
> -	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) < 0)
> +
> +	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
> +
> +	if (val < 0)
>   		val = igt_sysfs_get_boolean(dir, "semaphores");
> +
>   	return val;
>   }
>   
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a2061924..18cd922b 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -57,14 +57,11 @@ static bool has_gpu_reset(int fd)
>   {
>   	static int once = -1;
>   	if (once < 0) {
> -		struct drm_i915_getparam gp;
>   		int val = 0;
>   
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 35; /* HAS_GPU_RESET */
> -		gp.value = &val;
> +		val = drm_get_param(fd, 35); /* HAS_GPU_RESET */
>   
> -		if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> +		if (val > 0)

This check is the wrong way around, you want val < 0 here (it's the 
failure case)

>   			once = intel_gen(intel_get_drm_devid(fd)) >= 5;
>   		else
>   			once = val > 0;
> diff --git a/lib/intel_chipset.c b/lib/intel_chipset.c
> index 4748a3fb..da354fb6 100644
> --- a/lib/intel_chipset.c
> +++ b/lib/intel_chipset.c
> @@ -41,6 +41,7 @@
>   #include "drmtest.h"
>   #include "intel_chipset.h"
>   #include "igt_core.h"
> +#include "ioctl_wrappers.h"
>   
>   /**
>    * SECTION:intel_chipset
> @@ -125,7 +126,6 @@ intel_get_pci_device(void)
>   uint32_t
>   intel_get_drm_devid(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	const char *override;
>   	int devid = 0;
>   
> @@ -135,10 +135,7 @@ intel_get_drm_devid(int fd)
>   	if (override)
>   		return strtol(override, NULL, 0);
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_CHIPSET_ID;
> -	gp.value = &devid;
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
>   
>   	return devid;
>   }
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index aec4f15e..d156103b 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -494,16 +494,12 @@ int drm_get_param(int fd, uint32_t param)
>   bool gem_create__has_stolen_support(int fd)
>   {
>   	static int has_stolen_support = -1;
> -	struct drm_i915_getparam gp;
>   	int val = -1;
>   
>   	if (has_stolen_support < 0) {
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 38; /* CREATE_VERSION */
> -		gp.value = &val;
> +		val = drm_get_param(fd, 38); /* CREATE_VERSION */
>   
>   		/* Do we have the extended gem_create_ioctl? */
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>   		has_stolen_support = val >= 2;
>   	}
>   
> @@ -723,21 +719,13 @@ bool gem_mmap__has_wc(int fd)
>   	static int has_wc = -1;
>   
>   	if (has_wc == -1) {
> -		struct drm_i915_getparam gp;
>   		int mmap_version = -1;
>   		int gtt_version = -1;
>   
>   		has_wc = 0;
>   
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 40; /* 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.value = &mmap_version;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +		gtt_version = drm_get_param(fd, 40); /* MMAP_GTT_VERSION */
> +		mmap_version = drm_get_param(fd, 30); /* MMAP_VERSION */
>   
>   		/* Do we have the new mmap_ioctl with DOMAIN_WC? */
>   		if (mmap_version >= 1 && gtt_version >= 2) {
> @@ -982,17 +970,11 @@ bool gem_bo_busy(int fd, uint32_t handle)
>    */
>   static int gem_gtt_type(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 18; /* HAS_ALIASING_PPGTT */
> -	gp.value = &val;
> -
> -	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> -		return 0;
> -
> +	val = drm_get_param(fd, 18); /* HAS_ALIASING_PPGTT */

You've lost the error case

if (val < 0)
	return 0

Or update the docs to say we return a negative val on error, we always 
check the return of this func with a > so that'd work anyway

>   	errno = 0;
> +
>   	return val;
>   }
>   
> @@ -1036,13 +1018,9 @@ bool gem_uses_full_ppgtt(int fd)
>    */
>   int gem_gpu_reset_type(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int gpu_reset_type = -1;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_HAS_GPU_RESET;
> -	gp.value = &gpu_reset_type;
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	gpu_reset_type = drm_get_param(fd, I915_PARAM_HAS_GPU_RESET);
>   
>   	return gpu_reset_type;
>   }
> @@ -1090,14 +1068,8 @@ int gem_available_fences(int fd)
>   	static int num_fences = -1;
>   
>   	if (num_fences < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_NUM_FENCES_AVAIL;
> -		gp.value = &num_fences;
> -
>   		num_fences = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		num_fences = drm_get_param(fd, I915_PARAM_NUM_FENCES_AVAIL);

You've lost the assumption that if the kernel doesn't know 
I915_PARAM_NUM_FENCES_AVAIL num_fences stays 0.

>   		errno = 0;
>   	}
>   
> @@ -1109,14 +1081,8 @@ bool gem_has_llc(int fd)
>   	static int has_llc = -1;
>   
>   	if (has_llc < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_HAS_LLC;
> -		gp.value = &has_llc;
> -
>   		has_llc = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		has_llc = drm_get_param(fd, I915_PARAM_HAS_LLC);

same as above. Since this is a boolean, with the wrapper I've suggested 
in the other patch this could become:

	has_llc = i915_has_feature(fd, I915_PARAM_HAS_LLC);

Which should set has_llc to 0 on ioctl failure

>   		errno = 0;
>   	}
>   
> @@ -1366,14 +1332,8 @@ bool gem_has_softpin(int fd)
>   	static int has_softpin = -1;
>   
>   	if (has_softpin < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_HAS_EXEC_SOFTPIN;
> -		gp.value = &has_softpin;
> -
>   		has_softpin = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		has_softpin = drm_get_param(fd, I915_PARAM_HAS_EXEC_SOFTPIN);

same as above

>   		errno = 0;
>   	}
>   
> @@ -1394,14 +1354,8 @@ bool gem_has_exec_fence(int fd)
>   	static int has_exec_fence = -1;
>   
>   	if (has_exec_fence < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_HAS_EXEC_FENCE;
> -		gp.value = &has_exec_fence;
> -
>   		has_exec_fence = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		has_exec_fence = drm_get_param(fd, I915_PARAM_HAS_EXEC_FENCE);

Same again.

Daniele

>   		errno = 0;
>   	}
>   
>