Re: [PATCH v5 15/38] drm/vkms: Introduce config for plane rotation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vkms_config_valid_plane_rotation() incorrectly allows multiple base rotation bits to be set in default_rotation, which will trigger a kernel WARNING.
- [Low] The validation check ensuring that supported_rotations contains at least one rotation is mathematically unreachable dead code.
--

commit f25358e4a8dbe82dcc32cb1193229da2bf2b59f3
Author: Louis Chauvet <louis.chauvet@bootlin.com>

drm/vkms: Introduce config for plane rotation

This commit introduces a new configuration interface that allows customizing 
the supported rotations for each plane. This is useful for testing specific 
rotation setups without advertising all possible plane rotations.

> diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> index 1e4ea1863420f..c33fe737678ff 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
[ ... ]
> @@ -481,6 +488,44 @@ static void vkms_config_test_valid_plane_type(struct kunit *test)
[ ... ]
> +	/* Invalid, supported rotations must contains at least one rotation */
> +	vkms_config_plane_set_supported_rotations(plane_cfg, DRM_MODE_REFLECT_MASK);
> +	vkms_config_plane_set_default_rotation(plane_cfg, DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_0);
> +	KUNIT_EXPECT_FALSE(test, vkms_config_valid_plane_rotation(config, plane_cfg));

[Severity: Low]
Does this test actually verify the intended check?

Since default_rotation is not a subset of supported_rotations here, this test 
fails the earlier subset check in vkms_config_valid_plane_rotation(), 
meaning it never actually reaches the supported_rotations validation check.

> +	KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
> +
> +	/* Invalid, default rotation must contains at least one rotation */
> +	vkms_config_plane_set_supported_rotations(plane_cfg,
> +						  DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0);
> +	vkms_config_plane_set_default_rotation(plane_cfg, DRM_MODE_REFLECT_X);
> +	KUNIT_EXPECT_FALSE(test, vkms_config_valid_plane_rotation(config, plane_cfg));
> +	KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
> +
> +	vkms_config_destroy(config);
> +}
> +
>  static void vkms_config_test_valid_plane_possible_crtcs(struct kunit *test)
>  {
>  	struct vkms_config *config;
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 54994f3c8e144..5c86eb24fedf3 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
[ ... ]
> @@ -150,6 +151,33 @@ static bool valid_plane_number(const struct vkms_config *config)
>  	return true;
>  }
>  
> +VISIBLE_IF_KUNIT
> +bool vkms_config_valid_plane_rotation(const struct vkms_config *config,
> +				      const struct vkms_config_plane *plane_cfg)
> +{
> +	struct drm_device *dev = config->dev ? &config->dev->drm : NULL;
> +
> +	if ((vkms_config_plane_get_default_rotation(plane_cfg) &
> +	     vkms_config_plane_get_supported_rotations(plane_cfg)) !=
> +	    vkms_config_plane_get_default_rotation(plane_cfg)) {
> +		drm_info(dev, "Configured default rotation is not supported by the plane\n");
> +		return false;
> +	}
> +
> +	if ((vkms_config_plane_get_default_rotation(plane_cfg) & DRM_MODE_ROTATE_MASK) == 0) {
> +		drm_info(dev, "Configured default rotation must contains at least one possible rotation\n");
> +		return false;
> +	}

[Severity: Medium]
Will this allow multiple base rotation bits (e.g., DRM_MODE_ROTATE_0 | 
DRM_MODE_ROTATE_90) to be set simultaneously?

The API invariant in drm_plane_create_rotation_property() requires exactly one 
base rotation angle and uses is_power_of_2() to check this. If multiple bits 
pass this validation here, it could trigger a kernel WARNING stack trace later 
when creating the property.

Should this check use is_power_of_2() instead?

> +
> +	if ((vkms_config_plane_get_supported_rotations(plane_cfg) & DRM_MODE_ROTATE_MASK) == 0) {

[Severity: Low]
Is this block mathematically unreachable dead code?

If default_rotation contains at least one rotation bit (from the previous check) 
and default_rotation is a bitwise subset of supported_rotations (from the first 
check), then supported_rotations is mathematically guaranteed to contain at least 
one rotation bit.

> +		drm_info(dev, "Configured supported rotations must contains at least one possible rotation\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_IF_KUNIT(vkms_config_valid_plane_rotation);
> +
[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index f351202031244..8a64ea41734b8 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -235,8 +235,9 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  
>  	drm_plane_helper_add(&plane->base, &vkms_plane_helper_funcs);
>  
> -	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
> -					   DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK);
> +	drm_plane_create_rotation_property(&plane->base,
> +					   vkms_config_plane_get_default_rotation(plane_cfg),
> +					   vkms_config_plane_get_supported_rotations(plane_cfg));

[Severity: Medium]
This is where the invalid configuration could be passed to the DRM core.

If vkms_config_valid_plane_rotation() allows a default_rotation mask with 
multiple rotation bits, calling drm_plane_create_rotation_property() here will 
trigger its WARN_ON() invariant check.

>  
>  	drm_plane_create_color_properties(&plane->base,
>  					  BIT(DRM_COLOR_YCBCR_BT601) |

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627-vkms-all-config-v5-0-854aa0840926@bootlin.com?part=15