[igt-dev] [PATCH i-g-t] tests/kms_properties: Validate properties harder
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Fri Apr 27 14:19:21 UTC 2018
On Wed, 2018-03-07 at 23:33 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> 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ä <ville.syrjala at linux.intel.com>
> ---
> 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 43dd15fe3b32..b261060ed565 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -385,4 +385,6 @@ static inline bool igt_list_empty(const struct
> igt_list *list)
> &pos->member != (head);
> \
> pos = tmp, tmp = igt_list_next_entry(pos, member))
>
> +#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 66523a208c31..17c9371cf6f7 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 a63b69b4decf..68c0518fca5f 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);
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
--
Best Regards,
Lisovskiy Stanislav
More information about the igt-dev
mailing list