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

[Melissa Wen]

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);

> 
> 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 at ctm-0-25@*
> > >    kms_color at ctm-0-50@*
> > >    kms_color at ctm-0-75@*
> > >    kms_color at ctm-red-to-blue@*
> > >    kms_color at ctm-negative@*
> > > 
> > > Signed-off-by: Alex Hung <alex.hung at 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
> > > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20221220/227d5f6d/attachment.sig>