Re: [igt-dev] [PATCH] tests/kms_color: Reduce skips on some ctm-* tests

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

On 2022-12-20 12:06, Melissa Wen wrote:
> On 12/20, Alex Hung wrote:
>>
>>
>> On 2022-12-20 04:55, Melissa Wen wrote:
>>> On 12/19, Alex Hung wrote:
>>>> 64d0ff72 introduced a workaround for ctm-max but the condition caused
>>>> other ctm tests to skip unintentionally on some platforms.
>>>
>>> Hi Alex,
>>>
>>> Can't we just drop this gamma/degamma setup for CTM tests?
>>>
>>> I remember thinking about removing this setup before... From DRM docs,
>>> "Each of the properties are optional." so I don't see an explicity
>>> reason for degamma/gamma dependencies. I think we should only test the
>>> CTM property here, or change the name of the subtest to describe a full
>>> color mgmt testing.
>>>
>>> I took a look at previous discussions on having or not degamma/gamma
>>> here and if it's an intel limitation, than I prefer to explicitly
>>> restrict it for i915 HW. What do you think?
>>
>> Hi Melissa,
>>
>> I may not understand you 100%.
>>
>> Could you please share what you'd like to change, or create a patch for more
>> discussion?
> 
> My suggestion is basically disable degamma and gamma and only set them for
> i915 + not max ctm case, something like the following diff:
> 
> diff --git a/tests/kms_color.c b/tests/kms_color.c
> index d016cefb..2be4095d 100644
> --- a/tests/kms_color.c
> +++ b/tests/kms_color.c
> @@ -493,12 +493,16 @@ static bool test_pipe_ctm(data_t *data,
>          igt_assert(fb_modeset_id);
>          igt_plane_set_fb(primary, &fb_modeset);
> 
> +       disable_degamma(primary->pipe);
> +       disable_gamma(primary->pipe);
> +
>          /*
> -        * Don't program LUT's for max CTM cases, as limitation of
> -        * representing intermediate values between 0 and 1.0 causes
> -        * rounding issues and inaccuracies leading to crc mismatch.
> +        * Program LUT's only for i915, but not in max CTM cases, as limitation
> +        * of representing intermediate values between 0 and 1.0 causes
> +        * rounding issues and inaccuracies leading to crc mismatch in i915
> +        * devices.
>           */
> -       if (memcmp(before, after, sizeof(color_t))) {
> +       if (is_i915_device(data->drm_fd) && memcmp(before, after, sizeof(color_t))) {
>                  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));
> 
> @@ -507,10 +511,6 @@ static bool test_pipe_ctm(data_t *data,
> 
>                  set_degamma(data, primary->pipe, degamma_linear);
>                  set_gamma(data, primary->pipe, gamma_linear);
> -       } else {
> -               /* Disable Degamma and Gamma for ctm max test */
> -               disable_degamma(primary->pipe);
> -               disable_gamma(primary->pipe);
>          }
> 
>          disable_ctm(primary->pipe);
> 

This looks good to me, and I also tested it on 2A + 1I systems and it 
ran fine.



>>
>> Cheers,
>> Alex
>>
>>>
>>> Best regards,
>>>
>>> Melissa
>>>
>>>>
>>>> The patch improves the condition by keeping the original condition
>>>> for i915 and checking degamma and gamma props for others. The
>>>> following subtests will be executed instead of skipped.
>>>>
>>>>     kms_color@ctm-0-25@*
>>>>     kms_color@ctm-0-50@*
>>>>     kms_color@ctm-0-75@*
>>>>     kms_color@ctm-red-to-blue@*
>>>>     kms_color@ctm-negative@*
>>>>
>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>>> ---
>>>>    tests/kms_color.c | 30 +++++++++++++++++++++---------
>>>>    1 file changed, 21 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/tests/kms_color.c b/tests/kms_color.c
>>>> index d016cefb..450bf7ca 100644
>>>> --- a/tests/kms_color.c
>>>> +++ b/tests/kms_color.c
>>>> @@ -446,6 +446,26 @@ end:
>>>>    	return ret;
>>>>    }
>>>> +static bool is_gamma_degamma_supported(data_t *data,
>>>> +				       igt_plane_t *primary,
>>>> +				       color_t *before,
>>>> +				       color_t *after)
>>>> +{
>>>> +	/*
>>>> +	 * Don't program LUT's for max CTM cases, as limitation of
>>>> +	 * representing intermediate values between 0 and 1.0 causes
>>>> +	 * rounding issues and inaccuracies leading to crc mismatch.
>>>> +	 */
>>>> +	if (is_i915_device(data->drm_fd) && memcmp(before, after, sizeof(color_t)))
>>>> +		return true;
>>>> +
>>>> +	if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_DEGAMMA_LUT) &&
>>>> +	    igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_LUT))
>>>> +		return true;
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>>    /*
>>>>     * Draw 3 rectangles using before colors with the ctm matrix apply and verify
>>>>     * the CRC is equal to using after colors with an identify ctm matrix.
>>>> @@ -493,14 +513,7 @@ static bool test_pipe_ctm(data_t *data,
>>>>    	igt_assert(fb_modeset_id);
>>>>    	igt_plane_set_fb(primary, &fb_modeset);
>>>> -	/*
>>>> -	 * Don't program LUT's for max CTM cases, as limitation of
>>>> -	 * representing intermediate values between 0 and 1.0 causes
>>>> -	 * rounding issues and inaccuracies leading to crc mismatch.
>>>> -	 */
>>>> -	if (memcmp(before, after, sizeof(color_t))) {
>>>> -		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));
>>>> +	if (is_gamma_degamma_supported(data, primary, before, after)) {
>>>>    		degamma_linear = generate_table(data->degamma_lut_size, 1.0);
>>>>    		gamma_linear = generate_table(data->gamma_lut_size, 1.0);
>>>> @@ -508,7 +521,6 @@ static bool test_pipe_ctm(data_t *data,
>>>>    		set_degamma(data, primary->pipe, degamma_linear);
>>>>    		set_gamma(data, primary->pipe, gamma_linear);
>>>>    	} else {
>>>> -		/* Disable Degamma and Gamma for ctm max test */
>>>>    		disable_degamma(primary->pipe);
>>>>    		disable_gamma(primary->pipe);
>>>>    	}
>>>> -- 
>>>> 2.25.1
>>>>