Re: [igt-dev] [PATCH i-g-t] tests/kms_color: check degamma and gamma props in CTM subtests

[Alex Hung <alex.hung@amd.com>]

Hi Melissa,

Thanks for helping out.

The "set_luts" is not necessary because free_lut() already checks NULL 
pointers. See inline comments.

On 2022-11-03 12:40, Melissa Wen wrote:
> Some CTM subtests generate and set degamma/gamma LUTs without checking
> if these properties are supported by the driver. Therefore, if degamma
> and gamma are used in the subtest, check the color properties before
> generate and set LUTs. If these properties are not supported, these
> subtests will skip instead of failing.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   tests/kms_color.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/kms_color.c b/tests/kms_color.c
> index e4dc2465..e0222c42 100644
> --- a/tests/kms_color.c
> +++ b/tests/kms_color.c
> @@ -463,7 +463,7 @@ static bool test_pipe_ctm(data_t *data,
>   	};
>   	gamma_lut_t *degamma_linear, *gamma_linear;
>   	igt_output_t *output = data->output;
> -	bool ret = true;
> +	bool set_luts, ret = true;

set_luts is not needed.

>   	igt_display_t *display = &data->display;
>   	drmModeModeInfo *mode = data->mode;
>   	struct igt_fb fb_modeset, fb;
> @@ -472,9 +472,6 @@ static bool test_pipe_ctm(data_t *data,
>   
>   	igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_CTM));
>   
> -	degamma_linear = generate_table(data->degamma_lut_size, 1.0);
> -	gamma_linear = generate_table(data->gamma_lut_size, 1.0);
> -
>   	igt_output_set_pipe(output, primary->pipe->pipe);
>   	igt_output_override_mode(output, mode);
>   
> @@ -501,7 +498,14 @@ static bool test_pipe_ctm(data_t *data,
>   	 * representing intermediate values between 0 and 1.0 causes
>   	 * rounding issues and inaccuracies leading to crc mismatch.
>   	 */
> -	if (memcmp(before, after, sizeof(color_t))) {
> +	set_luts = memcmp(before, after, sizeof(color_t));
> +	if (set_luts) {

The above if condition can just stay the same since set_luts is not 
needed afterwards.

> +		igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_DEGAMMA_LUT));
> +		igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_LUT));
> +
> +		degamma_linear = generate_table(data->degamma_lut_size, 1.0);
> +		gamma_linear = generate_table(data->gamma_lut_size, 1.0);
> +
>   		set_degamma(data, primary->pipe, degamma_linear);
>   		set_gamma(data, primary->pipe, gamma_linear);
>   	} else {
> @@ -542,8 +546,10 @@ static bool test_pipe_ctm(data_t *data,
>   	igt_remove_fb(data->drm_fd, &fb);
>   	igt_remove_fb(data->drm_fd, &fb_modeset);
>   
> -	free_lut(degamma_linear);
> -	free_lut(gamma_linear);
> +	if (set_luts) {
> +		free_lut(degamma_linear);
> +		free_lut(gamma_linear);
> +	}

set_luts is not needed, so no changes here because free_lut() checks 
NULL pointers, like in the function "test_pipe_legacy_gamma_reset".

>   
>   	return ret;
>   }