Re: [PATCH 07/11] drm/vkms: rename get_lut_index() to get_uniform_lut_index()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Test coverage for linear LUT interpolation was severely reduced due to a mismatched scaling factor.

Pre-existing issues:
- [High] Incorrect element size used for gamma LUT validation in atomic check rejects valid LUTs on 32-bit platforms.
--

--- Patch [7]: [PATCH 07/11] drm/vkms: rename get_lut_index() to get_uniform_lut_index() ---
commit e4d9729afe7e5d9d0339bd7d09cfdb05b9aad78b
Author: Leandro Ribeiro <leandro.ribeiro@collabora.com>

drm/vkms: rename get_lut_index() to get_uniform_lut_index()

In the next commits we'll add non-uniform LUTs, and this function will
be used exclusively by uniform LUTs. So rename it to make this clearer.

> diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> index cfcd7e8e76407..bb9d84377b979 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> @@ -86,31 +86,31 @@ static const struct vkms_color_lut test_linear_lut = {
>  	.channel_value2index_ratio = 0xf000fll
>  };

[Severity: Medium]
In drivers/gpu/drm/vkms/tests/vkms_color_test.c:vkms_color_test_linear(), the
multiplier is set to 0x101:

	for (int i = 0; i < test_linear_lut.lut_length; i++) {
		int linear = apply_lut_to_channel_value(&test_linear_lut, i * 0x101, LUT_RED);

		KUNIT_EXPECT_EQ(test, DIV_ROUND_CLOSEST(linear, 0x101), i);
	}

Since the new 16-element LUT has a step size of 0x1111, do we need to update
the 0x101 multiplier to match?

Currently, the 16 test iterations only reach up to 15 * 0x101 = 0xF0F. Because
the first interval spans from 0x0 to 0x1111, all test iterations fall entirely
within the very first interpolation interval, leaving over 90% of the channel
range untested.

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but I noticed a potential problem in vkms_atomic_check() in
drivers/gpu/drm/vkms/vkms_drv.c:

	if (new_crtc_state->gamma_lut->length / sizeof(struct drm_color_lut *)
	    > VKMS_GAMMA_LUT_SIZE)
		return -EINVAL;

Does this calculation correctly determine the number of elements? 

The blob length is divided by the size of a pointer
(sizeof(struct drm_color_lut *)) instead of the size of the structure itself
(sizeof(struct drm_color_lut)). On 32-bit architectures, this doubles the
calculated element count and causes valid atomic commits containing a full
gamma LUT to be rejected with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804203351.148521-1-leandro.ribeiro@collabora.com?part=7