| Message ID | 20181204162526.26687-1-lukasz.kalamarz@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series |
"Series without cover letter"
( rev:
1
)
in
IGT |
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);
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
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
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); >
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(+)