[igt-dev] [PATCH i-g-t v3 3/3] lib: Use drm_get_param where it is applicable
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Wed Nov 28 23:01:22 UTC 2018
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 at intel.com>
>
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at 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 = 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 = >t_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;
> }
>
>
More information about the igt-dev
mailing list