Re: [PATCH i-g-t] tests/kms_properties: drop immutability checks

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Wed, Jul 17, 2024 at 05:39:47PM +0300, Dmitry Baryshkov 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.
> 
> Drop the single-value-is-immutable tests.
> 
> Fixes: 29ae12bd764e ("tests/kms_properties: Validate properties harder")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Link: https://oftc.irclog.whitequark.org/dri-devel/2024-07-16#33374622
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Makes sense, but we might drop some test coverage for the properties where
we _do_ want the single value to be immutable. Like zpos.

So I think at least a cursory audit of existing properties would be good,
checking that the kerneldoc is accurate and then maybe limiting these
checks here to properties which are documented to have this behaviour?

I know that's a bunch more work, but I fear if we just drop this we only
move from one a bit confusing state of the uapi to another one, without
real improvements.

Ofc if all the compositor folks tell me that this doesn't matter anyway,
I'll shut up :-)

Cheers, Sima

> ---
>  tests/kms_properties.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tests/kms_properties.c b/tests/kms_properties.c
> index c0e5be1c90a8..c41b88bef76f 100644
> --- a/tests/kms_properties.c
> +++ b/tests/kms_properties.c
> @@ -421,11 +421,9 @@ static void validate_range_prop(const struct drm_mode_get_property *prop,
>  {
>  	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]);
> @@ -461,12 +459,10 @@ 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])
> @@ -481,12 +477,10 @@ 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);
> @@ -535,7 +529,6 @@ static void validate_object_prop(int fd,
>  				 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;
>  
> @@ -543,7 +536,6 @@ 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);
>  
>  	switch (values[0]) {
>  	case DRM_MODE_OBJECT_CRTC:
> -- 
> 2.43.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch