[PATCH i-g-t v2] tests/kms_properties: rework immutability checks
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri Oct 25 09:53:29 UTC 2024
On Fri, 25 Oct 2024 at 08:51, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
>
> On Thu, Oct 24, 2024 at 09:29:03PM +0300, Dmitry Baryshkov wrote:
> > On Fri, 18 Oct 2024 at 15:01, Dmitry Baryshkov
> > <dmitry.baryshkov at linaro.org> wrote:
> > >
> > > Following the discussion on IRC, it is actually an error to require that
> > > properties that can not be chaged are marked as immutable.
> > >
> > > First of all, it creates inconsistent uAPI. Some drivers might have an
> > > immutable property, while others will have it mutable. Yes, there are
> > > known examples for such behaviour (e.g. zpos), but they are clearly
> > > documented in this way.
> > >
> > > Second, by the nature of the flag, the DRM_MODE_PROP_IMMUTABLE defines
> > > more of the 'direction' of the property (whether it is set by the kernel
> > > or it is expected to be set by the userspace) rather than simply states
> > > that there is no way for the userspace to change the property.
> > >
> > > Rework the immutability checks to verify that the properties defined as
> > > immutable have this flag set. Keep the "immutable if single value"
> > > property just for the "zpos" property.
> > >
> > > Fixes: 29ae12bd764e ("tests/kms_properties: Validate properties harder")
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Link: https://oftc.irclog.whitequark.org/dri-devel/2024-07-16#33374622
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> >
> > Gracious ping for the patch. We need it to be able to proceed with the
> > HDMI rework for the drm/msm driver, otherwise IGT tests fail.
> >
> > >
> > > ---
> > > Changes since v1:
> > > - Moved GAMMA_LUT_SIZE and DEGAMMA_LUT_SIZE to DRM_MODE_OBJECT_CRTC.
> > > - Added debug print to help debugging possible issues.
> > > ---
> > > tests/kms_properties.c | 110 ++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 87 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/tests/kms_properties.c b/tests/kms_properties.c
> > > index a93c93cccf64..57f07e69909a 100644
> > > --- a/tests/kms_properties.c
> > > +++ b/tests/kms_properties.c
> > > @@ -416,16 +416,80 @@ 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);
> > > }
> > >
> > > +enum prop_imm_flags {
> > > + IMMUTABLE_REQ,
> > > + IMMUTABLE_IF_SINGLE_VALUE,
> > > +};
> > > +
> > > +static const struct {
> > > + uint32_t obj_type;
> > > + const char *name;
> > > + enum prop_imm_flags flags;
> > > +} prop_settings[] = {
> > > + /* generic */
> > > + { DRM_MODE_OBJECT_CONNECTOR, "EDID", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "PATH", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "TILE", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "WRITEBACK_PIXEL_FORMATS", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "non-desktop", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "panel orientation" ,IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "privacy-screen hw-state", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "subconnector", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "suggested X", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "suggested Y", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CONNECTOR, "vrr_capable", IMMUTABLE_REQ },
> > > +
> > > + { DRM_MODE_OBJECT_CRTC, "DEGAMMA_LUT_SIZE", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_CRTC, "GAMMA_LUT_SIZE", IMMUTABLE_REQ },
> > > +
> > > + { DRM_MODE_OBJECT_PLANE, "IN_FORMATS", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_PLANE, "SIZE_HINTS", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_PLANE, "type", IMMUTABLE_REQ },
> > > + { DRM_MODE_OBJECT_PLANE, "zpos", IMMUTABLE_IF_SINGLE_VALUE },
> > > +
> > > + /* driver-specific */
> > > + { DRM_MODE_OBJECT_CONNECTOR, "hotplug_mode_update", IMMUTABLE_REQ }, // qxl, vmwgfx
> > > + { DRM_MODE_OBJECT_CONNECTOR, "implicit_placement", IMMUTABLE_REQ }, // vmwgfx
> > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_BLEND_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_DEGAMMA_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_LUT3D_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_SHAPER_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > +};
>
> Not really a fan of having a list like this. All of these look
> like they are just regular immutable properties, with zpos being
> the only exception. Or is that not the case?
Yes. The goal is to perform the ABI check: which properties are
documented to be immutable. All other properties are expected to be
mutable.
>
> What was the problem of just dropping the check that non-immutable
> properties must have multiple possible values?
See the discussion at [1] and then the response by Sima [2] to the
patch similar to your proposal.
[1] https://oftc.irclog.whitequark.org/dri-devel/2024-07-16#33374622
[2] https://lore.kernel.org/igt-dev/Zpjn2dTHDrBBuTVH@phenom.ffwll.local/
>
> And perhaps there should be instead a check that immutable
> properties must not have more than one possible value?
This is not a correct assumption, immutable props might have any
number of values. The difference is on semantics side: the driver
controls immutable properties, userspace controls mutable ones.
>
> > > +
> > > +static void validate_prop_immutable(const struct drm_mode_get_property *prop,
> > > + uint32_t obj_type, bool single_value)
> > > +{
> > > + bool immutable = prop->flags & DRM_MODE_PROP_IMMUTABLE;
> > > + int i;
> > > +
> > > + igt_debug("Testing property \"%s\"\n", prop->name);
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(prop_settings); i++) {
> > > + if (prop_settings[i].obj_type == obj_type &&
> > > + !strcmp(prop_settings[i].name, prop->name))
> > > + break;
> > > + }
> > > +
> > > + if (i == ARRAY_SIZE(prop_settings)) {
> > > + igt_assert(!immutable);
> > > + return;
> > > + }
> > > +
> > > + igt_assert(immutable || prop_settings[i].flags != IMMUTABLE_REQ);
> > > + igt_assert(immutable || !single_value ||
> > > + prop_settings[i].flags != IMMUTABLE_IF_SINGLE_VALUE);
> > > +}
> > > +
> > > static void validate_range_prop(const struct drm_mode_get_property *prop,
> > > - uint64_t value)
> > > + uint64_t value, uint32_t obj_type)
> > > {
> > > 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);
> > > +
> > > + validate_prop_immutable(prop, obj_type, values[0] == values[1]);
> > >
> > > if (is_unsigned) {
> > > igt_assert_lte_u64(values[0], values[1]);
> > > @@ -458,15 +522,14 @@ static void validate_enums(const struct drm_mode_get_property *prop)
> > > }
> > >
> > > static void validate_enum_prop(const struct drm_mode_get_property *prop,
> > > - uint64_t value)
> > > + uint64_t value, uint32_t obj_type)
> > > {
> > > 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);
> > > + validate_prop_immutable(prop, obj_type, prop->count_values == 1);
> > >
> > > for (i = 0; i < prop->count_values; i++) {
> > > if (value == values[i])
> > > @@ -478,15 +541,14 @@ static void validate_enum_prop(const struct drm_mode_get_property *prop,
> > > }
> > >
> > > static void validate_bitmask_prop(const struct drm_mode_get_property *prop,
> > > - uint64_t value)
> > > + uint64_t value, uint32_t obj_type)
> > > {
> > > 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);
> > > + validate_prop_immutable(prop, obj_type, prop->count_values == 1);
> > >
> > > for (int i = 0; i < prop->count_values; i++) {
> > > igt_assert_lte_u64(values[i], 63);
> > > @@ -501,7 +563,7 @@ static void validate_bitmask_prop(const struct drm_mode_get_property *prop,
> > >
> > > static void validate_blob_prop(int fd,
> > > const struct drm_mode_get_property *prop,
> > > - uint64_t value)
> > > + uint64_t value, uint32_t obj_type)
> > > {
> > > struct drm_mode_get_blob blob;
> > >
> > > @@ -515,6 +577,8 @@ static void validate_blob_prop(int fd,
> > >
> > > igt_assert_lte_u64(value, 0xffffffff);
> > >
> > > + validate_prop_immutable(prop, obj_type, false);
> > > +
> > > /*
> > > * Immutable blob properties can have value==0.
> > > * Happens for example with the "EDID" property
> > > @@ -532,10 +596,9 @@ static void validate_blob_prop(int fd,
> > >
> > > static void validate_object_prop(int fd,
> > > const struct drm_mode_get_property *prop,
> > > - uint64_t value)
> > > + uint64_t value, uint32_t obj_type)
> > > {
> > > 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;
> > >
> > > @@ -543,7 +606,7 @@ static void validate_object_prop(int fd,
> > > igt_assert_eq(prop->count_enum_blobs, 0);
> > >
> > > igt_assert_lte_u64(value, 0xffffffff);
> > > - igt_assert(!immutable || value != 0);
> > > + validate_prop_immutable(prop, obj_type, value == 0);
> > >
> > > switch (values[0]) {
> > > case DRM_MODE_OBJECT_CRTC:
> > > @@ -568,7 +631,7 @@ static void validate_object_prop(int fd,
> > >
> > > static void validate_property(int fd,
> > > const struct drm_mode_get_property *prop,
> > > - uint64_t value, bool atomic)
> > > + uint64_t value, bool atomic, uint32_t obj_type)
> > > {
> > > uint32_t flags = prop->flags;
> > > uint32_t legacy_type = flags & DRM_MODE_PROP_LEGACY_TYPE;
> > > @@ -589,16 +652,16 @@ static void validate_property(int fd,
> > >
> > > switch (legacy_type) {
> > > case DRM_MODE_PROP_RANGE:
> > > - validate_range_prop(prop, value);
> > > + validate_range_prop(prop, value, obj_type);
> > > break;
> > > case DRM_MODE_PROP_ENUM:
> > > - validate_enum_prop(prop, value);
> > > + validate_enum_prop(prop, value, obj_type);
> > > break;
> > > case DRM_MODE_PROP_BITMASK:
> > > - validate_bitmask_prop(prop, value);
> > > + validate_bitmask_prop(prop, value, obj_type);
> > > break;
> > > case DRM_MODE_PROP_BLOB:
> > > - validate_blob_prop(fd, prop, value);
> > > + validate_blob_prop(fd, prop, value, obj_type);
> > > break;
> > > default:
> > > igt_assert_eq(legacy_type, 0);
> > > @@ -606,17 +669,18 @@ static void validate_property(int fd,
> > >
> > > switch (ext_type) {
> > > case DRM_MODE_PROP_OBJECT:
> > > - validate_object_prop(fd, prop, value);
> > > + validate_object_prop(fd, prop, value, obj_type);
> > > break;
> > > case DRM_MODE_PROP_SIGNED_RANGE:
> > > - validate_range_prop(prop, value);
> > > + validate_range_prop(prop, value, obj_type);
> > > break;
> > > default:
> > > igt_assert_eq(ext_type, 0);
> > > }
> > > }
> > >
> > > -static void validate_prop(int fd, uint32_t prop_id, uint64_t value, bool atomic)
> > > +static void validate_prop(int fd, uint32_t prop_id, uint64_t value,
> > > + bool atomic, uint32_t obj_type)
> > > {
> > > struct drm_mode_get_property prop;
> > > struct drm_mode_property_enum *enums = NULL;
> > > @@ -649,7 +713,7 @@ static void validate_prop(int fd, uint32_t prop_id, uint64_t value, bool atomic)
> > > for (int i = 0; i < prop.count_enum_blobs; i++)
> > > igt_assert_neq_u64(enums[i].value, 0x5c5c5c5c5c5c5c5cULL);
> > >
> > > - validate_property(fd, &prop, value, atomic);
> > > + validate_property(fd, &prop, value, atomic, obj_type);
> > >
> > > free(values);
> > > free(enums);
> > > @@ -687,7 +751,7 @@ static void validate_props(int fd, uint32_t obj_type, uint32_t obj_id, bool atom
> > > igt_assert(properties.count_props == count);
> > >
> > > for (int i = 0; i < count; i++)
> > > - validate_prop(fd, props[i], values[i], atomic);
> > > + validate_prop(fd, props[i], values[i], atomic, obj_type);
> > >
> > > free(values);
> > > free(props);
> > > --
> > > 2.45.2
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> --
> Ville Syrjälä
> Intel
--
With best wishes
Dmitry
More information about the igt-dev
mailing list