From 29ae12bd764e3b1876356e7628a32192b4ec9066 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 7 Mar 2018 23:33:07 +0200 Subject: tests/kms_properties: Validate properties harder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the property validation more thorough: - validate property flags - make sure there's an expected number of values/enums - make sure the possible values make sense - make sure the current value makes sense - actually iterate through all planes/crtc/connectors to check their properties - make sure encoders don't expose properties while at it - check that atomic props aren't exposed to non-atomic clients Still passes on my ivb. Not tested anything else so far. Signed-off-by: Ville Syrjälä Reviewed-by: Stanislav Lisovskiy --- lib/igt_aux.h | 2 + lib/igt_core.h | 52 +++++++ tests/kms_properties.c | 364 +++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 374 insertions(+), 44 deletions(-) diff --git a/lib/igt_aux.h b/lib/igt_aux.h index faddd478..9bb03b77 100644 --- a/lib/igt_aux.h +++ b/lib/igt_aux.h @@ -390,4 +390,6 @@ static inline bool igt_list_empty(const struct igt_list *list) __builtin_popcountll(x), \ __builtin_popcount(x)) +#define is_power_of_two(x) (((x) & ((x)-1)) == 0) + #endif /* IGT_AUX_H */ diff --git a/lib/igt_core.h b/lib/igt_core.h index 57e97f2b..bf8cd66c 100644 --- a/lib/igt_core.h +++ b/lib/igt_core.h @@ -575,6 +575,32 @@ static inline void igt_ignore_warn(bool value) */ #define igt_assert_lte(n1, n2) igt_assert_cmpint(n1, <=, >, n2) +/** + * igt_assert_lte_u64: + * @n1: first integer + * @n2: second integer + * + * Fails (sub-)test if the second integer is strictly smaller than the first. + * Beware that for now this only works on integers. + * + * Like igt_assert(), but displays the values being compared on failure instead + * of simply printing the stringified expression. + */ +#define igt_assert_lte_u64(n1, n2) igt_assert_cmpu64(n1, <=, >, n2) + +/** + * igt_assert_lte_s64: + * @n1: first integer + * @n2: second integer + * + * Fails (sub-)test if the second integer is strictly smaller than the first. + * Beware that for now this only works on integers. + * + * Like igt_assert(), but displays the values being compared on failure instead + * of simply printing the stringified expression. + */ +#define igt_assert_lte_s64(n1, n2) igt_assert_cmps64(n1, <=, >, n2) + /** * igt_assert_lt: * @n1: first integer @@ -588,6 +614,32 @@ static inline void igt_ignore_warn(bool value) */ #define igt_assert_lt(n1, n2) igt_assert_cmpint(n1, <, >=, n2) +/** + * igt_assert_lt_u64: + * @n1: first integer + * @n2: second integer + * + * Fails (sub-)test if the second integer is smaller than or equal to the first. + * Beware that for now this only works on integers. + * + * Like igt_assert(), but displays the values being compared on failure instead + * of simply printing the stringified expression. + */ +#define igt_assert_lt_u64(n1, n2) igt_assert_cmpu64(n1, <, >=, n2) + +/** + * igt_assert_lt_s64: + * @n1: first integer + * @n2: second integer + * + * Fails (sub-)test if the second integer is smaller than or equal to the first. + * Beware that for now this only works on integers. + * + * Like igt_assert(), but displays the values being compared on failure instead + * of simply printing the stringified expression. + */ +#define igt_assert_lt_s64(n1, n2) igt_assert_cmps64(n1, <, >=, n2) + /** * igt_assert_fd: * @fd: file descriptor diff --git a/tests/kms_properties.c b/tests/kms_properties.c index a63b69b4..68c0518f 100644 --- a/tests/kms_properties.c +++ b/tests/kms_properties.c @@ -325,63 +325,327 @@ static void test_object_invalid_properties(igt_display_t *display, test_invalid_properties(display->drm_fd, id, type, output->id, DRM_MODE_OBJECT_CONNECTOR, atomic); } -static void get_prop_sanity(igt_display_t *display) +static void validate_range_prop(const struct drm_mode_get_property *prop, + uint64_t value) { - int i, fd; - uint64_t *values; - struct drm_mode_property_enum *enums; + const uint64_t *values = from_user_pointer(prop->values_ptr); + bool is_unsigned = prop->flags & DRM_MODE_PROP_RANGE; + bool immutable = prop->flags & DRM_MODE_PROP_IMMUTABLE; + + igt_assert_eq(prop->count_values, 2); + igt_assert_eq(prop->count_enum_blobs, 0); + igt_assert(values[0] != values[1] || immutable); + + if (is_unsigned) { + igt_assert_lte_u64(values[0], values[1]); + igt_assert_lte_u64(values[0], value); + igt_assert_lte_u64(value, values[1]); + } else { + igt_assert_lte_s64(values[0], values[1]); + igt_assert_lte_s64(values[0], value); + igt_assert_lte_s64(value, values[1]); + } + +} + +static void validate_enums(const struct drm_mode_get_property *prop) +{ + const uint64_t *values = from_user_pointer(prop->values_ptr); + const struct drm_mode_property_enum *enums = + from_user_pointer(prop->enum_blob_ptr); + + for (int i = 0; i < prop->count_enum_blobs; i++) { + int name_len = strnlen(enums[i].name, + sizeof(enums[i].name)); - fd = display->drm_fd; + igt_assert_lte(1, name_len); + igt_assert_lte(name_len, sizeof(enums[i].name) - 1); + + /* no idea why we have this duplicated */ + igt_assert_eq_u64(values[i], enums[i].value); + } +} + +static void validate_enum_prop(const struct drm_mode_get_property *prop, + uint64_t value) +{ + const uint64_t *values = from_user_pointer(prop->values_ptr); + bool immutable = prop->flags & DRM_MODE_PROP_IMMUTABLE; + int i; + + igt_assert_lte(1, prop->count_values); + igt_assert_eq(prop->count_enum_blobs, prop->count_values); + igt_assert(prop->count_values != 1 || immutable); + + for (i = 0; i < prop->count_values; i++) { + if (value == values[i]) + break; + } + igt_assert(i != prop->count_values); + + validate_enums(prop); +} + +static void validate_bitmask_prop(const struct drm_mode_get_property *prop, + uint64_t value) +{ + const uint64_t *values = from_user_pointer(prop->values_ptr); + bool immutable = prop->flags & DRM_MODE_PROP_IMMUTABLE; + uint64_t mask = 0; + + igt_assert_lte(1, prop->count_values); + igt_assert_eq(prop->count_enum_blobs, prop->count_values); + igt_assert(prop->count_values != 1 || immutable); + + for (int i = 0; i < prop->count_values; i++) { + igt_assert_lte_u64(values[i], 63); + mask |= 1ULL << values[i]; + } + + igt_assert_eq_u64(value & ~mask, 0); + igt_assert_neq_u64(value & mask, 0); + + validate_enums(prop); +} + +static void validate_blob_prop(int fd, + const struct drm_mode_get_property *prop, + uint64_t value) +{ + struct drm_mode_get_blob blob; /* - * There's no way to enumerate all properties, we just have to - * brute-force the first few kms ids. 1000 should be enough. + * Despite what libdrm makes you believe, we never supply + * additional information for BLOB properties, only for enums + * and bitmasks */ - for (i = 0; i < 1000; i++) { - struct drm_mode_get_property prop; + igt_assert_eq(prop->count_values, 0); + igt_assert_eq(prop->count_enum_blobs, 0); - memset(&prop, 0, sizeof(prop)); - prop.prop_id = i; + igt_assert_lte_u64(value, 0xffffffff); - if (drmIoctl(fd, DRM_IOCTL_MODE_GETPROPERTY, &prop)) - continue; + /* + * Immutable blob properties can have value==0. + * Happens for example with the "EDID" property + * when there is nothing hooked up to the connector. + */ - if (prop.count_values) { - values = calloc(prop.count_values, sizeof(uint64_t)); - igt_assert(values); - memset(values, 0x5c, sizeof(uint64_t)*prop.count_values); - prop.values_ptr = to_user_pointer(values); - } + if (!value) + return; - /* despite what libdrm makes you believe, we never supply - * additional information for BLOB properties, only for enums - * and bitmasks */ - igt_assert_eq(!!prop.count_enum_blobs, - !!(prop.flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK))); - if (prop.flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)) - igt_assert(prop.count_enum_blobs == prop.count_values); - - if (prop.count_enum_blobs) { - enums = calloc(prop.count_enum_blobs, sizeof(*enums)); - memset(enums, 0x5c, sizeof(*enums)*prop.count_enum_blobs); - igt_assert(enums); - prop.enum_blob_ptr = to_user_pointer(enums); - } + memset(&blob, 0, sizeof(blob)); + blob.blob_id = value; - do_ioctl(fd, DRM_IOCTL_MODE_GETPROPERTY, &prop); + do_ioctl(fd, DRM_IOCTL_MODE_GETPROPBLOB, &blob); +} - for (int j = 0; j < prop.count_values; j++) { - igt_assert(values[j] != 0x5c5c5c5c5c5c5c5cULL); +static void validate_object_prop(int fd, + const struct drm_mode_get_property *prop, + uint64_t value) +{ + const uint64_t *values = from_user_pointer(prop->values_ptr); + bool immutable = prop->flags & DRM_MODE_PROP_IMMUTABLE; + struct drm_mode_crtc crtc; + struct drm_mode_fb_cmd fb; - if (!(prop.flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK))) - continue; - igt_assert(enums[j].value != 0x5c5c5c5c5c5c5c5cULL); - igt_assert(enums[j].value == values[j]); - igt_assert(enums[j].name[0] != '\0'); - } + igt_assert_eq(prop->count_values, 1); + igt_assert_eq(prop->count_enum_blobs, 0); + + igt_assert_lte_u64(value, 0xffffffff); + igt_assert(!immutable || value != 0); + + switch (values[0]) { + case DRM_MODE_OBJECT_CRTC: + if (!value) + break; + memset(&crtc, 0, sizeof(crtc)); + crtc.crtc_id = value; + do_ioctl(fd, DRM_IOCTL_MODE_GETCRTC, &crtc); + break; + case DRM_MODE_OBJECT_FB: + if (!value) + break; + memset(&fb, 0, sizeof(fb)); + fb.fb_id = value; + do_ioctl(fd, DRM_IOCTL_MODE_GETFB, &fb); + break; + default: + /* These are the only types we have so far */ + igt_assert(0); } } +static void validate_property(int fd, + const struct drm_mode_get_property *prop, + uint64_t value, bool atomic) +{ + uint32_t flags = prop->flags; + uint32_t legacy_type = flags & DRM_MODE_PROP_LEGACY_TYPE; + uint32_t ext_type = flags & DRM_MODE_PROP_EXTENDED_TYPE; + + igt_assert_eq((flags & ~(DRM_MODE_PROP_LEGACY_TYPE | + DRM_MODE_PROP_EXTENDED_TYPE | + DRM_MODE_PROP_IMMUTABLE | + DRM_MODE_PROP_ATOMIC)), 0); + + igt_assert(atomic || + (flags & DRM_MODE_PROP_ATOMIC) == 0); + + igt_assert_neq(!legacy_type, !ext_type); + + igt_assert(legacy_type == 0 || + is_power_of_two(legacy_type)); + + switch (legacy_type) { + case DRM_MODE_PROP_RANGE: + validate_range_prop(prop, value); + break; + case DRM_MODE_PROP_ENUM: + validate_enum_prop(prop, value); + break; + case DRM_MODE_PROP_BITMASK: + validate_bitmask_prop(prop, value); + break; + case DRM_MODE_PROP_BLOB: + validate_blob_prop(fd, prop, value); + break; + default: + igt_assert_eq(legacy_type, 0); + } + + switch (ext_type) { + case DRM_MODE_PROP_OBJECT: + validate_object_prop(fd, prop, value); + break; + case DRM_MODE_PROP_SIGNED_RANGE: + validate_range_prop(prop, value); + break; + default: + igt_assert_eq(ext_type, 0); + } +} + +static void validate_prop(int fd, uint32_t prop_id, uint64_t value, bool atomic) +{ + struct drm_mode_get_property prop; + struct drm_mode_property_enum *enums = NULL; + uint64_t *values = NULL; + + memset(&prop, 0, sizeof(prop)); + prop.prop_id = prop_id; + + do_ioctl(fd, DRM_IOCTL_MODE_GETPROPERTY, &prop); + + if (prop.count_values) { + values = calloc(prop.count_values, sizeof(values[0])); + igt_assert(values); + memset(values, 0x5c, sizeof(values[0])*prop.count_values); + prop.values_ptr = to_user_pointer(values); + } + + if (prop.count_enum_blobs) { + enums = calloc(prop.count_enum_blobs, sizeof(enums[0])); + memset(enums, 0x5c, sizeof(enums[0])*prop.count_enum_blobs); + igt_assert(enums); + prop.enum_blob_ptr = to_user_pointer(enums); + } + + do_ioctl(fd, DRM_IOCTL_MODE_GETPROPERTY, &prop); + + for (int i = 0; i < prop.count_values; i++) + igt_assert_neq_u64(values[i], 0x5c5c5c5c5c5c5c5cULL); + + for (int i = 0; i < prop.count_enum_blobs; i++) + igt_assert_neq_u64(enums[i].value, 0x5c5c5c5c5c5c5c5cULL); + + validate_property(fd, &prop, value, atomic); + + free(values); + free(enums); +} + +static void validate_props(int fd, uint32_t obj_type, uint32_t obj_id, bool atomic) +{ + struct drm_mode_obj_get_properties properties; + uint32_t *props = NULL; + uint64_t *values = NULL; + uint32_t count; + + memset(&properties, 0, sizeof(properties)); + properties.obj_type = obj_type; + properties.obj_id = obj_id; + + do_ioctl(fd, DRM_IOCTL_MODE_OBJ_GETPROPERTIES, &properties); + + count = properties.count_props; + + if (count) { + props = calloc(count, sizeof(props[0])); + memset(props, 0x5c, sizeof(props[0])*count); + igt_assert(props); + properties.props_ptr = to_user_pointer(props); + + values = calloc(count, sizeof(values[0])); + memset(values, 0x5c, sizeof(values[0])*count); + igt_assert(values); + properties.prop_values_ptr = to_user_pointer(values); + } + + do_ioctl(fd, DRM_IOCTL_MODE_OBJ_GETPROPERTIES, &properties); + + igt_assert(properties.count_props == count); + + for (int i = 0; i < count; i++) + validate_prop(fd, props[i], values[i], atomic); + + free(values); + free(props); +} + +static void expect_no_props(int fd, uint32_t obj_type, uint32_t obj_id) +{ + struct drm_mode_obj_get_properties properties; + + memset(&properties, 0, sizeof(properties)); + properties.obj_type = obj_type; + properties.obj_id = obj_id; + + igt_assert_neq(drmIoctl(fd, DRM_IOCTL_MODE_OBJ_GETPROPERTIES, &properties), 0); +} + +static void get_prop_sanity(igt_display_t *display, bool atomic) +{ + int fd = display->drm_fd; + drmModePlaneResPtr plane_res; + drmModeResPtr res; + + res = drmModeGetResources(fd); + plane_res = drmModeGetPlaneResources(fd); + + for (int i = 0; i < plane_res->count_planes; i++) { + validate_props(fd, DRM_MODE_OBJECT_PLANE, + plane_res->planes[i], atomic); + } + + for (int i = 0; i < res->count_crtcs; i++) { + validate_props(fd, DRM_MODE_OBJECT_CRTC, + res->crtcs[i], atomic); + } + + for (int i = 0; i < res->count_connectors; i++) { + validate_props(fd, DRM_MODE_OBJECT_CONNECTOR, + res->connectors[i], atomic); + } + + for (int i = 0; i < res->count_encoders; i++) { + expect_no_props(fd, DRM_MODE_OBJECT_ENCODER, + res->encoders[i]); + } + + drmModeFreePlaneResources(plane_res); + drmModeFreeResources(res); +} + static void invalid_properties(igt_display_t *display, bool atomic) { igt_output_t *output; @@ -441,8 +705,20 @@ igt_main igt_subtest("invalid-properties-atomic") invalid_properties(&display, true); - igt_subtest("get_properties-sanity") - get_prop_sanity(&display); + igt_subtest("get_properties-sanity-atomic") { + igt_skip_on(!display.is_atomic); + get_prop_sanity(&display, true); + } + + igt_subtest("get_properties-sanity-non-atomic") { + if (display.is_atomic) + igt_assert_eq(drmSetClientCap(display.drm_fd, DRM_CLIENT_CAP_ATOMIC, 0), 0); + + get_prop_sanity(&display, false); + + if (display.is_atomic) + igt_assert_eq(drmSetClientCap(display.drm_fd, DRM_CLIENT_CAP_ATOMIC, 1), 0); + } igt_fixture { igt_display_fini(&display); -- cgit v1.2.3