Re: [PATCH i-g-t v1] tests/kms_color: Add 10-bit color depth support to gamma test for MediaTek

["Sharma, Swati2" <swati2.sharma@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3805 bytes --]

Hi Liu,

Change is functionally is correct but the commit message and code need 
more clarity and justification, especially around:
-10-bit requirement
-4092 LUT value

On 20-03-2026 01:03 pm, Jay Liu wrote:
> 1.Add 10-bit color depth flow to gamma test for MediaTek devices, which only
> support bit-true results with 10-bit data. The test now selects 8-bit or
> 10-bit color depth and framebuffer format based on device type.
>
> 2. Change the maximum gamma LUT value from 4095 to 4092 for MediaTek devices.

The commit message mentions changing LUT values from 4095 to 4092, but 
the code updates values from 0xffff to 0xffd0.

Since the LUT is programmed using 16-bit values, the commit message 
should reflect that.

>
> Signed-off-by: Jay Liu<jay.liu@mediatek.com>
> ---
>   tests/kms_color.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/tests/kms_color.c b/tests/kms_color.c
> index 565edb2dc..27fc08ab9 100644
> --- a/tests/kms_color.c
> +++ b/tests/kms_color.c
> @@ -288,7 +288,7 @@ static bool test_pipe_legacy_gamma(data_t *data,
>   	fb_id = igt_create_fb(data->drm_fd,
>   			      mode->hdisplay,
>   			      mode->vdisplay,
> -			      DRM_FORMAT_XRGB8888,
> +			      data->drm_format,
>   			      DRM_FORMAT_MOD_LINEAR,
>   			      &fb);
>   	igt_assert(fb_id);
> @@ -296,7 +296,7 @@ static bool test_pipe_legacy_gamma(data_t *data,
>   	fb_modeset_id = igt_create_fb(data->drm_fd,
>   				      mode->hdisplay,
>   				      mode->vdisplay,
> -				      DRM_FORMAT_XRGB8888,
> +				      data->drm_format,
>   				      DRM_FORMAT_MOD_LINEAR,
>   				      &fb_modeset);
>   	igt_assert(fb_modeset_id);
> @@ -322,8 +322,16 @@ static bool test_pipe_legacy_gamma(data_t *data,
>   	igt_plane_set_fb(primary, &fb);
>   
>   	red_lut[0] = green_lut[0] = blue_lut[0] = 0;
> -	for (i = 1; i < legacy_lut_size; i++)
> -		red_lut[i] = green_lut[i] = blue_lut[i] = 0xffff;
> +	for (i = 1; i < legacy_lut_size; i++) {
> +		/*
> +		 * Use 4092 for mtk gamma LUT to remap all values
> +		 * to max red/green/blue.
> +		 */

This comment can be improved stating why MTK gamma doesn't map to 0xffff?

> +		if (is_mtk_device(data->drm_fd))
> +			red_lut[i] = green_lut[i] = blue_lut[i] = 0xffd0;
> +		else
> +			red_lut[i] = green_lut[i] = blue_lut[i] = 0xffff;
> +	}
>   	igt_assert_eq(drmModeCrtcSetGamma(data->drm_fd, primary->crtc->crtc_id,
>   					  legacy_lut_size, red_lut, green_lut, blue_lut), 0);
>   	igt_display_commit(&data->display);
> @@ -752,14 +760,20 @@ static void
>   run_gamma_degamma_tests_for_crtc(data_t *data, igt_crtc_t *crtc,
>   				 bool (*test_t)(data_t*, igt_plane_t*))
>   {
> +	bool depth_10bit = false;
> +
>   	test_setup(data, crtc);
>   
> +	/* MediaTek can only support bit-ture in 10-bit depth pre color */

Typo: probably meant**bit-true. May be rephrased to

/* MediaTek requires 10-bit pipeline for accurate (bit-true) color 
processing */

> +	if (is_mtk_device(data->drm_fd))
> +		depth_10bit = true;

Instead of setting depth_10bit to false

May be we can do

bool is_mtk = is_mtk_device(data->drm_fd);

> +
>   	/*
> -	 * We assume an 8bits depth per color for degamma/gamma LUTs
> +	 * We assume an 8bits or 10bits depth per color for degamma/gamma LUTs
>   	 * for CRC checks with framebuffer references.
>   	 */
> -	data->color_depth = 8;
> -	data->drm_format = DRM_FORMAT_XRGB8888;
> +	data->color_depth = depth_10bit ? 10 : 8;
> +	data->drm_format = depth_10bit ? DRM_FORMAT_XRGB2101010 : DRM_FORMAT_XRGB8888;
>   	data->mode = igt_output_get_mode(data->output);
/* MTK requires 10-bit pipeline for correct color processing */
data->color_depth=is_mtk?10: 8;
data->drm_format=is_mtk?DRM_FORMAT_XRGB2101010: DRM_FORMAT_XRGB8888;
>   
>   	igt_require(crtc_output_combo_valid(data, crtc));

[-- Attachment #2: Type: text/html, Size: 5757 bytes --]